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 S4200 FN: Add support for LibraryImportAttribute; run only on non-generated code #6603

Merged
merged 11 commits into from
Jan 17, 2023
Merged

Fix S4200 FN: Add support for LibraryImportAttribute; run only on non-generated code #6603

merged 11 commits into from
Jan 17, 2023

Conversation

martin-strecker-sonarsource
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource commented Jan 5, 2023 •
edited
Loading

Part of #6488
Related to #6476

Rule S4200 should support LibraryImportAttribute.

@martin-strecker-sonarsource martin-strecker-sonarsource mentioned this pull request Jan 5, 2023
Closed
5 tasks
@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review January 5, 2023 14:29
andrei-epure-sonarsource
andrei-epure-sonarsource requested changes Jan 9, 2023
View reviewed changes
Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

This introduces a difference in the existing behavior of the rule --> please update the PR title description, as this will show up in the github history

analyzers/src/SonarAnalyzer.CSharp/Rules/NativeMethodsShouldBeWrapped.cs
@@ -34,20 +34,28 @@ public sealed class NativeMethodsShouldBeWrapped : SonarDiagnosticAnalyzer
protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterSymbolAction(ReportPublicExternalMethods, SymbolKind.Method);
context.RegisterSyntaxNodeAction(ReportTrivialWrappers, SyntaxKind.MethodDeclaration);
context.RegisterSyntaxNodeActionInNonGenerated(ReportTrivialWrappers, SyntaxKind.MethodDeclaration);
Copy link
Contributor

Choose a reason for hiding this comment

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

[education] why is this necessary to not be raised on generated code?

I get why it's necessary for the case of LibraryImport (to not raise the issue twice + users don't control the source generator) - is it the case that in general we should not raise on generated code ever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It was requested by Pavel at one point that we should fix this here. I added it as item 4 on the todo list here #6488 when he suggested it. It is also needed to properly support the LibraryImport attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

the PR name should reflect that IMO because it's a change of functionality for the whole rule and it's not only related to adding support for a new attribute

@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/S4200_LibraryImportAttribute branch from 8f42977 to eaa35c1 Compare January 16, 2023 08:38
@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/S4200_LibraryImportAttribute branch from eaa35c1 to 88283c0 Compare January 16, 2023 09:44
@sonarqubecloud
Copy link

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

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

andrei-epure-sonarsource
andrei-epure-sonarsource approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM, I will update the PR name and put in the message the info about non generated detection change of functionality

analyzers/src/SonarAnalyzer.CSharp/Rules/NativeMethodsShouldBeWrapped.cs
@@ -34,20 +34,28 @@ public sealed class NativeMethodsShouldBeWrapped : SonarDiagnosticAnalyzer
protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterSymbolAction(ReportPublicExternalMethods, SymbolKind.Method);
context.RegisterSyntaxNodeAction(ReportTrivialWrappers, SyntaxKind.MethodDeclaration);
context.RegisterSyntaxNodeActionInNonGenerated(ReportTrivialWrappers, SyntaxKind.MethodDeclaration);
Copy link
Contributor

Choose a reason for hiding this comment

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

the PR name should reflect that IMO because it's a change of functionality for the whole rule and it's not only related to adding support for a new attribute

@andrei-epure-sonarsource andrei-epure-sonarsource changed the title Fix S4200 FN: Add support for LibraryImportAttribute Fix S4200 FN: Add support for LibraryImportAttribute; run only on non-generated code Jan 17, 2023
@andrei-epure-sonarsource andrei-epure-sonarsource merged commit fb0685b into master Jan 17, 2023
@andrei-epure-sonarsource andrei-epure-sonarsource deleted the Martin/S4200_LibraryImportAttribute branch January 17, 2023 09:37
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource modified the milestone: 8.52 Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrei-epure-sonarsource andrei-epure-sonarsource andrei-epure-sonarsource approved these changes

Assignees

@martin-strecker-sonarsource martin-strecker-sonarsource

Labels
Area: C# C# rules related issues.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@martin-strecker-sonarsource @andrei-epure-sonarsource @zsolt-kolbay-sonarsource