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>
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>
| /// (e.g., <c>Outer`1+Inner</c>). This format is compatible with | ||
| /// <see cref="Microsoft.CodeAnalysis.Compilation.GetTypeByMetadataName"/>. | ||
| /// </summary> | ||
| public string GetClrMetadataName() |
There was a problem hiding this comment.
@copilot if we are going to add this to this type, then we should make it a lazy loaded property on CSharpType
There was a problem hiding this comment.
@copilot ensure we add unit test coverage for this as well
There was a problem hiding this comment.
Done. GetClrMetadataName() has been converted to a lazy-loaded property ClrMetadataName on CSharpType:
public string ClrMetadataName => _clrMetadataName ??= BuildClrMetadataName();
private string BuildClrMetadataName()
{
var simpleName = Arguments.Count > 0 ? $"{Name}`{Arguments.Count}" : Name;
return DeclaringType is null
? simpleName
: $"{DeclaringType.ClrMetadataName}+{simpleName}";
}The backing field _clrMetadataName is initialized once on first access and reused on subsequent accesses. The call site in ModelReaderWriterContextDefinition was updated from targetType.GetClrMetadataName() to targetType.ClrMetadataName.
Unit tests added to CSharpTypeTests.cs covering: simple type, generic with 1 type arg, generic with 2 type args, nested non-generic, generic outer + nested, and lazy caching (verifying AreSame on two accesses).
…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>
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: