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

Trim analyzer: Implement intrinsics handling of object.GetType #93732

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

vitek-karas
Copy link
Member

@vitek-karas vitek-karas commented Oct 19, 2023

This fixes #86921.

Analyzer so far didn't handle correct data flow around object.GetType and DynamicallyAccessedMembersAttribute on types. This change implements that behavior.

Main changes:

  • Move IValueWithStaticType to the shared code and refactor its existing usage in trimmer/AOT to use the shared code instead. Also implement it for the analyzer.
  • Refactor method call handling in the analyzer to a single static method which is called both from the visitor and from the patterns.
  • In order to get same behavior, start tracking values for all fields and method parameters.

Outside of the actual fix, the other main change is that analyzer now tracks values for all fields and method parameters, regardless if their type is interesting to analysis. This is necessary because the static type now matters, even if it's something else than System.Type.

The effect of that is that the analyzer now recognizes lot more invalid cases because it can determine if the value is something unrecognizable. Before the change such values where tracked as "empty", and thus analysis ignored them. Now they're track as "value of a field, without annotations" which can lead to producing more warnings.

That means this resolves lot of cases in dotnet/linker#2755. At least all the test cases which were added because of that bug, or which expected different behavior because of it now produce consistent behavior with trimmer/NativeAOT.

This fixes dotnet#86921.

Analyzer so far didn't handle correct data flow around `object.GetType` and `DynamicallyAccessedMembersAttribute` on types. This change implements that behavior.

Main changes:
* Move `IValueWithStaticType` to the shared code and refactor its existing usage in trimmer/AOT to use the shared code instead. Also implement it for the analyzer.
* Refactor method call handling in the analyzer to a single static method which is called both from the visitor and from the patterns.
* In order to get same behavior, start tracking values for all fields and method parameters.

Outside of the actual fix, the other main change is that analyzer now tracks values for all fields and method parameters, regardless if their type is interesting to analysis. This is necessary because the static type now matterns, even if it's something else than `System.Type`.

The effect of that is that the analyzer now recognizes lot more invalid cases because it can determine if the value is something unrecognizable. Before the change such values where tracked as "empty", and thus anslysis ignored them. Now they're track as "value of a field, without annotations" which can lead to producing more warnings.

That means this effectively fixes dotnet/linker#2755. At least all the test cases which were added because of that bug, or which expected different behavior because of it now produce consistent behavior with trimmer/NativeAOT.
@vitek-karas vitek-karas added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Oct 19, 2023
@vitek-karas vitek-karas added this to the 9.0.0 milestone Oct 19, 2023
@vitek-karas vitek-karas self-assigned this Oct 19, 2023
@ghost
Copy link

ghost commented Oct 19, 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 fixes #86921.

Analyzer so far didn't handle correct data flow around object.GetType and DynamicallyAccessedMembersAttribute on types. This change implements that behavior.

Main changes:

  • Move IValueWithStaticType to the shared code and refactor its existing usage in trimmer/AOT to use the shared code instead. Also implement it for the analyzer.
  • Refactor method call handling in the analyzer to a single static method which is called both from the visitor and from the patterns.
  • In order to get same behavior, start tracking values for all fields and method parameters.

Outside of the actual fix, the other main change is that analyzer now tracks values for all fields and method parameters, regardless if their type is interesting to analysis. This is necessary because the static type now matters, even if it's something else than System.Type.

The effect of that is that the analyzer now recognizes lot more invalid cases because it can determine if the value is something unrecognizable. Before the change such values where tracked as "empty", and thus analysis ignored them. Now they're track as "value of a field, without annotations" which can lead to producing more warnings.

That means this effectively fixes dotnet/linker#2755. At least all the test cases which were added because of that bug, or which expected different behavior because of it now produce consistent behavior with trimmer/NativeAOT.

Author: vitek-karas
Assignees: vitek-karas
Labels:

area-Tools-ILLink

Milestone: 9.0.0

@vitek-karas
Copy link
Member Author

Tracking more values means that the analyzer is potentially slower. I'll try to measure this somehow.

@agocke
Copy link
Member

agocke commented Oct 20, 2023

This looks good -- but I'm not sure why some things weren't being tracked before.

That is, my understanding of why typeof(Blah).MakeGenericType doesn't warn is that the receiver wasn't tracked for some reason. But typeof(Blah) is definitely an instance of System.Type. So why wasn't it tracked?

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.

One question about the treatment of "empty", otherwise LGTM, thanks!

This leaves 2 test cases where the analyzer doesn't detect a warning, but they are relative corner cases.
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.

Trim analyzer doesn't warn on GetType().GetMethods()
3 participants