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

Implement S6326 for both C# and VB.NET #7345

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Implement S6326 for both C# and VB.NET #7345

wants to merge 8 commits into from

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Jun 7, 2023

The rule already exists for Java, and with the work done for S5856 it not that much work.

@Corniel Corniel mentioned this pull request Jun 7, 2023
Closed
3 tasks
@Corniel
Copy link
Contributor Author

Corniel commented Jun 25, 2023

@pavel-mikula-sonarsource Any ETA on this?

pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

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

Some improvements suggested

...src/SonarAnalyzer.Common/Rules/RegularExpressions/RegexShouldNotContainMultipleSpacesBase.cs Outdated
Comment on lines 35 to 52
protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(
Language.GeneratedCodeRecognizer,
c => Analyze(c, RegexContext.FromCtor(Language, c.SemanticModel, c.Node)),
Language.SyntaxKind.ObjectCreationExpressions);

context.RegisterNodeAction(
Language.GeneratedCodeRecognizer,
c => Analyze(c, RegexContext.FromMethod(Language, c.SemanticModel, c.Node)),
Language.SyntaxKind.InvocationExpression);

context.RegisterNodeAction(
Language.GeneratedCodeRecognizer,
c => Analyze(c, RegexContext.FromAttribute(Language, c.SemanticModel, c.Node)),
Language.SyntaxKind.Attribute);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole thing will repeat in every single Regex rule and will be hard to change for new APIs.
Please create a new abstract base class RegexBase or RegexAnalyzerBase that will override and seal Initialize and will provide abstract Analyze method to override by this, and the other rule.

Suggested change
protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterNodeAction(
Language.GeneratedCodeRecognizer,
c => Analyze(c, RegexContext.FromCtor(Language, c.SemanticModel, c.Node)),
Language.SyntaxKind.ObjectCreationExpressions);
context.RegisterNodeAction(
Language.GeneratedCodeRecognizer,
c => Analyze(c, RegexContext.FromMethod(Language, c.SemanticModel, c.Node)),
Language.SyntaxKind.InvocationExpression);
context.RegisterNodeAction(
Language.GeneratedCodeRecognizer,
c => Analyze(c, RegexContext.FromAttribute(Language, c.SemanticModel, c.Node)),
Language.SyntaxKind.Attribute);
}

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 fully agree. But I was only repeating myself once. And some people say you should only start to refactor when you repeat yourself twice. ;)

Nevertheless, the setup so far was with a base class in mind, I now added it.

...src/SonarAnalyzer.Common/Rules/RegularExpressions/RegexShouldNotContainMultipleSpacesBase.cs Outdated

private void Analyze(SonarSyntaxNodeReportingContext c, RegexContext context)
{
if (context?.Regex is { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick

Suggested change
if (context?.Regex is { }
if (context?.Regex is not null

...s/SonarAnalyzer.UnitTest/TestCases/RegularExpressions/RegexShouldNotContainMultipleSpaces.cs Outdated
@@ -0,0 +1,124 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a test case with regex comment (?# comment with spaces), mark it as FP and leave it like that

...s/SonarAnalyzer.UnitTest/TestCases/RegularExpressions/RegexShouldNotContainMultipleSpaces.cs Outdated

void Unknown(string unknown)
{
var regex = new NoRegex(unknown + "multiple white spaces"); // Compliant
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a FN, because no matter what the unknown part does, there are multiple spaces

...s/SonarAnalyzer.UnitTest/TestCases/RegularExpressions/RegexShouldNotContainMultipleSpaces.cs Outdated Show resolved Hide resolved
...s/SonarAnalyzer.UnitTest/TestCases/RegularExpressions/RegexShouldNotContainMultipleSpaces.cs Outdated
|| Regex.IsMatch(input, "ignore pattern white space", RegexOptions.IgnorePatternWhitespace);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's document other white-spacing characters? Like hardspace, no-break space and others from UnicodeCategory.SpaceSeparator?

See https://learn.microsoft.com/en-us/dotnet/api/system.char.iswhitespace?view=net-7.0#remarks

I guess 0x09, 0xA0, 0xD0 should not be checked.

analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs Outdated
@@ -44,3 +44,23 @@ public interface ILanguageFacade<TSyntaxKind> : ILanguageFacade
ISyntaxKindFacade<TSyntaxKind> SyntaxKind { get; }
ITrackerFacade<TSyntaxKind> Tracker { get; }
}

public static class LanguageFacadeExtensions
Copy link
Contributor

Choose a reason for hiding this comment

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

The concept of extensions of LangaugeFacade doesn't feel right. It's like dependencies going in both directions.

Why not change just is RegexOptions to is int and then cast it?

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 added (and rebased) a set of whitespaces that are fine in regexes.

@pavel-mikula-sonarsource
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 19, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@pavel-mikula-sonarsource
Copy link
Contributor

RegexShouldNotContainMultipleSpaces_CS UT is failing. Can you please take a look?

@Corniel
Copy link
Contributor Author

Corniel commented Mar 26, 2024

Failing test has been fixed.

@Corniel
Copy link
Contributor Author

Corniel commented Jul 31, 2024

Rebase done.

@Corniel
Copy link
Contributor Author

Corniel commented Aug 29, 2024

What is the status here?

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 requested changes

Assignees

@pavel-mikula-sonarsource pavel-mikula-sonarsource

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Corniel @pavel-mikula-sonarsource