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.

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

Simplify analyzer handling of properties/events #92449

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Simplify analyzer handling of properties/events #92449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
378958e
Simplify handling of properties in illink analyzer
sbomer Sep 21, 2023
7efb349
Stop warning on properties
sbomer Sep 21, 2023
b6129a0
Same treatment for events
sbomer Sep 21, 2023
d806d44
Update comment
sbomer Sep 22, 2023
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
25 changes: 2 additions & 23 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,21 +54,6 @@ public override void Initialize (AnalysisContext context)
CheckMatchingAttributesInInterfaces (symbolAnalysisContext, typeSymbol);
}, SymbolKind.NamedType);


context.RegisterSymbolAction (symbolAnalysisContext => {
var propertySymbol = (IPropertySymbol) symbolAnalysisContext.Symbol;
if (AnalyzerDiagnosticTargets.HasFlag (DiagnosticTargets.Property)) {
CheckMatchingAttributesInOverrides (symbolAnalysisContext, propertySymbol);
}
}, SymbolKind.Property);

context.RegisterSymbolAction (symbolAnalysisContext => {
var eventSymbol = (IEventSymbol) symbolAnalysisContext.Symbol;
if (AnalyzerDiagnosticTargets.HasFlag (DiagnosticTargets.Event)) {
CheckMatchingAttributesInOverrides (symbolAnalysisContext, eventSymbol);
}
}, SymbolKind.Event);

context.RegisterOperationAction (operationContext => {
var methodInvocation = (IInvocationOperation) operationContext.Operation;
CheckCalledMember (operationContext, methodInvocation.TargetMethod, incompatibleMembers);
Expand Down Expand Up @@ -96,9 +81,6 @@ public override void Initialize (AnalysisContext context)

if (usageInfo.HasFlag (ValueUsageInfo.Write) && prop.SetMethod != null)
CheckCalledMember (operationContext, prop.SetMethod, incompatibleMembers);

if (AnalyzerDiagnosticTargets.HasFlag (DiagnosticTargets.Property))
CheckCalledMember (operationContext, prop, incompatibleMembers);
}, OperationKind.PropertyReference);

context.RegisterOperationAction (operationContext => {
Expand All @@ -114,9 +96,6 @@ public override void Initialize (AnalysisContext context)

if (eventSymbol.RaiseMethod is IMethodSymbol eventRaiseMethod)
CheckCalledMember (operationContext, eventRaiseMethod, incompatibleMembers);

if (AnalyzerDiagnosticTargets.HasFlag (DiagnosticTargets.Event))
CheckCalledMember (operationContext, eventSymbol, incompatibleMembers);
}, OperationKind.EventReference);

