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

.NET 8 and C# 12: add repros and UTs for rules starting with R in SonarWay #8093

Merged
merged 16 commits into from
Oct 2, 2023
Merged

.NET 8 and C# 12: add repros and UTs for rules starting with R in SonarWay #8093

merged 16 commits into from
Oct 2, 2023

Conversation

antonioaversa
Copy link
Contributor

@antonioaversa antonioaversa commented Sep 27, 2023 •
edited
Loading

Rules affected by .NET 8 and C# 12 for rules starting with R in SonarWay.

Adds repros:

Adds UTs with .NET 8 and C# 12 features for:

antonioaversa added a commit that referenced this pull request Sep 27, 2023
@antonioaversa
S3254: Add repro for #8093 and UTs for default lambda parameters
7ee6fd3
antonioaversa added a commit that referenced this pull request Sep 28, 2023
@antonioaversa
S3254: Add repro for #8093 and UTs for default lambda parameters
09d04b5
@antonioaversa antonioaversa force-pushed the antonio/dotnet8-cs12-add-repros-and-uts-r branch from 68bd2db to baed000 Compare September 28, 2023 07:19
@antonioaversa antonioaversa marked this pull request as ready for review September 28, 2023 07:21
antonioaversa added a commit that referenced this pull request Sep 28, 2023
@antonioaversa
S3254: Add repro for #8093 and UTs for default lambda parameters
d040019
@antonioaversa antonioaversa force-pushed the antonio/dotnet8-cs12-add-repros-and-uts-r branch from baed000 to 1806771 Compare September 28, 2023 08:36
cristian-ambrosini-sonarsource
cristian-ambrosini-sonarsource suggested changes Sep 28, 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.

Left some suggestions

...ts/SonarAnalyzer.UnitTest/TestCases/RedundancyInConstructorDestructorDeclaration.CSharp12.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/RedundantArgument.CSharp12.cs Outdated
Comment on lines 50 to 51
_ = new C2(41, 42); // FN, multiple violations
// ~~
Copy link
Contributor

Choose a reason for hiding this comment

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

Educational: what is this ~~ doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is supposed to be the equivalent of ^^^, but for a FN.
See here for example.

I have added it to test the multiple violations. When multiple violations are present on the same line, they can't be asserted with a single comment, but the ^^ has to be used. In the case of FN, I have used ~~ to indicate the location of each of the two False Negative cases.
However, I don't think there is a standard for that (because it doesn't really assert anything) and different people may use different notations.

Anyway, it was misplaced due to the vertical auto-alignment of comments, and that's also why it was hard to guess what those comments were about. I fixed the location.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting, I never encountered such a method to indicate the FN location. Not a strong opinion: I think we should decide whether we want to keep it and document it somewhere (as of now it's not mentioned in the test cases comment pattern), or avoid using it entirely. WDYT?

Copy link
Contributor Author

@antonioaversa antonioaversa Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not defined in the IssueLocationCollector because it's an FN location, so it cannot be asserted.
Moreover, it keeps being vertically aligned by Visual Studio to the // FN, multiple violations of the line above, and different people use different notations.
So I have removed it.

...zers/tests/SonarAnalyzer.UnitTest/TestCases/RedundantDeclaration.CSharp12.ArraySize.Fixed.cs Show resolved Hide resolved
Base automatically changed from antonio/dotnet8-cs12-add-repros-and-uts to master September 29, 2023 14:42
@antonioaversa antonioaversa force-pushed the antonio/dotnet8-cs12-add-repros-and-uts-r branch 2 times, most recently from 769ed49 to fe8d841 Compare September 29, 2023 15:40
@antonioaversa antonioaversa mentioned this pull request Sep 29, 2023
Open
@antonioaversa antonioaversa force-pushed the antonio/dotnet8-cs12-add-repros-and-uts-r branch from fe8d841 to 34c1322 Compare September 29, 2023 16:08
@antonioaversa
Copy link
Contributor Author

@cristian-ambrosini-sonarsource As discussed offline, this PR, as for its title and description, only contains rules starting with R.

The comments added in the second round of review are all on rules starting with P, which appear since Friday on this PR due to the automatic change of base made by GitHub when merging.

I have created a new Trello card and a new PR to deal with comments on rules starting by P.

I am going to recreate comments on the new PR, so that those can be resolved here so that this PR remains faithful to its title and description.

@antonioaversa antonioaversa mentioned this pull request Oct 2, 2023
Merged
@antonioaversa
Copy link
Contributor Author

@cristian-ambrosini-sonarsource I have referenced all the comments on rules starting with P in the new PR. Back to you for the validation of this PR.

cristian-ambrosini-sonarsource
cristian-ambrosini-sonarsource approved these changes Oct 2, 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!

...ts/SonarAnalyzer.UnitTest/TestCases/RedundancyInConstructorDestructorDeclaration.CSharp12.cs
record struct ARecordStructWithBody() { } // FN
record struct ARecordStructWithoutBody(); // FN

namespace FieldInitializerInStructRequiresConstructor
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: should we leave a comment here to give some context?
// https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/constructor-errors?f1url=%3FappId%3Droslyn%26k%3Dk(CS8983)#constructors-in-struct-types

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 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
0.0% 0.0% Duplication

@antonioaversa antonioaversa merged commit 23f21a6 into master Oct 2, 2023
@antonioaversa antonioaversa deleted the antonio/dotnet8-cs12-add-repros-and-uts-r branch October 2, 2023 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource approved these changes

Assignees

@antonioaversa antonioaversa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@antonioaversa @cristian-ambrosini-sonarsource