Skip to content

Commit

Permalink
NET-989 S3444: Add secondary location message
Browse files Browse the repository at this point in the history
  • Loading branch information
@sebastien-marichal
sebastien-marichal authored and sonartech committed Jan 28, 2025
1 parent f9a456d commit caa1608
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 131 deletions.
239 changes: 127 additions & 112 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/InheritedCollidingInterfaceMembers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,150 +14,165 @@
* along with this program; if not, see https://sonarsource.com/license/ssal/
*/

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class InheritedCollidingInterfaceMembers : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S3444";
private const string MessageFormat = "Rename or add member{1} {0} to this interface to resolve ambiguities.";
private const int MaxMemberDisplayCount = 2;
private const int MinBaseListTypes = 2;
namespace SonarAnalyzer.Rules.CSharp;

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class InheritedCollidingInterfaceMembers : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S3444";
private const string MessageFormat = "Rename or add member{1} {0} to this interface to resolve ambiguities.";
private const string SecondaryMessageFormat = "This member collides with '{0}'";
private const int MaxMemberDisplayCount = 2;
private const int MinBaseListTypes = 2;

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
private static readonly ISet<SymbolDisplayPartKind> PartKindsToStartWith = new HashSet<SymbolDisplayPartKind>
{
SymbolDisplayPartKind.MethodName,
SymbolDisplayPartKind.PropertyName,
SymbolDisplayPartKind.EventName
};

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(
c =>
{
var interfaceDeclaration = (InterfaceDeclarationSyntax)c.Node;
if (interfaceDeclaration.BaseList == null || interfaceDeclaration.BaseList.Types.Count < MinBaseListTypes)
{
return;
}

var interfaceSymbol = c.Model.GetDeclaredSymbol(interfaceDeclaration);
if (interfaceSymbol == null)
{
return;
}

var collidingMembers = GetCollidingMembers(interfaceSymbol).Take(MaxMemberDisplayCount + 1).ToList();
if (collidingMembers.Any())
{
var membersText = GetIssueMessageText(collidingMembers, c.Model, interfaceDeclaration.SpanStart);
var pluralize = collidingMembers.Count > 1 ? "s" : string.Empty;
var secondaryLocations = collidingMembers.SelectMany(x => x.Locations).Where(x => x.IsInSource).ToSecondary();
c.ReportIssue(Rule, interfaceDeclaration.Identifier, secondaryLocations, membersText, pluralize);
}
},
SyntaxKind.InterfaceDeclaration);

private static IEnumerable<IMethodSymbol> GetCollidingMembers(ITypeSymbol interfaceSymbol)
{
var implementedInterfaces = interfaceSymbol.Interfaces;
private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);

var membersFromDerivedInterface = interfaceSymbol.GetMembers().OfType<IMethodSymbol>().ToList();
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

for (var i = 0; i < implementedInterfaces.Length; i++)
protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(
c =>
{
var notRedefinedMembersFromInterface = implementedInterfaces[i]
.GetMembers()
.OfType<IMethodSymbol>()
.Where(method => method.DeclaredAccessibility != Accessibility.Private
&& !membersFromDerivedInterface.Any(redefinedMember => AreCollidingMethods(method, redefinedMember)));
var interfaceDeclaration = (InterfaceDeclarationSyntax)c.Node;
if (interfaceDeclaration.BaseList is null || interfaceDeclaration.BaseList.Types.Count < MinBaseListTypes)
{
return;
}

var collidingMembers = notRedefinedMembersFromInterface.SelectMany(member => GetCollidingMembersForMember(member, implementedInterfaces.Skip(i + 1)));
var interfaceSymbol = c.Model.GetDeclaredSymbol(interfaceDeclaration);
if (interfaceSymbol is null)
{
return;
}

foreach (var collidingMember in collidingMembers)
var collidingMembers = GetCollidingMembers(interfaceSymbol).Take(MaxMemberDisplayCount + 1).ToList();
if (collidingMembers.Any())
{
yield return collidingMember;
var membersText = GetIssueMessageText(collidingMembers, c.Model, interfaceDeclaration.SpanStart);
var pluralize = collidingMembers.Count > 1 ? "s" : string.Empty;
c.ReportIssue(Rule, interfaceDeclaration.Identifier, SecondaryLocations(collidingMembers, c.Model), membersText, pluralize);
}
},
SyntaxKind.InterfaceDeclaration);

IEnumerable<IMethodSymbol> GetCollidingMembersForMember(IMethodSymbol member, IEnumerable<INamedTypeSymbol> interfaces) =>
interfaces.SelectMany(x => GetCollidingMembersForMemberAndInterface(member, x));
private static IEnumerable<SecondaryLocation> SecondaryLocations(List<CollidingMember> collidingMembers, SemanticModel model)
{
return collidingMembers
.SelectMany(x => x.Member.Locations.Select(l => new { Location = l, x.CollideWith, CollideWithSymbolName = CollideWithSymbolName(x) }))
.Where(x => x.Location.IsInSource)
.Select(x => x.Location.ToSecondary(x.CollideWithSymbolName is { Length: > 0 } ? SecondaryMessageFormat : null, x.CollideWithSymbolName));

string CollideWithSymbolName(CollidingMember collidingMember) =>
collidingMember.CollideWith.Locations.FirstOrDefault() is { } location
? GetMemberDisplayName(collidingMember.CollideWith, location.SourceSpan.Start, model, [SymbolDisplayPartKind.ClassName, SymbolDisplayPartKind.InterfaceName])
: string.Empty;
}

IEnumerable<IMethodSymbol> GetCollidingMembersForMemberAndInterface(IMethodSymbol member, INamedTypeSymbol interfaceToCheck) =>
interfaceToCheck
.GetMembers(member.Name)
.OfType<IMethodSymbol>()
.Where(IsNotEventRemoveAccessor)
.Where(x => AreCollidingMethods(member, x));
}
}
private static IEnumerable<CollidingMember> GetCollidingMembers(ITypeSymbol interfaceSymbol)
{
var implementedInterfaces = interfaceSymbol.Interfaces;

private static bool IsNotEventRemoveAccessor(IMethodSymbol methodSymbol) =>
// we only want to report on events once, so we are not collecting the "remove" accessors,
// and handle the "add" accessor reporting separately in <see cref="GetMemberDisplayName"/>
methodSymbol.MethodKind != MethodKind.EventRemove;
var membersFromDerivedInterface = interfaceSymbol.GetMembers().OfType<IMethodSymbol>().ToList();

private static string GetIssueMessageText(IEnumerable<IMethodSymbol> collidingMembers, SemanticModel semanticModel, int spanStart)
for (var i = 0; i < implementedInterfaces.Length; i++)
{
var names = collidingMembers.Take(MaxMemberDisplayCount)
.Select(member => GetMemberDisplayName(member, spanStart, semanticModel))
.Distinct()
.ToList();
var notRedefinedMembersFromInterface = implementedInterfaces[i]
.GetMembers()
.OfType<IMethodSymbol>()
.Where(x => x.DeclaredAccessibility != Accessibility.Private
&& !membersFromDerivedInterface.Any(redefinedMember => AreCollidingMethods(x, redefinedMember)));

var collidingMembers = notRedefinedMembersFromInterface.SelectMany(x => GetCollidingMembersForMember(x, implementedInterfaces.Skip(i + 1)));

return names.Count switch
foreach (var collidingMember in collidingMembers)
{
1 => names[0],
2 => $"{names[0]} and {names[1]}",
_ => names.JoinStr(", ") + ", ..."
};
yield return collidingMember;
}

IEnumerable<CollidingMember> GetCollidingMembersForMember(IMethodSymbol member, IEnumerable<INamedTypeSymbol> interfaces) =>
interfaces.SelectMany(x => GetCollidingMembersForMemberAndInterface(member, x));

IEnumerable<CollidingMember> GetCollidingMembersForMemberAndInterface(IMethodSymbol member, INamedTypeSymbol interfaceToCheck) =>
interfaceToCheck
.GetMembers(member.Name)
.OfType<IMethodSymbol>()
.Where(IsNotEventRemoveAccessor)
.Where(x => AreCollidingMethods(member, x))
.Select(x => new CollidingMember(x, member));
}
}

private static bool IsNotEventRemoveAccessor(IMethodSymbol methodSymbol) =>
// we only want to report on events once, so we are not collecting the "remove" accessors,
// and handle the "add" accessor reporting separately in <see cref="GetMemberDisplayName"/>
methodSymbol.MethodKind != MethodKind.EventRemove;

