Skip to content

Analyzer warns when Requires... Attribute is on a static constructor #2455

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
jtschuster merged 24 commits into from
Jan 24, 2022
Merged

Analyzer warns when Requires... Attribute is on a static constructor #2455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
2ff076e
Init for adding analyzer support to warn on Requires on cctor
jtschuster Dec 15, 2021
500a542
Adds an ExpectedWarning for IL3051 to DerivedClassWithAllWarnings
jtschuster Dec 14, 2021
c35ab2b
Update test to expect RAF and RDC on cctor warnings in analyzer only
jtschuster Dec 14, 2021
f9c57ee
Revert Cecil version Change
jtschuster Dec 15, 2021
db0084b
Add a visit to ConstructorDeclaration in syntax walker test infra
jtschuster Dec 15, 2021
e781898
Test case for ExpectWarning not working as expected in analyzer tests
jtschuster Dec 15, 2021
0835e40
Adds warning on static constructors to analyzers
jtschuster Dec 16, 2021
b95fc44
Remove RequiresDynamicCode ExpectedWarnings
jtschuster Dec 16, 2021
7c03ea5
reset Cecil to correct version
jtschuster Dec 16, 2021
645cee2
Renumber diagnostic IDs, use Roslyn member notation instead of IL not…
jtschuster Dec 16, 2021
a19ffb6
remove ..cctor from static constructor diagnostic messages
jtschuster Dec 16, 2021
da05d3f
uses 'static' modifiers in cctor diagnostic messages
jtschuster Dec 20, 2021
04270d6
uses ..cctor() notation in diagnostic messages
jtschuster Dec 20, 2021
2425bb2
Add test for ExpectedWarning on ctor and cctor
jtschuster Jan 6, 2022
54dbedc
Fix formatting
jtschuster Jan 6, 2022
cb9fc19
Accomodate for potential bug found in linker
jtschuster Jan 6, 2022
aa720df
fix shared string formatting
jtschuster Jan 6, 2022
b26e96a
Remove extra line in DiagnosticId.cs
jtschuster Jan 6, 2022
e6eb758
References the new test class in the test code so it's not trimmed
jtschuster Jan 6, 2022
66f39a8
Merge branch 'RequiresOnCctorWarns' of https://github.com/jtschuster/…
jtschuster Jan 6, 2022
c78a81a
Fix formatting
jtschuster Jan 6, 2022
011dcd2
Merge with main
jtschuster Jan 20, 2022
46f0c68
update cecil and fix extra line
jtschuster Jan 21, 2022
6bdad7a
Remove extra comma in DiagnosticId.cs
jtschuster Jan 21, 2022
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
7 changes: 5 additions & 2 deletions src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,12 @@ public static string GetDisplayName (this ISymbol symbol)
case IParameterSymbol parameterSymbol:
sb.Append (parameterSymbol.Name);
break;

default:
sb.Append (symbol.ToDisplayString ());
if (symbol.IsStaticConstructor ()) {
sb.Append (symbol.ContainingType.ToDisplayString ());
sb.Append ("..cctor()");
} else
sb.Append (symbol.ToDisplayString ());
break;
}

Expand Down
11 changes: 11 additions & 0 deletions src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public abstract class RequiresAnalyzerBase : DiagnosticAnalyzer
private protected abstract DiagnosticDescriptor RequiresDiagnosticRule { get; }

private protected abstract DiagnosticDescriptor RequiresAttributeMismatch { get; }
private protected abstract DiagnosticDescriptor RequiresOnStaticCtor { get; }

private protected virtual ImmutableArray<(Action<OperationAnalysisContext> Action, OperationKind[] OperationKind)> ExtraOperationActions { get; } = ImmutableArray<(Action<OperationAnalysisContext> Action, OperationKind[] OperationKind)>.Empty;

Expand All @@ -43,6 +44,8 @@ public override void Initialize (AnalysisContext context)
var incompatibleMembers = GetSpecialIncompatibleMembers (compilation);
context.RegisterSymbolAction (symbolAnalysisContext => {
var methodSymbol = (IMethodSymbol) symbolAnalysisContext.Symbol;
if (methodSymbol.IsStaticConstructor () && methodSymbol.HasAttribute (RequiresAttributeName))
ReportRequiresOnStaticCtorDiagnostic (symbolAnalysisContext, methodSymbol);
CheckMatchingAttributesInOverrides (symbolAnalysisContext, methodSymbol);
CheckAttributeInstantiation (symbolAnalysisContext, methodSymbol);
foreach (var typeParameter in methodSymbol.TypeParameters)
Expand Down Expand Up @@ -332,6 +335,14 @@ private void ReportRequiresDiagnostic (OperationAnalysisContext operationContext
url));
}

