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

NativeAot produces warning for generic instantiation which can't be suppressed #77455

Closed
sbomer opened this issue Oct 25, 2022 · 3 comments · Fixed by #80956, #81532 or #82392
Closed

NativeAot produces warning for generic instantiation which can't be suppressed #77455

sbomer opened this issue Oct 25, 2022 · 3 comments · Fixed by #80956, #81532 or #82392

Comments

@sbomer
Copy link
Member

sbomer commented Oct 25, 2022

Description

Instantiating a generic which has DynamicallyAccessedMemberTypes requirements with a type that has members marked RequiresUnreferencedCode correctly produces a warning. However, the warning is not suppressible by RequiresUnreferencedCodeAttribute or UnconditionalSuppressMessageAttribute.

The problem is that the message origin for the warning is an InstantiatedMethod, a type system entity that isn't associated with the calling context. Encountered while porting compiler-generated code tests from the linker as part of #68786.

Reproduction Steps

Publish the following with PublishAot:

using RUC = System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute;
using DAM = System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute;
using DAMT = System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes;
using USM = System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute;

class Program {
    static void Main() {
        Test();
    }
    [USM("", "IL2026:RequiresUnreferencedCode")]
    [RUC("")]
    static void Test() {
        MethodWithGenericWhichRequiresMethods<TypeWithMethodWithRequires>();
    }
    static void MethodWithGenericWhichRequiresMethods<[DAM(DAMT.All)] T>() {}
}

class TypeWithMethodWithRequires {
    [RUC("")]
 
    static void MethodWithRequires() {}
}

Expected behavior

RequiresUnreferencedCodeAttribute or UnconditionalSuppressMessageAttribute on Test should suppress the warning.

Actual behavior

ILC : Trim analysis warning IL2026: Program.MethodWithGenericWhichRequiresMethods<TypeWithMethodWithRequires>(): Using member 'TypeWithMethodWithRequires.MethodWithRequires()' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 25, 2022
@kant2002
Copy link
Contributor

kant2002 commented Jan 3, 2023

This issue was caused by last line with call to GetFlowDependenciesForInstantiation which is called inside MethodGenericDictionaryNode.ComputeNonRelocationBasedDependencies

public override void GetDependenciesForGenericDictionary(ref DependencyList dependencies, NodeFactory factory, MethodDesc method)
{
TypeDesc owningType = method.OwningType;
if (FlowAnnotations.HasAnyAnnotations(owningType))
{
MethodDesc typicalMethod = method.GetTypicalMethodDefinition();
Debug.Assert(typicalMethod != method);
GetFlowDependenciesForInstantiation(ref dependencies, factory, method.Instantiation, typicalMethod.Instantiation, method);

instantiated generic method variable with value Program.MethodWithGenericWhichRequiresMethods<TypeWithMethodWithRequires>() was given as source for depenndency flow analysis which is incorect because actual source of the call is Test method. From my limited understanding when MethodGenericDictionaryNode created information about call site is lost.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 25, 2023
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Jan 25, 2023
@vitek-karas
Copy link
Member

The fix has been reverted due to #81358.

@vitek-karas vitek-karas reopened this Jan 30, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 30, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 2, 2023
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Feb 7, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 20, 2023
vitek-karas added a commit that referenced this issue Feb 21, 2023
[This is a revert of a revert of #81532 with additional fixes for #81779]

This reworks how generic parameter data flow validation is done in the NativeAOT compiler.

Previously generic data flow was done from generic dictionary nodes. Problem with that approach is that there's no origin information at that point. The warnings can't point to the place where the problematic instantiation is in the code - we only know that it exists.
Aside from it being unfriendly for the users, it means any RUC or suppressions don't work on these warnings the same way they do in linker/analyzer.

This change modifies the logic to tag the method as "needs data flow" whenever we spot an instantiation of an annotated generic in it somewhere.
Then the actual validation/marking is done from data flow using the trim analysis patterns.

The only exception to this is generic data flow for base types and interface implementations, that one is done on the EEType nodes.

Note that AOT implements a much more precise version of the generic data flow validation as compared to linker/analyzer. See the big comment at the beginning of `GenericParameterWarningLocation.cs` for how that works.

Due to an issue with DependencyInjection, this change also implements a behavior where if a method or field is reflection accessible, the compiler will perform generic argument data flow on all types in the signature of the method/field (which it normally wouldn't do). See #81358 for details about the issue and discussions on the fix approach.

Due to the DI behavior described above, there's also the problem with nested generics. If a nested generic applies annotation on a specific type and this whole thing is done from within a DI, the compiler will not apply the annotation, since it doesn't see the type being used anywhere for real. See #81779 for detailed description of the issue. The fix for this is to extend the "needs data flow analysis" logic to look into generic arguments recursively and finding any annotation then triggers the data flow processing of the calling code. Then in that processing when applying generic argument data flow, do so recursively over all generic parameters.

Test changes:
Adds the two tests from linker which cover this functionality.

Change the test infra to use token to compare message origins for expected warnings. Consistently converting generic types/methods into strings across two type systems is just very difficult - the tokens are simple and reliable.

Changes the tests to avoid expecting specific generic types/methods formatting in the messages - again, it's too hard to make this consistent without lot of effort. And the tests don't really need it.

Adds a test for marking behavior related to generic argument data flow. This is to catch issues like #81779.

Adds a smoke test which has a simplified version of the DI problem from #81358.

Fixes #77455
Fixes #75898
Fixes #81358
Fixes #81779
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.