You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix C# paging helper naming collisions and long filenames - #12292
Nest operation-specific paging helpers inside their owning client and emit them together in <ClientName>.Collections.cs, following option 2 discussed in the issue. ClientProvider.CollectionResultProviders lazily creates and caches this partial declaration.
Simplify helper names to <OperationName>[Async]CollectionResult[OfT], without a redundant client-name prefix or original-name fallback. Resolve same-client normalized-name and sync/async collisions in input operation order: keep the first name unchanged, then append 2, 3, etc. For example, two operations normalized to GetAll produce GetAllCollectionResult and GetAllCollectionResult2.
Traverse nested helpers in library visitors and dependency analysis, preserving downstream customization and response models referenced only by helper bodies. Omit the implementation-only collections partial from public API stubs.
Preserve reference-based pruning: client ownership alone does not retain paging helpers. Resolve handwritten references within their containing type, remove unused helpers and otherwise-unreferenced dependencies with RemoveOrInternalize, and skip removed nested types during emission. Preserve existing ownership-based retention for other nested types and keepAll behavior.
Disable duplicate compatibility-overload synthesis in the collections partial. Propagate client name and namespace updates to the collections partial and cached nested helpers while retaining visitor constructor customizations.
Document provider ownership and naming; add regression coverage for collisions, long operation names, nested member visitation, dependency retention and pruning, compatibility overloads, and cached identity updates; regenerate the checked-in sample output and update helper-name fixtures.
Generated SDK public APIs and paging iteration logic are unchanged. Internal helper names and qualified locations change with the nested layout.
Validation
C# emitter/generator build passed.
Core generator suite: 2,919 passed, rerun after the pruning fix.
Client-model suite: 2,479 passed after the pruning fix, using a temporary short Windows drive mapping to avoid existing fixture path limitations. The four pruning cases also passed after extending coverage to same-named helpers in another client.
Regenerated local sample suite: 730 passed, rerun after the latest fixes; regeneration produced no additional checked-in output changes.
Full C# regeneration completed for the initial nested layout; the affected local sample was regenerated and rebuilt after subsequent changes. The full generated pageable Spector SDK was also built successfully for net8.0 with the initial nested layout and emitted five per-client collections files instead of per-operation files.
New compatibility, cached-identity, and pruning regressions were observed failing before implementation and passing after the fixes.
Cop checks, repository-configured Oxlint, and root pnpm format passed after the latest changes.
Spector HTTP runtime integration tests were not run.
Document the collections partial declaration and let the output library reuse the client-owned provider.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
Retain paging helpers through generated or handwritten references instead of client ownership, resolve handwritten nested references in their containing type, and omit removed helpers during emission.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
With RemoveOrInternalize, suppressing the generated paging methods and referencing new TestClient.GetItemsCollectionResult(client, options) from another handwritten type in namespace Sample still removes the helper: NamedTypeSymbolProvider.cs:690–699 records the relative qualifier as namespace TestClient, but AddNamespaceBodyDependencyType only performs scoped lookup for empty namespaces. Resolve namespace-relative qualified nested references too, and add a pruning regression so this valid custom code does not fail with CS0426.
Review summary: two P2 correctness issues remain in the current diff:
Nested replacement providers lose their members.LibraryVisitor.cs:90 now calls VisitTypeCore, which traverses the original provider's members and overwrites those on the replacement returned by VisitType. Traverse the returned provider and add a replacement-provider regression. Existing discussion.
Paging helpers can collide with client methods.CollectionResultDefinition.cs:198 checks only helper-to-helper collisions. Pageable GetItems alongside a regular GetItemsCollectionResult operation produces a nested type and method with the same name (CS0102). This limitation has been acknowledged and explicitly deferred. Existing discussion.
No additional high-confidence findings from the review. These summarize the existing threads rather than duplicate inline comments.
…per names
Reserve generated and custom client member names and the enclosing type name before allocating numeric paging helper suffixes.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
Resolve static imports to their type identities, cover qualified generic dependencies, and avoid a leading separator in nested-only declarations. Remove unnecessary architecture documentation and regenerate fixtures.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
The reason will be displayed to describe this comment to others. Learn more.
With removeOrInternalize, placing global using static Sample.TestClient; in a separate Usings.cs causes a handwritten factory’s new GetItemsCollectionResult(...) to lose its referenced helper and fail compilation, because GetNamespaceCandidates only reads imports from the current syntax tree. Please include compilation-wide global imports, resolving each with its own semantic model, and add a two-file regression alongside SuppressedPagingMethodsPruneUnreferencedHelpers.
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Fixed in d9c71ac. Namespace candidates now include global imports from the other syntax trees in the compilation, with each directive resolved using its own tree’s semantic model. Added a two-file regression with a global static import in Usings.cs; it verifies that the referenced helper survives pruning without retaining another client’s same-named helper. A negative case checks that an ordinary, non-global static import in another file does not leak into the factory’s scope. Both cases also cover keepAll.
…orts
Include global imports from other syntax trees using their own semantic models. Cover cross-file retention and ensure ordinary imports remain file-scoped. Extract the global alias prefix into a constant.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Dependency discovery repeatedly scans all syntax trees for each custom type and public member signature, causing O(N² × M) work that should be avoided by caching global-using candidates once per compilation.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Two critical reference-resolution gaps can cause RemoveOrInternalize to prune paging helpers still used by handwritten code with escaped qualifiers or inherited unqualified names, breaking compilation.
Remove unused nested types from their owning providers during reference analysis so writers only receive types that should be emitted.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Dependency collection repeatedly scans the full compilation for each custom type body and public member signature, causing quadratic scaling in source-file count unless global-using candidates are cached per compilation.
Avoid repeatedly scanning every syntax tree for each custom body and signature while preserving file-local import isolation.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Downstream Azure/management visitor implementations are absent, so a maintainer must validate paging-provider customization compatibility with the new nested traversal and reset-driven helper rebuilding.
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ We cannot use Client.CanonicalView.Methods here: it builds Client.Methods, which builds the paging helpers and re-enters this name allocation. The current reservation combines generated field/property names, input service-method names (sync and async), the client name, and all handwritten member names from its Roslyn symbol. That already covers custom fields/properties as well as methods and nested types, so we are keeping this logic rather than switching views.
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Updated in 6bfd19b. The namespace set is now allocated only when the scan encounters a global using directive; compilations without global imports cache an empty array instead.
Skip the namespace HashSet allocation when a compilation has no global using directives.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 268abd28-b2c0-4898-87f0-4ca429090c1f
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Changes to helper ownership and recursive traversal affect downstream emitter extensions, but available evidence cannot establish compatibility with Azure management and tracing visitors.
The reason will be displayed to describe this comment to others. Learn more.
Can we please run a regen preview and also handle the fallout, if any, from this in the subsequent upgrade pr once this merges? thanks
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
Jorge Rangel (jorgerangel-msft)
left a comment
Fixes #12147.
Summary
<ClientName>.Collections.cs, following option 2 discussed in the issue.ClientProvider.CollectionResultProviderslazily creates and caches this partial declaration.<OperationName>[Async]CollectionResult[OfT], without a redundant client-name prefix or original-name fallback. Resolve same-client normalized-name and sync/async collisions in input operation order: keep the first name unchanged, then append2,3, etc. For example, two operations normalized toGetAllproduceGetAllCollectionResultandGetAllCollectionResult2.RemoveOrInternalize, and skip removed nested types during emission. Preserve existing ownership-based retention for other nested types andkeepAllbehavior.Generated SDK public APIs and paging iteration logic are unchanged. Internal helper names and qualified locations change with the nested layout.
Validation
net8.0with the initial nested layout and emitted five per-client collections files instead of per-operation files.pnpm formatpassed after the latest changes.Spector HTTP runtime integration tests were not run.