private void ReportRequiresOnStaticCtorDiagnostic (SymbolAnalysisContext symbolAnalysisContext, IMethodSymbol ctor)
{
symbolAnalysisContext.ReportDiagnostic (Diagnostic.Create (
RequiresOnStaticCtor,
ctor.Locations[0],
ctor.GetDisplayName ()));
}

private void ReportMismatchInAttributesDiagnostic (SymbolAnalysisContext symbolAnalysisContext, ISymbol member, ISymbol baseMember, bool isInterface = false)
{
string message = MessageFormat.FormatRequiresAttributeMismatch (member.HasAttribute (RequiresAttributeName), isInterface, RequiresAttributeName, member.GetDisplayName (), baseMember.GetDisplayName ());
Expand Down
6 changes: 5 additions & 1 deletion src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ public sealed class RequiresAssemblyFilesAnalyzer : RequiresAnalyzerBase

static readonly DiagnosticDescriptor s_requiresAssemblyFilesAttributeMismatch = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresAssemblyFilesAttributeMismatch);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create (s_locationRule, s_getFilesRule, s_requiresAssemblyFilesRule, s_requiresAssemblyFilesAttributeMismatch);
static readonly DiagnosticDescriptor s_requiresAssemblyFilesOnStaticCtor = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresAssemblyFilesOnStaticConstructor);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create (s_locationRule, s_getFilesRule, s_requiresAssemblyFilesRule, s_requiresAssemblyFilesAttributeMismatch, s_requiresAssemblyFilesOnStaticCtor);

private protected override string RequiresAttributeName => RequiresAssemblyFilesAttribute;

Expand All @@ -38,6 +40,8 @@ public sealed class RequiresAssemblyFilesAnalyzer : RequiresAnalyzerBase

private protected override DiagnosticDescriptor RequiresAttributeMismatch => s_requiresAssemblyFilesAttributeMismatch;

private protected override DiagnosticDescriptor RequiresOnStaticCtor => s_requiresAssemblyFilesOnStaticCtor;

protected override bool IsAnalyzerEnabled (AnalyzerOptions options, Compilation compilation)
{
var isSingleFileAnalyzerEnabled = options.GetMSBuildPropertyValue (MSBuildPropertyOptionNames.EnableSingleFileAnalyzer, compilation);
Expand Down
5 changes: 4 additions & 1 deletion src/ILLink.RoslynAnalyzer/RequiresDynamicCodeAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase
const string RequiresDynamicCodeAttribute = nameof (RequiresDynamicCodeAttribute);
public const string FullyQualifiedRequiresDynamicCodeAttribute = "System.Diagnostics.CodeAnalysis." + RequiresDynamicCodeAttribute;

static readonly DiagnosticDescriptor s_requiresDynaicCodeOnStaticCtor = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCodeOnStaticConstructor);
static readonly DiagnosticDescriptor s_requiresDynamicCodeRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCode);
static readonly DiagnosticDescriptor s_requiresDynamicCodeAttributeMismatch = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresDynamicCodeAttributeMismatch);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create (s_requiresDynamicCodeRule, s_requiresDynamicCodeAttributeMismatch);
ImmutableArray.Create (s_requiresDynamicCodeRule, s_requiresDynamicCodeAttributeMismatch, s_requiresDynaicCodeOnStaticCtor);

private protected override string RequiresAttributeName => RequiresDynamicCodeAttribute;

Expand All @@ -30,6 +31,8 @@ public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase

private protected override DiagnosticDescriptor RequiresAttributeMismatch => s_requiresDynamicCodeAttributeMismatch;

private protected override DiagnosticDescriptor RequiresOnStaticCtor => s_requiresDynaicCodeOnStaticCtor;

protected override bool IsAnalyzerEnabled (AnalyzerOptions options, Compilation compilation) =>
options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableAOTAnalyzer, compilation);

