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

Change .Verify to fail when no issues are reported #9250

Merged
merged 6 commits into from
May 14, 2024

Conversation

Tim-Pohlmann
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann commented May 3, 2024

Fixes #9237
Based on #9259

This branch has two commits:

  1. Change Verifier to raise on errors: This is the functional change and directly related UTs. Review carefully!
  2. Update UTs: This updates a lot of rule UTs by doing one of the following actions:
    1. Change Verify to VerifyNoIssues. Sometimes with a comment, sometimes the reason is clear from context.
    2. Change Verify to VerifyNoIssuesIgnoreErrors. Pay special attention to these, because they can, if used wrongly, hide problems.
    3. Add a Noncompliant line to the test case.
    4. In rare circumstances I had to fix the test case.
    5. Everything that does not fall in one of these categories is CaYC (in addition to one of the points above).

Side note: Ignore the branch names. They are confusing, unfortunately.

@Tim-Pohlmann Tim-Pohlmann changed the base branch from master to Tim/VerifyNoIssues May 8, 2024 07:00
@Tim-Pohlmann Tim-Pohlmann marked this pull request as draft May 8, 2024 07:01
Base automatically changed from Tim/VerifyNoIssues to master May 8, 2024 12:37
@Tim-Pohlmann Tim-Pohlmann marked this pull request as ready for review May 8, 2024 12:51
Copy link
Contributor

@CristianAmbrosini CristianAmbrosini left a comment

Choose a reason for hiding this comment

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

Wow that was a lot, left some comments (some are nitpicks!)

@@ -27,21 +27,20 @@ public static class DiagnosticVerifier
private const string AD0001 = nameof(AD0001);
private const string LineContinuationVB12 = "BC36716"; // Visual Basic 12.0 does not support line continuation comments.

public static void Verify(
public static int Verify(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please align the parameters consistently across the file, either placing Compilation compilation inlined with the method declaration or changing the other methods

Comment on lines -38 to -44
public static void Verify(
Compilation compilation,
DiagnosticAnalyzer[] analyzers,
CompilationErrorBehavior checkMode, // ToDo: Remove this parameter in https://github.com/SonarSource/sonar-dotnet/issues/8588
string additionalFilePath = null,
string[] onlyDiagnostics = null,
string[] additionalSourceFiles = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer the old parameter alignment tho (opinionated)

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 agree, I was simply following our coding style.

Comment on lines 7 to 8
int a;
(var shortSalt, a) = (new byte[15], 42);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it actually require C# 10 (mixing assignment and declaration in deconstruction). However, since I removed the comment in the test file, this change does not need to be in this PR. I'll revert it.

Comment on lines 11 to 12
int a;
(var rgb, a) = (new byte[16], 42);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the other. Reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see it 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

What is this file about? if it's just adding more UTs for SE rules please cherry-pick the changes and move them into a different PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added by accident.

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 file about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Live Unit Testing. I committed it by accident.

@Tim-Pohlmann Tim-Pohlmann force-pushed the Tim/VerifiyNoIssuesReported branch 3 times, most recently from 16a8b88 to 8f4f5f6 Compare May 13, 2024 15:01
Copy link
Contributor

@CristianAmbrosini CristianAmbrosini left a comment

Choose a reason for hiding this comment

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

Nice! Only nitpicks left

{
public void Foo<T>(Action action1, Action<T> action2, Action<T, T> action3, Action<T, T, T> action4, Action<T, T, T, T> action5) { }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line

{
public void Foo<T>(Func<T> func1, Func<T, T> func2, Func<T, T, T> func3, Func<T, T, T, T> func4) { }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line

{
public void Foo(int* pointer) { } // Error [CS0214]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line

{
public ConsoleColor Foo(ConsoleColor c) { return ConsoleColor.Black; }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Empty line

@@ -44,7 +44,8 @@ public class CallerInformationParametersShouldBeLastTest

[TestMethod]
public void CallerInformationParametersShouldBeLast_CSharp11() =>
builder.AddPaths("CallerInformationParametersShouldBeLast.CSharp11.cs").WithOptions(ParseOptionsHelper.FromCSharp11).Verify();
builder.AddPaths("CallerInformationParametersShouldBeLast.CSharp11.cs").WithOptions(ParseOptionsHelper.FromCSharp11)
.VerifyNoIssues(); // overriding an abstract default implementation is 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 was not solved although it's marked with 👍

@@ -10,7 +10,8 @@ public class C
// If the field is not initialized and nullable is set to enabled then
// there's a warning "CS8618: Non-nullable property 'MyProperty' must contain a non-null value when exiting constructor. Consider declaring the property as nullable."
// To silence the warning one needs to use a field initializer and the suppression operator. We should not raise and issue in the case ! is present.
public object MyProperty { get; set; } = default!; // Compliant
public object Noncompliant { get; set; } = default; // FN
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you asking if there is a ticket for this? I did not create one. The rule is non-sonarway - I don't think it's worth tracking this.

@@ -1,8 +1,10 @@
class SomeClass(int arg)
{
public void RandomMethod1() { }
public void RandomMethod1() { } // Noncompliant
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this compliant before? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there was no other overload for RandomMethod1 present.

interface Foo
{
[DllImport("mynativelib")]
extern public static void Bar(string s, int x); // FN
Copy link
Contributor

Choose a reason for hiding this comment

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

Documented somewhere? aka GH issue

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 don't know. I did not write the test. I just merged it from the .CSharp10 file.

Comment on lines 11 to 12
int a;
(var rgb, a) = (new byte[16], 42);
Copy link
Contributor

Choose a reason for hiding this comment

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

I still see it 🤔

Comment on lines 11 to 12
int a;
(var rgb, a) = (new byte[16], 42);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@Tim-Pohlmann Tim-Pohlmann enabled auto-merge (squash) May 14, 2024 15:30
Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

Copy link

sonarcloud bot commented May 14, 2024

@Tim-Pohlmann Tim-Pohlmann merged commit 2081bf4 into master May 14, 2024
26 checks passed
@Tim-Pohlmann Tim-Pohlmann deleted the Tim/VerifiyNoIssuesReported branch May 14, 2024 15:47
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.

Change .Verify to fail when no issues are reported
3 participants