Fix S2190 FN: Support indexer, conversion operator and event declaration #8464
Conversation
pavel-mikula-sonarsource
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can you please increase the coverage and fix the 3 issues? |
|
I fixed the code smells and opened an issue for the rest of the FPs that remain in the file. Regarding the coverage I could not come up with any valid or invalid code snippet that would cover the cases, however I would rather leave it there as defensive coding than to create AD0001s if anything changes in Roslyn. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
| IIncrementOrDecrementOperationWrapper.FromOperation(operation).OperatorMethod, | ||
| OperationKindEx.Unary => | ||
| IUnaryOperationWrapper.FromOperation(operation).OperatorMethod, | ||
| OperationKindEx.Conversion=> |
There was a problem hiding this comment.
| OperationKindEx.Conversion=> | |
| OperationKindEx.Conversion => |
| public void CheckForNoExitIndexer(SonarSyntaxNodeReportingContext c, IndexerDeclarationSyntax indexer, IPropertySymbol propertySymbol) | ||
| { | ||
| // SonarCFG is out of support | ||
| } |
There was a problem hiding this comment.
I think these two methods can/should be tested by adding two compliant event/indexers UTs in InfiniteRecursion.SonarCfg test file
| OperationKindEx.Binary => | ||
| IBinaryOperationWrapper.FromOperation(operation).OperatorMethod, | ||
| OperationKindEx.Decrement => | ||
| IIncrementOrDecrementOperationWrapper.FromOperation(operation).OperatorMethod, |
There was a problem hiding this comment.
It's weird because this line is tested in InfiniteRecursion.CSharp11.cs through:
public class Decrement : IDecrementOperators<Decrement>
{
public static Decrement operator --(Decrement val) => --val; // Noncompliant
}same for Unary:
public class BitWise : IBitwiseOperators<BitWise, BitWise, BitWise>
{
public static BitWise operator ~(BitWise value) => ~value; // Noncompliant
public static BitWise operator &(BitWise left, BitWise right) => left & right; // Noncompliant
public static BitWise operator |(BitWise left, BitWise right) => left | right; // Noncompliant
public static BitWise operator ^(BitWise left, BitWise right) => left ^ right; // Noncompliant
}but the code coverage tool is not detecting those lines as covered.
| c => | ||
| { | ||
| var indexer = (IndexerDeclarationSyntax)c.Node; | ||
| if (c.SemanticModel.GetDeclaredSymbol(indexer) is { } indexerSymbol) |
There was a problem hiding this comment.
We are registering for a declaration syntax node, looking at the documentation, I don't see how c.SemanticModel.GetDeclaredSymbol(indexer) might return null. I think we can avoid this check entirely and simply run:
checker.CheckForNoExitIndexer(c, indexer, c.SemanticModel.GetDeclaredSymbol(indexer));
| c => | ||
| { | ||
| var eventDeclaration = (EventDeclarationSyntax)c.Node; | ||
| if (c.SemanticModel.GetDeclaredSymbol(eventDeclaration) is { } eventSymbol) |
There was a problem hiding this comment.
Same as above
| property.ExpressionBody, | ||
| property.AccessorList, | ||
| property.Identifier.GetLocation(), |
There was a problem hiding this comment.
What about reducing the number of parameters of CheckForNoExit by passing directly the MemberDeclarationSyntax, and then retrieving all those properties within the method?
private static void CheckForNoExit(SonarSyntaxNodeReportingContext c, MemberDeclarationSyntax declarationSyntax, string arrowExpressionMessageArg, string accessorMessageArg)
Fixes #6642
Fixes #6643
Fixes #6644