icerpc / slicec

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

Rework TypeAlias #72

Closed pepone closed 2 years ago

pepone commented 2 years ago

I think we currently don't distinguish typelias from other types

This is correct. The idea behind type-aliases is that they're for convenience when writing your Slice definition. After parsing is complete, everywhere where a type-alias was used is replaced with the actual underlying type.

For example:

typealias MyType = [cs:type("foo")] sequence<int>;
class C {
    my_data: MyType,
}

After parsing, this will become:

typealias MyType = sequence<int>;
class C {
    my_data: [cs:type("foo")] sequence<int>,
}

There is currently no way to tell whether a type was originally from an alias or not. To implement this proposal we'd need to add either A) Add an extra field to TypeRef which stores a Option<TypeAlias> that we could check to if an alias was used B) Change the parser to no longer "parse out" aliases.

Originally posted by @InsertCreativityHere in https://github.com/zeroc-ice/icerpc-csharp/issues/674#issuecomment-1009021057

pepone commented 2 years ago

I think the compilers need to be able to get the TypeAlias name and scope, this allow to generate methods, types that are specific for the TypeAlias and use them when the alias is used as a type.

No clear what option B would look like, there is also the case of TypeAlias that point to other TypeAlias, I don't think we need to access the whole chain of alias here.

InsertCreativityHere commented 2 years ago

I don't think we need to access the whole chain of alias here

What about something like

typealias CustomType = [cs:type("foo")] long;
typealias Alias = CustomType;

struct S
{
    myData: Alias,
}

If we only check the immediate TypeAlias, we'll miss the cs:type attribute at the start of the chain. Or would we always generate a decode method, even for aliases that don't use a custom type attribute?

pepone commented 2 years ago

We should also consider disallowing TypeAlias pointing to other TypeAlias, they clearly complicate this and if there isn't a good use case might be simpler to just disallow them.

InsertCreativityHere commented 2 years ago

Not allowing 'nested' TypeAliass would definitely simplify both approaches (A and B), but I could see some use-cases getting lost. The original motivation behind adding TypeAlias was to avoid needing to type out deeply nested namespaces, once we added support for nested module syntax. This would become partially broken.

Take this from our own code 'IceRpc/Transports/Internal/SlicDefinitions.ice'

module IceRpc::Transports::Internal
{ ...
    typealias ParameterFields = dictionary<varint, sequence<byte>>;
... }

Being able to do

typealias IceParameterFields = IceRpc::Transports::Internal::ParameterFields;

my_data: IceParameterFields

Is much nicer than needing to type out

my_data: IceRpc::Transports::Internal::ParameterFields

Every time you need to use it.

InsertCreativityHere commented 2 years ago

To me, it feels like we're trying to use TypeAlias as a TypeDef. TypeAliases are about providing a new name for an existing type, not defining an entirely new stand-alone type.

In Swift: https://www.avanderlee.com/swift/typealias-usage-swift/#is-a-typealias-a-new-type

No, basically, it’s only a named alias of an existing type

In C#: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/using-directive#using-alias

Create a using alias directive to make it easier to qualify an identifier to a namespace or type

In Rust: http://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/first-edition/type-aliases.html

Note, however, that this is an alias, not a new type entirely

In the past I proposed splitting typealias into alias and typedef, I think this might be a good point to reconsider doing this.

pepone commented 2 years ago

To me, it feels like we're trying to use TypeAlias as a TypeDef.

I agree, alias should be just a new name for the same thing, and the def can introduce a new thing by providing different attributes, splitting both sounds like a good idea.

InsertCreativityHere commented 2 years ago

Then we can add whatever restrictions we want on TypeDef as well, and make it non-transparent, solving the issue you brought up on https://github.com/zeroc-ice/icerpc-csharp/issues/674

I'll blog about it tonight, and open a proposal to split them.

InsertCreativityHere commented 2 years ago

Although personally, I still think it would be nice to support typedefs of typedefs. I'll have to see what an implementation of Option B would look like. Maybe it's too ugly.

bernardnormier commented 2 years ago

I agree, alias should be just a new name for the same thing, and the def can introduce a new thing by providing different attributes, splitting both sounds like a good idea.

I don't see it that way: the cs::type attribute changes the mapping, it doesn't change the Slice type. In particular, it doesn't change / must not change the encoded representation. For example:

typealias StringSet = [cs::type("StringSet")] sequence<string>;

struct S
{
    data: StringSet, // in encoded form, must be identical to `sequence<string>`
}
pepone commented 2 years ago

I don't see it that way: the cs::type attribute changes the mapping, it doesn't change the Slice type. In particular, it doesn't change / must not change the encoded representation. For example:

I see your point it is true that the Slice type remains the same.

One difficulty with supporting

typealias CustomType = [cs:type("foo")] long;
typealias Alias = CustomType;

We need to keep track of what attributes are attached to each typealias, so that we can correctly call DecodeCustomType/EncodeCustomType for methods that use Alias

bernardnormier commented 2 years ago

After discussing with Austin and Joe, I agree it makes sense to introduce two separate constructs: type = new type, with possibly custom-mapping attributes and alias = new alias (custom mapping attributes not allowed).

InsertCreativityHere commented 2 years ago

I'm going to open a new proposal describing this split, and then close this one if everyone is okay with that.

bernardnormier commented 2 years ago

See #73