Centralize C# type attribute back-compat merging in TypeProvider#11237
Centralize C# type attribute back-compat merging in TypeProvider#11237jorgerangel-msft with Copilot wants to merge 8 commits into
Conversation
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
| /// types and exclude CodeGen-specific attributes that should not be preserved. The merged result is | ||
| /// deduplicated so identical attributes are only emitted once. | ||
| /// </summary> | ||
| protected internal IReadOnlyList<AttributeStatement> BuildAttributesForBackCompatibility() |
There was a problem hiding this comment.
@copilot this method should simply add any new back compat attributes that don't exist in the original list (generated + custom).
There was a problem hiding this comment.
Reworked in 4f3265a. BuildAttributesForBackCompatibility() now only appends back-compat attributes from the last contract that aren't already in the original (generated + custom) set, returning the original list unchanged when there's nothing new. It's now invoked from the centralized ProcessTypeForBackCompatibility flow (which updates the generated provider's attributes) rather than from CanonicalTypeProvider.
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
… attributes, perf Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| /// <summary> | ||
| /// Builds the set of type-level attributes to emit, adding any back-compatibility attributes | ||
| /// restored from the last contract that are not already present in the original set of generated | ||
| /// and custom-code attributes. Attributes are only restored for non-public types (which are the |
There was a problem hiding this comment.
@copilot "Attributes are only restored for non-public type" . We should only be restoring attributes for public types. Why are we doing the opposite based on this comment?
There was a problem hiding this comment.
Removed the public/non-public gating (and the misleading comment) entirely. The method no longer conditions restoration on type visibility — it simply appends any new back-compat attributes from the last contract.
| /// only ones that can be internalized) and CodeGen-specific attributes are never restored. The | ||
| /// original attributes are returned unchanged when there is nothing new to add. | ||
| /// </summary> | ||
| protected internal IReadOnlyList<AttributeStatement> BuildAttributesForBackCompatibility() |
There was a problem hiding this comment.
@copilot this signature should be BuildAttributesForBackCompatibility(IEnumerable<AttributeStatement> originalattributes
There was a problem hiding this comment.
Updated the signature to BuildAttributesForBackCompatibility(IEnumerable<AttributeStatement> originalAttributes), matching the other Build*ForBackCompatibility methods.
| { | ||
| var originalAttributes = Attributes; | ||
|
|
||
| // Only non-public types can be internalized for back-compatibility, so attributes from the last |
There was a problem hiding this comment.
Removed the internalization logic. There's no longer any public/non-public gating — the method just merges new back-compat attributes into the original set.
commit: |
…p internalize gating, add tests Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
| // Restore any back-compat attributes from the last contract that are not already present in the | ||
| // generated or custom-code attributes. BuildAttributesForBackCompatibility returns either the | ||
| // current attributes unchanged or a superset with the restored attributes appended, so a larger | ||
| // count reliably signals that new attributes were added. |
There was a problem hiding this comment.
Removed the comment.
| var attributes = provider.GetBackCompatibilityAttributes([]); | ||
|
|
||
| Assert.AreEqual(1, attributes.Count); | ||
| Assert.AreEqual("ObsoleteAttribute", attributes[0].Type.Name); |
There was a problem hiding this comment.
@copilot we should be using TestData for all assertions against generated code
There was a problem hiding this comment.
Updated the back-compat attribute tests to render the type and compare against TestData .cs files (via Helpers.GetExpectedFromFile()) instead of inspecting attributes[0].Type.Name.
…tions Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Attribute back-compat merging (generated + last-contract + custom-code) was owned locally by
CanonicalTypeProvider.BuildAttributes(). This moves that logic into a sharedTypeProviderAPI so it follows the existing centralized back-compat pattern (Build*ForBackCompatibility).Changes
TypeProvider: AddedBuildAttributesForBackCompatibility()that:Attributes) and custom-code (CustomCodeView.Attributes) attributes,CodeGenSuppress/CodeGenMember/CodeGenType/CodeGenSerialization) that should not be preserved,CanonicalTypeProvider:BuildAttributes()now delegates to the shared API instead of owning merge/dedup logic.Because the method reads
Attributes,CustomCodeView,LastContractView, andDeclarationModifiersfrom the provider, invoking it on the generated provider reproduces the previous merge exactly: