Ruc on classes #2143

tlakollo · 2021-07-13T01:59:34Z

Add support for RequiresUnreferencedCode on classes
Added a cache to fast lookup for RequiresUnreferencedCode on the type hierarchy
Added special case for warning in constructors while marking types
Added logic to handle suppressions of other trim analysis warnings when the declaring type has RUC
Added tests

Fixes #1742

Added a cache to fast lookup for RequiresUnreferencedCode on the type hierarchy Added special case for warning in constructors while marking types Added logic to handle suppressions of other trim analysis warnings, when the declaring type has RUC Added tests

src/linker/Linker/Annotations.cs

MichalStrehovsky · 2021-07-13T08:08:09Z

src/linker/Linker.Steps/MarkStep.cs

-				string arg2 = MessageFormat.FormatRequiresAttributeUrlArg (requiresUnreferencedCode.Url);
-				string message = string.Format (formatString, method.GetDisplayName (), arg1, arg2);
-				_context.LogWarning (message, 2026, currentOrigin, MessageSubCategory.TrimAnalysis);
+			if (method.IsStatic || method.IsConstructor) {


We might need to think about how this applies to static constructors.

Is this going to warn anywhere?

[RUC] class Foo { public static object Instance; static Foo() { Instance = Activator.CreateInstance(Type.GetType(WebClient.DownloadString("https//example.com/"))); } } class Program { static void Main() => Console.WriteLine(Foo.Instance.ToString()); }

The more I think about it, the more I feel we should disallow placing RUC on static constructors because it makes things too damn complicated.

Given that we can now put RUC on the type itself - the value of supporting it on static .ctor seems questionable. The RUC on static .ctor should produce warnings in almost all the cases where RUC on type will (I know that there are some potential exceptions, where the runtime is allowed to call a static method without calling the static .ctor). From a UX perspective I don't see much of a difference.

I think it should be fine to remove support for RUC on static .ctor once we have this merged. Note that we should see how many cases in libraries use RUC on static .ctor today.

I would expect the above to warn in Main - that said this is something which currently probably doesn't work and we'll have to add that (check for RUC on declaring type when accessing static fields). Obsolete already handles this as expected (access to any static member issues the warning).

I agree that once we have RUC on types, it makes a lot less sense and is a lot more complicated to make it work for static constructors.

Placing Obsolete on a static constructor doesn't seem to do anything. It's not a behavior we can be okay with for RUC.

I was more thinking along the lines of:

RUC on a class doesn't apply to the cctor (it will not suppress anything within the cctor)

Placing RUC on a cctor is always a warning

We cannot deprecate RUC on a cctor in favor of RUC on a class because we don't allow RUC on structs and interfaces but those can have cctors.

Allowing RUC on a cctor complicates things by a lot. I'm not sure we handle them:

// Where do we warn? ((IFoo)new A()).Frob(); interface IFoo { static IFoo { // This code will run when the default implementation on IFoo is used } void Frob() { } } interface IBar : IFoo { static IBar() { // This will run when the default implementation is used } void IFoo.Frob() { } } class A : IFoo { }

// Where do we warn? _ = Foo.MyField; // Where do we warn? typeof(Foo).GetField("MyField").GetValue(null); class Foo { static Foo() { // Will run if a field is accessed } public static int MyField; }

Of course there will be a customer who really really wants to put this on a cctor. We tell them to lazy-initialize using their own devices.

Roslyn is actually always warning about the type - not about the member. That's why it also warns on a cast to the type, or warns when declaring a local. We're not going to warn on those I assume.

What would be the problem with warning for these patterns? It doesn't seem like you can do anything too useful with just casts/local declarations, unless you also try to instantiate the type or call static methods.

What would be the problem with warning for these patterns? It doesn't seem like you can do anything too useful with just casts/local declarations, unless you also try to instantiate the type or call static methods.

It will produce a lot more unnecessary warnings.

I think there's a lot of value in avoiding too much unnecessary noise. It we can avoid generating a warning for something that is not actually unsafe, we should. The point of the warnings it to prevent different behavior after trimming, not "to be consistent with ObsoleteAttribute". If the thing illink is warning about can under no circumstances cause a runtime difference, that warning is useless and noise for the developer.

(That's why I'm also asking about the static fields.)

Don't let the class RUC apply to the static constructor. People get a warning in the static constructor and have to deal with it.

The main problem is this. How do we expect people to deal with calls to RUC methods in static initializers? It seems like it would imply a significant code change, mostly to reduce our implementation complexity.

I think we can probably resolve this in a separate PR -- this one probably doesn't change the user experience significantly.

The main problem is this. How do we expect people to deal with calls to RUC methods in static initializers? It seems like it would imply a significant code change, mostly to reduce our implementation complexity.

Yup, same as we scoped out RUC on structs and interfaces. I don't even know how we would be able to support the semantics of "cctor is considered RUC" there (the problem is that for structs and interfaces, the cctor is going to run when virtual methods get invoked - Equals/GetHashCode/ToString/interface methods - so we would have to mark as RUC those too; but that immediately causes a warning because the RUC annotation is inconsistent across virtuals and we can't guarantee it would warn - we can ignore the problem for classes because the cctor is triggered from the ctor there). I'm sure we would come up with some solution. But also it's very complex.

If we say RUC on struct and interface cctors is never going to work (we're also going to block it if people explicily add it to their cctor), we are already blocking a scenario because of complexity. People will have to work around if they hit it. We can document it.

Allowing RUC on class cctors (either explicityly placed on the cctor or propagated from the class-level RUC) is also pretty complex if we want to do it in a way that it doesn't produce meaningless warnings. The workarounds in the absence of that RUC handling are the same as for interfaces and structs.

src/linker/Linker/Annotations.cs

src/linker/Linker.Steps/MarkStep.cs

vitek-karas · 2021-07-13T15:05:09Z

src/linker/Linker.Steps/MarkStep.cs

+
+			// If the caller of a type is already marked with `RequiresUnreferencedCodeAttribute` a new warning should not
+			// be produced for the callee.
+			if (ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode ())


This will probably also not handle suppressions correctly since the current scope will not be right.

I have some scenarios with suppressions that are working fine, although is not extensive. Do you have a particular scenario that you would like me to test?

test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs

src/linker/Linker.Steps/MarkStep.cs

-Instead of cache now the lookup is via a while loop -Added new warning code for using types which reference a basetype annotated with RUC, also inlined this warning generation on MarkType instead of having another method -Solved issue where the new warning will only warn if the basetype if it had the attribute but not if any of the parents of the basetype had it -Renamed methods to be more clear and added doc comments -Added a test for UnconditionalSuppressMessage -Moved IL2109 warning generation to a later stage when CustomAttributes are already marked to support suppressions via attribute

src/linker/Linker.Steps/MarkStep.cs

src/linker/Linker/Annotations.cs

agocke · 2021-07-16T20:29:53Z

src/linker/Linker.Steps/MarkStep.cs

-				string arg2 = MessageFormat.FormatRequiresAttributeUrlArg (requiresUnreferencedCode.Url);
-				string message = string.Format (formatString, method.GetDisplayName (), arg1, arg2);
-				_context.LogWarning (message, 2026, currentOrigin, MessageSubCategory.TrimAnalysis);
+			if (method.IsStatic || method.IsConstructor) {


I think we can probably resolve this in a separate PR -- this one probably doesn't change the user experience significantly.

agocke · 2021-07-16T20:31:08Z

src/linker/Linker/Annotations.cs

+		/// <returns>Returns true along with the RequiresUnreferencedCodeAttribute if found, otherwise returns false</returns>
+		public bool TryGetEffectiveRequiresUnreferencedCodeAttributeOnType (TypeDefinition type, out RequiresUnreferencedCodeAttribute attribute)
+		{
+			do {


Is this the same as the above function, but also produces the attribute?

Yup, I was thinking on calling always TryGetEffectiveRequiresUnreferencedCodeAttributeOnType and discard the attribute for the cases I do HasEffectiveRequiresUnreferencedCodeOnType. Don't have a strong opinion for either way.

I don't see why we wouldn't call the other method and discard the extra item rather than duplicate the code.

Sorry, I don't think I was clear -- you could have kept the other method, and had it just call this one with a discard. My objection was to duplicating the while loop, not having two methods.

I dont know it will be just

public bool HasEffectiveRequiresUnreferencedCodeOnType (TypeDefinition type) => TryGetEffectiveRequiresUnreferencedCodeAttributeOnType (type, out RequiresUnreferencedCodeAttribute _);

I think I prefer just call it

test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs

src/linker/Linker.Steps/MarkStep.cs

with RUC

agocke

LGTM, thanks!

vitek-karas · 2021-07-19T08:37:21Z

I think @MichalStrehovsky has a great point in #2151. We should also change the behavior for that.

Add support for RequiresUnreferencedCode on classes Added a method to retrieve the effective RequiresUnreferencedCode attribute, which means it will look for the attribute on the type or any of its declaring types Added new warning IL2109 for when a type derives from a base type with effective RequiresUnreferencedCode, and the derived doesn't have RequiresUnreferencedCode on its effective type Added logic to handle suppressions Adds tests Commit migrated from dotnet/linker@460dd6d

tlakollo added 2 commits July 12, 2021 18:50

Typo

4ba2108

tlakollo requested review from agocke, sbomer and vitek-karas July 13, 2021 01:59

tlakollo self-assigned this Jul 13, 2021

tlakollo requested a review from marek-safar as a code owner July 13, 2021 01:59

MichalStrehovsky reviewed Jul 13, 2021

View reviewed changes

vitek-karas reviewed Jul 13, 2021

View reviewed changes

sbomer reviewed Jul 13, 2021

View reviewed changes

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Jul 15, 2021

View reviewed changes

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Jul 15, 2021

View reviewed changes

src/linker/Linker/Annotations.cs Outdated Show resolved Hide resolved

tlakollo added 3 commits July 15, 2021 11:34

PR comments

30d549b

Adding some more testing to the PR

6fbacb1

Merge branch 'main' of https://github.com/mono/linker into RucOnClasses

94dfd27

agocke reviewed Jul 16, 2021

View reviewed changes

src/linker/Linker.Steps/MarkStep.cs Show resolved Hide resolved

tlakollo added 3 commits July 16, 2021 14:33

Add a test where a nested method inside a nested class calls something

b8e4420

with RUC

Delete HasEffectiveRequiresUnreferencedCodeOnType

cf59cc4

PR Comments

3d0d719

agocke approved these changes Jul 16, 2021

View reviewed changes

tlakollo merged commit 460dd6d into dotnet:main Jul 17, 2021

tlakollo deleted the RucOnClasses branch July 17, 2021 00:00

sbomer mentioned this pull request Jul 28, 2021

Add document explaining RUC on types usage and scenarios #2177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruc on classes #2143

Ruc on classes #2143

tlakollo commented Jul 13, 2021

MichalStrehovsky Jul 13, 2021

vitek-karas Jul 13, 2021

vitek-karas Jul 13, 2021

agocke Jul 13, 2021

MichalStrehovsky Jul 14, 2021

sbomer Jul 15, 2021

MichalStrehovsky Jul 16, 2021

agocke Jul 16, 2021

agocke Jul 16, 2021

MichalStrehovsky Jul 17, 2021 •

edited

Loading

vitek-karas Jul 13, 2021

tlakollo Jul 15, 2021

agocke Jul 16, 2021

agocke Jul 16, 2021

tlakollo Jul 16, 2021

agocke Jul 16, 2021

agocke Jul 16, 2021

tlakollo Jul 16, 2021

agocke left a comment

vitek-karas commented Jul 19, 2021

Ruc on classes #2143

Ruc on classes #2143

Conversation

tlakollo commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

vitek-karas commented Jul 19, 2021

MichalStrehovsky Jul 17, 2021 •

edited

Loading