Skip to content

Fix S1144: Nested type constructor accessibility is wrong in the rule message #9108

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

Merged
CristianAmbrosini merged 7 commits into from
Apr 17, 2024
Merged

Fix S1144: Nested type constructor accessibility is wrong in the rule message #9108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e35794e
Fix S1144: Nested type constructor accessibility is wrong in the rule…
CristianAmbrosini Apr 16, 2024
c381eb4
Fix UTs
CristianAmbrosini Apr 16, 2024
2b1dece
Update ITs expected issues
CristianAmbrosini Apr 16, 2024
f485839
Fix ITs
CristianAmbrosini Apr 17, 2024
150483f
Fix missing uri
CristianAmbrosini Apr 17, 2024
8b600c6
Add compliant internal class test
CristianAmbrosini Apr 17, 2024
50364d9
Address comment
CristianAmbrosini Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions analyzers/its/expected/CSharpLatest/S1144-CSharpLatest-net8.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027Record\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S3603.cs#L7",
"Location": "Line 7 Position 5-11"
"Message": "Remove unused constructor of private type \u0027Record\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S3603.cs#L7-L17",
"Location": "Lines 7-17 Position 5-6"
},
{
"Id": "S1144",
Expand Down
18 changes: 9 additions & 9 deletions analyzers/its/expected/akka.net/S1144-Akka-netstandard2.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027FactoryConsumer\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Actor/Props.cs#L687",
"Location": "Line 687 Position 20-35"
"Message": "Remove unused constructor of private type \u0027FactoryConsumer\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Actor/Props.cs#L687-L690",
"Location": "Lines 687-690 Position 13-14"
},
{
"Id": "S1144",
Expand Down Expand Up @@ -68,15 +68,15 @@
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027UpdatePendingWriteAndThen\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/IO/TcpConnection.cs#L892",
"Location": "Line 892 Position 20-45"
"Message": "Remove unused constructor of private type \u0027UpdatePendingWriteAndThen\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/IO/TcpConnection.cs#L892-L896",
"Location": "Lines 892-896 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027WriteFileFailed\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/IO/TcpConnection.cs#L903",
"Location": "Line 903 Position 20-35"
"Message": "Remove unused constructor of private type \u0027WriteFileFailed\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/IO/TcpConnection.cs#L903-L906",
"Location": "Lines 903-906 Position 13-14"
},
{
"Id": "S1144",
Expand Down
18 changes: 9 additions & 9 deletions analyzers/its/expected/akka.net/S1144-Akka.Benchmarks-netcoreapp3.1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027Parent\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/benchmark/Akka.Benchmarks/Actor/SpawnActorBenchmarks.cs#L72",
"Location": "Line 72 Position 20-26"
"Message": "Remove unused constructor of private type \u0027Parent\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/benchmark/Akka.Benchmarks/Actor/SpawnActorBenchmarks.cs#L72-L91",
"Location": "Lines 72-91 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027Child\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/benchmark/Akka.Benchmarks/Actor/SpawnActorBenchmarks.cs#L97",
"Location": "Line 97 Position 20-25"
"Message": "Remove unused constructor of private type \u0027Child\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/benchmark/Akka.Benchmarks/Actor/SpawnActorBenchmarks.cs#L97-L100",
"Location": "Lines 97-100 Position 13-14"
},
{
"Id": "S1144",
Expand All @@ -20,9 +20,9 @@
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027Echo\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/benchmark/Akka.Benchmarks/Serialization/SerializationBenchmarks.cs#L54",
"Location": "Line 54 Position 20-24"
"Message": "Remove unused constructor of private type \u0027Echo\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/benchmark/Akka.Benchmarks/Serialization/SerializationBenchmarks.cs#L54-L57",
"Location": "Lines 54-57 Position 13-14"
}
]
}
36 changes: 18 additions & 18 deletions analyzers/its/expected/akka.net/S1144-Akka.DI.TestKit-netstandard2.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,39 +2,39 @@
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027UnboundedStashActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L170",
"Location": "Line 170 Position 20-39"
"Message": "Remove unused constructor of private type \u0027UnboundedStashActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L170-L173",
"Location": "Lines 170-173 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027BoundedStashActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L182",
"Location": "Line 182 Position 20-37"
"Message": "Remove unused constructor of private type \u0027BoundedStashActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L182-L185",
"Location": "Lines 182-185 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027DiPerRequestActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L34",
"Location": "Line 34 Position 20-37"
"Message": "Remove unused constructor of private type \u0027DiPerRequestActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L34-L40",
"Location": "Lines 34-40 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027DiSingletonActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L47",
"Location": "Line 47 Position 20-36"
"Message": "Remove unused constructor of private type \u0027DiSingletonActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L47-L53",
"Location": "Lines 47-53 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027DiParentActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L62",
"Location": "Line 62 Position 20-33"
"Message": "Remove unused constructor of private type \u0027DiParentActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L62-L65",
"Location": "Lines 62-65 Position 13-14"
},
{
"Id": "S1144",
"Message": "Remove the unused private constructor \u0027DisposableActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L82",
"Location": "Line 82 Position 20-35"
"Message": "Remove unused constructor of private type \u0027DisposableActor\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/contrib/dependencyinjection/Akka.DI.TestKit/DiResolverSpec.cs#L82-L88",
"Location": "Lines 82-88 Position 13-14"
}
]
}
6 changes: 5 additions & 1 deletion analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ public sealed class UnusedPrivateMember : SonarDiagnosticAnalyzer
{
internal const string S1144DiagnosticId = "S1144";
private const string S1144MessageFormat = "Remove the unused {0} {1} '{2}'.";
private const string S1144MessageFormatForPublicCtor = "Remove unused constructor of {0} type '{1}'.";

private const string S4487DiagnosticId = "S4487";
private const string S4487MessageFormat = "Remove this unread {0} field '{1}' or refactor the code to use its value.";

private static readonly DiagnosticDescriptor RuleS1144 = DescriptorFactory.Create(S1144DiagnosticId, S1144MessageFormat);
private static readonly DiagnosticDescriptor RuleS1144ForPublicCtor = DescriptorFactory.Create(S1144DiagnosticId, S1144MessageFormatForPublicCtor);
private static readonly DiagnosticDescriptor RuleS4487 = DescriptorFactory.Create(S4487DiagnosticId, S4487MessageFormat);

private static readonly ImmutableArray<KnownType> IgnoredTypes = ImmutableArray.Create(
Expand Down Expand Up @@ -223,7 +225,9 @@ static IEnumerable<VariableDeclaratorSyntax> GetSiblingDeclarators(SyntaxNode va
};

Diagnostic CreateS1144Diagnostic(SyntaxNode syntaxNode, ISymbol symbol) =>
Diagnostic.Create(RuleS1144, GetIdentifierLocation(syntaxNode), accessibility, symbol.GetClassification(), GetMemberName(symbol));
symbol.IsConstructor() && !syntaxNode.GetModifiers().Any(SyntaxKind.PrivateKeyword)
? Diagnostic.Create(RuleS1144ForPublicCtor, syntaxNode.GetLocation(), accessibility, symbol.ContainingType.Name)
: Diagnostic.Create(RuleS1144, GetIdentifierLocation(syntaxNode), accessibility, symbol.GetClassification(), GetMemberName(symbol));

static Location GetIdentifierLocation(SyntaxNode syntaxNode) =>
syntaxNode.GetIdentifier() is { } identifier
Expand Down
19 changes: 19 additions & 0 deletions analyzers/tests/SonarAnalyzer.Test/Rules/UnusedPrivateMemberTest.Constructors.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,25 @@ public interface IInterface
}
").Verify();

[DataTestMethod]
[DataRow("private", "Remove the unused private constructor 'Foo'.")]
[DataRow("protected", "Remove unused constructor of private type 'Foo'.")]
[DataRow("internal", "Remove unused constructor of private type 'Foo'.")]
[DataRow("public", "Remove unused constructor of private type 'Foo'.")]
public void UnusedPrivateMember_NonPrivateConstructorInPrivateClass(string accessModifier, string expectedMessage) =>
builder.AddSnippet($$$"""
public class Some
{
private class Foo // Noncompliant
{
{{{accessModifier}}} Foo() // Noncompliant {{{{{expectedMessage}}}}}
{
var a = 1;
}
}
}
""").Verify();
Comment on lines +149 to +161
Copy link
Copy Markdown
Contributor

@Tim-Pohlmann Tim-Pohlmann Apr 17, 2024

Choose a reason for hiding this comment

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

The indentation can be improved.

Suggested change
public void UnusedPrivateMember_NonPrivateConstructorInPrivateClass(string accessModifier, string expectedMessage) =>
builder.AddSnippet($$$"""
public class Some
{
private class Foo // Noncompliant
{
{{{accessModifier}}} Foo() // Noncompliant {{{{{expectedMessage}}}}}
{
var a = 1;
}
}
}
""").Verify();
public void UnusedPrivateMember_NonPrivateConstructorInPrivateClass(string accessModifier, string expectedMessage) =>
builder.AddSnippet($$$"""
public class Some
{
private class Foo // Noncompliant
{
{{{accessModifier}}} Foo() // Noncompliant {{{{{expectedMessage}}}}}
{
var a = 1;
}
}
}
""").Verify();

Copy link
Copy Markdown
Contributor Author

@CristianAmbrosini CristianAmbrosini Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

All the test classes:

  • UnusedPrivateMemberTest.Constructors.cs
  • UnusedPrivateMemberTest.Fields.cs
  • UnusedPrivateMemberTest.Methods.cs
  • UnusedPrivateMemberTest.Properties.cs
  • UnusedPrivateMemberTest.Types.cs

are following this indentation, I don't like it either, but I like it even less refactoring all the above :D.
Given this context, are you ok with merging as is?

Copy link
Copy Markdown
Contributor

@Tim-Pohlmann Tim-Pohlmann Apr 17, 2024

Choose a reason for hiding this comment

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

There are three options:

  1. Leave as is
  2. Refactor everything
  3. Clean as you code

I'd go with 3, but if you think it's too much of a sore on the eye, sticking with 1 is also fine.


#if NET

[TestMethod]
Expand Down
12 changes: 12 additions & 0 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.Batch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,18 @@ public void M39()
}
}

