Skip to content

Commit

Permalink
Fix S1144 FP: types used through reflection (#9406)
Browse files Browse the repository at this point in the history 
Co-authored-by: Cristian Ambrosini <cristian.ambrosini@sonarsource.com>
  • Loading branch information
@zsolt-kolbay-sonarsource @CristianAmbrosini
zsolt-kolbay-sonarsource and CristianAmbrosini authored Jun 13, 2024
1 parent 24ec128 commit c485d21
Show file tree
Hide file tree
Showing 9 changed files with 791 additions and 616 deletions.
2 changes: 2 additions & 0 deletions analyzers/rspec/cs/S1144.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ <h3>Exceptions</h3>
href="https://learn.microsoft.com/en-us/dotnet/api/system.serializableattribute">System.SerializableAttribute</a> attribute </li>
<li> internal members in assemblies that have a <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.internalsvisibletoattribute">System.Runtime.CompilerServices.InternalsVisibleToAttribute</a> attribute. </li>
<li> types and members decorated with the <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.dynamicallyaccessedmembersattribute">System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute</a> attribute (available in .NET 5.0+) or a custom attribute named <code>DynamicallyAccessedMembersAttribute</code>. </li>
</ul>
<h2>Resources</h2>
<h3>Documentation</h3>
Expand Down
671 changes: 344 additions & 327 deletions analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSymbolUsageCollector.cs
View file

Large diffs are not rendered by default.

37 changes: 26 additions & 11 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,20 @@ private static void ReportUnusedPrivateMembers<TContext>(SonarCompilationReporti
ReportDiagnosticsForMembers(context, unusedSymbols, accessibility, fieldLikeSymbols);
}

private static bool IsUsedWithReflection(ISymbol symbol, HashSet<ISymbol> symbolsUsedWithReflection)
{
var currentSymbol = symbol;
while (currentSymbol is not null)
{
if (symbolsUsedWithReflection.Contains(currentSymbol))
{
return true;
}
currentSymbol = currentSymbol.ContainingSymbol;
}
return false;
}

private static bool IsMentionedInDebuggerDisplay(ISymbol symbol, CSharpSymbolUsageCollector usageCollector) =>
usageCollector.DebuggerDisplayValues.Any(x => x.Contains(symbol.Name));

Expand All @@ -161,7 +175,9 @@ private static void ReportUsedButUnreadFields(SonarSymbolReportingContext contex
var usedButUnreadFields = usageCollector.FieldSymbolUsages.Values
.Where(x => x.Symbol.DeclaredAccessibility == Accessibility.Private || x.Symbol.ContainingType?.DeclaredAccessibility == Accessibility.Private)
.Where(x => x.Symbol.Kind == SymbolKind.Field || x.Symbol.Kind == SymbolKind.Event)
.Where(x => !unusedSymbols.Contains(x.Symbol) && !IsMentionedInDebuggerDisplay(x.Symbol, usageCollector))
.Where(x => !unusedSymbols.Contains(x.Symbol)
&& !IsMentionedInDebuggerDisplay(x.Symbol, usageCollector)
&& !IsUsedWithReflection(x.Symbol, usageCollector.TypesUsedWithReflection))
.Where(x => x.Declaration is not null && !x.Readings.Any());

foreach (var usage in usedButUnreadFields)
Expand All @@ -173,7 +189,9 @@ private static void ReportUsedButUnreadFields(SonarSymbolReportingContext contex
private static HashSet<ISymbol> GetUnusedSymbols(CSharpSymbolUsageCollector usageCollector, IEnumerable<ISymbol> removableSymbols) =>
removableSymbols
.Except(usageCollector.UsedSymbols)
.Where(x => !IsMentionedInDebuggerDisplay(x, usageCollector) && !IsAccessorUsed(x, usageCollector))
.Where(x => !IsMentionedInDebuggerDisplay(x, usageCollector)
&& !IsAccessorUsed(x, usageCollector)
&& !IsUsedWithReflection(x, usageCollector.TypesUsedWithReflection))
.ToHashSet();

private static bool IsAccessorUsed(ISymbol symbol, CSharpSymbolUsageCollector usageCollector) =>
Expand Down Expand Up @@ -232,10 +250,10 @@ static IEnumerable<VariableDeclaratorSyntax> GetSiblingDeclarators(SyntaxNode va
_ => Enumerable.Empty<VariableDeclaratorSyntax>(),
};

static Location GetIdentifierLocation(SyntaxNode syntaxNode) =>
syntaxNode.GetIdentifier() is { } identifier
static Location GetIdentifierLocation(SyntaxNode node) =>
node.GetIdentifier() is { } identifier
? identifier.GetLocation()
: syntaxNode.GetLocation();
: node.GetLocation();
}

private static void ReportProperty<TContext>(SonarCompilationReportingContextBase<TContext> context,
Expand Down Expand Up @@ -278,12 +296,9 @@ bool IsGenerated(SyntaxReference syntaxReference) =>
private static CSharpRemovableSymbolWalker RetrieveRemovableSymbols(INamedTypeSymbol namedType, Compilation compilation, SonarSymbolReportingContext context)
{
var removableSymbolsCollector = new CSharpRemovableSymbolWalker(compilation.GetSemanticModel, namedType.DeclaredAccessibility);
if (!VisitDeclaringReferences(namedType, removableSymbolsCollector, context, includeGeneratedFile: false))
{
return null;
}

return removableSymbolsCollector;
return VisitDeclaringReferences(namedType, removableSymbolsCollector, context, includeGeneratedFile: false)
? removableSymbolsCollector
: null;
}

private static void CopyRetrievedSymbols(CSharpRemovableSymbolWalker removableSymbolCollector,
Expand Down
1 change: 1 addition & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ public sealed partial class KnownType
public static readonly KnownType System_DateTimeOffset = new("System.DateTimeOffset");
public static readonly KnownType System_Decimal = new("System.Decimal");
public static readonly KnownType System_Delegate = new("System.Delegate");
public static readonly KnownType System_Diagnostics_CodeAnalysis_DynamicallyAccessedMembersAttribute = new("System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute");
public static readonly KnownType System_Diagnostics_CodeAnalysis_ExcludeFromCodeCoverageAttribute = new("System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverageAttribute");
public static readonly KnownType System_Diagnostics_CodeAnalysis_SuppressMessageAttribute = new("System.Diagnostics.CodeAnalysis.SuppressMessageAttribute");
public static readonly KnownType System_Diagnostics_CodeAnalysis_StringSyntaxAttribute = new("System.Diagnostics.CodeAnalysis.StringSyntaxAttribute");
Expand Down
145 changes: 72 additions & 73 deletions analyzers/tests/SonarAnalyzer.Test/Rules/UnusedPrivateMemberTest.Constructors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Test.Rules
namespace SonarAnalyzer.Test.Rules;

public partial class UnusedPrivateMemberTest
{
public partial class UnusedPrivateMemberTest
{
[TestMethod]
public void UnusedPrivateMember_Constructor_Accessibility() =>
builder.AddSnippet(@"
[TestMethod]
public void UnusedPrivateMember_Constructor_Accessibility() =>
builder.AddSnippet(@"
public class PrivateConstructors
{
private PrivateConstructors(int i) { var x = 5; } // Noncompliant {{Remove the unused private constructor 'PrivateConstructors'.}}
Expand Down Expand Up @@ -62,40 +62,40 @@ public class InnerPublicClass
}
").Verify();

[TestMethod]
public void UnusedPrivateMember_Constructor_DirectReferences() =>
builder.AddSnippet("""
public abstract class PrivateConstructors
[TestMethod]
public void UnusedPrivateMember_Constructor_DirectReferences() =>
builder.AddSnippet("""
public abstract class PrivateConstructors
{
public class Constructor1
{
public static readonly Constructor1 Instance = new Constructor1();
private Constructor1() { var x = 5; }
}

public class Constructor2
{
public Constructor2(int a) { }
private Constructor2() { var x = 5; } // Compliant - FN
}

public class Constructor3
{
public Constructor3(int a) : this() { }
private Constructor3() { var x = 5; }
}

public class Constructor4
{
public class Constructor1
{
public static readonly Constructor1 Instance = new Constructor1();
private Constructor1() { var x = 5; }
}

public class Constructor2
{
public Constructor2(int a) { }
private Constructor2() { var x = 5; } // Compliant - FN
}

public class Constructor3
{
public Constructor3(int a) : this() { }
private Constructor3() { var x = 5; }
}

public class Constructor4
{
static Constructor4() { var x = 5; }
}
static Constructor4() { var x = 5; }
}
}

""").VerifyNoIssues();
""").VerifyNoIssues();

[TestMethod]
public void UnusedPrivateMember_Constructor_Inheritance() =>
builder.AddSnippet(@"
[TestMethod]
public void UnusedPrivateMember_Constructor_Inheritance() =>
builder.AddSnippet(@"
public class Inheritance
{
private abstract class BaseClass1
Expand All @@ -121,35 +121,35 @@ public DerivedClass2() { }
}
").Verify();

[TestMethod]
public void UnusedPrivateMember_Empty_Constructors() =>
builder.AddSnippet("""
public class PrivateConstructors
{
private PrivateConstructors(int i) { } // Compliant, empty ctors are reported from another rule
}
[TestMethod]
public void UnusedPrivateMember_Empty_Constructors() =>
builder.AddSnippet("""
public class PrivateConstructors
{
private PrivateConstructors(int i) { } // Compliant, empty ctors are reported from another rule
}

""").VerifyNoIssues();
""").VerifyNoIssues();

[TestMethod]
public void UnusedPrivateMember_Illegal_Interface_Constructor() =>
// While typing code in IDE, we can end up in a state where an interface has a constructor defined.
// Even though this results in a compiler error (CS0526), IDE will still trigger rules on the interface.
builder.AddSnippet(@"
[TestMethod]
public void UnusedPrivateMember_Illegal_Interface_Constructor() =>
// While typing code in IDE, we can end up in a state where an interface has a constructor defined.
// Even though this results in a compiler error (CS0526), IDE will still trigger rules on the interface.
builder.AddSnippet(@"
public interface IInterface
{
// UnusedPrivateMember rule does not trigger AD0001 error from NullReferenceException
IInterface() {} // Error [CS0526]
}
").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($$$"""
[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
Expand All @@ -164,32 +164,31 @@ private class Foo // Noncompliant

#if NET

[TestMethod]
public void UnusedPrivateMember_RecordPositionalConstructor() =>
builder.AddSnippet("""
// https://github.com/SonarSource/sonar-dotnet/issues/5381
public abstract record Foo
[TestMethod]
public void UnusedPrivateMember_RecordPositionalConstructor() =>
builder.AddSnippet("""
// https://github.com/SonarSource/sonar-dotnet/issues/5381
public abstract record Foo
{
Foo(string value)
{
Foo(string value)
{
Value = value;
}
Value = value;
}

public string Value { get; }
public string Value { get; }

public sealed record Bar(string Value) : Foo(Value);
}
""").WithOptions(ParseOptionsHelper.FromCSharp9).VerifyNoIssues();
public sealed record Bar(string Value) : Foo(Value);
}
""").WithOptions(ParseOptionsHelper.FromCSharp9).VerifyNoIssues();

[TestMethod]
public void UnusedPrivateMember_NonExistentRecordPositionalConstructor() =>
builder.AddSnippet(@"
[TestMethod]
public void UnusedPrivateMember_NonExistentRecordPositionalConstructor() =>
builder.AddSnippet(@"
public abstract record Foo
{
public sealed record Bar(string Value) : RandomRecord(Value); // Error [CS0246, CS1729] no suitable method found to override
}").WithOptions(ParseOptionsHelper.FromCSharp10).Verify();

#endif

}
}
Loading

0 comments on commit c485d21

Please sign in to comment.