-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Trimmer doesn't warn on reflection access to RUC override method #86008
Comments
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue DetailsFound by @MichalStrehovsky interface IBase
{
[RequriesUnreferencedCode("")]
void DoSomethingDangerous();
}
class Implementation : IBase
{
[RequiresUnreferencedCode("")]
public void DoSomethingDangerous() { Console.WriteLine("!!!DANGER!!!"); }
}
void CallMethodOnInstance<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] T>(T instance)
{
typeof(T).GetMethod("DoSomethingDangerous").Invoke(instance, Array.Empty<object>());
}
static void Main()
{
CallMethodOnInstance(new Implementation);
} The above will not produce any warning when trimmed, but when executed it will print out
|
There's an interesting discrepancy in behavior: TestDirectReflectionAccess(new Derived());
// IL2026 - Base.DoSomethingDangerous
TestAnnotatedReflectionAccess(new Derived());
void TestDirectReflectionAccess(object instance)
{
// IL2026 - Base.DoSomethingDangerous
// IL2026 - Derived.DoSomethingDangerous
typeof(Derived).GetMethod("DoSomethingDangerous")!.Invoke(instance, Array.Empty<object>());
}
void TestAnnotatedReflectionAccess<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] T>(T instance)
{
typeof(T).GetMethod("DoSomethingDangerous")!.Invoke(instance, Array.Empty<object>());
}
class Base
{
[RequiresUnreferencedCode("Base")]
public virtual void DoSomethingDangerous() { }
}
class Derived : Base
{
[RequiresUnreferencedCode("Derived")]
public override void DoSomethingDangerous() { }
} If the reflection access is "direct", so the analysis can figure out the exact type and the method name, it will actually warn about both the derived and base methods. But if the access is indirect through annotation, then it only warns about the base method (the same logic as described in the original issue description kicks in an override methods do not warn). |
Would it be very bad if we just warn about all occurrences? Was the deduplication added based on negative feedback or are we just being proactive? |
I'm still trying to figure out all the cases here, but in general I agree. My current working theory:
There's another interesting thing I found: I'm not sure we should try to do that - it's complex, linker will get it wrong (due to its buggy override resolution algo) and the value is questionable. |
Another interesting discrepancy. Analyzer doesn't implement any of the above mentioned logic in ILLink and so it will actually generate 2 warnings in the DAM case (for base and derived). |
Also related #86032 |
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 #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
Found by @MichalStrehovsky
The above will not produce any warning when trimmed, but when executed it will print out
!!!DANGER!!!
.The bug is that ILLink will not warn if a method is an override and is in RUC scope and is marked due to annotation (so broader marking like
PublicMethods
).This was done assuming that the same broad marking will also mark the base method which will warn. But that's not always true. Reflection doesn't return base interface methods when usual
GetMethod
is called. ILLink doesn't enumerate the base interface methods in this case either.The text was updated successfully, but these errors were encountered: