Skip to content

Escape reserved names injected by the IceRpc Slice generator #4567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pepone wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Escape reserved names injected by the IceRpc Slice generator #4567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/IceRpc.Slice.Generator/EncodeHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ namespace IceRpc.Slice.Generator;
internal static class EncodeHelper
{
/// <summary>Wraps an encode body in the standard pipe creation/size-placeholder boilerplate.</summary>
internal static CodeBlock BuildEncodeBody(CodeBlock encodeBody)
/// <param name="encodeBody">The encode body.</param>
/// <param name="encodeOptionsName">The escaped name of the encodeOptions parameter referenced by the body.</param>
internal static CodeBlock BuildEncodeBody(CodeBlock encodeBody, string encodeOptionsName)
{
return new CodeBlock($$"""
var pipe_ = new global::System.IO.Pipelines.Pipe(
encodeOptions?.PipeOptions ?? SliceEncodeOptions.Default.PipeOptions);
{{encodeOptionsName}}?.PipeOptions ?? SliceEncodeOptions.Default.PipeOptions);
var encoder_ = new SliceEncoder(pipe_.Writer);

Span<byte> sizePlaceholder_ = encoder_.GetPlaceholderSpan(4);
Expand Down
32 changes: 28 additions & 4 deletions src/IceRpc.Slice.Generator/OperationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,26 @@ internal static class OperationExtensions
internal string FeaturesParamName =>
op.Parameters.Any(p => p.ParameterName == "features") ? "features_" : "features";

/// <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";
Comment on lines +41 to +49
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.).

Copilot uses AI. Check for mistakes.

/// <summary>Gets the escaped name for the injected "encodeOptions" parameter on Response.* helpers,
/// appending "_" if any operation return field uses the name "encodeOptions".</summary>
internal string ResponseEncodeOptionsParamName =>
op.ReturnType.Any(f => f.ParameterName == "encodeOptions") ? "encodeOptions_" : "encodeOptions";

/// <summary>Gets the escaped name for the synthetic "Payload" tuple element used by cs::encodedReturn
/// operations with a streamed return, appending "_" if the streamed return field uses the name "Payload".
/// </summary>
internal string EncodedReturnPayloadName => op.StreamedReturn?.Name == "Payload" ? "Payload_" : "Payload";

/// <summary>Returns the C# type string for a streamed field (parameter or return).
/// Non-optional stream uint8 maps to PipeReader, all others to IAsyncEnumerable&lt;T&gt;.</summary>
internal static string GetStreamTypeString(Field streamField, string currentNamespace)
Expand All @@ -51,7 +71,10 @@ internal static string GetStreamTypeString(Field streamField, string currentName
}

/// <summary>Builds the EncodeStreamOf{Op} method for a streamed field (parameter or return value).</summary>
internal CodeBlock BuildEncodeStreamMethod(Field streamParam, string currentNamespace)
/// <param name="streamParam">The streamed field.</param>
/// <param name="currentNamespace">The current C# namespace.</param>
/// <param name="encodeOptionsName">The escaped name to use for the injected encodeOptions parameter.</param>
internal CodeBlock BuildEncodeStreamMethod(Field streamParam, string currentNamespace, string encodeOptionsName)
{
string opName = op.Name;
string streamType = GetStreamTypeString(streamParam, currentNamespace);
Expand All @@ -65,7 +88,7 @@ internal CodeBlock BuildEncodeStreamMethod(Field streamParam, string currentName
"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.");

if (streamParam.IsByteStream)
Expand All @@ -92,7 +115,7 @@ internal CodeBlock BuildEncodeStreamMethod(Field streamParam, string currentName
{{streamParam.ParameterName}}.ToPipeReader(
{{encodeLambda}},
{{(useSegments ? "true" : "false")}},
encodeOptions)
{{encodeOptionsName}})
""");
}

Expand All @@ -114,7 +137,8 @@ internal string GetServiceReturnType(string currentNamespace)
if (op.StreamedReturn is Field streamReturn)
{
string streamType = GetStreamTypeString(streamReturn, currentNamespace);
return $"global::System.Threading.Tasks.ValueTask<(global::System.IO.Pipelines.PipeReader Payload, {streamType} {streamReturn.Name})>";
string payloadName = op.EncodedReturnPayloadName;
return $"global::System.Threading.Tasks.ValueTask<(global::System.IO.Pipelines.PipeReader {payloadName}, {streamType} {streamReturn.Name})>";
}
return op.NonStreamedReturns.Count > 0
? "global::System.Threading.Tasks.ValueTask<global::System.IO.Pipelines.PipeReader>"
Expand Down
28 changes: 16 additions & 12 deletions src/IceRpc.Slice.Generator/ProxyGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ private static CodeBlock BuildClientOperationDeclaration(Operation op, string cu
builder.AddParameter("IceRpc.Features.IFeatureCollection?", featuresParam, "null", "The invocation features.");
builder.AddParameter(
"global::System.Threading.CancellationToken",
"cancellationToken",
op.CancellationTokenParamName,
"default",
"A cancellation token that receives the cancellation requests.");

Expand Down Expand Up @@ -193,6 +193,7 @@ private static CodeBlock BuildProxyRequestClass(

ImmutableList<Field> nonStreamedParams = op.NonStreamedParameters;
string opName = op.Name;
string encodeOptionsName = op.RequestEncodeOptionsParamName;

CodeBlock? encodeBody = nonStreamedParams.GenerateEncodeBody(currentNamespace);

Expand All @@ -213,7 +214,7 @@ private static CodeBlock BuildProxyRequestClass(
.AddComment(
"summary",
$"Encodes the arguments of operation <c>{op.Identifier}</c> into a request payload.")
.AddParameter("SliceEncodeOptions?", "encodeOptions", "null", "The Slice encode options.")
.AddParameter("SliceEncodeOptions?", encodeOptionsName, "null", "The Slice encode options.")
.AddComment("returns", "The Slice-encoded payload.")
.SetBody(emptyPayload)
.Build());
Expand All @@ -237,16 +238,16 @@ private static CodeBlock BuildProxyRequestClass(
}

request.AddBlock(
fn.AddParameter("SliceEncodeOptions?", "encodeOptions", "null", "The Slice encode options.")
fn.AddParameter("SliceEncodeOptions?", encodeOptionsName, "null", "The Slice encode options.")
.AddComment("returns", "The Slice-encoded payload.")
.SetBody(EncodeHelper.BuildEncodeBody(encodeBody))
.SetBody(EncodeHelper.BuildEncodeBody(encodeBody, encodeOptionsName))
.Build());
}

// Add EncodeStreamOf{Op} method for streamed parameters
if (op.StreamedParameter is Field streamParam)
{
request.AddBlock(op.BuildEncodeStreamMethod(streamParam, currentNamespace));
request.AddBlock(op.BuildEncodeStreamMethod(streamParam, currentNamespace, encodeOptionsName));
}
}

Expand Down Expand Up @@ -440,16 +441,18 @@ private static CodeBlock BuildProxyOperationCore(Operation op, string currentNam
}

string featuresParam = op.FeaturesParamName;
string cancellationTokenParam = op.CancellationTokenParamName;
string requestEncodeOptionsParam = op.RequestEncodeOptionsParamName;
builder.AddParameter("IceRpc.Features.IFeatureCollection?", featuresParam, "null");
builder.AddParameter("global::System.Threading.CancellationToken", "cancellationToken", "default");
builder.AddParameter("global::System.Threading.CancellationToken", cancellationTokenParam, "default");

string encodeArgs = nonStreamedParams.Count > 0
? string.Join(", ", nonStreamedParams.Select(p => p.ParameterName)) + ", encodeOptions: EncodeOptions"
: "encodeOptions: EncodeOptions";
? string.Join(", ", nonStreamedParams.Select(p => p.ParameterName)) + $", {requestEncodeOptionsParam}: EncodeOptions"
: $"{requestEncodeOptionsParam}: EncodeOptions";

// payloadContinuation: either null or the stream encoder
string payloadContinuation = streamParam is not null
? $"Request.EncodeStreamOf{opName}({streamParam.ParameterName}, encodeOptions: EncodeOptions)"
? $"Request.EncodeStreamOf{opName}({streamParam.ParameterName}, {requestEncodeOptionsParam}: EncodeOptions)"
: "null";

FunctionCallBuilder bodyBuilder = new FunctionCallBuilder("this.InvokeOperationAsync")
Expand All @@ -462,7 +465,7 @@ private static CodeBlock BuildProxyOperationCore(Operation op, string currentNam
.AddArgument(featuresParam)
.AddArgumentIf(op.IsIdempotent, "idempotent: true")
.AddArgumentIf(op.Attributes.HasAttribute("oneway"), "oneway: true")
.AddArgument("cancellationToken: cancellationToken");
.AddArgument($"cancellationToken: {cancellationTokenParam}");

if (compressArgs)
{
Expand Down Expand Up @@ -506,8 +509,9 @@ private static CodeBlock BuildProxyBaseOperationDelegation(
paramNames.Add(streamParam.ParameterName);
}
string featuresParam = op.FeaturesParamName;
string cancellationTokenParam = op.CancellationTokenParamName;
paramNames.Add(featuresParam);
paramNames.Add("cancellationToken");
paramNames.Add(cancellationTokenParam);

FunctionBuilder builder = new FunctionBuilder(
"public", returnType, $"{opName}Async", FunctionType.ExpressionBody)
Expand All @@ -526,7 +530,7 @@ private static CodeBlock BuildProxyBaseOperationDelegation(
}

builder.AddParameter("IceRpc.Features.IFeatureCollection?", featuresParam, "null");
builder.AddParameter("global::System.Threading.CancellationToken", "cancellationToken", "default");
builder.AddParameter("global::System.Threading.CancellationToken", cancellationTokenParam, "default");

builder.SetBody($"(({baseProxyName})this).{opName}Async({string.Join(", ", paramNames)})");

Expand Down
11 changes: 6 additions & 5 deletions src/IceRpc.Slice.Generator/ServiceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ private static CodeBlock BuildServiceResponseClass(

string opName = op.Name;
ImmutableList<Field> nonStreamedReturns = op.NonStreamedReturns;
string encodeOptionsName = op.ResponseEncodeOptionsParamName;
CodeBlock? encodeBody = nonStreamedReturns.GenerateEncodeBody(currentNamespace);

if (encodeBody is null)
Expand All @@ -231,7 +232,7 @@ private static CodeBlock BuildServiceResponseClass(
.AddComment(
"summary",
$"Encodes the return value of operation <c>{op.Identifier}</c> into a response payload.")
.AddParameter("SliceEncodeOptions?", "encodeOptions", "null", "The Slice encode options.")
.AddParameter("SliceEncodeOptions?", encodeOptionsName, "null", "The Slice encode options.")
.AddComment("returns", "A new response payload.")
.SetBody(emptyPayload)
.Build());
Expand All @@ -256,16 +257,16 @@ private static CodeBlock BuildServiceResponseClass(

response.AddBlock(
encodeBuilder
.AddParameter("SliceEncodeOptions?", "encodeOptions", "null", "The Slice encode options.")
.AddParameter("SliceEncodeOptions?", encodeOptionsName, "null", "The Slice encode options.")
.AddComment("returns", "A new response payload.")
.SetBody(EncodeHelper.BuildEncodeBody(encodeBody))
.SetBody(EncodeHelper.BuildEncodeBody(encodeBody, encodeOptionsName))
.Build());
}

// Add EncodeStreamOf method for streamed returns
if (op.StreamedReturn is Field streamReturn)
{
response.AddBlock(op.BuildEncodeStreamMethod(streamReturn, currentNamespace));
response.AddBlock(op.BuildEncodeStreamMethod(streamReturn, currentNamespace, encodeOptionsName));
}
}

Expand Down Expand Up @@ -305,7 +306,7 @@ private static CodeBlock BuildServiceOperationDeclaration(Operation op, string c
docComment: "The dispatch features.");
operationBuilder.AddParameter(
"global::System.Threading.CancellationToken",
"cancellationToken",
op.CancellationTokenParamName,
docComment: "A cancellation token that receives the cancellation requests.");

if (op.NonStreamedReturns.Count == 0 && !op.HasStreamedReturn)
Expand Down
32 changes: 32 additions & 0 deletions tests/IceRpc.Slice.Generator.Tests/OperationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,38 @@ public ValueTask OpWithSpecialParameterNamesAsync(
IFeatureCollection features_,
CancellationToken cancellationToken) => default;

public ValueTask OpWithCancellationTokenParameterAsync(
int cancellationToken,
IFeatureCollection features,
CancellationToken cancellationToken_) => default;

public ValueTask OpWithEncodeOptionsParameterAsync(
int encodeOptions,
IFeatureCollection features,
CancellationToken cancellationToken) => default;

public ValueTask<(int R1, int EncodeOptions)> OpWithEncodeOptionsReturnAsync(
IFeatureCollection features,
CancellationToken cancellationToken) => new((1, 42));

public ValueTask<(PipeReader Payload_, IAsyncEnumerable<int> Payload)> OpWithPayloadStreamReturnAsync(
IFeatureCollection features,
CancellationToken cancellationToken)
{
var payload = IMyOperationsAService.Response.EncodeOpWithPayloadStreamReturn(
new int[] { 1, 2, 3 },
features.Get<ISliceFeature>()?.EncodeOptions);
return new((payload, GetDataAsync()));

static async IAsyncEnumerable<int> GetDataAsync()
{
await Task.Yield();
yield return 1;
yield return 2;
yield return 3;
}
}

public ValueTask<PipeReader> OpWithSingleReturnValueAndEncodedReturnAttributeAsync(
IFeatureCollection features,
CancellationToken cancellationToken) =>
Expand Down
12 changes: 12 additions & 0 deletions tests/IceRpc.Slice.Generator.Tests/OperationTests.slice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,18 @@ interface MyOperationsA {
// cancel and features as regular parameter names
opWithSpecialParameterNames(cancel: int32, features: int32)

// cancellationToken as regular parameter name (collides with the injected cancellationToken parameter)
opWithCancellationTokenParameter(cancellationToken: int32)

// encodeOptions as regular parameter name (collides with the injected encodeOptions parameter on Request.Encode*)
opWithEncodeOptionsParameter(encodeOptions: int32)

// encodeOptions as regular return name (collides with the injected encodeOptions parameter on Response.Encode*)
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)
Copy link
Copy Markdown
Member

@InsertCreativityHere InsertCreativityHere May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Encoded return
[cs::encodedReturn] opWithSingleReturnValueAndEncodedReturnAttribute() -> Sequence<int32>
[cs::encodedReturn] opWithMultipleReturnValuesAndEncodedReturnAttribute() -> (r1: Sequence<int32>, r2: Sequence<int32>)
Expand Down
Loading