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.

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

Unify trim warnings for accessing compiler generated code #86816

Merged
merged 10 commits into from Jun 3, 2023

Conversation

vitek-karas
Copy link
Member

This modifies the behavior of both the illink and ilc to match and to implement some changes we've discussed recently - #86008.

The semantic changes (mostly as compared to previous illink behavior):

  • Reflection access to a RUC virtual method will produce warning on all methods which have RUC, regardless of their relationships. Specifically if both the override and base methods are accessed, and they both have RUC, they will both generate a warning. (This effectively removes an optimization which illink had to avoid "duplicate" warnings where it tried to warn only on the base methods in this case. But this has holes - see Trimmer doesn't warn on reflection access to RUC override method #86008).
  • Reflection access to compiler generated code will not produce warnings even if the target has RUC on it. It proved to be really difficult to design a good experience for reporting warnings in these cases. The problem is that developer has little control over the generated code and fully reporting all warnings leads to lot of noise. The above optimization in the illink tried to solve some of this, but it's not very successful. So we're removing all of these warnings. The reasoning is that reflection access to compiler generated members is an undefined behavior (even on non-trimmed, normal CLR) - and is likely to break for example between compiler versions - and there's no diagnostics about it ignoring trimming. Trimming/AOT just makes it a little bit worse.
  • AOT compiler will not implement warnings caused by reflection access to compiler generated code which has annotations either (so IL2118, IL2119 and IL2120). The plan is to eventually remove these from illink as well.

Note that there are exceptions to the above rules, which can be described as: Accessing a token of a member in IL is considered a reflection access (because the token can be turned into a reflection object), but it is also considered a direct reference. So in that case marking is implemented as "reflection access", but diagnostics is implemented as "direct reference". What this means is that accessing a compiler generated member through its token will still produce all warnings (RUC or DAM) as for non-compiler-generated member. This is important for things like creation of Action from a lambda, where the lambda is a compiler generated method, but we need to produce warnings if it uses annotations for example.

Refactorings:

  • Refactored code in MarkStep to move most of the diagnostics logic into ReportWarningsForReflectionAccess - this is to mirror the design in ilc and also to make it more in sync with the already existing ReportWarningsForTypeHierarchyReflectionAccess.

Test changes:

  • Adapting existing tests to the new behavior
  • Adding some new tests, specifically around reflection access to compiler generated code and token access to compiler generated code.
  • Enables CompilerGeneratedCodeAccessedViaReflection for ilc

@vitek-karas vitek-karas added the area-Tools-ILLink .NET linker development as well as trimming analyzers label May 26, 2023
@vitek-karas vitek-karas added this to the 8.0.0 milestone May 26, 2023
@vitek-karas vitek-karas requested a review from sbomer May 26, 2023 22:08
@vitek-karas vitek-karas self-assigned this May 26, 2023
@ghost
Copy link

ghost commented May 26, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

This modifies the behavior of both the illink and ilc to match and to implement some changes we've discussed recently - #86008.

The semantic changes (mostly as compared to previous illink behavior):

  • Reflection access to a RUC virtual method will produce warning on all methods which have RUC, regardless of their relationships. Specifically if both the override and base methods are accessed, and they both have RUC, they will both generate a warning. (This effectively removes an optimization which illink had to avoid "duplicate" warnings where it tried to warn only on the base methods in this case. But this has holes - see Trimmer doesn't warn on reflection access to RUC override method #86008).
  • Reflection access to compiler generated code will not produce warnings even if the target has RUC on it. It proved to be really difficult to design a good experience for reporting warnings in these cases. The problem is that developer has little control over the generated code and fully reporting all warnings leads to lot of noise. The above optimization in the illink tried to solve some of this, but it's not very successful. So we're removing all of these warnings. The reasoning is that reflection access to compiler generated members is an undefined behavior (even on non-trimmed, normal CLR) - and is likely to break for example between compiler versions - and there's no diagnostics about it ignoring trimming. Trimming/AOT just makes it a little bit worse.
  • AOT compiler will not implement warnings caused by reflection access to compiler generated code which has annotations either (so IL2118, IL2119 and IL2120). The plan is to eventually remove these from illink as well.

Note that there are exceptions to the above rules, which can be described as: Accessing a token of a member in IL is considered a reflection access (because the token can be turned into a reflection object), but it is also considered a direct reference. So in that case marking is implemented as "reflection access", but diagnostics is implemented as "direct reference". What this means is that accessing a compiler generated member through its token will still produce all warnings (RUC or DAM) as for non-compiler-generated member. This is important for things like creation of Action from a lambda, where the lambda is a compiler generated method, but we need to produce warnings if it uses annotations for example.

Refactorings:

  • Refactored code in MarkStep to move most of the diagnostics logic into ReportWarningsForReflectionAccess - this is to mirror the design in ilc and also to make it more in sync with the already existing ReportWarningsForTypeHierarchyReflectionAccess.

Test changes:

  • Adapting existing tests to the new behavior
  • Adding some new tests, specifically around reflection access to compiler generated code and token access to compiler generated code.
  • Enables CompilerGeneratedCodeAccessedViaReflection for ilc
Author: vitek-karas
Assignees: vitek-karas
Labels:

area-Tools-ILLink

Milestone: 8.0.0

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Thanks!

@vitek-karas vitek-karas merged commit 7319f86 into dotnet:main Jun 3, 2023
125 of 129 checks passed
@vitek-karas vitek-karas deleted the UnifyReflectionWarnings branch June 3, 2023 20:54
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants