Fallback to treating as object if not collection

[ericstj]

Fixes #96652

The reflection binder will fallback if a type does not meet collection heuristics, but the source generator did not.

[eiriktsarpalis]

What types of inputs are deemed to be collections but then return UnsupportedTypeSpec? Could these checks be integrated in the IsCollection predicate instead?

[ericstj]

I considered that -- it's all of the checks here:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs

Lines 363 to 540 in dcf9814

	private TypeSpec CreateCollectionSpec(TypeParseInfo typeParseInfo)
	{
	INamedTypeSymbol type = (INamedTypeSymbol)typeParseInfo.TypeSymbol;
	TypeSpec spec;
	if (IsCandidateDictionary(type, out ITypeSymbol? keyType, out ITypeSymbol? elementType))
	{
	spec = CreateDictionarySpec(typeParseInfo, keyType, elementType);
	Debug.Assert(spec is DictionarySpec or UnsupportedTypeSpec);
	}
	else
	{
	spec = CreateEnumerableSpec(typeParseInfo);
	Debug.Assert(spec is EnumerableSpec or UnsupportedTypeSpec);
	}
	return spec;
	}
	private TypeSpec CreateDictionarySpec(TypeParseInfo typeParseInfo, ITypeSymbol keyTypeSymbol, ITypeSymbol elementTypeSymbol)
	{
	if (IsUnsupportedType(keyTypeSymbol) || IsUnsupportedType(elementTypeSymbol))
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	INamedTypeSymbol type = (INamedTypeSymbol)typeParseInfo.TypeSymbol;
	CollectionInstantiationStrategy instantiationStrategy;
	CollectionInstantiationConcreteType instantiationConcreteType;
	CollectionPopulationCastType populationCastType;
	bool shouldTryCast = false;
	if (HasPublicParameterLessCtor(type))
	{
	instantiationStrategy = CollectionInstantiationStrategy.ParameterlessConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.Self;
	if (HasAddMethod(type, keyTypeSymbol, elementTypeSymbol))
	{
	populationCastType = CollectionPopulationCastType.NotApplicable;
	}
	else if (_typeSymbols.GenericIDictionary is not null && GetInterface(type, _typeSymbols.GenericIDictionary_Unbound) is not null)
	{
	// implements IDictionary<,> -- cast to it.
	populationCastType = CollectionPopulationCastType.IDictionary;
	}
	else
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	}
	else if (_typeSymbols.Dictionary is not null &&
	(IsInterfaceMatch(type, _typeSymbols.GenericIDictionary_Unbound) || IsInterfaceMatch(type, _typeSymbols.IDictionary)))
	{
	instantiationStrategy = CollectionInstantiationStrategy.ParameterlessConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.Dictionary;
	populationCastType = CollectionPopulationCastType.NotApplicable;
	}
	else if (_typeSymbols.Dictionary is not null && IsInterfaceMatch(type, _typeSymbols.IReadOnlyDictionary_Unbound))
	{
	instantiationStrategy = CollectionInstantiationStrategy.LinqToDictionary;
	instantiationConcreteType = CollectionInstantiationConcreteType.Dictionary;
	// is IReadonlyDictionary<,> -- test cast to IDictionary<,>
	populationCastType = CollectionPopulationCastType.IDictionary;
	shouldTryCast = true;
	}
	else
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	TypeRef keyTypeRef = EnqueueTransitiveType(typeParseInfo, keyTypeSymbol, DiagnosticDescriptors.DictionaryKeyNotSupported);
	TypeRef elementTypeRef = EnqueueTransitiveType(typeParseInfo, elementTypeSymbol, DiagnosticDescriptors.ElementTypeNotSupported);
	Debug.Assert(!shouldTryCast || !type.IsValueType, "Should not test cast for value types.");
	return new DictionarySpec(type)
	{
	KeyTypeRef = keyTypeRef,
	ElementTypeRef = elementTypeRef,
	InstantiationStrategy = instantiationStrategy,
	InstantiationConcreteType = instantiationConcreteType,
	PopulationCastType = populationCastType,
	ShouldTryCast = shouldTryCast
	};
	}
	private TypeSpec CreateEnumerableSpec(TypeParseInfo typeParseInfo)
	{
	INamedTypeSymbol type = (INamedTypeSymbol)typeParseInfo.TypeSymbol;
	if (!TryGetElementType(type, out ITypeSymbol? elementType))
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	if (IsUnsupportedType(elementType))
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	CollectionInstantiationStrategy instantiationStrategy;
	CollectionInstantiationConcreteType instantiationConcreteType;
	CollectionPopulationCastType populationCastType;
	bool shouldTryCast = false;
	if (HasPublicParameterLessCtor(type))
	{
	instantiationStrategy = CollectionInstantiationStrategy.ParameterlessConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.Self;
	if (HasAddMethod(type, elementType))
	{
	populationCastType = CollectionPopulationCastType.NotApplicable;
	}
	else if (_typeSymbols.GenericICollection is not null && GetInterface(type, _typeSymbols.GenericICollection_Unbound) is not null)
	{
	// implements ICollection<> -- cast to it
	populationCastType = CollectionPopulationCastType.ICollection;
	}
	else
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	}
	else if ((IsInterfaceMatch(type, _typeSymbols.GenericICollection_Unbound) || IsInterfaceMatch(type, _typeSymbols.GenericIList_Unbound)))
	{
	instantiationStrategy = CollectionInstantiationStrategy.ParameterlessConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.List;
	populationCastType = CollectionPopulationCastType.NotApplicable;
	}
	else if (IsInterfaceMatch(type, _typeSymbols.GenericIEnumerable_Unbound))
	{
	instantiationStrategy = CollectionInstantiationStrategy.CopyConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.List;
	// is IEnumerable<> -- test cast to ICollection<>
	populationCastType = CollectionPopulationCastType.ICollection;
	shouldTryCast = true;
	}
	else if (IsInterfaceMatch(type, _typeSymbols.ISet_Unbound))
	{
	instantiationStrategy = CollectionInstantiationStrategy.ParameterlessConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.HashSet;
	populationCastType = CollectionPopulationCastType.NotApplicable;
	}
	else if (IsInterfaceMatch(type, _typeSymbols.IReadOnlySet_Unbound))
	{
	instantiationStrategy = CollectionInstantiationStrategy.CopyConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.HashSet;
	// is IReadOnlySet<> -- test cast to ISet<>
	populationCastType = CollectionPopulationCastType.ISet;
	shouldTryCast = true;
	}
	else if (IsInterfaceMatch(type, _typeSymbols.IReadOnlyList_Unbound) || IsInterfaceMatch(type, _typeSymbols.IReadOnlyCollection_Unbound))
	{
	instantiationStrategy = CollectionInstantiationStrategy.CopyConstructor;
	instantiationConcreteType = CollectionInstantiationConcreteType.List;
	// is IReadOnlyList<> or IReadOnlyCollection<> -- test cast to ICollection<>
	populationCastType = CollectionPopulationCastType.ICollection;
	shouldTryCast = true;
	}
	else
	{
	return CreateUnsupportedCollectionSpec(typeParseInfo);
	}
	TypeRef elementTypeRef = EnqueueTransitiveType(typeParseInfo, elementType, DiagnosticDescriptors.ElementTypeNotSupported);
	Debug.Assert(!shouldTryCast || !type.IsValueType, "Should not test cast for value types.");
	return new EnumerableSpec(type)
	{
	ElementTypeRef = elementTypeRef,
	InstantiationStrategy = instantiationStrategy,
	InstantiationConcreteType = instantiationConcreteType,
	PopulationCastType = populationCastType,
	ShouldTryCast = shouldTryCast
	};
	}

