Enable NetAnalyzers reliability and globalization rules #22625
Conversation
| _enumerator?.DisposeAsync(); | ||
| _enumerator = null; | ||
|
|
||
| var enumerator = _enumerator; |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What is the severity of this rule?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Potentially important missing cancellation token
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK.
In that case is it right for ExecuteAsync to be async and even accept a cancellation token?
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
Potentially important missing cancellation token
There was a problem hiding this comment.
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 (
|
ghost
merged commit 
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