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

Analyzer - support unknown values for some untracked values #2755

Closed
vitek-karas opened this issue Apr 22, 2022 · 2 comments
Closed

Analyzer - support unknown values for some untracked values #2755

vitek-karas opened this issue Apr 22, 2022 · 2 comments
Milestone

Comments

@vitek-karas
Copy link
Member

vitek-karas commented Apr 22, 2022

Currently the analyzer only tracks values which are "interesting", which really means System.Type or System.String. Both of these can carry DAM annotations. Everything is not tracked and flows through the system as TopValue which means "empty".

In some cases though, the intrinsic handling works with other types of values, for example integers or arrays. Analyzer has some basic handling of integers mostly around constants and some basic handling of arrays when created locally. But it doesn't track "unknown" values, instead it tracks the rest as "empty" value.

"empty" value means that it will not cause warnings (that's the purpose of the value), unlike "unknown" which typically produces warnings as it represents "can't figure this our statically".

This means that for example code like this:

class MyOwnGeneric<[DAM(All)] T> { }

static void MakeMyOwnGeneric(Type[] types)
{
     typeof(MyOwnGeneric).MakeGenericType(types); // No warning in the analyzer, linker warns
}

In this case analyzer doesn't warn because the parameter to MakeGenericType is "empty" value, and so the entire call is effectively ignored. Linker warns, because it knows that the type has requirements on the generic parameters, and thus if it can't figure out the generic arguments it needs to warn.

@vitek-karas
Copy link
Member Author

class MyOwnGeneric<[DAM(All)] T> { }

static void MakeMyOwnGeneric(int length)
{
     var types = new Type[length]; // Since length is unknown the array should be either treated as unknown as a whole, or at least its size should be as such
     types[0] = typeof(TestType);
     typeof(MyOwnGeneric).MakeGenericType(types); // No warning in the analyzer, linker warns
}

Currently since the length is treated as "empty", the value of the array creation is also "empty" -> no warnings.

@vitek-karas
Copy link
Member Author

I'm going to add method tracking as well - similar scenario for MethodInfo values:

void GenericMethod<[DAM(All)] T>() {}

static void MakeItAGeneric(MethodInfo method)
{
    method.MakeGenericMethod(typeof(TestType));  // linker warns, analyzer doesn't
}

Same reason - the untracked value is considered "empty" (unlike linker where it's "unknown").

@vitek-karas vitek-karas changed the title Analyzer - support unknown values for arrays Analyzer - support unknown values for some untracked values Apr 22, 2022
@sbomer sbomer added this to the Future milestone Jul 28, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants