Don't report unsupported types for never bound properties

Project: dotnet/runtime | Language: C# | Concepts: api-design, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/dotnet/runtime/pull/132455

The problem

The configuration binder source generator was reporting diagnostics (errors/warnings) about unsupported property types for properties that would never be bound at runtime. This included properties marked with [ConfigurationIgnore] and properties with no public accessor. As a result, users saw misleading errors for properties that the binder would never touch.

Why it's non-trivial

A naive fix might only check for [ConfigurationIgnore] or accessibility at the point of emitting diagnostics, but the generator's type graph logic meant that types were registered for diagnostics before those checks. This led to errors being attached to properties that should have been ignored entirely. The fix needed to ensure that only properties that actually participate in binding are ever considered for type analysis and diagnostic reporting.

The approach

The fix changes the order and logic of how property types are enqueued for analysis and how diagnostics are emitted. Key code changes:

1. Only enqueue types for properties that can be bound

Previously, every property type was enqueued for analysis before checking if it should be ignored:

- TypeRef propertyTypeRef = EnqueueTransitiveType(typeParseInfo, property.Type, DiagnosticDescriptors.PropertyNotSupported, propertyName);

Now, the code first builds the PropertySpec, checks if the property should participate in binding, and only then enqueues its type:

+ PropertySpec spec = new(property, new TypeRef(property.Type))
+ {
+     ConfigurationKeyName = configKeyName,
+     IsIgnored = isIgnored,
+ };
+
+ if (!spec.IsIgnored && (spec.CanGet || spec.CanSet || BacksConstructorParameter(ctor, propertyName)))
+ {
+     EnqueueTransitiveType(typeParseInfo, property.Type, DiagnosticDescriptors.PropertyNotSupported, propertyName, spec.TypeRef);
+ }

This ensures that only properties that are actually bindable (not ignored, not inaccessible, or backing a constructor parameter) have their types analyzed and reported for diagnostics.

2. Add a helper to check if a property backs a constructor parameter

This ensures that even non-public properties are considered if they are used for constructor binding:

+ private static bool BacksConstructorParameter(IMethodSymbol? ctor, string propertyName)
+ {
+     return ctor is not null
+         && ctor.Parameters.Any(parameter => string.Equals(parameter.Name, propertyName, StringComparison.OrdinalIgnoreCase));
+ }

This logic ensures that constructor-bound properties are still analyzed, even if the property itself is not public.

3. Short-circuit binding checks in TypeIndex

Previously, the binding check did not short-circuit on ignored or inaccessible properties before resolving the type spec:

- TypeSpec propTypeSpec = GetEffectiveTypeSpec(property.TypeRef);
- return IsAccessible() && !property.IsIgnored && !IsCollectionAndCannotOverride() && !IsDictWithUnsupportedKey();

Now, it returns early if the property is ignored or inaccessible:

+ if (property.IsIgnored || !IsAccessible())
+ {
+     return false;
+ }
+
+ TypeSpec propTypeSpec = GetEffectiveTypeSpec(property.TypeRef);
+ return !IsCollectionAndCannotOverride() && !IsDictWithUnsupportedKey();

This prevents unnecessary type resolution and avoids false-positive diagnostics.

4. Add thorough tests for all edge cases

The PR adds tests to verify that:

• Ignored or inaccessible properties do not cause diagnostics.
• If a type is only reachable through an ignored property, no errors are reported.
• If a type is reachable through both ignored and non-ignored properties, errors are only reported for the non-ignored property.
• Constructor-bound properties are still analyzed.

Example test for an ignored property:

+ [Theory]
+ [InlineData("private UnbindableType Lazy => UnbindableType.Create();")]
+ [InlineData("internal UnbindableType Lazy { get; set; }")]
+ [InlineData("protected UnbindableType Lazy { get; set; }")]
+ [InlineData("[ConfigurationIgnore] public UnbindableType Lazy { get; set; }")]
+ public async Task PropertyExcludedFromBindingDoesNotReportItsType(string propertyDeclaration)
+ {
+     // ... test logic ...
+     Assert.Empty(result.Diagnostics);
+     Assert.DoesNotContain("UnbindableType", generated);
+ }

From the review thread

Copilot:
PropertySpec is constructed with new TypeRef(property.Type) and then EnqueueTransitiveType(...) is called (for bindable properties), which will typically allocate a second TypeRef for the same ITypeSymbol when the type isn’t already in _createdTypeSpecs. Since this is in the generator hot path, it’s worth avoiding the extra allocation/string computation by using the TypeRef returned from EnqueueTransitiveType when you do enqueue, and only creating a TypeRef when you don’t.

rosebyte:
Nice catch, fixed.

Takeaways

• Only perform type analysis and emit diagnostics for code paths that are actually reachable or used at runtime; avoid reporting errors for ignored or inaccessible members.
• Structure code so that filtering (e.g., for accessibility or attributes) happens before expensive or side-effecting operations like type registration or diagnostic emission.
• When optimizing hot paths in code generators or analyzers, avoid unnecessary allocations by reusing objects when possible.