Enable NetAnalyzers reliability and globalization rules #22625
Conversation
| _enumerator?.DisposeAsync(); | ||
| _enumerator = null; | ||
|
|
||
| var enumerator = _enumerator; |
roji
Sep 20, 2020
Author
Member
Note that this method returns and user code continues potentially before _enumerator actually gets disposed. Depending on whether CosmosClientWrapper cares if we call ExecuteSqlQuery before the previous enumerator completed, this could be a potentially serious concurrency bug.
Note that this method returns and user code continues potentially before _enumerator actually gets disposed. Depending on whether CosmosClientWrapper cares if we call ExecuteSqlQuery before the previous enumerator completed, this could be a potentially serious concurrency bug.
| @@ -122,7 +122,7 @@ public override bool Equals(object obj) | |||
|
|
|||
| private bool Equals(KeyAccessExpression keyAccessExpression) | |||
| => base.Equals(keyAccessExpression) | |||
| && string.Equals(Name, keyAccessExpression.Name) | |||
| && Name == keyAccessExpression.Name | |||
roji
Sep 20, 2020
Author
Member
This technically doesn't solve a problem, but the string equality operator is defined to be the same as string.Equals, and the code analysis flags this.
This technically doesn't solve a problem, but the string equality operator is defined to be the same as string.Equals, and the code analysis flags this.
AndriySvyryd
Sep 21, 2020
Member
What is the severity of this rule?
What is the severity of this rule?
roji
Sep 22, 2020
Author
Member
This comes from CA1309 (Use ordinal StringComparison), and is currently configured as error (we can of course change this).
Interestingly, configuring the rule as warning in .editorconfig doesn't make the build fail although WarningsAsErrors is on (I think the new rules are exempted via WarningsNotAsErrors...).
This comes from CA1309 (Use ordinal StringComparison), and is currently configured as error (we can of course change this).
Interestingly, configuring the rule as warning in .editorconfig doesn't make the build fail although WarningsAsErrors is on (I think the new rules are exempted via WarningsNotAsErrors...).
|
|
||
| namespace System.Threading.Tasks | ||
| { | ||
| internal static class RelationalTaskExtensions |
roji
Sep 20, 2020
Author
Member
This was dead code. Note also that ContinueWith performs significantly worse than just doing the simple thing, async/await (but the situation may have been different in the past).
This was dead code. Note also that ContinueWith performs significantly worse than just doing the simple thing, async/await (but the situation may have been different in the past).
| @@ -106,7 +106,9 @@ IEnumerator IEnumerable.GetEnumerator() | |||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | |||
| /// </summary> | |||
| public virtual IAsyncEnumerator<TResult> GetAsyncEnumerator(CancellationToken cancellationToken = default) | |||
| => _queryProvider.ExecuteAsync<IAsyncEnumerable<TResult>>(Expression).GetAsyncEnumerator(cancellationToken); | |||
| => _queryProvider | |||
roji
Sep 20, 2020
Author
Member
Potentially important missing cancellation token
Potentially important missing cancellation token
smitpatel
Sep 21, 2020
Member
Not actually important since ExecuteAsync which returns enumerable just compiles the expression and never execute it. The real execution uses the cancellation token from GetAsyncEnumerator. No functional impact.
Not actually important since ExecuteAsync which returns enumerable just compiles the expression and never execute it. The real execution uses the cancellation token from GetAsyncEnumerator. No functional impact.
roji
Sep 21, 2020
Author
Member
OK.
In that case is it right for ExecuteAsync to be async and even accept a cancellation token?
OK.
In that case is it right for ExecuteAsync to be async and even accept a cancellation token?
smitpatel
Sep 22, 2020
Member
If ExecuteAsync is called with non enumerable kind of return value then it is going to hit the database and that is where cancellation token will be used.
If ExecuteAsync is called with non enumerable kind of return value then it is going to hit the database and that is where cancellation token will be used.
| @@ -740,7 +740,7 @@ await using (var transaction = await beginTransaction(c, cancellationToken).Conf | |||
| return s.Result; | |||
| }, async (c, s, ct) => new ExecutionResult<TResult>( | |||
| s.CommitFailed && await s.VerifySucceeded(s.State, ct).ConfigureAwait(false), | |||
| s.Result)); | |||
| s.Result), cancellationToken); | |||
roji
Sep 20, 2020
Author
Member
Potentially important missing cancellation token
Potentially important missing cancellation token
AndriySvyryd
Sep 21, 2020
Member
This is only used when the operation failed and it wasn't in a transaction, so it would be a very corner case for cancellation
This is only used when the operation failed and it wasn't in a transaction, so it would be a very corner case for cancellation
The following rules led to code changes: - CA2000: Dispose objects before losing scope - CA2012: Use ValueTasks correctly - CA2008: Do not create tasks without passing a TaskScheduler - CA2016: Forward the 'CancellationToken' parameter to methods that take one - CA1310: Specify StringComparison for correctness - CA1309: Use ordinal stringcomparison - CA1304: Specify CultureInfo Part of #18870
|
Hello @roji! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
An update to dispose all the things in PR dotnet#22625 accidentally broke our argument parsing logic. This updates that change to preserve the original logic. Fixes dotnet#24251
An update to dispose all the things in PR dotnet#22625 accidentally broke our argument parsing logic. This updates that change to preserve the original logic. Fixes dotnet#24251
An update to dispose all the things in PR dotnet#22625 accidentally broke our argument parsing logic. This updates that change to preserve the original logic. Fixes dotnet#24251
The .NET 5.0 SDK comes with its own new code analysis, which does some of what FxCop previously did. We opt into the new warnings by adding
<AnalysisLevel>in the csproj, and then configuring rules via .editorconfig (the new way to manage code analysis rules).This enables the reliability and globalization rules (see the docs. Commits are split by rule along with the corresponding fixes, we can discuss rule-by-rule etc.
Part of #18870 (but via the built-in analysis instead of FxCop)
Replaces #22591