internal class MyClass
{
protected MyClass() // Compliant
{
var a = 1;
}
}

internal class MyClass2
{
}

public interface IPublicInterface { }
[Serializable]
public sealed class PublicClass : IPublicInterface
Expand Down
12 changes: 12 additions & 0 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,18 @@ public void M39()
}
}

internal class MyClass
{
protected MyClass() // Compliant
{
var a = 1;
}
}

internal class MyClass2
{
}

public interface IPublicInterface { }
[Serializable]
public sealed class PublicClass : IPublicInterface
Expand Down
18 changes: 17 additions & 1 deletion analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public Gen() : base(1)
Console.WriteLine();
}

public Gen(int i) : this() // Noncompliant {{Remove the unused private constructor 'Gen'.}}
public Gen(int i) : this() // Noncompliant {{Remove unused constructor of private type 'Gen'.}}
{
Console.WriteLine();
}
Expand Down Expand Up @@ -410,6 +410,22 @@ public void M39()
}
}

internal class MyClass
{
protected MyClass() // Compliant
{
var a = 1;
}
}
Comment on lines +413 to +419
Copy link
Copy Markdown
Contributor

@Tim-Pohlmann Tim-Pohlmann Apr 17, 2024

Choose a reason for hiding this comment

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

Good test! I actually meant one with a private constructor (should be Noncompliant with a slightly different message).

Copy link
Copy Markdown
Contributor Author

@CristianAmbrosini CristianAmbrosini Apr 17, 2024

Choose a reason for hiding this comment

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

The message will be the same (the default) as the first DataRow in UnusedPrivateMember_NonPrivateConstructorInPrivateClass: Remove the unused private constructor 'blabla'.
I'll add this as well anyway 👍

Copy link
Copy Markdown
Contributor

@Tim-Pohlmann Tim-Pohlmann Apr 17, 2024

Choose a reason for hiding this comment

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

You are absolutely right, of course. Still a good case to test!


internal class MyClass2
{
private MyClass2() // Noncompliant {{Remove the unused private constructor 'MyClass2'.}}
{
var a = 1;
}
}

public interface IPublicInterface { }
[Serializable]
public sealed class PublicClass : IPublicInterface
Expand Down