-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Configuration Binder SG: ignore unresolvable metadata types to prevent ambiguous/missing-type emissions #130118
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
base: main
Are you sure you want to change the base?
Changes from all commits
b6e2f37
b3f6f40
4f5109d
de3fbf3
a4fecd3
999253e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -556,6 +556,11 @@ private bool IsAssignableTo(ITypeSymbol source, ITypeSymbol dest) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private bool IsUnsupportedType(ITypeSymbol type, HashSet<ITypeSymbol>? visitedTypes = null) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (ContainsErrorType(type)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (type.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type = ((INamedTypeSymbol)type).TypeArguments[0]; // extract the T from a Nullable<T> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -713,8 +718,18 @@ private ObjectSpec CreateObjectSpec(TypeParseInfo typeParseInfo) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableArray<ISymbol> members = current.GetMembers(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach (ISymbol member in members) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (member is IPropertySymbol { IsIndexer: false, IsImplicitlyDeclared: false } property && !IsUnsupportedType(property.Type)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (member is IPropertySymbol { IsIndexer: false, IsImplicitlyDeclared: false } property) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (IsUnsupportedType(property.Type)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (ContainsErrorType(property.Type)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RecordDiagnostic(DiagnosticDescriptors.PropertyNotSupported, typeParseInfo.BinderInvocation?.Location, [property.Name, typeParseInfo.FullName]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string propertyName = property.Name; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (property.IsOverride || properties?.ContainsKey(propertyName) is true) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+721
to
735
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the property loop, this diagnostic is emitted before the Computing the override/duplicate condition first and gating the diagnostic on it keeps the single-warning behavior, and leaves binding for supported properties unchanged:
Suggested change
This handles the override case. A |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -885,6 +900,36 @@ private static bool IsInterfaceMatch(INamedTypeSymbol type, INamedTypeSymbol? @i | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return SymbolEqualityComparer.Default.Equals(type, @interface); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A member type is unsupported when it, a nested array element, or a generic type argument is an | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // error symbol — i.e. an unresolved or ambiguous metadata type from a reference that isn't available | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // to the current compilation. Emitting binding code that names such a type produces uncompilable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // output (missing/ambiguous type errors), so callers skip these members instead. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static bool ContainsErrorType(ITypeSymbol type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (type.TypeKind is TypeKind.Error) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (type is IArrayTypeSymbol arrayType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ContainsErrorType(arrayType.ElementType); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (type is INamedTypeSymbol { IsGenericType: true } genericType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach (ITypeSymbol typeArgument in genericType.TypeArguments) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (ContainsErrorType(typeArgument)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static bool ContainsGenericParameters(ITypeSymbol type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (type is not INamedTypeSymbol { IsGenericType: true } genericType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -210,6 +210,103 @@ internal class BType {} | |||||
| Assert.Empty(result.Diagnostics); | ||||||
| } | ||||||
|
|
||||||
| [Fact] | ||||||
| public async Task IgnorePropertiesWithUnresolvableMetadataTypes() | ||||||
| { | ||||||
| CSharpCompilationOptions compilationOptions = new(OutputKind.DynamicallyLinkedLibrary); | ||||||
| MetadataReference[] commonReferences = s_compilationAssemblyRefs | ||||||
| .Select(a => MetadataReference.CreateFromFile(a.Location)) | ||||||
| .ToArray(); | ||||||
|
|
||||||
| CSharpCompilation transitiveDependencyCompilation = CSharpCompilation.Create( | ||||||
| assemblyName: $"TransitiveDependency_{Guid.NewGuid():N}", | ||||||
| syntaxTrees: | ||||||
| [ | ||||||
| CSharpSyntaxTree.ParseText(""" | ||||||
| namespace MissingTypes; | ||||||
|
|
||||||
| public struct ValueTypeMessage {} | ||||||
| public sealed class HttpRequestMessage {} | ||||||
| public sealed class CredentialDescription {} | ||||||
| """) | ||||||
| ], | ||||||
| references: commonReferences, | ||||||
| options: compilationOptions); | ||||||
|
|
||||||
| byte[] transitiveDependencyImage = CreateAssemblyImage(transitiveDependencyCompilation); | ||||||
| MetadataReference transitiveDependencyReference = MetadataReference.CreateFromImage(transitiveDependencyImage); | ||||||
|
|
||||||
| CSharpCompilation modelCompilation = CSharpCompilation.Create( | ||||||
| assemblyName: $"UnresolvableModel_{Guid.NewGuid():N}", | ||||||
| syntaxTrees: | ||||||
| [ | ||||||
| CSharpSyntaxTree.ParseText(""" | ||||||
| namespace UnresolvableModel; | ||||||
|
|
||||||
| public sealed class Wrapper<T> | ||||||
| { | ||||||
| public int Count { get; set; } | ||||||
| } | ||||||
|
|
||||||
| public sealed class DstsOptions | ||||||
| { | ||||||
| public MissingTypes.ValueTypeMessage? ValueTypeMessage { get; set; } | ||||||
| public MissingTypes.HttpRequestMessage? HttpRequestMessage { get; set; } | ||||||
| public MissingTypes.CredentialDescription? CredentialDescription { get; set; } | ||||||
| public Wrapper<MissingTypes.HttpRequestMessage>? WrappedMessage { get; set; } | ||||||
| public System.Tuple<int, MissingTypes.CredentialDescription>? TupleMessage { get; set; } | ||||||
| public int Value { get; set; } | ||||||
| } | ||||||
| """) | ||||||
| ], | ||||||
| references: commonReferences.Concat([transitiveDependencyReference]), | ||||||
| options: compilationOptions); | ||||||
|
|
||||||
| // Reference the model as an in-memory metadata reference and omit the transitive dependency, so the | ||||||
| // generator sees the affected member types as unresolved error symbols. Using a metadata reference | ||||||
| // (rather than writing the assemblies to disk and using Assembly.LoadFrom) avoids locking files and | ||||||
| // leaking a temp directory on every run. | ||||||
| MetadataReference modelReference = MetadataReference.CreateFromImage(CreateAssemblyImage(modelCompilation)); | ||||||
|
|
||||||
| string source = """ | ||||||
| using Microsoft.Extensions.Configuration; | ||||||
| using UnresolvableModel; | ||||||
|
|
||||||
| public static class Program | ||||||
| { | ||||||
| public static void Main() | ||||||
| { | ||||||
| var configuration = new ConfigurationBuilder().Build(); | ||||||
| _ = configuration.Get<DstsOptions>(); | ||||||
| } | ||||||
| } | ||||||
| """; | ||||||
|
|
||||||
| ConfigBindingGenRunResult result = await RunGeneratorAndUpdateCompilation(source, metadataReferences: [modelReference]); | ||||||
|
|
||||||
| result.ValidateDiagnostics(ExpectedDiagnostics.None); | ||||||
| Assert.NotNull(result.GeneratedSource); | ||||||
| Assert.Contains("Value", result.GeneratedSource.Value.SourceText.ToString()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion doesn't actually verify that Asserting on the actual assignment makes it meaningful:
Suggested change
The |
||||||
| Assert.DoesNotContain("ValueTypeMessage", result.GeneratedSource.Value.SourceText.ToString()); | ||||||
| Assert.DoesNotContain("HttpRequestMessage", result.GeneratedSource.Value.SourceText.ToString()); | ||||||
| Assert.DoesNotContain("CredentialDescription", result.GeneratedSource.Value.SourceText.ToString()); | ||||||
| Assert.DoesNotContain("WrappedMessage", result.GeneratedSource.Value.SourceText.ToString()); | ||||||
| Assert.DoesNotContain("TupleMessage", result.GeneratedSource.Value.SourceText.ToString()); | ||||||
|
rosebyte marked this conversation as resolved.
Comment on lines
+287
to
+294
|
||||||
|
|
||||||
| // Each skipped member surfaces a SYSLIB1101 warning so the incomplete binding is not silent. | ||||||
| foreach (string skippedProperty in new[] { "ValueTypeMessage", "HttpRequestMessage", "CredentialDescription", "WrappedMessage", "TupleMessage" }) | ||||||
| { | ||||||
| Assert.Contains(result.Diagnostics, diagnostic => | ||||||
| diagnostic.Id == "SYSLIB1101" && | ||||||
| diagnostic.Severity == DiagnosticSeverity.Warning && | ||||||
| diagnostic.GetMessage(CultureInfo.InvariantCulture).Contains($"'{skippedProperty}'")); | ||||||
| } | ||||||
|
|
||||||
| // The bindable member must still bind without a diagnostic. | ||||||
| Assert.DoesNotContain(result.Diagnostics, diagnostic => | ||||||
| diagnostic.GetMessage(CultureInfo.InvariantCulture).Contains("'Value'")); | ||||||
| } | ||||||
|
|
||||||
| [Fact] | ||||||
| public async Task SucceedWhenGivenMinimumRequiredReferences() | ||||||
| { | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.