S2674: Code cleanup, refactoring, and performance improvements #5794
Conversation
| using (var stream = File.Open(fileName, FileMode.Open)) | ||
| { | ||
| var result = new byte[stream.Length]; | ||
| (int a, int b) = (stream.Read(result, 0, (int)stream.Length), 42); // FN |
There was a problem hiding this comment.
This was marked as FN but is actually compliant. I added some test cases with some real issues (with discards). I don't think, these should be addressed in this PR, though. I would say, that these cases are out of scope.
| @@ -42,6 +42,16 @@ public static class ExpressionSyntaxExtensions | |||
| semanticModel.GetTypeInfo(expression).Type is { } expressionType | |||
| && (expressionType.IsReferenceType || expressionType.Is(KnownType.System_Nullable_T)); | |||
|
|
|||
| public static ExpressionSyntax RemoveConditionalAccess(this ExpressionSyntax node) | |||
There was a problem hiding this comment.
This was moved from CSharpSyntaxFacade.cs. No tests for this were added. Update: Tests were added.
| expression = expression.RemoveConditionalAccess(); | ||
| if (expression is InvocationExpressionSyntax invocation | ||
| && invocation.GetMethodCallIdentifier() is { } methodIdentifier | ||
| && ReadMethodNames.Contains(methodIdentifier.Text, StringComparer.Ordinal) |
There was a problem hiding this comment.
Syntactic checks were added before the semantic model is queried. Test cases for different invocations were added.
| _ = stream.Read(result, 0, (int)stream.Length); // FN. The result is discarded | ||
| (_, var c) = (stream.Read(result, 0, (int)stream.Length), 42); // FN. The result is discarded | ||
| _ = stream.Read(result, 0, (int)stream.Length) is { }; // FN. The result is discarded | ||
| _ = stream.Read(result, 0, (int)stream.Length) is { } _; // FN. The result is discarded | ||
| _ = (stream.Read(result, 0, (int)stream.Length), 42) is (_, _); // FN. The result is discarded |
There was a problem hiding this comment.
These are some coding patterns, where the return value is read, but discarded. To support these and other related patterns, flow analysis is needed IMO. We should document it on the Kanban board.
There was a problem hiding this comment.
Good point! This would be an Improvement ticket for the backlog. We would need to have an overview of all rules which would need dataflow, but that is a different problem
There was a problem hiding this comment.
@martin-strecker-sonarsource did you capture this somewhere?
There was a problem hiding this comment.
Not yet. There are three issues with this PR now:
- Discards not handled properly
- Upcoming ReadAtLeast.. methods not handled properly
- RemoveConditionalAccess needs some companioning methods from roslyn (e.g. This one)
Where should we document these?
There was a problem hiding this comment.
For Upcoming ReadAtLeast.. methods not handled properly -> you can add a comment to the C# 11 trello card
For RemoveConditionalAccess needs some companioning methods from roslyn (e.g. This one) - this can be a standalone GH issue
For Discards not handled properly - what does this mean? Does it generate FPs or FNs? This should be an "improvement" issue or an FP card in our GitHub backlog.
github-actions
bot
assigned andrei-epure-sonarsource and unassigned martin-strecker-sonarsource
analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StreamReadStatement.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@martin-strecker-sonarsource do I understand well that this only adds tests? I am confused by the title of the PR
That's right. See this comment why that's the case: |
analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs
Outdated
Show resolved
Hide resolved
github-actions
bot
assigned martin-strecker-sonarsource and unassigned andrei-epure-sonarsource
martin-strecker-sonarsource
changed the title
|
@andrei-epure-sonarsource This rule is about the |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Kudos, SonarCloud Quality Gate passed! |
github-actions
bot
assigned andrei-epure-sonarsource and unassigned martin-strecker-sonarsource
@martin-strecker-sonarsource please wait, let's separate concerns and do a sprint focused on that. |
github-actions
bot
assigned martin-strecker-sonarsource and unassigned andrei-epure-sonarsource
| [DataRow("A.B?.C.M()", ".C.M()")] | ||
| [DataRow("A.B?.C?.M()", ".M()")] | ||
| [DataRow("A.B?.C?.D", ".D")] | ||
| public void RemoveConditionalAccess_SimpleInvocation_CS(string invocation, string expected) => |
There was a problem hiding this comment.
[education] @martin-strecker-sonarsource what's the usefulness of this method, what's a scenario when this is useful?
There was a problem hiding this comment.
ConditionalAccess messes up the syntax tree because of left/right associativity of the . operator vs. the ?. operator (I don't remember the details, but there is a 200+ thread on csharplang discussing the pros and cons). Do get an idea about the complexity involved you may want to have a look at:
This helper method simplifies any invocation to get just the "simple" part without the conditionals messing up the syntax tree. It is easier to get the Method symbol if the tree is cleaned up.
What we should actually do is to use GetParentConditionalAccessExpression of Roslyn, but I did not have looked whether this (and some other related helpers) are already present in our code base.
No description provided.