MakeGenericType generates wrong warning for annotation mismatch

[vitek-karas]

For example:

class GenericType<[DynamicallyAccessedMembers(PublicFields)] TWithFields> { }

void Test([DynamicallyAccessedMembers(PublicMethods)] Type type)
{
    typeof (GenericType).MakeGenericType (type);
}

This generates a warning like:

ILLink: Trim analysis warning IL2070: Test(Type): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicFields' in call to 'System.Type.MakeGenericType(Type[])'. The parameter 'type' of method 'Test(Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

This says that the "this" parameter for the MakeGenericType - which in this case the typeof (GenericType) as having requirements. That is not correct, the requirement comes from a generic parameter of that type. The warning should be:

IL2071: Generic argument for 'TWithFields` does not satisfy ....

The bug is at this line:
https://github.com/dotnet/linker/blob/6880454ee1a29cb9e8239cb28d352228712661ee/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs#L1900

The target context should be the genericParameter[i], not the method itself (which means "this" for that method).

[vitek-karas]

I'm adding test for this case in dotnet/linker#2429.

[vitek-karas]

Note that this applies to MakeGenericMethod as well.

Also note that if we fix this it is potentially a breaking change since we would be changing warning codes. So code which suppressed this warning, for example suppressing IL2070 would now produce the new warning IL2071 in spite of the suppression.

Not sure what the correct fix for this is, it might be that we use the warning level feature for the first time and only fix the warnings for .NET7+.

[MichalStrehovsky]

Not sure what the correct fix for this is, it might be that we use the warning level feature for the first time and only fix the warnings for .NET7+.

My 2 cents: I would just fix it without complicating the codebase.

I have doubts the warning waves feature will be useful. It works for Roslyn because it controls warnings generated for the C# code being compiled. For illink, the warning wave is chosen by the assembly being compiled, but it controls warnings for the entire application. We don't know what warning wave the referenced NuGets are compatible with. It may well be that come .NET 7, there will be NuGet packages generated with warning wave 7+ being used in .NET 6 apps. So the NuGet suppresses 2071, but the app gets 2070 because it's .NET 6.

Putting the same compat burden on illink as is put on the C# compiler (orders of magnitude more users) might not be the best use of time. (I didn't actually even bother porting warning waves to NativeAOT yet.)

[vitek-karas]

That's a really good point - that linker mixes assemblies built by many different versions of the compiler and SDK.
An alternative would be that we keep the warning codes and just fix the warning messages somehow.

I guess we'll have to figure out how sensitive are we going to be to potentially breaking changes in the linker/NativeAOT.

[dotnet-policy-service]

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

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

#104921 fixes this, including a change to the warning codes. This is a breaking change in the warning behavior, but only for code that isn't trim-compatible or that suppressed this warning. I think it's worth taking this breaking change, but since it is getting late in .NET 9 maybe it's better to do so early in .NET 10. Thoughts @vitek-karas @MichalStrehovsky?

[vitek-karas]

I think the only worry is the cases where somebody suppressed the warning specifically. Maybe we could search github for example for the old warning number and see if there are any cases like that. Otherwise, I think it would be OK to change even in 9 - especially since it will only affect projects which have 9.0 TFMs, so intentional upgrade.

[sbomer]

I didn't find any instances of IL2070 suppressions that specifically targeted this case, so it's probably rare this would break anyone.