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 doesn't warn on GetType().GetMethods() #86921

Closed
agocke opened this issue May 30, 2023 · 2 comments · Fixed by #93732
Closed

Trim analyzer doesn't warn on GetType().GetMethods() #86921

agocke opened this issue May 30, 2023 · 2 comments · Fixed by #93732
Assignees
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Milestone

Comments

@agocke
Copy link
Member

agocke commented May 30, 2023

The following doesn't seem to produce an analyzer warning:

class C
{
    public static void Main()
    {
        M("abc");
    }
    public static void M(object o)
    {
        var methods = o.GetType().GetMethods();
    }
}
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 30, 2023
@ghost
Copy link

ghost commented May 30, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

The following doesn't seem to produce an analyzer warning:

class C
{
    public static void Main()
    {
        M("abc");
    }
    public static void M(object o)
    {
        var methods = o.GetType().GetMethods();
    }
}
Author: agocke
Assignees: -
Labels:

area-System.Reflection

Milestone: -

@agocke agocke added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-System.Reflection untriaged New issue has not been triaged by the area owner labels May 30, 2023
@agocke agocke added this to the 8.0.0 milestone May 30, 2023
@vitek-karas
Copy link
Member

There are two parts to this:

One is that we haven't implemented type hierarchy annotations in the analyzer at all yet. That said implementing the object.GetType() is definitely the simple part - implementing full type hierarchy marking diagnostics is much more work. But the two could be done separatly.

Second is that analyzer has a different philosophy to trimmer and ilc. The design is such that if the analyzer doesn't know about something, it will NOT warn about it. Which is the opposite of trimmer and ilc both of which would warn as soon as they hit something they can't reason about. This is intentional to avoid producing warnings from the analyzer which are later on NOT reported by the trimmer/ilc and thus just confusing.
This was very important during analyzer bring up, as we didn't want to produce tons of warnings just because the analyzer didn't implement some specific behavior yet.
This specific case is an example of such case (what if the call is OK, without implementing the logic the analyzer would always warn about it).

Maybe it's about time we change the policy - given that analyzer is almost on par with trimmer now.

That said, there are (and always will be) cases where the analyzer can't produce the same warnings because it doesn't have a full program view. These should be rare, but they will always exist. Hopefully if we switch the policy these cases won't start to show up as extra noise (and only as missing warnings, which there are already quite a few).

@agocke agocke modified the milestones: 8.0.0, 9.0.0 Sep 1, 2023
vitek-karas added a commit to vitek-karas/runtime that referenced this issue Oct 19, 2023
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.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 19, 2023
vitek-karas added a commit that referenced this issue Nov 1, 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 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.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 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
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants