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

jtschuster · 2021-12-16T02:09:39Z

Adds warnings in the analyzer for IL2116 (RUC not allowed on static ctor) and adds IL2117 (RequiresDynamicCode not allowed on static ctor) and IL2118 (RequiresAssemblyFiles not allowed on static ctor).

Fixes ExpectedWarning not having effects on constructors by adding a Visit override in the compilation tree walker in the test infra.

Adds case in GetDisplayName to differentiate .cctor's and .ctor's.

Fixes #2446
Fixes #2347

Customer Impact

Analyzer correctly reports warnings due to RequiresUnreferencedCode, RequiresDynamicCode and RequiresAssemblyFiles on static constructors. For RequriesUnreferencedCode the warnings would be reported by trimmer during publish. But for RequiresDynamicCode and RequiresAssemblyFiles nothing would report these warnings.

Testing

Added targeted tests.

Risk

Low - added a check for new case (static .ctor) and the new diagnostics in the analyzer.

tlakollo · 2021-12-16T02:17:46Z

There are some ranges for warnings described in DiagnosticDescriptors this along with currently registered warnings in DiagnosticIds means that the warning produced by RequiresDynamicCode should be IL3052 and by RequiresAssemblyFiles should be IL3004

src/ILLink.RoslynAnalyzer/ILLink.RoslynAnalyzer.csproj

tlakollo · 2021-12-16T02:27:21Z

src/ILLink.RoslynAnalyzer/IMethodSymbolExtensions.cs

+			sb.Insert (0, '<');
+		}
+
+		public static string GetDisplayName (this IMethodSymbol method)


The GetDisplayName method was made to match C# naming which is what Roslyn uses and most people are used to reading instead of IL namings. Therefore if there is a discrepancy between the way it's represented by the analyzer and linker is likely a fix in the GetDisplayName in the linker not an implementation on the analyzer

Worst case I think you can append directly the string ".cctor" at the end when displaying the diagnostic

I would expect most cases to already be covered by Roslyn. I could understand that we need to do something special for property getter/setter (and events), and possibly even .cctor, but other than that I would expect to simply call Roslyn to do this for us.

For now, would it be best to use the ISymbolExtensions.GetDisplayName and have separate ExpectedWarning's for the analyzer and trimmer, and then later change the trimmer's GetDisplayName to match the Analyzer/Roslyn?

I added a check in the GetDisplayName in the linker that exists for ctors, but was missing cctors. Now they should look the same.

I think I was wrong is not a fix on the linker side, because if you fix in the linker then constructor and static constructor are represented equally, there is no way to differentiate them. So in the cases like the BeforeInitField test, I think things can get confusing. Same for RequiresInCompilerGeneratedCode tests, they get more confusing although you don't see the changes because we don't check for the caller name only messages in the attribute.
But yeah I think the best place to fix it is ISymbolExtensions.GetDisplayName an if statement before printing the method name check if the method symbol is a static constructor

.cctors are weird - so we might need to add a special case unfortunately.

I recently pushed a couple commits with different formats for the diagnostic messages. The first one includes the static modifier (e.g. static Mono.Linker.Tests.Cases.RequiresCapability.RequiresCapability.StaticCtor.StaticCtor()). The second uses the ..cctor() notation (e.g. Mono.Linker.Tests.Cases.RequiresCapability.RequiresCapability.StaticCtor..cctor()).

I think the static modifier notation would be best since it still differentiates the constructors and is more familiar to most people.

I just created something in SharpLab to see how a warning would be printed

public class Foo { static Foo (int a) {} }

This generates error CS0132: 'Foo.Foo(int)': a static constructor must be parameterless also inserting the [Obsolete] attribute will generate a message [deprecated] Foo.Foo() so the compiler warnings at this moment don't show the modifiers nor have a clear distinction between static and instance constructors, so whatever decision we make is going to deviate from whatever the compiler normally prints.

I think is key for the trimmer to have some differentiation but I don't have a preference for which one to choose

The first option I have the concern that not a lot of people will even see the static modifier, it's at the beginning of the string and no other member will print it. Although I agree, I like more the readability of it.

The second option is a weird notation, but static constructors are weird, we don't even allow to annotate them. Static constructors are handled in a special way. So the fact that the name is weird might even be beneficial. The problem here would be that we already don't name instance constructors as .ctor()

In general I think I feel more inclined for the first approach

