Fix C# back-compat service method overloads for reserved param names; skip overload when previous method ends in CancellationToken#10642
Open
Copilot wants to merge 6 commits into
Open
Conversation
… preserve optional CancellationToken Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/09ec7e5f-2d2f-4a58-8662-7d148879bb09 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
… on non-trailing params Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/09ec7e5f-2d2f-4a58-8662-7d148879bb09 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix generated back compat service methods for reserved param names
Fix C# back-compat service method overloads for reserved param names and CancellationToken
May 11, 2026
…; use testdata for new test Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/3f9baf01-88a8-4b30-a7ca-d0a5b5897fe4 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Copilot
AI
changed the title
Fix C# back-compat service method overloads for reserved param names and CancellationToken
Fix C# back-compat service method overloads for reserved param names
May 11, 2026
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/6dbf78c7-b80c-4935-9257-76966b6f1923 Co-authored-by: jorgerangel-msft <102122018+jorgerangel-msft@users.noreply.github.com>
Copilot
AI
changed the title
Fix C# back-compat service method overloads for reserved param names
Fix C# back-compat service method overloads for reserved param names; skip overload when previous method ends in CancellationToken
May 11, 2026
commit: |
JonathanCrd
requested changes
May 11, 2026
Agent-Logs-Url: https://github.com/microsoft/typespec/sessions/e8de2bea-833b-4054-a871-6cf5fd2e23f4 Co-authored-by: JonathanCrd <17486462+JonathanCrd@users.noreply.github.com>
JonathanCrd
approved these changes
May 11, 2026
Contributor
jorgerangel-msft
left a comment
There was a problem hiding this comment.
Can we also please get a regen preview? It might also be worth running the debug profile script against Azure/azure-sdk-for-net#59156 to verify the changes
| // before it). | ||
| // - Making the trailing CancellationToken required avoids the ambiguity but violates the | ||
| // SDK guideline that CancellationToken parameters must be optional. | ||
| // In this case the library author must address the gap via custom code or by updating the spec. |
Contributor
There was a problem hiding this comment.
nit: do we need such a long explanation? The log message seems self explanatory 😃
| } | ||
|
|
||
| var lastParam = previousSignature.Parameters[previousSignature.Parameters.Count - 1]; | ||
| return lastParam.Type.Equals(typeof(CancellationToken)); |
Contributor
There was a problem hiding this comment.
should we ignore nullability as well?
| // Match parameters by their C# variable name (which is what is rendered in C# source) so | ||
| // that previous parameters whose raw input name differs only by reserved characters such as | ||
| // a leading '$' (e.g. OData "$select"/"$top") are matched to the corresponding current | ||
| // parameter. The named-argument label below must also use the C# variable name. |
Contributor
There was a problem hiding this comment.
Copilot had added tests for this, then undid them. It would be great if we can add those back in or add new unit tests exercising this scenario
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Back-compat overloads generated for service methods (added in 72a95c2) produced uncompilable C# when previous parameters had raw input names that are not valid C# identifiers (e.g. OData query parameters such as
$select,$top,$skip,$count). They also could not be emitted correctly when the previous method ended in aCancellationTokenparameter — making CT optional introduces aCS0121ambiguity at basic call sites with the new method, and making CT required violates the SDK guideline thatCancellationTokenparameters must be optional.Changes
ClientProvider.BuildBackCompatOverloadForNewOptionalParameters— emit named-argument labels using the C# variable name (select:,top:) rather than the raw input name ($select:,$top:). Match previous→current parameters byToVariableName()so renames that differ only by reserved characters still align.ClientProvider.ProcessBackCompatForNewOptionalParameters— skip generating the back-compat overload entirely when the previous method's trailing parameter is aCancellationToken. In that case the library author must address the gap via custom code or by updating the spec.CancellationToken). The previously addedBackCompatibility_NewOptionalParameterWithReservedNametest was removed because its scenario also ends inCancellationTokenand is now skipped.Example (reserved-name fix, applies when an overload IS emitted)
Before:
After:
Note
For the original OData scenario (previous method ends in
CancellationToken), no back-compat overload is now emitted at all, so the buggy generated C# can no longer occur. The reserved-name fix inBuildBackCompatOverloadForNewOptionalParametersis retained as defensive correctness for any future scenario where a back-compat overload is still emitted.