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

Fix S1144 FP: types used through reflection #9406

Merged
merged 13 commits into from
Jun 13, 2024
2 changes: 2 additions & 0 deletions analyzers/rspec/cs/S1144.html
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

Large diffs are not rendered by default.

37 changes: 26 additions & 11 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs
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))
martin-strecker-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
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
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
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