New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix S3415 FP/FN: Support named arguments #6759
Conversation
eb1bb6e
to
dfb9b66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the duality of
public IMethodSymbol MethodSymbol { get; }
private SymbolInfo? MethodSymbolInfo { get; }
in the class. Let's discuss if we can get rid of it.
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs
Outdated
Show resolved
Hide resolved
...rs/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs
Outdated
Show resolved
Hide resolved
...rs/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs
Outdated
Show resolved
Hide resolved
...s/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Most comments are cosmetic.
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs
Outdated
Show resolved
Hide resolved
...s/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes only.
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpObjectCreationTracker.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Extensions/SymbolInfoExtensions.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
caadb17
to
3d46ab2
Compare
analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs
Outdated
Show resolved
Hide resolved
...rs/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs
Outdated
Show resolved
Hide resolved
3d46ab2
to
02c2f22
Compare
02c2f22
to
393537c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Tim-Pohlmann There is a test failing (possibly due to the change in the MathodLookup class: |
One final thing: there are 10 code smells. Can you please have a look? A few of them seem to be valid. |
@martin-strecker-sonarsource I fixed one (even though I personally prefer switch over ternary operators) but the others seem to be false positives:
|
…mbolInfo.CandidateSymbols
Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
Co-authored-by: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com>
f664566
to
9dc1928
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo that should be fixed.
KnownAssertParameters knownParameters) => | ||
container.Is(knownParameters.AssertClass) | ||
&& CSharpFacade.Instance.MethodParameterLookup(argumentList, symbol) is var parameterLookup | ||
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParamterName, out var expectedArguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParamterName, out var expectedArguments) | |
&& parameterLookup.TryGetSyntax(knownParameters.ExpectedParameterName, out var expectedArguments) |
&& semanticModel.GetConstantValue(expected).HasValue is false | ||
&& parameterLookup.TryGetSyntax(knownParameters.ActualParameterName, out var actualArguments) | ||
&& actualArguments.FirstOrDefault() is { } actual | ||
&& semanticModel.GetConstantValue(actual).HasValue is true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& semanticModel.GetConstantValue(actual).HasValue is true | |
&& semanticModel.GetConstantValue(actual).HasValue |
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #6630