Use provider C# reference map with pre-write internalization#10976
Use provider C# reference map with pre-write internalization#10976live1206 wants to merge 5 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
You can try these changes here
|
|
Correction to my follow-up review (point #1). @live1206 rightly pointed out that The real reason the textual So the same marker mechanism you introduced for --generated by Copilot |
Design suggestion: derive body references from expression trees rather than declared deps + source re-parsingFollowing up on the body-reference discussion — the current approach builds graph edges from provider metadata (signatures, properties, fields, attributes, etc.) and then compensates for body-level references three ways: manually-declared The concern is soundness: a pure-metadata graph is an under-approximation of reachability, because a C# type reference can appear anywhere a method body can ( Suggestion: walk the body expression trees instead. The bodies already exist as structured
The fact that the PR re-parses generated source for the client/serialization candidates (rather than walking expression trees) suggests the tree walk wasn't feasible for all bodies — presumably where bodies are raw Not a merge blocker, but I think it's the change that would turn this from "correct-by-testing" into "correct-by-construction." Would also be worth attributing how much of the ~21% comes from skipping body analysis vs. from pre-write internalization (skipping the Roslyn --generated by Copilot |
7eed7ff to
8a08923
Compare
|
|
||
| namespace Microsoft.TypeSpec.Generator | ||
| { | ||
| internal static partial class ProviderReferenceMapAnalyzer |
There was a problem hiding this comment.
I think splitting this functionality over 10+ files is a bit overkill. Can we consolidate into a few files?
There was a problem hiding this comment.
I revisited the split. The analyzer is about 3,000 lines; combining these responsibility-based partials would leave several 700-1,000 line files and make the graph construction, root selection, and candidate rules harder to navigate. I kept the split, but cleaned stale terminology and simplified the candidate flow in b612999.
| public class PostProcessorTests | ||
| { | ||
| [Test] | ||
| public async Task RemovesInvalidUsings() |
There was a problem hiding this comment.
Even though this type is going away, we need to make sure we still have all the same scenarios covered using the replacement type.
There was a problem hiding this comment.
Audited the six removed scenarios. Invalid/valid MRW attribute filtering is covered by RemovedProvidersDoNotContributeBuildableAttributes and the surrounding MRW context tests, including mixed kept/removed providers. The two using-removal tests exercised Roslyn source rewriting that no longer exists: generated references are provider-written/global-qualified, and we no longer rewrite source files to remove usings.
|
|
||
| foreach (var method in provider.Methods) | ||
| { | ||
| if (method.IsMethodSuppressed()) |
There was a problem hiding this comment.
Why is the suppression check only added for methods? Doesn't the Methods property already handle suppressions?
There was a problem hiding this comment.
Methods does not apply CodeGenSuppress; it filters customized members, while IsMethodSuppressed evaluates the enclosing type suppression attributes. The body graph must skip suppressed methods because their generated bodies are not emitted. Other members do not have the equivalent CodeGenSuppress method-signature behavior, so this check is method-specific.
JoshLove-msft
left a comment
There was a problem hiding this comment.
Found several correctness gaps in the provider-based reference map that can cause generated compilation failures, API compatibility regressions, or missing MRW registrations.
|
|
||
| foreach (var invocation in syntax.DescendantNodes().OfType<InvocationExpressionSyntax>()) | ||
| { | ||
| if (GetInvocationName(invocation) == "SetDelimited") |
There was a problem hiding this comment.
why is this needed?
There was a problem hiding this comment.
This replaces the equivalent Roslyn reference discovery currently used on main for custom code.
The existing map calls SymbolFinder.FindReferencesAsync for each generated type, and it has a separate path that finds references to every extension method. Those references are then attributed to the type declared by the referencing document, including customized documents. That is how a generated provider referenced only from handwritten custom code remains reachable today.
This PR removes that post-generation Roslyn map, so NamedTypeSymbolProvider extracts the same custom-code dependencies once from its existing semantic compilation and exposes them to the provider graph before writing files. The SetDelimited handling corresponds specifically to the old extension-method path: request.Headers.SetDelimited(...) contains no explicit syntax reference to the generated extension class, so the normal type-syntax walk cannot discover its declaring provider. The unresolved method dependency is subsequently matched to the unique generated extension provider by method name and receiver type. Without this replacement edge, a generated type/helper used only by custom code could be removed or internalized.
Replace Roslyn-based generated-source reference analysis with provider metadata while preserving generated contracts and customization dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
30cc73c to
980c6ad
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c4b68173-2f2b-467d-9a94-9be9d14b5e17
|
latest regen: Azure/azure-sdk-for-net#60965 (comment) |
| return attributes; | ||
| } | ||
|
|
||
| private static bool ShouldPreserveLastContractAttribute(AttributeStatement attribute) |
There was a problem hiding this comment.
As I'm thinking about this some more, does it always make sense that we want to restore a type's attributes from the last GA version? What if a type went from being experimental with the Experimental attribute, then it GAs in the new release. I think we wouldn't want that attribute re-added. I'm leaning towards not having attributes supported as part of the back compat support, at least in this PR, until we have a stronger solution. @JoshLove-msft - any thoughts?
Summary
Replaces the C# generator's Roslyn-generated reference-map/internalization path with a provider-based reference map that runs before files are written. The generator now computes reachability and accessibility from the in-memory provider model, filters unreachable providers up front, and removes stale generated files by cleaning
src/Generatedbefore writing.What changed
ModelFactoryusings.Performance
Latest full-generation benchmark on the updated benchmark branch (
mtg-manual-name-reduction-latest-bench, commit4cf86686e) averaged 544.5 ms / 53.85 MB with the provider reference map enabled vs 847.4 ms / 138.87 MB with it disabled: 35.7% faster and 61.2% less allocation.Benchmark artifacts/logs were saved locally under
/tmp/typespec-benchmark-results-latest-runs-20260710030912.Azure SDK management-plane local project-reference regen speedups:
Latest measured PR head:
434e76802(test(csharp): rename body dependency reference map tests). Each package was regenerated one-by-one withRegenSdkLocal.ps1 -Parallel 1using the local Azure SDK for .NET project-reference setup.Validation
Azure.ResourceManager.Network,Azure.ResourceManager.DataFactory, andAzure.ResourceManager.AppServicepassed one-by-one with no public API listing diffs.Azure.Data.AppConfiguration(ResponseErrorMRW context coverage)Azure.AI.Projects(custom-code parent-namespace body dependency root coverage)Azure.AI.Extensions.OpenAI(polymorphic/unknown model retention coverage)mainhas been merged into the PR branch.