icerpc / slicec

The Slice compiler library
Apache License 2.0
13 stars 5 forks source link

Reserved tag numbers #647

Open pepone opened 1 year ago

pepone commented 1 year ago

We should consider adding a way to reserve tag numbers, to avoid breaking the client-server contract once you remove a tagged field, by reintroducing a new field that uses the same tag number.

Protobuf provides this functionality with the reserved keyword https://protobuf.dev/programming-guides/proto3/#fieldreserved

This can be implemented as an Slice attribute for constructs that support tags

[reserved(1)]
struct Person { 
   name: string
   tag(1) address: sequence<string>? // <-- Compilation error using a reserved tag number
}

Or

interface Greeter {
    [reserved(1)]
    greet(
        tag(1) name: string? // <-- Compilation error using a reserved tag number
    ) -> string 
}

Not clear where to put this for the return value.

bernardnormier commented 1 year ago

In Slice: attribute => optional, does not affect the contract, client and server can have different attributes keyword => usually affects the contract

pepone commented 1 year ago

Here reserved doesn't affect the contract, you can remove it without breaking the contract, is just an aid for the compiler to catch the reusing of tag numbers.

InsertCreativityHere commented 1 year ago

This behavior can already be achieved by just not removing tags:

struct Foo {
    tag(1) b: bool, // Unused
}

Now, I see that leaving a bunch of unused fields in your struct isn't desirable, but adding a reserved attribute is really just moving it, more than removing it. Ie. reserved is still "leaving unused fields in your struct", you just moved them somewhere else.


Maybe instead of adding a reserved attribute, we can support anonymous tags?

struct Foo {
    tag(1) // Reserved
}

It's kind of logically equivalent to reserved, but uses a familiar syntax, and would solve the return type problem:

op(a: bool, tag(1) b: string, tag(5)) -> (r: int32, tag(1), tag(2))
pepone commented 1 year ago

This behavior can already be achieved by just not removing tags:

It can be partially, the issue with this solution is that the generated code would still contain a field that nobody should be using.

It's kind of logically equivalent to reserved, but uses a familiar syntax, and would solve the return type problem:

That is fine with me, I was just wondering if the functionality is worth it. And my first impression is that it can be helpful in maintaining Slice definitions that need to evolve over time.

InsertCreativityHere commented 1 year ago

A downside of this is it might give the impression that these will still be mapped to fields and parameters which they can't be. That might be confusing to users.

InsertCreativityHere commented 1 year ago

That is fine with me, I was just wondering if the functionality is worth it. And my first impression is that it can be helpful in maintaining Slice definitions that need to evolve over time.

I'm in the same boat. It doesn't seem very important to support, but long-term users might find it helpful.

The only issue I can see if that these 'reserved' tags might pile up over time, since anyone who uses this feature would probably be apprehensive to remove them (clearly backwards compatibility is a large concern for them).

externl commented 1 year ago

I don't think this issue will be as big a problem for Slice. With protobuf everything is tagged, where as in Slice that is not the case. You have to be a little more careful adding or removing fields.

I'm not big not he reserved attribute for operations, it's not obvious how to apply it to parameters or return values separately. My concern for an anonymous tag is that a developer may easily choose to add a type to this tag, not understanding that it's for a previously removed field that should no longer be used (because it's still in use somewhere else in another service)?

One option could be to add something like:

struct Foo {
    a: string
    retired(5)
    tag(6) c: string?
}

I find that reserved keyword gives the impression that you may want to use it later. But from reading the proto docs, it's really for retiring old tags that should no longer be used.

bernardnormier commented 1 year ago

I prefer retired/reserved over the "anonymous" tag syntax. The anonymous tag gives the impression you forgot the body of your field. It would also be easy to write hard to read:

 -> (r: int32 tag(1) tag(2) s: string)

Note that Protobuf also allows you to reserve a range. And it has a related feature called "extensions" where you reserve a range of numbers for "extensions". extensions is a proto2 feature... confusing.

https://protobuf.com/docs/language-spec#reserved-names-and-numbers

InsertCreativityHere commented 1 year ago

My concern for an anonymous tag is that a developer may easily choose to add a type to this tag, not understanding that it's for a previously removed field that should no longer be used (because it's still in use somewhere else in another service)?

Good point. If we do decide to support this I also prefer 'retired' over 'reserved' for this use-case.

Not clear what a good syntax is for this + return types.

bernardnormier commented 1 year ago

Currently, a struct/class/operation has a field|parameter list, where each field|parameter has a mostly consistent syntax:

(tag(X)] | stream) name: Type

I'd like to see a proposal that keeps this nice "list" consistency.

externl commented 1 year ago

What about this?

op(a: string, b: int32, tag(1) c: string?, retired(2), d: stream int32) 

I still think tag should be in the same location as stream: after the : šŸ˜„

bernardnormier commented 1 year ago

retired(2) is a totally different element in your list; that's what I don't like.

InsertCreativityHere commented 1 year ago

Maybe it's best to shelve this proposal for now.

I suspect there just isn't a good syntax for expressing this for return types. Protobuf gets around this by not having tagged parameters at all.

And while this idea would be useful, It doesn't really add anything to the language, and I suspect it's use would be niche.

bernardnormier commented 1 year ago

Related: https://protobuf.dev/programming-guides/proto3/#updating

Fields can be removed, as long as the field number is not used again in your updated message type. You may want to rename the field instead, perhaps adding the prefix ā€œOBSOLETE_ā€, or make the field number reserved, so that future users of your .proto canā€™t accidentally reuse the number.

Naturally you can also use comments to list retired tag numbers.