context.RegisterOperationAction (operationContext => {
Expand Down Expand Up @@ -330,8 +309,8 @@ private bool HasMismatchingAttributes (ISymbol member1, ISymbol member2)
{
bool member1CreatesRequirement = member1.DoesMemberRequire (RequiresAttributeName, out _);
bool member2CreatesRequirement = member2.DoesMemberRequire (RequiresAttributeName, out _);
bool member1FulfillsRequirement = member1.IsOverrideInRequiresScope (RequiresAttributeName);
bool member2FulfillsRequirement = member2.IsOverrideInRequiresScope (RequiresAttributeName);
bool member1FulfillsRequirement = member1.IsInRequiresScope (RequiresAttributeName);
bool member2FulfillsRequirement = member2.IsInRequiresScope (RequiresAttributeName);
return (member1CreatesRequirement && !member2FulfillsRequirement) || (member2CreatesRequirement && !member1FulfillsRequirement);
}

Expand Down
4 changes: 1 addition & 3 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,13 @@ protected override bool ReportSpecialIncompatibleMembersDiagnostic (OperationAna
}
else if (method.AssociatedSymbol is ISymbol associatedSymbol &&
ImmutableArrayOperations.Contains (dangerousPatterns, associatedSymbol, SymbolEqualityComparer.Default)) {
operationContext.ReportDiagnostic (Diagnostic.Create (s_locationRule, operationContext.Operation.Syntax.GetLocation (), member.GetDisplayName ()));
// The getters for CodeBase and EscapedCodeBase have RAF attribute on them
// so our caller will produce the RAF warning (IL3002) by default. Since we handle these properties specifically
// here and produce different warning (IL3000) we don't want the caller to produce IL3002.
// So we need to return true from here for the getters, to suppress the RAF warning.
return true;
}
} else if (member is IPropertySymbol && ImmutableArrayOperations.Contains (dangerousPatterns, member, SymbolEqualityComparer.Default)) {
operationContext.ReportDiagnostic (Diagnostic.Create (s_locationRule, operationContext.Operation.Syntax.GetLocation (), member.GetDisplayName ()));
return true;
}

return false;
Expand Down
28 changes: 9 additions & 19 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresISymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,26 @@ public static bool DoesMemberRequire (this ISymbol member, string requiresAttrib
if (!member.IsStaticConstructor () && member.TryGetAttribute (requiresAttribute, out requiresAttributeData))
return true;

if (member is IMethodSymbol { AssociatedSymbol: { } associated } && associated.TryGetAttribute (requiresAttribute, out requiresAttributeData))
return true;

// Also check the containing type
if (member.IsStatic || member.IsConstructor ())
return member.ContainingType.TryGetAttribute (requiresAttribute, out requiresAttributeData);

return false;
}

// TODO: Consider sharing with ILLink IsInRequiresScope method
/// <summary>
/// True if the source of a call is considered to be annotated with the Requires... attribute
/// </summary>
public static bool IsInRequiresScope (this ISymbol member, string attributeName, [NotNullWhen (true)] out AttributeData? requiresAttribute)
public static bool IsInRequiresScope (this ISymbol member, string attributeName)
{
return member.IsInRequiresScope (attributeName, true, out requiresAttribute);
return member.IsInRequiresScope (attributeName, out _);
}

// TODO: Consider sharing with ILLink IsInRequiresScope method
/// <summary>
/// True if member of a call is considered to be annotated with the Requires... attribute.
/// Doesn't check the associated symbol for overrides and virtual methods because the analyzer should warn on mismatched between the property AND the accessors
/// True if the source of a call is considered to be annotated with the Requires... attribute
/// </summary>
/// <param name="member">
/// Symbol that is either an overriding member or an overriden/virtual member
/// </param>
public static bool IsOverrideInRequiresScope (this ISymbol member, string requiresAttribute)
{
return member.IsInRequiresScope (requiresAttribute, false, out _);
}

private static bool IsInRequiresScope (this ISymbol member, string attributeName, bool checkAssociatedSymbol, [NotNullWhen (true)] out AttributeData? requiresAttribute)
public static bool IsInRequiresScope (this ISymbol member, string attributeName, [NotNullWhen (true)] out AttributeData? requiresAttribute)
{
// Requires attribute on a type does not silence warnings that originate
// from the type directly. We also only check the containing type for members
Expand All @@ -67,8 +58,7 @@ private static bool IsInRequiresScope (this ISymbol member, string attributeName
if (member.ContainingType is ITypeSymbol containingType && containingType.TryGetAttribute (attributeName, out requiresAttribute))
return true;

// Only check associated symbol if not override or virtual method
if (checkAssociatedSymbol && member is IMethodSymbol { AssociatedSymbol: { } associated } && associated.TryGetAttribute (attributeName, out requiresAttribute))
if (member is IMethodSymbol { AssociatedSymbol: { } associated } && associated.TryGetAttribute (attributeName, out requiresAttribute))
return true;

// When using instance fields suppress the warning if the constructor has already the Requires annotation
Expand Down
23 changes: 12 additions & 11 deletions src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,14 @@ class C

void M()
{
var handler = E;
E += (sender, e) => { };
var evt = E;
}
}
""";
return VerifyRequiresAssemblyFilesAnalyzer (TestRequiresAssemblyFieldsOnEvent,
// (11,17): warning IL3002: Using member 'C.E' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app.
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (11, 17, 11, 18).WithArguments ("C.E", "", ""));
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (11, 3, 11, 4).WithArguments ("C.E.add", "", ""));
}

[Fact]
Expand All @@ -105,9 +106,9 @@ void M()
""";
return VerifyRequiresAssemblyFilesAnalyzer (TestRequiresAssemblyFilesOnProperty,
// (11,3): warning IL3002: Using member 'C.P' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app.
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (11, 3, 11, 4).WithArguments ("C.P", "", ""),
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (11, 3, 11, 4).WithArguments ("C.P.set", "", ""),
// (12,35): warning IL3002: Using member 'C.P' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app.
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (12, 35, 12, 36).WithArguments ("C.P", "", ""));
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (12, 35, 12, 36).WithArguments ("C.P.get", "", ""));
}

[Fact]
Expand Down Expand Up @@ -142,7 +143,7 @@ void M ()
""";
return VerifyRequiresAssemblyFilesAnalyzer (TestRequiresAssemblyFilesOnMethodInsideProperty,
// (23,3): warning IL3002: Using member 'C.P' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app.
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (23, 3, 23, 4).WithArguments ("C.P", "", ""));
VerifyCS.Diagnostic (DiagnosticId.RequiresAssemblyFiles).WithSpan (23, 3, 23, 4).WithArguments ("C.P.set", "", ""));
}

[Fact]
Expand Down Expand Up @@ -227,7 +228,7 @@ class C

return VerifyRequiresAssemblyFilesAnalyzer (src,
// (5,26): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (4, 23, 4, 63).WithArguments ("System.Reflection.Assembly.Location"));
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (4, 23, 4, 63).WithArguments ("System.Reflection.Assembly.Location.get"));
}

[Fact]
Expand All @@ -249,7 +250,7 @@ public void M()
""";
return VerifyRequiresAssemblyFilesAnalyzer (src,
// (7,7): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.Assembly.Location")
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.Assembly.Location.get")
);
}