Expand Down
5 changes: 4 additions & 1 deletion src/ILLink.RoslynAnalyzer/RequiresUnreferencedCodeAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public sealed class RequiresUnreferencedCodeAnalyzer : RequiresAnalyzerBase
new LocalizableResourceString (nameof (SharedStrings.DynamicTypeInvocationMessage), SharedStrings.ResourceManager, typeof (SharedStrings)));
static readonly DiagnosticDescriptor s_makeGenericTypeRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.MakeGenericType);
static readonly DiagnosticDescriptor s_makeGenericMethodRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.MakeGenericMethod);
static readonly DiagnosticDescriptor s_requiresUnreferencedCodeOnStaticCtor = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresUnreferencedCodeOnStaticConstructor);

static readonly DiagnosticDescriptor s_typeDerivesFromRucClassRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.RequiresUnreferencedCodeOnBaseClass);

Expand Down Expand Up @@ -53,7 +54,7 @@ private Action<SymbolAnalysisContext> typeDerivesFromRucBase {
}

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create (s_dynamicTypeInvocationRule, s_makeGenericMethodRule, s_makeGenericTypeRule, s_requiresUnreferencedCodeRule, s_requiresUnreferencedCodeAttributeMismatch, s_typeDerivesFromRucClassRule);
ImmutableArray.Create (s_dynamicTypeInvocationRule, s_makeGenericMethodRule, s_makeGenericTypeRule, s_requiresUnreferencedCodeRule, s_requiresUnreferencedCodeAttributeMismatch, s_typeDerivesFromRucClassRule, s_requiresUnreferencedCodeOnStaticCtor);

private protected override string RequiresAttributeName => RequiresUnreferencedCodeAttribute;

Expand All @@ -65,6 +66,8 @@ private Action<SymbolAnalysisContext> typeDerivesFromRucBase {

private protected override DiagnosticDescriptor RequiresAttributeMismatch => s_requiresUnreferencedCodeAttributeMismatch;

private protected override DiagnosticDescriptor RequiresOnStaticCtor => s_requiresUnreferencedCodeOnStaticCtor;

protected override bool IsAnalyzerEnabled (AnalyzerOptions options, Compilation compilation) =>
options.IsMSBuildPropertyValueTrue (MSBuildPropertyOptionNames.EnableTrimAnalyzer, compilation);

Expand Down
4 changes: 3 additions & 1 deletion src/ILLink.Shared/DiagnosticId.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,12 @@ public enum DiagnosticId
AvoidAssemblyGetFilesInSingleFile = 3001,
RequiresAssemblyFiles = 3002,
RequiresAssemblyFilesAttributeMismatch = 3003,
RequiresAssemblyFilesOnStaticConstructor = 3004,

// Dynamic code diagnostic ids.
RequiresDynamicCode = 3050,
RequiresDynamicCodeAttributeMismatch = 3051
RequiresDynamicCodeAttributeMismatch = 3051,
RequiresDynamicCodeOnStaticConstructor = 3052
}

public static class DiagnosticIdExtensions
Expand Down
20 changes: 19 additions & 1 deletion src/ILLink.Shared/SharedStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,4 +654,22 @@
<data name="InterfaceRequiresMismatchMessage" xml:space="preserve">
<value>Interface member '{2}' with '{0}' has an implementation member '{1}' without '{0}'</value>
</data>
</root>
<data name="RequiresOnBaseClassMessage" xml:space="preserve">
<value>Type '{0}' derives from '{1}' which has 'RequiresUnreferencedCodeAttribute'. {2}{3}</value>
</data>
<data name="RequiresOnBaseClassTitle" xml:space="preserve">
<value>Types that derive from a base class with 'RequiresUnreferencedCodeAttribute' need to explicitly use the 'RequiresUnreferencedCodeAttribute' or suppress this warning</value>
</data>
<data name="RequiresDynamicCodeOnStaticConstructorMessage" xml:space="preserve">
<value>'RequiresDynamicCodeAttribute' cannot be placed directly on static constructor '{0}'.</value>
</data>
<data name="RequiresDynamicCodeOnStaticConstructorTitle" xml:space="preserve">
<value>The use of 'RequiresDynamicCodeAttribute' on static constructors is disallowed since is a method not callable by the user, is only called by the runtime. Placing the attribute directly on the static constructor will have no effect, instead use 'RequiresUnreferencedCodeAttribute' on the type which will handle warning and silencing from the static constructor.</value>
</data>
<data name="RequiresAssemblyFilesOnStaticConstructorMessage" xml:space="preserve">
<value>'RequiresAssemblyFilesAttribute' cannot be placed directly on static constructor '{0}'.</value>
</data>
<data name="RequiresAssemblyFilesOnStaticConstructorTitle" xml:space="preserve">
<value>The use of 'RequiresAssemblyFilesAttribute' on static constructors is disallowed since is a method not callable by the user, is only called by the runtime. Placing the attribute directly on the static constructor will have no effect, instead use 'RequiresUnreferencedCodeAttribute' on the type which will handle warning and silencing from the static constructor.</value>
</data>
</root>
6 changes: 6 additions & 0 deletions test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ public override void VisitClassDeclaration (ClassDeclarationSyntax node)
CheckMember (node);
}