…ation in diagnostics

jtschuster · 2022-01-04T20:49:53Z

test/ILLink.RoslynAnalyzer.Tests/TestChecker.cs

 			CheckMember (node);
 		}

+		public override void VisitConstructorDeclaration (ConstructorDeclarationSyntax node)


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

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

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.

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.

src/ILLink.Shared/SharedStrings.resx

Linker doesn't seem to be checking ctors for calls to methods with RUC. This commit removes the check for the expected warnings in the linker until the bug is fixed.

src/ILLink.Shared/DiagnosticId.cs

…linker into RequiresOnCctorWarns

tlakollo

Other than lint, looks good to me. Thanks :D

tlakollo · 2022-01-19T23:51:26Z

Does another person want to sign off on these changes so they can be merged?

tlakollo · 2022-01-21T01:00:10Z

src/ILLink.RoslynAnalyzer/RequiresDynamicCodeAnalyzer.cs

 	[DiagnosticAnalyzer (LanguageNames.CSharp)]
 	public sealed class RequiresDynamicCodeAnalyzer : RequiresAnalyzerBase
 	{
+


nit: extra line

Design proposal for DAM-on-type and RUC interactions (dotnet#2168) Co-authored-by: Sven Boemer <[email protected]> Fix analyzer nullref on assembly attribute (dotnet#2534) The analyzer runs for property assignment operations, including those in attributes. To check whether the property assignment should warn, we look for RUC on the containing symbol. However, assembly-level attributes are contained in the global namespace, which has a null containing type. Create a schema for the LinkAttribute XML files (dotnet#2500) Adds xml schema for ILLink.LinkAttributes.xml, and adds relative paths to the schema in xml used in unit tests to enable linting. [main] Update dependencies from dotnet/runtime (dotnet#2539) [main] Update dependencies from dotnet/runtime Title and message resources should be enforced to exist to prevent printing empty messages (dotnet#2538) Currently, missing resources will default to an empty string printing an empty message for the diagnostics. Code should not try to gracefully handle errors. Included in this PR: - DiagnosticString should be able to locate a title and message resource strings otherwise it throws - Add title resource strings for all missing diagnostic Ids Update dependencies from https://github.com/dotnet/runtime build 20220123.5 (dotnet#2542) [main] Update dependencies from dotnet/runtime Update dependencies from https://github.com/dotnet/arcade build 20220121.6 (dotnet#2541) [main] Update dependencies from dotnet/arcade Analyzer warns when Requires... Attribute is on a static constructor (dotnet#2455) Adds warnings in the analyzer for IL2116 (RUC not allowed on static ctor) and adds IL2117 (RequiresDynamicCode not allowed on static ctor) and IL2118 (RequiresAssemblyFiles not allowed on static ctor). Fixes ExpectedWarning not having effects on constructors by adding a Visit override in the compilation tree walker in the test infra. Adds case in GetDisplayName to differentiate .cctor's and .ctor's. Linker doesn't seem to be checking ctors for calls to methods with RUC. This commit removes the check for the expected warnings in the linker until the bug is fixed.

…otnet/linker#2455) Adds warnings in the analyzer for IL2116 (RUC not allowed on static ctor) and adds IL2117 (RequiresDynamicCode not allowed on static ctor) and IL2118 (RequiresAssemblyFiles not allowed on static ctor). Fixes ExpectedWarning not having effects on constructors by adding a Visit override in the compilation tree walker in the test infra. Adds case in GetDisplayName to differentiate .cctor's and .ctor's. Linker doesn't seem to be checking ctors for calls to methods with RUC. This commit removes the check for the expected warnings in the linker until the bug is fixed. Commit migrated from dotnet/linker@2999a6a

jtschuster added 9 commits December 15, 2021 18:58

Init for adding analyzer support to warn on Requires on cctor

2ff076e

Adds an ExpectedWarning for IL3051 to DerivedClassWithAllWarnings

500a542

Update test to expect RAF and RDC on cctor warnings in analyzer only

c35ab2b

Revert Cecil version Change

f9c57ee

Add a visit to ConstructorDeclaration in syntax walker test infra

db0084b

Test case for ExpectWarning not working as expected in analyzer tests

e781898

Adds warning on static constructors to analyzers

0835e40

Remove RequiresDynamicCode ExpectedWarnings

b95fc44

reset Cecil to correct version

7c03ea5

jtschuster requested a review from marek-safar as a code owner December 16, 2021 02:09

jtschuster requested review from tlakollo and vitek-karas December 16, 2021 02:09

tlakollo reviewed Dec 16, 2021

View reviewed changes

src/ILLink.RoslynAnalyzer/ILLink.RoslynAnalyzer.csproj Outdated Show resolved Hide resolved

tlakollo reviewed Dec 16, 2021

View reviewed changes

jtschuster added 2 commits December 16, 2021 11:08

Renumber diagnostic IDs, use Roslyn member notation instead of IL not…

645cee2

…ation in diagnostics

remove ..cctor from static constructor diagnostic messages

a19ffb6

jtschuster force-pushed the RequiresOnCctorWarns branch from a430e76 to a19ffb6 Compare December 16, 2021 19:12

jtschuster requested a review from tlakollo December 16, 2021 19:24

jtschuster added 2 commits December 20, 2021 10:27

uses 'static' modifiers in cctor diagnostic messages

da05d3f

uses ..cctor() notation in diagnostic messages

04270d6

This was linked to issues Dec 20, 2021

GetDisplayName() and FullName use different notation for nested classes #2441

Closed

ExpectedWarning doesn't check for warnings on constructors #2446

Closed

Analyzer allows RUC/RAF in static constructor #2347

Closed

jtschuster removed a link to an issue Dec 21, 2021

GetDisplayName() and FullName use different notation for nested classes #2441

Closed

jtschuster requested review from agocke and sbomer January 4, 2022 20:46

jtschuster commented Jan 4, 2022

View reviewed changes

tlakollo reviewed Jan 6, 2022

View reviewed changes

src/ILLink.Shared/SharedStrings.resx Outdated Show resolved Hide resolved

vitek-karas changed the title ~~Analyzer warns when Requires... Attribute is on a static constructor~~ [release/6.0.2xx] Analyzer warns when Requires... Attribute is on a static constructor Jan 6, 2022

jtschuster added 4 commits January 6, 2022 10:52

Add test for ExpectedWarning on ctor and cctor

2425bb2

Fix formatting

54dbedc

Accomodate for potential bug found in linker

cb9fc19

Linker doesn't seem to be checking ctors for calls to methods with RUC. This commit removes the check for the expected warnings in the linker until the bug is fixed.

fix shared string formatting

aa720df

tlakollo reviewed Jan 6, 2022

View reviewed changes

src/ILLink.Shared/DiagnosticId.cs Show resolved Hide resolved

jtschuster and others added 3 commits January 6, 2022 13:17

Remove extra line in DiagnosticId.cs

b26e96a

References the new test class in the test code so it's not trimmed

e6eb758

Merge branch 'RequiresOnCctorWarns' of https://github.com/jtschuster/…

66f39a8

…linker into RequiresOnCctorWarns

tlakollo approved these changes Jan 6, 2022

View reviewed changes

Fix formatting

c78a81a

jtschuster changed the base branch from release/6.0.2xx to main January 20, 2022 00:13

vitek-karas approved these changes Jan 20, 2022

View reviewed changes

Merge with main

011dcd2

tlakollo reviewed Jan 21, 2022

View reviewed changes

update cecil and fix extra line

46f0c68

jtschuster changed the title ~~[release/6.0.2xx] Analyzer warns when Requires... Attribute is on a static constructor~~ Analyzer warns when Requires... Attribute is on a static constructor Jan 21, 2022

Remove extra comma in DiagnosticId.cs

6bdad7a

jtschuster merged commit 2999a6a into dotnet:main Jan 24, 2022

jtschuster deleted the RequiresOnCctorWarns branch February 17, 2022 17:25

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

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

Uh oh!

Conversation

jtschuster commented Dec 16, 2021 • edited by vitek-karas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer Impact

Testing

Risk

Uh oh!

tlakollo commented Dec 16, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtschuster Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlakollo Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtschuster Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlakollo Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tlakollo left a comment

Choose a reason for hiding this comment

Uh oh!

tlakollo commented Jan 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jtschuster commented Dec 16, 2021 •

edited by vitek-karas

Loading

jtschuster Dec 16, 2021 •

edited

Loading

tlakollo Dec 20, 2021 •

edited

Loading

jtschuster Jan 4, 2022 •

edited

Loading

tlakollo Jan 6, 2022 •

edited

Loading