private static string GetIssueMessageText(IEnumerable<CollidingMember> collidingMembers, SemanticModel model, int spanStart)
{
var names = collidingMembers.Take(MaxMemberDisplayCount)
.Select(x => $"'{GetMemberDisplayName(x.Member, spanStart, model)}'")
.Distinct()
.ToList();

private static readonly ISet<SymbolDisplayPartKind> PartKindsToStartWith = new HashSet<SymbolDisplayPartKind>
return names.Count switch
{
SymbolDisplayPartKind.MethodName,
SymbolDisplayPartKind.PropertyName,
SymbolDisplayPartKind.EventName
1 => names[0],
2 => $"{names[0]} and {names[1]}",
_ => names.JoinStr(", ") + ", ..."
};
}

private static string GetMemberDisplayName(IMethodSymbol method, int spanStart, SemanticModel semanticModel)
private static string GetMemberDisplayName(IMethodSymbol method, int spanStart, SemanticModel model, HashSet<SymbolDisplayPartKind> additionalPartKindsToStartWith = null)
{
if (method.AssociatedSymbol is IPropertySymbol { IsIndexer: true } property)
{
if (method.AssociatedSymbol is IPropertySymbol { IsIndexer: true } property)
{
var text = property.ToMinimalDisplayString(semanticModel, spanStart, SymbolDisplayFormat.CSharpShortErrorMessageFormat);
return $"'{text}'";
}
var text = property.ToMinimalDisplayString(model, spanStart, SymbolDisplayFormat.CSharpShortErrorMessageFormat);
return $"{text}";
}

var parts = method.ToMinimalDisplayParts(semanticModel, spanStart, SymbolDisplayFormat.CSharpShortErrorMessageFormat)
.SkipWhile(part => !PartKindsToStartWith.Contains(part.Kind))
.ToList();
var parts = method.ToMinimalDisplayParts(model, spanStart, SymbolDisplayFormat.CSharpShortErrorMessageFormat)
.SkipWhile(x => (additionalPartKindsToStartWith is null || !additionalPartKindsToStartWith.Contains(x.Kind)) && !PartKindsToStartWith.Contains(x.Kind))
.ToList();

if (method.MethodKind == MethodKind.EventAdd)
{
parts = parts.Take(parts.Count - 2).ToList();
}
if (method.MethodKind == MethodKind.EventAdd)
{
parts = parts.Take(parts.Count - 2).ToList();
}

return $"{string.Join(string.Empty, parts)}";
}

return $"'{string.Join(string.Empty, parts)}'";
private static bool AreCollidingMethods(IMethodSymbol methodSymbol1, IMethodSymbol methodSymbol2)
{
if (methodSymbol1.Name != methodSymbol2.Name
|| methodSymbol1.MethodKind != methodSymbol2.MethodKind
|| methodSymbol1.Parameters.Length != methodSymbol2.Parameters.Length
|| methodSymbol1.Arity != methodSymbol2.Arity)
{
return false;
}

private static bool AreCollidingMethods(IMethodSymbol methodSymbol1, IMethodSymbol methodSymbol2)
for (var i = 0; i < methodSymbol1.Parameters.Length; i++)
{
if (methodSymbol1.Name != methodSymbol2.Name
|| methodSymbol1.MethodKind != methodSymbol2.MethodKind
|| methodSymbol1.Parameters.Length != methodSymbol2.Parameters.Length
|| methodSymbol1.Arity != methodSymbol2.Arity)
{
return false;
}
var param1 = methodSymbol1.Parameters[i];
var param2 = methodSymbol2.Parameters[i];

for (var i = 0; i < methodSymbol1.Parameters.Length; i++)
if (param1.RefKind != param2.RefKind
|| !Equals(param1.Type, param2.Type))
{
var param1 = methodSymbol1.Parameters[i];
var param2 = methodSymbol2.Parameters[i];

if (param1.RefKind != param2.RefKind
|| !Equals(param1.Type, param2.Type))
{
return false;
}
return false;
}

return true;
}

return true;
}

private sealed record CollidingMember(IMethodSymbol Member, IMethodSymbol CollideWith);
}
4 changes: 2 additions & 2 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/InheritedCollidingInterfaceMembers.CSharp11.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
public interface IAbstractSecond
{
public static abstract string StaticAbstractMethod(string value);
// ^^^^^^^^^^^^^^^^^^^^ Secondary
// ^^^^^^^^^^^^^^^^^^^^ Secondary {{This member collides with 'IAbstractFirst.StaticAbstractMethod(string)'}}
}

