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.

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

S3900: Fix issue message for constructors #7018

Merged

Conversation

zsolt-kolbay-sonarsource
Copy link
Contributor

Task 12 of #6997

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource force-pushed the Zsolt/S3900-fix-constructor-message branch from 37fcc37 to 0b3df44 Compare March 31, 2023 13:30
Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

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

LGTM

As discussed offline, I wonder whether this change is really needed.
I understand that in principle this is required to align the new implementation to the old one, which used to emit a different message for the specific scenario where the method parameter dereference is done in a this constructor initializer, or in a base constructor initialized.

However, this change requires using the syntactic model in the implementation of the rule, coupling (more) the rule to the underlying language, whereas the previous check was at symbolic level and it was quite clean (SemanticModel.GetDeclaredSymbol(Node).IsConstructor). Moreover the new implementation requires navigating the AST upwards, which is not ideal.

So I would challenge the need for this change.

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.

LGTM, please address the comments before merging

Comment on lines 92 to 93
static bool IsUsedInBaseConstructorCall(SyntaxNode node) =>
node.FirstAncestorOrSelf<ConstructorInitializerSyntax>() != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This simple single-line condition is not worth extracting. Embed it on the call site. It reads pretty fluently.

Comment on lines 92 to 93
static bool IsUsedInBaseConstructorCall(SyntaxNode node) =>
node.FirstAncestorOrSelf<ConstructorInitializerSyntax>() != null;
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 bool IsUsedInBaseConstructorCall(SyntaxNode node) =>
node.FirstAncestorOrSelf<ConstructorInitializerSyntax>() != null;
static bool IsUsedInBaseConstructorCall(SyntaxNode node) =>
node.FirstAncestorOrSelf<ConstructorInitializerSyntax>() is not null;

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 3, 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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 3, 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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource merged commit 181f359 into feature/SE Apr 3, 2023
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource deleted the Zsolt/S3900-fix-constructor-message branch April 3, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants