From f3064f7b3d95b5c63eb7b13f85048caf9a1545d2 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Mon, 26 Jul 2021 07:41:11 -0700 Subject: [PATCH 1/5] Design proposal for DAM-on-type and RUC interactions --- .../design/DAM-on-type-and-RUC-interaction.md | 151 ++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 docs/design/DAM-on-type-and-RUC-interaction.md diff --git a/docs/design/DAM-on-type-and-RUC-interaction.md b/docs/design/DAM-on-type-and-RUC-interaction.md new file mode 100644 index 000000000000..48229aceab87 --- /dev/null +++ b/docs/design/DAM-on-type-and-RUC-interaction.md @@ -0,0 +1,151 @@ +# Interaction of DAM and RUC on type and members + +## Existing rules + +* RUC on method suppresses warnings from the method's body and attributes. +* RUC on method doesn't suppress warnings about the method usage (call to the method, accessing the method via reflection, ...) +* RUC on type externally behaves as if all static members and constructors have RUC +* RUC on type suppresses warnings as if all methods in the type have RUC on them (so method bodies and attributes) +* RUC on type does NOT suppress warnings about/on nested types in any way + +## DAM on type behavior + +Annotating type with DAM effectively creates a reflection access to all affected members (depends on the annotation). The behavior in terms of getting a warning or not should be equivalent to accessing the member directly through reflection (`GetMethod` or similar). + +For better warning UX and suppression we decided to generate the warnings from: + +* The member itself if the type or one of its base types is annotated with a DAM which accesses the member in question +* The type which adds DAM annotation which accesses a member on a base type (so unlike the above rule, we don't warn on the member, but on the type which added the DAM) + +*Note: the reasoning behind this behavior is to warn on the thing which is to blame for the problem. If type derives from annotated type, then the source of the problem is adding a member which will be accessed by such DAM and has issues (like RUC on it). If type adds DAM on itself, but such DAM causes access to a problematic member on base type, then the one to blame is the annotated type.* + +## RUC on type and interaction with DAM on type + +```csharp +[DAM(All)] +class AnnotatedBase {} + +[RUC] +class DerivedWithRUC : AnnotatedBase +{ + [RUC] + public void MethodWithRUC() {} + + public void MethodWithDAM([DAM(Fields)] Type type) {} +} +``` + +Following the existing rules, accessing `MethodWithRUC` or `MethodWithDAM` via reflection will generate a warning, regardless of the RUC on type. +Since DAM on type is meant to behave as if accessing the members via reflection, those warnings should not be affected by the RUC on type either. + +## RUC on method and interaction with DAM on type + +```csharp +[DAM(All)] +class AnnotatedBase {} + +class Derived : AnnotatedBase +{ + [RUC] + public void MethodWithRUC() {} + + public void MethodWithDAM([DAM(Fields)] Type type) {} + + [RUC] + public void MethodWithRUCAndDAM([DAM(Fields)] Type type) {} +} +``` + +Following the existing rules, accessing `MethodWithRUC` or `MethodWithDAM` via reflection will generate a warning. The same rules also imply that accessing `MethodWithRUCAndDAM` via reflection will also generate a warning (actually two of them). +RUC on method does not affect usage of the method (other than it should generate a warning on the usage). + +## RUC or DAM on nested type + +```csharp +[DAM(All)] +class AnnotatedBase {} + +class Derived : AnnotatedBase +{ + [RUC] + class NestedWithRUC {} + + [DAM] + class NestedWithDAM {} +} +``` + +This is probably an existing hole. Accessing a RUC or DAM type itself via reflection is not problematic on its own (since both annotations should behave as applying to members only). But it can create a hole in the analysis because of annotation propagation through `GetNestedType`. For example (given the above code): + +```csharp +void TestMethod(Derived instance) +{ + Type typeOfDerived = instance.GetType(); // The type value is effectively annotated with All + Type nestedWithRUC = typeOfDerived.GetNestedType("NestedWithRUC"); // The resulting type value is effectively annotated with All + Activator.CreateInstance(nestedWithRUC); // No warning - nestedWithRUC has All annotation on it +} +``` + +The above MUST warn, since it's calling a `.ctor` on a RUC annotated type. + +DAM on nested type doesn't seem to have a problem mainly because for marking purposes it's not needed (the DAM on the outer type if it applies to the nested type will mark the entire nested type, so DAM on it can't mark more). +Also accessing DAM annotated types themselves is not a problem, and accessing their members is drive by the DAM annotation. +This needs verification!!! (It is tricky) + +## Conclusion + +Personally I'm not a big fan of the fact that RUC on type doesn't suppress the new warnings on the type's members - as a normal user I would expect that to happen. That said, doing so would introduce an "Exception" to the existing rules, which is not ideal either. If we warn more (RUC on type doesn't affect the new warnings), it sort of creates noise, but it's not exactly wrong. And we can change our mind later on (by not warning anymore some day in the future). + +In the end the interaction is very simple, the new warnings caused by DAM on type are not affected by RUC in any way (as in, they're never suppressed by RUC). + +## Samples + +```csharp +[DAM(All)] +class AnnotatedBase {} + +[RUC] +class DerivedWithRUC : AnnotatedBase +{ + // IL2112 + [RUC] + public void MethodWithRUC() {} + + // IL2114 + public static void MethodWithDAM([DAM] Type type) {} + + // IL2112 + // IL2114 + [RUC] + public void MethodWithDAMAndRUC([DAM] Type type) {} + + // IL2114 + [DAM] + public Type FieldWithDAM; + + // IL2112 - we must do this here, since we won't be able to see the RUC anywhere else + [RUC] + public class NestedWithRUC {} + + // No warnings - ??? Probably ??? - I think the DAM on the outer type effectively implies DAM(All) on the nested type + // and thus declaring it explicitly has no effect. But this needs validation specifically for cases where the outer type + // has DAM(PublicNestedTypes) on it (the new behavior should still imply All on the nested type in such case I think) + [DAM(Fields)] + public class NestedWithDAM {} +} + +class Derived : AnnotatedBase +{ + // IL2112 + [RUC] + public void MethodWithRUC() {} + + // IL2114 + public static void MethodWithDAM([DAM] Type type) {} + + // IL2112 + // IL2114 + [RUC] + public void MethodWithDAMAndRUC([DAM] Type type) {} +} +``` From 1d919ddcebca98037a4624d944a1b6b65ca443ac Mon Sep 17 00:00:00 2001 From: Vitek Karas Date: Mon, 26 Jul 2021 21:40:03 +0200 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Sven Boemer --- docs/design/DAM-on-type-and-RUC-interaction.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/DAM-on-type-and-RUC-interaction.md b/docs/design/DAM-on-type-and-RUC-interaction.md index 48229aceab87..4c75f38217b6 100644 --- a/docs/design/DAM-on-type-and-RUC-interaction.md +++ b/docs/design/DAM-on-type-and-RUC-interaction.md @@ -89,7 +89,7 @@ void TestMethod(Derived instance) The above MUST warn, since it's calling a `.ctor` on a RUC annotated type. DAM on nested type doesn't seem to have a problem mainly because for marking purposes it's not needed (the DAM on the outer type if it applies to the nested type will mark the entire nested type, so DAM on it can't mark more). -Also accessing DAM annotated types themselves is not a problem, and accessing their members is drive by the DAM annotation. +Also accessing DAM annotated types themselves is not a problem, and accessing their members is driven by the DAM annotation. This needs verification!!! (It is tricky) ## Conclusion From 20b57e3aecb0ce2f83ac8013c25178802394b249 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Mon, 26 Jul 2021 12:45:47 -0700 Subject: [PATCH 3/5] Add a warning about suppressing these warnings. --- docs/design/DAM-on-type-and-RUC-interaction.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/design/DAM-on-type-and-RUC-interaction.md b/docs/design/DAM-on-type-and-RUC-interaction.md index 4c75f38217b6..6997650572e0 100644 --- a/docs/design/DAM-on-type-and-RUC-interaction.md +++ b/docs/design/DAM-on-type-and-RUC-interaction.md @@ -98,6 +98,8 @@ Personally I'm not a big fan of the fact that RUC on type doesn't suppress the n In the end the interaction is very simple, the new warnings caused by DAM on type are not affected by RUC in any way (as in, they're never suppressed by RUC). +**Important note:** Suppressing the new warnings is very likely to create unexpected breaks in the app. The DAM annotation on the type is effectively "public" in that anybody can use it. So for example reasoning like "This warning is OK because the one use where the DAM is utilized will not actually access this method." is very problematic, since if there's more than one use of the DAM (either already, or in the future), the trimmer will not generate new warnings. The DAM on type is effectively part of the type's contract, and as such the type should fulfill the contract. And part of such contract is that none of the members referenced by the DAM is dangerous to use in trimmed apps. + ## Samples ```csharp From f4ed417733203c7a18016d476b8587473de59664 Mon Sep 17 00:00:00 2001 From: vitek-karas <10670590+vitek-karas@users.noreply.github.com> Date: Fri, 21 Jan 2022 02:46:17 -0800 Subject: [PATCH 4/5] Small improvements --- docs/design/DAM-on-type-and-RUC-interaction.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/design/DAM-on-type-and-RUC-interaction.md b/docs/design/DAM-on-type-and-RUC-interaction.md index 6997650572e0..03be7c6d6f8d 100644 --- a/docs/design/DAM-on-type-and-RUC-interaction.md +++ b/docs/design/DAM-on-type-and-RUC-interaction.md @@ -1,5 +1,8 @@ # Interaction of DAM and RUC on type and members +DAM = `DynamicallyAccessedMembers` attribute +RUC = `RequiresUnreferencedCode` attribute + ## Existing rules * RUC on method suppresses warnings from the method's body and attributes. @@ -11,8 +14,9 @@ ## DAM on type behavior Annotating type with DAM effectively creates a reflection access to all affected members (depends on the annotation). The behavior in terms of getting a warning or not should be equivalent to accessing the member directly through reflection (`GetMethod` or similar). +Applying "reflection access" on a member can produce additional warnings. Specifically if the member in question has RUC on it (or if itself has DAM annotation somewhere in it, like method parameters for example). -For better warning UX and suppression we decided to generate the warnings from: +For better warning UX and suppression we decided to generate the warnings with this origin: * The member itself if the type or one of its base types is annotated with a DAM which accesses the member in question * The type which adds DAM annotation which accesses a member on a base type (so unlike the above rule, we don't warn on the member, but on the type which added the DAM) @@ -21,6 +25,8 @@ For better warning UX and suppression we decided to generate the warnings from: ## RUC on type and interaction with DAM on type +RUC on type doesn't suppress warnings caused by DAM on type. + ```csharp [DAM(All)] class AnnotatedBase {} @@ -28,18 +34,28 @@ class AnnotatedBase {} [RUC] class DerivedWithRUC : AnnotatedBase { + public DerivedWithRUC () {} + [RUC] public void MethodWithRUC() {} public void MethodWithDAM([DAM(Fields)] Type type) {} + + private static void StaticMethod() {} } ``` Following the existing rules, accessing `MethodWithRUC` or `MethodWithDAM` via reflection will generate a warning, regardless of the RUC on type. Since DAM on type is meant to behave as if accessing the members via reflection, those warnings should not be affected by the RUC on type either. +Note that RUC on type as mentioned above is equivalent to applying RUC to all static members and constructors. So following that logic it means +that the DAM will cause warnings generated by static members and .ctors. So in the above example both `DerivedWithRUC..ctor` and `DerivedWithRUC.StaticMethod` +should generate warning due to the `RUC` on the type. + ## RUC on method and interaction with DAM on type +RUC on a method which is made accessible due to DAM on type will generate a warning (the method is "accessed"). + ```csharp [DAM(All)] class AnnotatedBase {} From d4f92558fc5bdbbc253809f2ba9ea7b4b6fd3108 Mon Sep 17 00:00:00 2001 From: vitek-karas <10670590+vitek-karas@users.noreply.github.com> Date: Fri, 21 Jan 2022 03:28:49 -0800 Subject: [PATCH 5/5] More cleanup --- docs/design/DAM-on-type-and-RUC-interaction.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/design/DAM-on-type-and-RUC-interaction.md b/docs/design/DAM-on-type-and-RUC-interaction.md index 03be7c6d6f8d..d9f8cff2d31b 100644 --- a/docs/design/DAM-on-type-and-RUC-interaction.md +++ b/docs/design/DAM-on-type-and-RUC-interaction.md @@ -91,22 +91,21 @@ class Derived : AnnotatedBase } ``` -This is probably an existing hole. Accessing a RUC or DAM type itself via reflection is not problematic on its own (since both annotations should behave as applying to members only). But it can create a hole in the analysis because of annotation propagation through `GetNestedType`. For example (given the above code): +This is probably an existing hole. Accessing a RUC or DAM type itself via reflection is not problematic on its own (since both annotations should behave as applying to members only). But using such type can be problematic. +Since just asking for the type of a DAM annotated type is considered reflection access to all of the members affected by the DAM, RUC on nested types must be reported if the nested type has any static members or .ctors. ```csharp void TestMethod(Derived instance) { + // This will warn that it's accessing NestedWithRUC..ctor Type typeOfDerived = instance.GetType(); // The type value is effectively annotated with All Type nestedWithRUC = typeOfDerived.GetNestedType("NestedWithRUC"); // The resulting type value is effectively annotated with All Activator.CreateInstance(nestedWithRUC); // No warning - nestedWithRUC has All annotation on it } ``` -The above MUST warn, since it's calling a `.ctor` on a RUC annotated type. - DAM on nested type doesn't seem to have a problem mainly because for marking purposes it's not needed (the DAM on the outer type if it applies to the nested type will mark the entire nested type, so DAM on it can't mark more). Also accessing DAM annotated types themselves is not a problem, and accessing their members is driven by the DAM annotation. -This needs verification!!! (It is tricky) ## Conclusion