public override void VisitConstructorDeclaration (ConstructorDeclarationSyntax node)
Copy link
Member Author

@jtschuster jtschuster Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fix for #2446 to make sure we account for ExpectedWarning on constructors.

Copy link
Contributor

@tlakollo tlakollo Jan 6, 2022

Choose a reason for hiding this comment

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

Can you add a test for this? Something that Inside the static constructor calls a method annotated with RUC and just verify that we can use the ExpectedWarning attribute

Copy link
Member Author

@jtschuster jtschuster Jan 6, 2022

Choose a reason for hiding this comment

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

Good thing you mentioned this, I think I found a bug that the linker doesn't check in constructors for calls to methods with RUC. The test class for this is WarningsInCtor in RequiresCapability.cs. It calls a method annotated with RUC inside the constructors, but the linker doesn't seem to be producing a warning. I filed #2484 to track the issue.

Copy link
Contributor

@tlakollo tlakollo Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's because nothing references that code in the main method, since it's not referenced Trimmer will get rid of that piece of code. Notice that one of the differences between analyzer and trimmer is that analyzer will produce diagnostics even for code that will be trimmed.

{
base.VisitConstructorDeclaration (node);
CheckMember (node);
}

public override void VisitInterfaceDeclaration (InterfaceDeclarationSyntax node)
{
base.VisitInterfaceDeclaration (node);
Expand Down
14 changes: 6 additions & 8 deletions test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresOnStaticConstructor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public static void Main ()

class StaticCtor
{
[ExpectedWarning ("IL2116", "StaticCtor..cctor()", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2116", "StaticCtor..cctor()")]
[RequiresUnreferencedCode ("Message for --TestStaticCtor--")]
static StaticCtor ()
{
Expand All @@ -42,7 +42,7 @@ static void TestStaticCctorRequires ()

class StaticCtorTriggeredByFieldAccess
{
[ExpectedWarning ("IL2116", "StaticCtorTriggeredByFieldAccess..cctor()", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2116", "StaticCtorTriggeredByFieldAccess..cctor()")]
[RequiresUnreferencedCode ("Message for --StaticCtorTriggeredByFieldAccess.Cctor--")]
static StaticCtorTriggeredByFieldAccess ()
{
Expand All @@ -59,9 +59,7 @@ static void TestStaticCtorMarkingIsTriggeredByFieldAccess ()

struct StaticCCtorForFieldAccess
{
// TODO: Analyzer still allows RUC/RAF on static constructor with no warning
// https://github.com/dotnet/linker/issues/2347
[ExpectedWarning ("IL2116", "StaticCCtorForFieldAccess..cctor()", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2116", "StaticCCtorForFieldAccess..cctor()")]
[RequiresUnreferencedCode ("Message for --StaticCCtorForFieldAccess.cctor--")]
static StaticCCtorForFieldAccess () { }

Expand All @@ -75,9 +73,9 @@ static void TestStaticCtorMarkingIsTriggeredByFieldAccessOnExplicitLayout ()

class StaticCtorTriggeredByMethodCall
{
// TODO: Analyzer still allows RUC/RAF on static constructor with no warning
// https://github.com/dotnet/linker/issues/2347
[ExpectedWarning ("IL2116", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2116", "StaticCtorTriggeredByMethodCall..cctor()")]
[ExpectedWarning ("IL3004", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = ProducedBy.Analyzer)]
[ExpectedWarning ("IL3052", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = ProducedBy.Analyzer)]
[RequiresUnreferencedCode ("Message for --StaticCtorTriggeredByMethodCall.Cctor--")]
[RequiresAssemblyFiles ("Message for --StaticCtorTriggeredByMethodCall.Cctor--")]
[RequiresDynamicCode ("Message for --StaticCtorTriggeredByMethodCall.Cctor--")]
Expand Down