Add .ice files equivalent to the mode=Slice1 Slice files#29
Add .ice files equivalent to the mode=Slice1 Slice files#29bernardnormier wants to merge 3 commits intoicerpc:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new .ice (Slice2-style) definition files intended to mirror the existing mode=Slice1 .slice definitions, as preparation for switching icerpc-csharp to consume .ice sources.
Changes:
- Added
.iceequivalents for IceRpc internal Slice types (InvocationMode,TcpServerAddressBody). - Added
.icedefinitions for Ice protocol frame-related internal types (IceRpc/Internal/IceDefinitions.ice). - Added
.icedefinitions for several Ice interfaces/types (Ice/*.ice).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| IceRpc/Slice/Internal/TcpServerAddressBody.ice | Adds Slice2-style struct for tcp/ssl server address body. |
| IceRpc/Slice/Internal/InvocationMode.ice | Adds Slice2-style enum for invocation mode. |
| IceRpc/Internal/IceDefinitions.ice | Adds Slice2-style definitions for Ice frame/protocol structs/enums. |
| Ice/Process.ice | Adds .ice definition for Ice::Process. |
| Ice/Object.ice | Adds .ice definition for Ice::Object. |
| Ice/LocatorRegistry.ice | Adds .ice definition for Ice::LocatorRegistry and related exceptions. |
| Ice/Locator.ice | Adds .ice definition for Ice::Locator/LocatorFinder and related exceptions. |
| Ice/Identity.ice | Adds .ice definition for Ice::Identity. |
| Ice/BuiltinSequences.ice | Adds .ice definition for common built-in sequences. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Client applications use the Locator object to resolve Ice indirect proxies. This object also allows | ||
| /// server applications to retrieve a proxy to the associated {@link LocatorRegistry} object where they can register | ||
| /// their object adapters. | ||
| interface Locator | ||
| { | ||
| /// Finds an object by identity and returns a dummy proxy with the endpoint(s) that can be used to reach this | ||
| /// object. This dummy proxy may be an indirect proxy that requires further resolution using | ||
| /// {@link findAdapterById}. | ||
| /// @param id The identity. | ||
| /// @return A dummy proxy, or null if an object with the requested identity was not found. | ||
| /// @throws ObjectNotFoundException Thrown when an object with the requested identity was not found. The caller | ||
| /// should treat this exception like a null return value. | ||
| ["cpp:const"] | ||
| idempotent Object* findObjectById(Identity id) | ||
| throws ObjectNotFoundException; | ||
|
|
||
| /// Finds an object adapter by adapter ID and returns a dummy proxy with the object adapter's endpoint(s). | ||
| /// @param id The adapter ID. | ||
| /// @return A dummy proxy with the adapter's endpoints, or null if an object adapter with @p id was not found. | ||
| /// @throws AdapterNotFoundException Thrown when an object adapter with this adapter ID was not found. The | ||
| /// caller should treat this exception like a null return value. | ||
| ["cpp:const"] | ||
| idempotent Object* findAdapterById(string id) | ||
| throws AdapterNotFoundException; |
There was a problem hiding this comment.
These Ice/*.ice definitions don’t appear equivalent to the existing Ice/*.slice (mode=Slice1) versions: e.g., findObjectById takes Identity and returns Object* here, while the Slice1 file uses IdentityPath and returns IceRpc::ServiceAddress?. If the PR’s goal is 1:1 equivalents, the signatures/types need to match; otherwise, please update the PR description (or add a note in these files) to clarify that these are canonical Ice definitions rather than the IceRpc-adapted Slice1 surface.
There was a problem hiding this comment.
They are equivalent on-the-wire. The syntax is not 100% equivalent because of missing features in the .ice syntax, such as custom types.
| /// A proxy encoded with Slice1 includes an InvocationMode which specifies the behavior when sending requests using | ||
| /// this proxy. | ||
| enum InvocationMode | ||
| { | ||
| /// This is the default invocation mode; a request using this mode always expects a response. | ||
| Twoway, | ||
|
|
There was a problem hiding this comment.
The Slice1 version of this enum is marked [cs::internal], but this .ice definition has no equivalent C# visibility metadata. If icerpc-csharp will generate code from this .ice, this likely changes the enum from internal to public; please add the Slice2/C# metadata needed to keep this type internal (or confirm the generator enforces internal visibility elsewhere).
There was a problem hiding this comment.
We want to add "cs:internal" metadata directive for Ice-Slice. But it's not done yet.
| /// Represents the body of a tcp or ssl server address in an ice proxy. | ||
| struct TcpServerAddressBody | ||
| { | ||
| string host; | ||
| int port; | ||
| int timeout; | ||
| bool compress; | ||
| } |
There was a problem hiding this comment.
The Slice1 version of this type is a [cs::internal] [cs::readonly] compact struct. This .ice version drops the internal/readonly metadata and the compact encoding hint, which can change generated C# API surface and potentially the on-the-wire encoding/size. Please add the Slice2 equivalents (or otherwise ensure codegen/encoding remains compatible with the existing Slice1 definition).
There was a problem hiding this comment.
See above for "cs:internal" and "cs:readonly". There is no compact "hint" in Ice-Slice: structs are always compact in Ice-Slice.
|
|
||
| module IceRpc::Slice::Internal | ||
| { | ||
| /// Represents the body of a tcp or ssl server address in an ice proxy. |
There was a problem hiding this comment.
The Slice1 file documents this struct as "Used only with Slice1", but that qualifier is missing here. If this .ice is intended to be a drop-in equivalent, consider restoring that clarification to avoid confusion about when this type is applicable.
| /// Represents the body of a tcp or ssl server address in an ice proxy. | |
| /// Represents the body of a tcp or ssl server address in an ice proxy. Used only with Slice1. |
There was a problem hiding this comment.
This is implied since the .ice format implies Slice1.
| /// The identity of a service. | ||
| /// @remarks We named this internal struct IceIdentity to avoid confusion with Ice::Identity, the public version | ||
| /// of this struct. | ||
| struct IceIdentity | ||
| { | ||
| /// The name of the identity. | ||
| string name; | ||
|
|
||
| /// The category of the identity. | ||
| string category; | ||
| } | ||
|
|
||
| /// The facet of a service. A sequence with 0 element corresponds to the default, empty facet; a sequence with a | ||
| /// single element corresponds to a non-empty facet; and a sequence with more than 1 element is invalid. | ||
| sequence<string> Facet; | ||
|
|
||
| /// Each ice request frame has: | ||
| /// - a frame prologue, with the frame type and the overall frame size | ||
| /// - a request ID | ||
| /// - a request header (below) | ||
| /// - a request payload, with encapsulationSize - 6 bytes | ||
| struct IceRequestHeader | ||
| { | ||
| IceIdentity identity; | ||
| Facet facet; | ||
| string operation; | ||
| OperationMode operationMode; | ||
| // Manually encoded/decoded | ||
| // Context context; | ||
| // EncapsulationHeader encapsulationHeader; | ||
| } |
There was a problem hiding this comment.
This .ice file is described as an equivalent of IceRpc/Internal/IceDefinitions.slice, but several definitions are not equivalent: the Slice1 version marks most types as [cs::internal] [cs::readonly] compact, and IceRequestHeader uses Identity + Fragment whereas this uses IceIdentity + Facet. Please confirm the intended compatibility (API surface and encoding) and, if equivalence is required, adjust the types/metadata accordingly.
There was a problem hiding this comment.
These differences are fine and don't affect the on-the-wire representation.
|
|
||
| ["cs:identifier:IceRpc.Slice.Ice"] |
There was a problem hiding this comment.
All other newly-added Ice/*.ice files wrap the C# namespace/identifier override in #ifdef __ICERPC__, but this file applies "cs:identifier:IceRpc.Slice.Ice" unconditionally. If these Slice definitions are ever consumed outside an IceRpc build, this will force the C# namespace unexpectedly. Please wrap this attribute with the same __ICERPC__ guard for consistency and to avoid impacting non-IceRpc consumers.
| ["cs:identifier:IceRpc.Slice.Ice"] | |
| #ifdef __ICERPC__ | |
| ["cs:identifier:IceRpc.Slice.Ice"] | |
| #endif |
| @@ -0,0 +1,57 @@ | |||
| // Copyright (c) ZeroC, Inc. | |||
There was a problem hiding this comment.
BuiltinSequences.ice etc are identical to the files in https://github.com/zeroc-ice/ice/tree/main/slice (main and 3.8), except for the addition of the #ifdef __ICERPC__ block.
| { | ||
| /// Represents the implicit base interface of all Slice interfaces. | ||
| ["cs:identifier:IceObject"] | ||
| interface \Object |
There was a problem hiding this comment.
This interface is specific to IceRPC (not copied from zeroc-ice/ice).
| // These definitions help with the encoding of ice frames. | ||
|
|
||
| /// Each ice frame has a type identified by this enumeration. | ||
| enum IceFrameType |
There was a problem hiding this comment.
Ideally all these types would be internal in C#, but we don't provide yet a "cs:internal" metadata directive.
| } | ||
|
|
||
| /// The ice frame prologue. | ||
| struct IcePrologue |
There was a problem hiding this comment.
It would be nice to make these structs readonly with a "cs:readonly" metadata directive.
| { | ||
| /// A proxy encoded with Slice1 includes an InvocationMode which specifies the behavior when sending requests using | ||
| /// this proxy. | ||
| enum InvocationMode |
There was a problem hiding this comment.
zeroc-ice/ice doesn't define InvocationMode in Slice.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// An idempotent operation. The client runtime does not guarantee at-most-once semantics for such an | ||
| /// operation. | ||
| Idempotent, |
There was a problem hiding this comment.
OperationMode uses Idempotent as an enum enumerator. In the existing Slice1 definition this enumerator is escaped as \Idempotent (because idempotent is a Slice keyword). To avoid the same keyword collision in Ice-Slice and keep the definitions aligned, escape this enumerator here as well.
| Idempotent, | |
| \Idempotent, |
There was a problem hiding this comment.
In the latest Ice release (3.8), such conflicts are case-sensitive, as evidenced by OperationMode in ice:
https://github.com/zeroc-ice/ice/blob/3.8/slice/Ice/OperationMode.ice
I didn't reuse this ice OperationMode.ice file as I want to make it internal in IceRPC.
|
|
||
| ["cs:identifier:IceRpc.Slice.Ice"] | ||
| module Ice | ||
| { | ||
| /// Represents the implicit base interface of all Slice interfaces. | ||
| ["cs:identifier:IceObject"] |
There was a problem hiding this comment.
Ice/Object.ice is missing several file/module-level directives that are present in the other Ice/*.ice files in this PR (e.g., [["cpp:dll-export:ICE_API"]], [["js:module:@zeroc/ice"]], "java:identifier:com.zeroc.Ice", and the #ifdef __ICERPC__ guard around the C# identifier). If this file is meant to be compiled alongside the rest of the Ice .ice definitions, please add the same directives/guards for consistency and to avoid generating a different API surface for Object than for the rest of the Ice module.
| ["cs:identifier:IceRpc.Slice.Ice"] | |
| module Ice | |
| { | |
| /// Represents the implicit base interface of all Slice interfaces. | |
| ["cs:identifier:IceObject"] | |
| ["cpp:dll-export:ICE_API"] | |
| ["js:module:@zeroc/ice"] | |
| ["java:identifier:com.zeroc.Ice"] | |
| #ifdef __ICERPC__ | |
| ["cs:identifier:IceRpc.Slice.Ice"] | |
| #endif | |
| module Ice | |
| { | |
| /// Represents the implicit base interface of all Slice interfaces. | |
| #ifdef __ICERPC__ | |
| ["cs:identifier:IceObject"] | |
| #endif |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR adds .ice files equivalent to the existing mode=Slice1 Slice files.
Once we've updated icerpc-csharp to use these .ice files, I'll open a follow-up PR to delete the mode=Slice1 files.