Expand Down Expand Up @@ -297,9 +298,9 @@ public void M()
// (8,7): warning SYSLIB0044: 'AssemblyName.EscapedCodeBase' is obsolete: 'AssemblyName.CodeBase and AssemblyName.EscapedCodeBase are obsolete. Using them for loading an assembly is not supported.'
DiagnosticResult.CompilerWarning ("SYSLIB0044").WithSpan (8, 7, 8, 24).WithArguments ("System.Reflection.AssemblyName.EscapedCodeBase", "AssemblyName.CodeBase and AssemblyName.EscapedCodeBase are obsolete. Using them for loading an assembly is not supported."),
// (7,7): warning IL3000: 'System.Reflection.AssemblyName.CodeBase' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.AssemblyName.CodeBase"),
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.AssemblyName.CodeBase.get"),
// (8,7): warning IL3000: 'System.Reflection.AssemblyName.EscapedCodeBase' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (8, 7, 8, 24).WithArguments ("System.Reflection.AssemblyName.EscapedCodeBase")
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (8, 7, 8, 24).WithArguments ("System.Reflection.AssemblyName.EscapedCodeBase.get")
);
}

Expand All @@ -322,7 +323,7 @@ public void M()
""";
return VerifyRequiresAssemblyFilesAnalyzer (src,
// (7,7): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.Assembly.Location"),
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 7, 7, 17).WithArguments ("System.Reflection.Assembly.Location.get"),
// (8,7): warning IL3001: Assemblies embedded in a single-file app cannot have additional files in the manifest.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 19).WithArguments ("System.Reflection.Assembly.GetFiles()")
);
Expand Down Expand Up @@ -476,7 +477,7 @@ public void M2() {
fixedSource: fixtest,
baselineExpected: new[] {
// /0/Test0.cs(6,24): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (6, 24, 6, 41).WithArguments ("System.Reflection.Assembly.Location", "", ""),
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (6, 24, 6, 41).WithArguments ("System.Reflection.Assembly.Location.get", "", ""),
// /0/Test0.cs(8,7): warning IL3001: 'System.Reflection.Assembly.GetFiles()' will throw for assemblies embedded in a single-file app
VerifyCS.Diagnostic (DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (8, 7, 8, 26).WithArguments("System.Reflection.Assembly.GetFiles()", "", ""),
},
Expand Down
2 changes: 1 addition & 1 deletion ...tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnconditionalSuppressMessageCodeFixTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public void M2() {
fixtest,
baselineExpected: new[] {
// /0/Test0.cs(7,27): warning IL3000: 'System.Reflection.Assembly.Location' always returns an empty string for assemblies embedded in a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'.
VerifyCSUSMwithRAF.Diagnostic(DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 27, 7, 44).WithArguments ("System.Reflection.Assembly.Location", "", ""),
VerifyCSUSMwithRAF.Diagnostic(DiagnosticId.AvoidAssemblyLocationInSingleFile).WithSpan (7, 27, 7, 44).WithArguments ("System.Reflection.Assembly.Location.get", "", ""),
// /0/Test0.cs(9,13): warning IL3001: 'System.Reflection.Assembly.GetFiles()' will throw for assemblies embedded in a single-file app
VerifyCSUSMwithRAF.Diagnostic(DiagnosticId.AvoidAssemblyGetFilesInSingleFile).WithSpan (9, 13, 9, 32).WithArguments("System.Reflection.Assembly.GetFiles()", "", ""),
},
Expand Down
4 changes: 4 additions & 0 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/BasicRequires.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ static event EventHandler EventToTestAdd {
remove { }
}

[RequiresAssemblyFiles ("Message for --AnnotatedEvent--")]
static event EventHandler AnnotatedEvent;

[ExpectedWarning ("IL2026", "--EventToTestRemove.remove--")]
[ExpectedWarning ("IL3002", "--EventToTestRemove.remove--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL3050", "--EventToTestRemove.remove--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
Expand All @@ -165,6 +168,7 @@ public static void Test ()
{
EventToTestRemove -= (sender, e) => { };
EventToTestAdd += (sender, e) => { };
var evt = AnnotatedEvent;
}
}

Expand Down
Loading