Generic type array causes InvalidOperationException in ReflectionMethodBodyScanner.GetValueNodeFromGenericArgument

[vitek-karas]

Repro copied from dotnet/runtime#43222

Steps To Reproduce

0. Install .NET 5 RC1

1. Create some project:

dotnet new blazorwasm

2. Add some new *.cs file with following C# code:

using System.Text.Json;

namespace SomeNamespace
{
    public static class StaticClass
    {
        public static void GenericMethod<T>()
        {
            T[] value = null;
            JsonSerializer.Serialize(value);
        }
    }
}

3. Try to publish

dotnet publish

Exception

Class.cs(8,9): error IL1005: SomeNamespace.StaticClass.GenericMethod<T>(): Error processing method 'SomeNamespace.StaticClass.GenericMethod<T>
()' in assembly 'TrimmingIssue.dll' [C:\projectsBlazor\Net5Publish\TrimmingIssue\TrimmingIssue.csproj]
  Mono.Linker.LinkerFatalErrorException: C:\projectsBlazor\Net5Publish\TrimmingIssue\Class.cs(8,9): error IL1005: SomeNamespace.StaticClass.GenericMethod<T>(): Error processing method 'S
omeNamespace.StaticClass.GenericMethod<T>()' in assembly 'TrimmingIssue.dll'
   ---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
     at Mono.Linker.Dataflow.ReflectionMethodBodyScanner.GetValueNodeFromGenericArgument(TypeReference genericArgument)
     at Mono.Linker.Dataflow.ReflectionMethodBodyScanner.ProcessGenericArgumentDataFlow(GenericParameter genericParameter, TypeReference genericArgument, IMemberDefinition source)
     at Mono.Linker.Steps.MarkStep.MarkGenericArgumentConstructors(IGenericInstance instance, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkGenericArguments(IGenericInstance instance, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.GetOriginalMethod(MethodReference method, DependencyInfo reason, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkMethod(MethodReference reference, DependencyInfo reason, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkInstruction(Instruction instruction, MethodDefinition method, Boolean& requiresReflectionMethodBodyScanner)
     at Mono.Linker.Steps.MarkStep.MarkMethodBody(MethodBody body)
     at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.ProcessQueue()
     --- End of inner exception stack trace ---
     at Mono.Linker.Steps.MarkStep.ProcessQueue()
     at Mono.Linker.Steps.MarkStep.ProcessPrimaryQueue()
     at Mono.Linker.Steps.MarkStep.Process()
     at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
     at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step)
     at Mono.Linker.Pipeline.Process(LinkContext context)
     at Mono.Linker.Driver.Run(ILogger customLogger)
  Optimizing assemblies for size, which may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink

Further technical details

• .NET 5 RC1 is installed
• Project targets .NET 5
• Turning the trimming off for whole the app helps: (dotnet publish -p:PublishTrimmed=False)
• Preservation for the assembly - does not help (<linker> <assembly fullname="..." preserve="all" /> </linker>)

Originally created by @SergeySeleznyov

[vitek-karas]

This is a bug we should fix. It's somewhat related to #1500 as it fails in exactly the same spot, but for different reason:

• Unresolvable type cases InvalidOperationException in ReflectionMethodBodyScanner.GetValueNodeFromGenericArgument #1500 fails because the type reference points to assembly which can't be resolved - this should be handled as any other unresolvable type reference (either fail or ignore)
• this issue fails because the type reference is T[] which in Cecil is represented as ArrayType where the ElementType is GenericParameter - this should be handled "Somehow" (see below)

This uncovers an aspect of data flow we didn't test - handling of more complex types - so typically array types, but same would probably apply to all TypeSpecification derived types (by reference, function pointer, ...). Lot of these are probably not applicable as it won't be possible to get them into the right places, but we need to look into all of them eventually.
That said array types are the most important ones.

Current behavior:

• Passing int[] into a Type parameter which is annotated with data flow annotation will behave exactly the same as passing just int. We call .Resolve() on the type reference which in Cecil basically resolves the element type of the array - so we get back a type definition for the element type.
• Passing T[] into a Type parameter which is annotated will issue a warning - internally we basically "fail" and treat the value as an annotated type with no requirements (DynamicallyAccessedMemberType.None) and so it fails validation because it can't fulfill any non-empty requirement.
• Passing int[] into a generic parameter which is annotated will behave the same as passing int - same reason as above.
• Passing T[] into a generic parameter which is annotated will crash the linker - see this issue.

Given the existing behavior I think we should fix this by treating T[] as if it's just T - similar to what happens for non-generic case of int[] (which is treated as int). That would at least make the behavior consistent.

The downside is that linker will end up marking more than necessary - in theory this should only mark members of the System.Array type and not the members of the element type. But since we already have that behavior, we can "use" it in the future - as this might be a way to handle arrays anyway.
The fix for this should not include the propagation of annotations from the element type to the array type and vice versa and more importantly data flow validations around reading elements from arrays and assigning to elements in arrays.
The fix should only make the behavior consistent and avoid crashing the linker.

A separate issue should be filed to solve the array problem all up.

[vitek-karas]

/cc @MichalStrehovsky for ideas and opinions on the proposed fix.

[MichalStrehovsky]

How difficult would it be to fix this in a way where would simply reduce all arrays we see into System.Array? It's a wart (one could argue whether it's a bigger wart than treating T[]/int[] as T/int), but this wart would let us do the right thing for

Console.WriteLine(typeof(object[]).GetProperty("LongLength").GetValue(new object[0]));

which will probably fail right now.

[vitek-karas]

Side-note: I tried the sample and it works, but there could be many reasons why it happens to work.

I do agree that marking the System.Array itself should happen. But I'm worried about not marking the T/int. For one it will be a breaking change - some apps using 5.0 might rely on it (knowingly or not).

But also going forward, if we were to fully support arrays, that is correctly handle things like: Type.GetElementType. Let's say code like this:

object InitializeArray(Type arrayType, int count)
{
    IList array = (IList)Activator.CreateInstance(arrayType);
    for (int i = 0; i < count; i++)
    {
        array.Add(Activator.CreateInstance(arrayType.GetElementType());
    }

    return array;
}

This would mean we would have to support double annotations to be perfectly precise. Or alternatively we could "live with the existing behavior" and apply the annotation to both the array and the element type. So in the above case just adding [DynamicallyAccessedMembers(DynamicallyAccessedMemberType.PublicParameterlessConstructor)] would be enough.

[MichalStrehovsky]

For one it will be a breaking change - some apps using 5.0 might rely on it (knowingly or not).

In our model, unknowingly relying on something being kept should mean there's a warning in that spot. We could e.g. have a change in the libraries in 6.0 that results in a different set of members in the app being kept (e.g. System.Text.Json stops requiring reflection access to all properties because we switch it to a source generated model). That would be an acceptable change in our model because there must be a warning in the spot that unwittingly relies on things being kept. The fact that the spot used to work by accident is secondary - we're allowed to break code if there's a warning. Warning doesn't mean the code won't work - it means it might not work.

But I'm worried about not marking the T/int.

This will be marked using the default linker rules - the T/int is statically present within the app and linker will mark it. The worst that can happen is that the type will be marked hollowed out (only the type definition is kept). But you can still make an array of it at runtime. If you want to reflection set an element to something, you'll need to get at it either statically (in which case this is not even a reflection analysis problem), or through some other reflection that will either be properly annotated, or warn.

But also going forward, if we were to fully support arrays, that is correctly handle things like: Type.GetElementType.

Yes, we'll need to model things precisely for that, but going that route starts to smell a lot like the .NET Native analyzer. Such detailed analysis didn't buy us anything in the end. I would be wary of going there again.

Nit: Activator.CreateInstance cannot be used with arrays :).

[vitek-karas]

Once this is fixed we should enable test added in #1564.

[vitek-karas]

I think I came around on this one - thanks @MichalStrehovsky . We should definitely mark the Array type based on the annotations. The element type is probably not that interesting, unless somebody calls GetElementType which today will typically lead to a warning (not this call, but usage of the return value). While not ideal, I think it's OK. If we find that there are enough use cases of this, we can design a solution such that it's possible to somehow annotate this.

In theory this problem applies to multiple things, but in reality it's really only arrays and only cases where somebody has a very general reflection based implementation of handling elements in the array. The more common case is generics, but for those we already have a solution for.

[Tarmil]

I'm seeing the same error in FSharp.SystemTextJson, specifically in the converter for F# lists (this line).

[mateoatr]

I'm seeing the same error in FSharp.SystemTextJson, specifically in the converter for F# lists (this line).

Thanks. I could successfully link FSharp.SystemTextJson with a master branch linker. This issue was fixed here: #1566.

We should probably take that PR to release/5.0.

/cc @marek-safar @agocke

[Tarmil]

Awesome, thanks!

[marek-safar]

@mateoatr could you backport the fix to 5.0?

[sebfia]

I am having the same issues with FSharp.SystemTextJson as before again with the latest dotnetcore 5.0 sdk release. :-(

[CyberSinh]

I get a similar exception but with field instead of method. Here is the stack trace. Hope this helps.

2>IL Linker has encountered an unexpected error. Please report the issue at https://github.com/mono/linker/issues
2>Fatal error in IL Linker
2>Unhandled exception. System.InvalidOperationException: Operation is not valid due to the current state of the object.
2>   at Mono.Linker.Dataflow.ReflectionMethodBodyScanner.GetValueNodeFromGenericArgument(TypeReference genericArgument)
2>   at Mono.Linker.Dataflow.ReflectionMethodBodyScanner.ProcessGenericArgumentDataFlow(GenericParameter genericParameter, TypeReference genericArgument, IMemberDefinition source)
2>   at Mono.Linker.Steps.MarkStep.MarkGenericArgumentConstructors(IGenericInstance instance, IMemberDefinition sourceLocationMember)
2>   at Mono.Linker.Steps.MarkStep.MarkGenericArguments(IGenericInstance instance, IMemberDefinition sourceLocationMember)
2>   at Mono.Linker.Steps.MarkStep.GetOriginalType(TypeReference type, DependencyInfo reason, IMemberDefinition sourceLocationMember)
2>   at Mono.Linker.Steps.MarkStep.MarkType(TypeReference reference, DependencyInfo reason, IMemberDefinition sourceLocationMember)
2>   at Mono.Linker.Steps.MarkStep.MarkField(FieldDefinition field, DependencyInfo& reason)
2>   at Mono.Linker.Steps.MarkStep.MarkEntireTypeInternal(TypeDefinition type, Boolean includeBaseTypes, DependencyInfo& reason, IMemberDefinition sourceLocationMember, HashSet`1 typesAlreadyVisited)
2>   at Mono.Linker.Steps.MarkStep.MarkEntireTypeInternal(TypeDefinition type, Boolean includeBaseTypes, DependencyInfo& reason, IMemberDefinition sourceLocationMember, HashSet`1 typesAlreadyVisited)
2>   at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly)
2>   at Mono.Linker.Steps.MarkStep.InitializeAssembly(AssemblyDefinition assembly)
2>   at Mono.Linker.Steps.MarkStep.Initialize()
2>   at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
2>   at Mono.Linker.Pipeline.Process(LinkContext context)
2>   at Mono.Linker.Driver.Run(ILogger customLogger)
2>   at Mono.Linker.Driver.Main(String[] args)

[vitek-karas]

Thanks for the report @CyberSinh - the fix should work for fields as well, the interesting bit in the fix is the type of the field, or method parameter, not where it came from.

[CyberSinh]

Thanks @vitek-karas. Hope this fix will be released soon.

[vitek-karas]

#1637 should fix at least some occurrences of this problem (hard to be 100% without a specific repro, but the problem with arrays is solved there). That fix is now in 5.0 release branch, so should go out with the next servicing release of 5.0 SDK (I think it's 5.0.1, but I could be wrong).

Unfortunately the exact same error is produced by a different root cause: #1634. We're working on a fix for that.

[odhanson]

@vitek-karas, I have a consistent repro while trying to bump up to .NET 5, if it helps:

 Mono.Linker.LinkerFatalErrorException: C:\projects\unity\Container\src\Policy\BuildPlanCreator\GenericLazyBuildPlanCreatorPolicy.cs(16707566): error IL1005: Unity.Policy.BuildPlanCreator.GenericLazyBuildPlanCreatorPolicy.BuildResolveAllLazy<T>(IBuilderContext): Error processing method 'Unity.Policy.BuildPlanCreator.GenericLazyBuildPlanCreatorPolicy.BuildResolveAllLazy<T>(IBuilderContext)' in assembly 'Unity.Container.dll'
   ---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
     at Mono.Linker.Dataflow.ReflectionMethodBodyScanner.GetValueNodeFromGenericArgument(TypeReference genericArgument)
     at Mono.Linker.Dataflow.ReflectionMethodBodyScanner.ProcessGenericArgumentDataFlow(GenericParameter genericParameter, TypeReference genericArgument, IMemberDefinition source)
     at Mono.Linker.Steps.MarkStep.MarkGenericArgumentConstructors(IGenericInstance instance, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkGenericArguments(IGenericInstance instance, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.GetOriginalType(TypeReference type, DependencyInfo reason, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkType(TypeReference reference, DependencyInfo reason, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkMethod(MethodReference reference, DependencyInfo reason, IMemberDefinition sourceLocationMember)
     at Mono.Linker.Steps.MarkStep.MarkInstruction(Instruction instruction, MethodDefinition method, Boolean& requiresReflectionMethodBodyScanner)
     at Mono.Linker.Steps.MarkStep.MarkMethodBody(MethodBody body)
     at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method, DependencyInfo& reason)
     at Mono.Linker.Steps.MarkStep.ProcessQueue()
     --- End of inner exception stack trace ---
     at Mono.Linker.Steps.MarkStep.ProcessQueue()
     at Mono.Linker.Steps.MarkStep.ProcessPrimaryQueue()
     at Mono.Linker.Steps.MarkStep.Process()
     at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
     at Mono.Linker.Pipeline.Process(LinkContext context)
     at Mono.Linker.Driver.Run(ILogger customLogger)