Escape reserved names injected by the IceRpc Slice generator#4567
Escape reserved names injected by the IceRpc Slice generator#4567pepone wants to merge 1 commit intoicerpc:mainfrom
Conversation
User-defined Slice names could collide with synthetic identifiers the generator injects (cancellationToken, encodeOptions, Payload), producing uncompilable C#. Generalize the existing features_ rename to cover cancellationToken on proxy/service public methods, encodeOptions on Request.* and Response.* helpers, and the Payload tuple element on cs::encodedReturn streamed returns.
There was a problem hiding this comment.
Pull request overview
This PR addresses #4468 by making the IceRpc Slice generator escape additional synthetic identifiers it injects into generated APIs, preventing collisions with user-defined Slice parameter/return names that would otherwise produce uncompilable C#.
Changes:
- Add escaping for injected
cancellationToken, request/responseencodeOptions, and the synthetic encoded-return tuple element namePayload. - Thread escaped identifier names through
ProxyGenerator,ServiceGenerator, and shared encode helpers. - Add new regression operations to the generator test Slice file and update the corresponding C# test expectations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/IceRpc.Slice.Generator.Tests/OperationTests.slice | Adds operations that previously could collide with injected names (cancellationToken, encodeOptions, streamed payload). |
| tests/IceRpc.Slice.Generator.Tests/OperationTests.cs | Updates expected generated service signatures/return types for the new collision scenarios. |
| src/IceRpc.Slice.Generator/ServiceGenerator.cs | Uses escaped injected names for cancellationToken and response-side encodeOptions (including stream encode helpers). |
| src/IceRpc.Slice.Generator/ProxyGenerator.cs | Uses escaped injected names for cancellationToken and request-side encodeOptions (including stream encode helpers). |
| src/IceRpc.Slice.Generator/OperationExtensions.cs | Centralizes the escaping logic for the injected/synthetic names used by generators. |
| src/IceRpc.Slice.Generator/EncodeHelper.cs | Updates the shared encode-body wrapper to reference the escaped encodeOptions parameter name. |
| /// <summary>Gets the escaped name for the injected "cancellationToken" parameter, appending "_" if any | ||
| /// operation parameter uses the name "cancellationToken".</summary> | ||
| internal string CancellationTokenParamName => | ||
| op.Parameters.Any(p => p.ParameterName == "cancellationToken") ? "cancellationToken_" : "cancellationToken"; | ||
|
|
||
| /// <summary>Gets the escaped name for the injected "encodeOptions" parameter on Request.* helpers, | ||
| /// appending "_" if any operation parameter uses the name "encodeOptions".</summary> | ||
| internal string RequestEncodeOptionsParamName => | ||
| op.Parameters.Any(p => p.ParameterName == "encodeOptions") ? "encodeOptions_" : "encodeOptions"; |
There was a problem hiding this comment.
The injected-name escaping logic only appends a single underscore, which can still produce a duplicate identifier when the Slice operation already has both the base name and the underscored variant (e.g., parameters named cancellationToken and cancellationToken_ -> injected name becomes cancellationToken_ and collides). Consider generating a truly-unique injected name by looping/appending underscores until it doesn't match any existing parameter/return ParameterName values (apply consistently to features/cancellationToken/encodeOptions, etc.).
| .AddComment( | ||
| "summary", | ||
| $"Encodes the stream argument of operation <c>{op.Name}</c> into a request payload continuation.") | ||
| .AddParameter(streamType, streamParam.ParameterName) | ||
| .AddParameter("SliceEncodeOptions?", "encodeOptions", "null", "The Slice encode options.") | ||
| .AddParameter("SliceEncodeOptions?", encodeOptionsName, "null", "The Slice encode options.") | ||
| .AddComment("returns", "A new request payload continuation."); |
There was a problem hiding this comment.
BuildEncodeStreamMethod is used to generate both Request.* and Response.* helpers, but the generated XML doc comments always describe a “stream argument” and a “request payload continuation”. This is inaccurate for the Response class (streamed return -> response payload continuation). Consider parameterizing the comment text (request vs response) or splitting into separate helpers so the docs match the generated location/semantics.
| opWithEncodeOptionsReturn() -> (r1: int32, encodeOptions: int32) | ||
|
|
||
| // payload as streamed return name with cs::encodedReturn (collides with the synthetic Payload tuple element) | ||
| [cs::encodedReturn] opWithPayloadStreamReturn() -> (r1: Sequence<int32>, payload: stream int32) |
There was a problem hiding this comment.
Does it make any difference that this is payload (lowercase) and not Payload (uppercase)?
I imagine we normalize to Pascal-case, but figured I'd ask.
Fix #4468