Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix missing 'equivalenceKey' warning for code fixes #7554

Merged
merged 16 commits into from
Jul 14, 2023
Merged

Fix missing 'equivalenceKey' warning for code fixes #7554

merged 16 commits into from
Jul 14, 2023

Conversation

csaba-sagi-sonarsource
Copy link
Contributor

Fixes #3364

@csaba-sagi-sonarsource csaba-sagi-sonarsource marked this pull request as draft July 7, 2023 12:25
@csaba-sagi-sonarsource
Copy link
Contributor Author

RedundantDeclarationCodeFix needed a bigger refactoring to adapt it to the new registration.
While RedundantModifierCodeFix tests needed to be split to be able to assert per Title runs.

martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round.

I don't like the namespace choice for the SonarContext. It should be changed.

analyzers/src/SonarAnalyzer.Common/CodeFixContext/SonarCodeFixContext.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/CodeFixContext/SonarCodeFixContext.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Common/FixAllProviders/DocumentBasedFixAllProvider.cs
@@ -184,7 +184,7 @@ private static SyntaxNodeOrToken GetReportedElement(Diagnostic diagnostic, Synta
private static SyntaxNode RemoveAnnotationIfExists(SyntaxNode root, SyntaxAnnotation annotation)
{
var element = root.GetAnnotatedNodesAndTokens(annotation).FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit odd that we do not loop here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and I would not do the change in this PR, this one is already huge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you make a note or an issue? Also, #7554 (comment) might be worth keeping documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just opened 3 rules for them: #7608, #7609 and #7610.

analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs
forStatement,
forStatement.WithCondition(null).WithAdditionalAnnotations(Formatter.Annotation));

return Task.FromResult(context.Document.WithSyntaxRoot(newRoot));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe as a follow up PR:

  • All code fixer ignore the CancelationToken (as far as I can tell)
  • All code fixer do return Task.FromResult(context.Document.WithSyntaxRoot(newRoot));

We may add an overload for RegisterCodeFix

        public void RegisterCodeFix(string title, Func<SyntaxNode>> createNewRoot, ImmutableArray<Diagnostic> diagnostics) =>
context.RegisterCodeFix(CodeAction.Create(title, c => Task.FromResult(context.Document.WithSyntaxRoot(createNewRoot())), title), diagnostics);

Note: There might be problems with the lambda capture of context.

This call would be simplified to

                () => root.ReplaceNode(
                        forStatement,
                        forStatement.WithCondition(null).WithAdditionalAnnotations(Formatter.Annotation));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I finish the sprint a bit earlier I will create a PR for it.

analyzers/src/SonarAnalyzer.CSharp/Rules/GetHashCodeMutableCodeFix.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/UnchangedLocalVariablesShouldBeConstCodeFix.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/CodeFixVerifier.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There is one code smell about to ignore. I don't see how equivalence for the context is relevant.

@csaba-sagi-sonarsource csaba-sagi-sonarsource marked this pull request as ready for review July 14, 2023 07:28
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpicks in the UTs. Otherwise good.

analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeFixContextTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeFixContextTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeFixContextTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeFixContextTest.cs Outdated Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeFixContextTest.cs Outdated Show resolved Hide resolved
...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated Show resolved Hide resolved
...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated Show resolved Hide resolved
...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated Show resolved Hide resolved
...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated Show resolved Hide resolved
...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some optional suggestions. LGTM.

analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeFixContextTest.cs Outdated
var literal = syntaxTree.GetRoot().DescendantNodesAndSelf().OfType<LiteralExpressionSyntax>().Single();
var cancellationToken = new CancellationToken(true);
var diagnostic = Diagnostic.Create(new DiagnosticDescriptor("1", "title", "format", "category", DiagnosticSeverity.Hidden, false), literal.GetLocation());
var sonarCodefix = new SonarCodeFixContext(new CodeFixContext(document, diagnostic, (_, _) => Console.WriteLine("Hello world"), cancellationToken));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var sonarCodefix = new SonarCodeFixContext(new CodeFixContext(document, diagnostic, (_, _) => Console.WriteLine("Hello world"), cancellationToken));
var sonarCodefix = new SonarCodeFixContext(new CodeFixContext(document, diagnostic, (_, _) => { }, cancellationToken));

...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated
public async Task GetFixAsync_ForSupportedScope_HasCorrectTitle(FixAllScope scope, string expectedTitle)
{
var codeFix = new DummyCodeFixCS();
var document = CreateProject().FindDocument(Path.GetFileName("MyFile1.cs"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some more Path.GetFileName in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was sure I removed all. Sorry.

...yzers/tests/SonarAnalyzer.UnitTest/Common/FixAllProviders/DocumentBasedFixAllProviderTest.cs Outdated
Comment on lines 121 to 122
.AddSnippet(@"int number1 = 1;", "MyFile1.cs")
.AddSnippet(@"int number2 = 2;", "MyFile2.cs");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two files with top-level statements don't compile, right? You may want to fix this.

@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

92.2% 92.2% Coverage
0.1% 0.1% Duplication

@csaba-sagi-sonarsource csaba-sagi-sonarsource merged commit 4a09955 into master Jul 14, 2023
@csaba-sagi-sonarsource csaba-sagi-sonarsource deleted the čaba/SonarCodeFixContext branch July 14, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavel-mikula-sonarsource pavel-mikula-sonarsource pavel-mikula-sonarsource left review comments

@martin-strecker-sonarsource martin-strecker-sonarsource martin-strecker-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix missing 'equivalenceKey' warning for code fixes
3 participants
@csaba-sagi-sonarsource @pavel-mikula-sonarsource @martin-strecker-sonarsource