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

New rule S4663: Comments should not be empty #6672

Merged
merged 27 commits into from Feb 3, 2023
Merged

New rule S4663: Comments should not be empty #6672

merged 27 commits into from Feb 3, 2023

Conversation

gregory-paidis-sonarsource
Copy link
Contributor

Fixes #6663

@gregory-paidis-sonarsource gregory-paidis-sonarsource marked this pull request as ready for review January 27, 2023 09:18
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jan 27, 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.

Good approach. I think Partition can be made a bit more readable and we need to be careful to allocate not too much. Some pieces need to be shifted around between the base class and the derived classes.

analyzers/src/SonarAnalyzer.CSharp/Rules/CommentsShouldNotBeEmpty.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/CommentsShouldNotBeEmpty.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/CommentsShouldNotBeEmpty.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/CommentsShouldNotBeEmpty.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/CommentsShouldNotBeEmpty.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Rules/CommentsShouldNotBeEmpty.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jan 30, 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 annotations.

analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jan 30, 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.

Nice. The IsKind/IsAnyKind approach looks good.

analyzers/src/SonarAnalyzer.Common/Facade/ISyntaxKindFacade.cs Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated Show resolved Hide resolved
martin-strecker-sonarsource
martin-strecker-sonarsource requested changes Jan 31, 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 cosmetic changes and we should be fine.

analyzers/its/expected/Ember-MM/EmberAPI-{208AA35E-C6AE-4D2D-A9DD-B6EFD19A4279}-S4663.json Show resolved Hide resolved
...rs/its/expected/Ember-MM/scraper.EmberCore-{EF6A550E-DD76-4F4D-8250-8598140F828B}-S4663.json Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated
Comment on lines 42 to 43
CheckTrivia(c, token.LeadingTrivia);
CheckTrivia(c, token.TrailingTrivia);
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
CheckTrivia(c, token.LeadingTrivia);
CheckTrivia(c, token.TrailingTrivia);
// Hotpath: Don't allocate the trivia enumerable if not needed
if (token.HasLeadingTrivia)
{
CheckTrivia(c, token.LeadingTrivia);
}
if (token.HasTrailingTrivia)
{
CheckTrivia(c, token.TrailingTrivia);
}

analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated

foreach (var trivium in trivia)
{
if (IsSimpleComment(trivium)) // put it on the current block of "//"
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name AddTriviaToPartition says it already.

Suggested change
if (IsSimpleComment(trivium)) // put it on the current block of "//"
if (IsSimpleComment(trivium))

analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated
if (IsSimpleComment(trivium)) // put it on the current block of "//"
{
AddTriviaToPartition(ref current, trivium);
firstEndOfLineFound = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

firstEndOfLineFound = false; can be moved to AddTriviaToPartition.

Suggested change
firstEndOfLineFound = false;

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/CommentsShouldNotBeEmpty.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/CommentsShouldNotBeEmpty.vb Show resolved Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated
CloseCurrentPartition(ref current, ref partitions, ref firstEndOfLineFound);
}
// This handles an empty line, for example:
// // some comment \n <- EOL found, set to true
Copy link
Contributor

Choose a reason for hiding this comment

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

firstEndOfLineFound should be mentioned once to make clear what the "true/false" refers to.

Suggested change
// // some comment \n <- EOL found, set to true
// // some comment \n <- EOL found, firstEndOfLineFound set to true

analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated
current.Add(trivia);
}

// Hotpath: Don't capture variables
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be on the first local function.

Suggested change
// Hotpath: Don't capture variables

analyzers/src/SonarAnalyzer.Common/Rules/CommentsShouldNotBeEmptyBase.cs Outdated
CloseCurrentPartition(ref current, ref partitions, ref firstEndOfLineFound);
return partitions;

static void AddTriviaToPartition(ref List<SyntaxTrivia> current, SyntaxTrivia trivia)
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
static void AddTriviaToPartition(ref List<SyntaxTrivia> current, SyntaxTrivia trivia)
// Hotpath: Don't capture variables.
static void AddTriviaToPartition(ref List<SyntaxTrivia> current, SyntaxTrivia trivia)

martin-strecker-sonarsource
martin-strecker-sonarsource approved these changes Jan 31, 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.

LGTM. But one small issue still to be resoved.

analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs Outdated
Comment on lines 64 to 65
public SyntaxKind RightShiftAssignmentStatement => SyntaxKind.RightShiftAssignmentExpression;
public SyntaxKind RightShiftExpression => SyntaxKind.RightShiftExpression;
public SyntaxKind RefKeyword => SyntaxKind.RefKeyword;
public SyntaxKind ReturnStatement => SyntaxKind.ReturnStatement;
Copy link
Contributor

Choose a reason for hiding this comment

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

The order is mixed up.

@sonarcloud
Copy link

sonarcloud bot commented Feb 2, 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 Feb 2, 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 9 Code Smells

97.4% 97.4% Coverage
0.6% 0.6% Duplication

@gregory-paidis-sonarsource gregory-paidis-sonarsource merged commit 81d0d19 into master Feb 3, 2023
@gregory-paidis-sonarsource gregory-paidis-sonarsource deleted the greg/implement-S4663 branch February 3, 2023 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

New rule S4663: Comments should not be empty
2 participants
@gregory-paidis-sonarsource @martin-strecker-sonarsource