Restore missing last-contract buildable attributes on ModelReaderWriterContextDefinition#11379
Restore missing last-contract buildable attributes on ModelReaderWriterContextDefinition#11379jorgerangel-msft with Copilot wants to merge 9 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
jorgerangel-msft
left a comment
jorgerangel-msft
left a comment
There was a problem hiding this comment.
@copilot add at least 1 more unit test that covers a case where there are attributes in custom code, back compat, and in the generated layer
commit: |
…te layers, validate via TestData Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Added |
2445304 to
73eada8
Compare
73eada8 to
2c6cf95
Compare
|
No changes needing a change description found. |
…-name fix, suppression path Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
…ttributes
- Replace source-style FindForTypeInCustomization call with CLR metadata names:
add GetClrSimpleMetadataName (name with arity suffix, e.g. Type`1) and
GetClrDeclaringTypeChain ('+'-chain for nested types, e.g. Outer`1+Middle),
mirroring NamedTypeSymbolProvider.GetMetadataName so that GetTypeByMetadataName
correctly resolves generic and nested types.
- Use resolvedProvider's normalized type identity (GetTypeIdentity) for the
customized-buildable dedup check instead of the last-contract target identity,
ensuring both dedup paths use the same key source.
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| } | ||
|
|
||
| var attributeType = new CSharpType(typeof(ModelReaderWriterBuildableAttribute)); | ||
| var newAttributeStatement = new AttributeStatement(attributeType, TypeOf(resolvedProvider.Type)); |
There was a problem hiding this comment.
Resolving by metadata name returns the generic type definition, but this then emits resolvedProvider.Type instead of preserving the constructed targetType. A last-contract typeof(Foo<string>) can therefore become typeof(Foo<T>), where T is unavailable in this non-generic context. typeKey also omits the constructed arguments, so Foo<string> and Foo<int> are incorrectly deduplicated. Please preserve the original constructed arguments and declaring-type construction when emitting and computing the identity.
--generated by Copilot
There was a problem hiding this comment.
Fixed in the latest commit. The changes:
-
typeKeynow usesGetTypeIdentity(targetType)— soFoo<string>andFoo<int>produce distinct keys and are not incorrectly deduplicated against each other. -
newAttributeStatementnow usesTypeOf(targetType)— so the emitted attribute preserves the constructed type arguments (e.g.typeof(Foo<string>)) instead of emitting the generic definition (typeof(Foo<T>)). -
New
AddAttributeForType(TypeProvider, string key)overload — so the suppression-handling path uses thetargetType-derived key when called fromAddLastContractBuildableAttributes. The original(TypeProvider)overload now delegates to it usingtypeProvider.Type.FullyQualifiedName.
| // add the attribute directly; their symbol model may not be fully representable | ||
| // through the generator's expression tree (e.g. BCL types), so skip the | ||
| // CanonicalView-based suppression path that is designed for output-library types. | ||
| attributes.Add(typeKey, newAttributeStatement); |
There was a problem hiding this comment.
This branch still leaves the suppression issue unresolved for customization and referenced-assembly types. NamedTypeSymbolProvider exposes symbol attributes through CanonicalView.Attributes, and typeof(...) usage of an [Experimental] or [Obsolete] referenced type can produce an error or warning-as-error. Please route these symbol-backed providers through the same suppression handling rather than adding the attribute directly.
--generated by Copilot
There was a problem hiding this comment.
@copilot I agree with the statement above. I already asked you to fix this. When we are parsing the back compat attributes, we should check if the attribute had an experimental / obsolete attribute and restore them as well. This is partially done in this type today and we can reuse some of that functionality. Ensure you add unit tests to validate the scenario
There was a problem hiding this comment.
Done. The previous implementation always called typeProvider.CanonicalView.Attributes for the suppression check, which triggered CanonicalTypeProvider.DeduplicateAttributes → ToDisplayString() → LiteralExpression.Write() → NotImplementedException for BCL types (e.g. BinaryData) that have attributes with unhandled literal argument types.
The fix in AddAttributeForType(TypeProvider, string key) now selects the attribute source based on whether the provider has a custom-code layer:
typeProvider.CustomCodeView != null→typeProvider.CanonicalView.Attributes(merges generated + custom, needed for output-library types with custom[Obsolete])typeProvider.CustomCodeView == null→typeProvider.Attributesdirectly (symbol-backedNamedTypeSymbolProvidertypes that have no custom-code layer — avoids theDeduplicateAttributespath entirely)
Added LastContractBuildableAttributesForObsoleteCustomTypeHaveSuppression unit test + TestData: a type defined only in the customization layer with [Obsolete] that was in the last contract is restored with the correct #pragma warning disable CS0618 suppression wrapping the attribute. All 1519 tests pass.
…om AddAttributeForType, expose GetClrMetadataName on CSharpType Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
…pe for key and emission Use targetType (the constructed type from the last contract) for both the dictionary key and the emitted TypeOf(...) expression, instead of resolvedProvider.Type (the generic type definition). This ensures: - typeof(Foo<string>) is emitted instead of typeof(Foo<T>) - Foo<string> and Foo<int> are treated as distinct entries (not deduplicated) Also refactor AddAttributeForType(TypeProvider) to accept an explicit key so the suppression-handling path uses the targetType-derived key when called from AddLastContractBuildableAttributes. Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
…ClrMetadataName property tests and ObsoleteCustomType suppression test Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| // Back-compat: restore any ModelReaderWriterBuildableAttribute that was present in the last contract | ||
| // but is missing from the freshly generated set, without introducing duplicates. |
| if (!string.Equals( | ||
| attribute.Type.FullyQualifiedName, | ||
| typeof(ModelReaderWriterBuildableAttribute).FullName, | ||
| s_buildableAttributeType.FullyQualifiedName, |
There was a problem hiding this comment.
@copilot why did we change this from FullName to FullyQualifiedName ?
| Dictionary<string, MethodBodyStatement> attributes, | ||
| AttributeStatement attributeStatement, | ||
| TypeProvider typeProvider) | ||
| => AddAttributeForType(attributes, attributeStatement, typeProvider, typeProvider.Type.FullyQualifiedName); |
There was a problem hiding this comment.
@copilot why did we change this from FullName to FullyQualifiedName ?
| string key) | ||
| { | ||
| AttributeStatement? experimentalOrObsoleteAttribute = typeProvider.CanonicalView.Attributes | ||
| // Use CanonicalView only when the provider has a custom-code layer, since CanonicalTypeProvider |
There was a problem hiding this comment.
@copilot we should always be using the canonicalview. We shouldn't need to check the custom code ourselves to determine which view to use
| // Use targetType (the original constructed type from the last contract) for both the key and the | ||
| // emitted typeof(...) expression, so that generic instantiations such as Foo<string> and Foo<int> | ||
| // are treated as distinct entries and the emitted attribute preserves the type arguments. | ||
| var typeKey = GetTypeIdentity(targetType); |
There was a problem hiding this comment.
@copilot we already calculated this in line 129. Why do we need to do it again?
| // Route through the suppression-handling path so that [Experimental] and [Obsolete] | ||
| // diagnostics are properly suppressed. NamedTypeSymbolProvider (returned for customization | ||
| // and referenced-assembly types) exposes symbol attributes through CanonicalView.Attributes, | ||
| // so the same suppression check works for all resolved providers. |
| { | ||
| } | ||
|
|
||
| [TestCase("MyNs", "SimpleType", ExpectedResult = "SimpleType")] |
There was a problem hiding this comment.
@copilot I'd like to see test cases where we handle attributes whose type are are experimental, obsolete, and are both experimental + obsolete
ModelReaderWriterContextDefinitionrebuilt its[ModelReaderWriterBuildable(...)]attributes purely from the current generation, so a type dropped from the spec silently lost its buildable entry — a source-breaking change for consumers that rely on the context to build that type. Attributes are now reconciled against the last contract.Changes
ModelReaderWriterContextDefinition.BuildAttributes()— after collecting generated + customized entries, restore anyModelReaderWriterBuildableAttributepresent onLastContractViewthat the current generation no longer produces.IsFrameworkType == false), so they're matched byType.FullyQualifiedNamerather thanType.FrameworkType.global::.Foo) when the referenced type isn't defined in that compilation.IsBuildableAttributealready recognizes the restored attributes, soBuildAttributesForWritedoes not re-emit them.Tests —
LastContractBuildableAttributesAreRestoredWhenMissing(removed type restored, present type not duplicated) andBuildAttributesForBackCompatibilityDeduplicatesAcrossGeneratedCustomAndLastContractBuildableAttributes(dedupe across all three sources), plus supportingSampleContext.csTestData.Given a last contract that shipped
RegularModel+RemovedModelbut a generation that only producesRegularModel, the emitted context is: