Type.GetType("TypeName") will pick the "first" such type found

[vitek-karas]

If we're resolving type name -> TypeDefinition (for example Type.GetType does this), and the type name is not assembly qualified, we will iterate over all assemblies and search each one for that type name. Once we find the first, we'll use that.

There's no guarantee it's the right one (if there are multiple). It would be easy to write a counter example.

[vitek-karas]

/cc @eerhardt

[sbomer]

I believe for non-qualified type names, GetType will only look in the calling assembly or in corelib - we might want to do the same in the linker.

[vitek-karas]

Yeah - I know, the problem is tricky though.

// Assembly A
public void Test()
{
    LoadRandomType("MyType");
}

// Assembly B
public object LoadRandomType([DynamicallyAccessedMembers(PublicConstructors)] string typeName)
{
    Activator.CreateInstance(Type.GetType(typeName));
}

The runtime behavior in this case is to search for MyType in assembly B. But currently (without cross-method analysis) there's no way for the linker to figure this out. At the time we're trying to resolve MyType we only see assembly A.

[sbomer]

I'm curious if there are cases where we really need to support non-qualified type names flowing into DynamicallyAccessedMembers. If we don't have a way to find the right type, maybe it's better to warn instead of marking all types with the same name.

Anyone wrapping reflection APIs this way will already be facing the limitation that the wrapper only works for certain non-qualified type names (those that exist in the wrapper assembly or corelib) - it might be useful for this to produce a warning even independently of trimming. I think it should be possible to convert most of these cases to use typeof since the wrapper assembly and corelib will already be referenced. In cases where there are nested wrappers from different assemblies this might not be true, but this seems like a fragile setup in the first place.

[eerhardt]

I'm curious if there are cases where we really need to support non-qualified type names flowing into DynamicallyAccessedMembers

Check out these places from ComponentModel.TypeConverter:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs

Lines 482 to 521 in 82ca681

	/// <summary>
	/// Retrieves a type from a name. The Assembly of the type
	/// that this PropertyDescriptor came from is first checked,
	/// then a global Type.GetType is performed.
	/// </summary>
	private Type GetTypeFromName(string typeName)
	{
	if (string.IsNullOrEmpty(typeName))
	{
	return null;
	}
	int commaIndex = typeName.IndexOf(',');
	Type t = null;
	if (commaIndex == -1)
	{
	t = _type.Assembly.GetType(typeName);
	}
	if (t == null)
	{
	t = Type.GetType(typeName);
	}
	if (t == null && commaIndex != -1)
	{
	// At design time, it's possible for us to reuse
	// an assembly but add new types. The app domain
	// will cache the assembly based on identity, however,
	// so it could be looking in the previous version
	// of the assembly and not finding the type. We work
	// around this by looking for the non-assembly qualified
	// name, which causes the domain to raise a type
	// resolve event.
	t = Type.GetType(typeName.Substring(0, commaIndex));
	}
	return t;
	}

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

Lines 914 to 941 in 82ca681

	/// <summary>
	/// Retrieves a type from a name.
	/// </summary>
	private static Type GetTypeFromName(string typeName)
	{
	Type t = Type.GetType(typeName);
	if (t == null)
	{
	int commaIndex = typeName.IndexOf(',');
	if (commaIndex != -1)
	{
	// At design time, it's possible for us to reuse
	// an assembly but add new types. The app domain
	// will cache the assembly based on identity, however,
	// so it could be looking in the previous version
	// of the assembly and not finding the type. We work
	// around this by looking for the non-assembly qualified
	// name, which causes the domain to raise a type
	// resolve event.
	//
	t = Type.GetType(typeName.Substring(0, commaIndex));
	}
	}
	return t;
	}

All the callsites to these methods are from attributes that allow the string type name to be passed to its constructor. I'm annotating those attributes with DynamicallyAccessedMembers in #49467.

maybe it's better to warn instead of marking all types with the same name.

I wouldn't be against this if the linker can't figure it out.

but this seems like a fragile setup in the first place.

Agreed. The above code looks like it mostly is working around odd design-time issues in .NET Framework.

[vitek-karas]

+1 on what @eerhardt said. My worry is mostly about old code, which happens to work. The current behavior of the linker in this case is just wrong - on the other hand it probably works lot of time (having the same namespace.type name in multiple assemblies is not very common).

[MichalStrehovsky]

We could potentially recognize two situations:

1. A known string is flowing into a Type.GetType call. We know the calling assembly, so we can correctly predict what the runtime is going to do. Allow unqualified strings for convenience.
2. A known string is flowing into a location with DynamicallyAccessedMembers. If it's unqualified, generate a warning and do nothing.

I'm not sure how common 1 would be because as Sven pointed out, people can just do typeof. We could just scope it out and keep it in mind if we get feedback.

[marek-safar]

I think we should just issue a warning. There seems to be an easy workaround with typeof or with the fully qualified name. IF this really becomes a problem in wild we can work on the heuristic but the warning will most likely stay there for the ambiguous cases anyway.

[sbomer]

@mateoatr can this be closed?

[mateoatr]

Yeah, TypeNameResolver now follows the behavior of Type.GetType when resolving a type from a type string (see here). Closing.

[sbomer]

Reopening this because illink isn't yet producing a warning when an unqualified type name flows into location with DynamicallyAccessedMembers.

The current behavior is to warn only if we fail to resolve an unqualified type name (by looking in the "current" assembly and corelib). But the "current" assembly where the type name flows into a location with DynamicallyAccessedMembers might be different than the assembly where Type.GetType is called.

ILC seems to have the desired behavior.

[ghost]

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

Issue Details

---

If we're resolving type name -> TypeDefinition (for example Type.GetType does this), and the type name is not assembly qualified, we will iterate over all assemblies and search each one for that type name. Once we find the first, we'll use that.

There's no guarantee it's the right one (if there are multiple). It would be easy to write a counter example.

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Reflection, untriaged
Milestone:	-

[ghost]

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

Issue Details

---

If we're resolving type name -> TypeDefinition (for example Type.GetType does this), and the type name is not assembly qualified, we will iterate over all assemblies and search each one for that type name. Once we find the first, we'll use that.

There's no guarantee it's the right one (if there are multiple). It would be easy to write a counter example.

Author:	vitek-karas
Assignees:	-
Labels:	area-System.Reflection, untriaged, area-Tools-ILLink
Milestone:	-

[sbomer]

ILC seems to have the desired behavior.

ILC wasn't doing this right either - it was only warning when failing to resolve a non-fully-qualified type name. Which meant it might resolve the type from the wrong assembly if the callsite where the string flowed into a DAM-annotated location was in a different assembly than the Type.GetType call. Fixing this in #104060.