public interface IAbstractCommon : IAbstractFirst, IAbstractSecond { } // Noncompliant
Expand All @@ -19,7 +19,7 @@ public interface IVirtualFirst
public interface IVirtualSecond
{
public static virtual string StaticVirtualMethod(string value) => value;
// ^^^^^^^^^^^^^^^^^^^ Secondary
// ^^^^^^^^^^^^^^^^^^^ Secondary {{This member collides with 'IVirtualFirst.StaticVirtualMethod(string)'}}
}

public interface IVirtualCommon : IVirtualFirst, IVirtualSecond { } // Noncompliant
26 changes: 13 additions & 13 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/InheritedCollidingInterfaceMembers.CSharp8.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public string Method(string value)
public interface IPublicMethodSecond
{
public string Method(string value);
// ^^^^^^ Secondary
// ^^^^^^ Secondary {{This member collides with 'IPublicMethodFirst.Method(string)'}}
}

public interface ICommon : IPublicMethodFirst, IPublicMethodSecond // Noncompliant {{Rename or add member 'Method(string)' to this interface to resolve ambiguities.}}
Expand All @@ -53,7 +53,7 @@ public interface IAbstractMethodFirst
public interface IAbstractMethodSecond
{
public abstract string AbstractMethod(string value);
// ^^^^^^^^^^^^^^ Secondary
// ^^^^^^^^^^^^^^ Secondary {{This member collides with 'IAbstractMethodFirst.AbstractMethod(string)'}}
}

public interface IAbstractMethodCommon : IAbstractMethodFirst, IAbstractMethodSecond // Noncompliant {{Rename or add member 'AbstractMethod(string)' to this interface to resolve ambiguities.}}
Expand All @@ -72,7 +72,7 @@ public virtual string Virtual(string value)
public interface IVirtualMethodSecond
{
public virtual string Virtual(string value)
// ^^^^^^^ Secondary
// ^^^^^^^ Secondary {{This member collides with 'IVirtualMethodFirst.Virtual(string)'}}
{
return value;
}
Expand All @@ -94,7 +94,7 @@ internal string Internal(string value)
public interface IInternalMethodSecond
{
internal string Internal(string value)
// ^^^^^^^^ Secondary
// ^^^^^^^^ Secondary {{This member collides with 'IInternalMethodFirst.Internal(string)'}}
{
return value;
}
Expand All @@ -116,7 +116,7 @@ protected string Protected(string value)
public interface IProtectedMethodSecond
{
protected string Protected(string value)
// ^^^^^^^^^ Secondary
// ^^^^^^^^^ Secondary {{This member collides with 'IProtectedMethodFirst.Protected(string)'}}
{
return value;
}
Expand Down Expand Up @@ -158,7 +158,7 @@ public sealed string Sealed(string value)
public interface ISealedMethodSecond
{
public sealed string Sealed(string value)
// ^^^^^^ Secondary
// ^^^^^^ Secondary {{This member collides with 'ISealedMethodFirst.Sealed(string)'}}
{
return value;
}
Expand All @@ -180,7 +180,7 @@ public static string Static(string value)
public interface IStaticMethodSecond
{
public static string Static(string value)
// ^^^^^^ Secondary
// ^^^^^^ Secondary {{This member collides with 'IStaticMethodFirst.Static(string)'}}
{
return value;
}
Expand Down Expand Up @@ -218,9 +218,9 @@ public interface IIndexer2
public string this[int i] // Indexer does not have a name, signature is used instead
{
get { return ""; }
// ^^^ Secondary
// ^^^ Secondary {{This member collides with 'IIndexer1.this[int]'}}
set { }
// ^^^ Secondary
// ^^^ Secondary {{This member collides with 'IIndexer1.this[int]'}}
}
}

Expand All @@ -243,9 +243,9 @@ public interface IProperty2
public string Property
{
get => string.Empty;
// ^^^ Secondary
// ^^^ Secondary {{This member collides with 'IProperty1.Property.get'}}
set => Console.WriteLine(value);
// ^^^ Secondary
// ^^^ Secondary {{This member collides with 'IProperty1.Property.set'}}
}
}

Expand Down Expand Up @@ -274,12 +274,12 @@ public event EventHandler OnClick
public interface IEvents2
{
public event EventHandler Click;
// ^^^^^ Secondary
// ^^^^^ Secondary {{This member collides with 'IEvents1.Click'}}

public event EventHandler OnClick
{
add
// ^^^ Secondary
// ^^^ Secondary {{This member collides with 'IEvents1.OnClick'}}
{
Click += value;
}
Expand Down
Loading

0 comments on commit caa1608

Please sign in to comment.