IMO pulling those out into the checking method would create two codepaths we have to keep in sync, since many of these checks would need to be duplicated when examining the type and deciding how it would bind.

[eiriktsarpalis]

Could they be merged into a single Try- method that just outs a valid model? Just a suggestion, I realize we have a deadline today and time is short.

[ericstj]

It's actually a bit more nuanced than just returning false here. Much of the time we want the diagnostics info from this. The fallback turns out to be a bit simpler than this.

It's if we didn't find a way to bind as a collection, and the type can be created and does not implement ICollection<> nor IDictionary<,> than treat as an object.

[ericstj]

So this has some slightly unusual behavior. The unsupported types test changed as follows.

List<nint> is now supported and binds only the Capacity member.

A derived Dictionary<string,Action> is now supported and tries to bind the Keys but fails to actually do anything.

While this is unusual I'm not sure if we care. Probably the most undesirable part of this is that it used to emit:

warning SYSLIB1100: The collection type is not supported: 'MyDictionary'. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1100)
 warning SYSLIB1100: The collection type is not supported: 'System.Collections.Generic.List<nint>'. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1100)

And now it emits:

warning SYSLIB1100: Cannot create instance of type 'System.Collections.Generic.IEqualityComparer<string>' because it is missing a public instance constructor. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1100)
warning SYSLIB1101: Property 'Comparer' on type 'MyDictionary' is not supported. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1101)
warning SYSLIB1100: Cannot create instance of type 'System.Collections.Generic.Dictionary<string, Action>+KeyCollection' because it is missing a public instance constructor. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1100)
warning SYSLIB1101: Property 'Keys' on type 'MyDictionary' is not supported. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1101)
warning SYSLIB1100: The collection type is not supported: 'System.Collections.ICollection'. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1100)
warning SYSLIB1101: Property 'System.Collections.IDictionary.Keys' on type 'MyDictionary' is not supported. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1101)

This is different than the reflection binder. The reflection binder only falls back to object binding if the type does not implement IDictionary<> or ICollection<>. I'll update that fallback logic to be consistent.

[ericstj]

Ok, I fixed that and it addressed the diff in baselines/diagnostics. This is ready for a re-review.

[ericstj]

/backport to release/9.0-rc1

[github-actions]

Started backporting to release/9.0-rc1: https://github.com/dotnet/runtime/actions/runs/10412560148