Skip to content
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.

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

Implementextends base type for union #2737

Open
timotheeguerin opened this issue Dec 6, 2023 · 13 comments
Open

Implementextends base type for union #2737

timotheeguerin opened this issue Dec 6, 2023 · 13 comments
Labels
compiler:core Issues for @typespec/compiler design:accepted Proposal for design has been discussed and accepted. triaged:core
Milestone

Comments

@timotheeguerin
Copy link
Member

timotheeguerin commented Dec 6, 2023

Goals

  1. Ability to assert a common supertype for all union variants to avoid a common class of error
  2. Ease code generation of unions in situations where a polymorphic base is desired
  3. Make named unions "just work" across all language targets

Extends clause for unions

Sometimes, an API may want to constrain a union's to all share a particular subtype. For example, this constraint allows for some languages to represent unions with a polymorphic base type. The need to express this relationship led to the undesirable pattern of applying @discriminator on a base model, where references to the base model either refer to that model or to a union of its subtypes depending on context. This pattern is not ideal for a number of reasons, and expressing the model-side relationship between super- and sub-types separately from the union of subtypes that might be present for a given API is generally preferred.

However, this presents two problems for where we're at today:

  1. It is easy for API authors to create unions that cannot easily be expressed with a polymorphic base type even if they intend to do so, e.g. by accidentally adding an unrelated type into a union, and
  2. It is hard for emitters to determine the common base type shared among all variants.

To address these problems, we can add an extends clause to unions like so:

// option 1
model Foo { toy: string }
model Cat extends Foo { name: string }
model Dog { name: string, food: string }
model PetBase { name: string }

union Pet extends PetBase {
  Cat, 
  Dog,
}

// option 2
model Cat extends PetBase { name: string, toy: string }
model Dog extends PetBase  { name: string, food: string }
model PetBase { name: string }

union Pet extends PetBase {
  Cat, 
  Dog,
}

// option 3
model Cat { name: string, food: string }
model Dog { name: string, food: string }
model NameBase { name: string }
model FoodBase { name: string }
union PetWithName extends NameBase {
  Cat, 
  Dog,
}
union PetWithFood extends FoodBase {
  Cat, 
  Dog,
}

From a TypeSpec semantics perspective, the extends type does not imply any subclassing relationship but merely documents a constraint that the union is a subtype of the given extends type, and thus that each of its variants are as well. When this constraint is violated, a diagnostic is given on the offending variant.

The extends type will show up in the type graph, giving emitters a convenient way to know if the union's variants share a common base type that can be used as a polymorphic base. It is not recommended for emitters to require this constraint to be present however, and should ideally handle a union with the same variants with or without the constraint in a similar way. In particular, the presence of an extends clause does not indicate that the union is "extensible", and has no new interaction with the @discriminator decorator.

The extends clause is an expression that matches the grammar of an extends constraint on a template parameter.

Change to the default behavior of named unions

Today, a named union is (I believe?) emitted identically to an unnamed union for most languages.

Extensible Unions

Extensible unions are unions where the set of union variants is not known, and so clients and servers need to handle unknown variants. In some cases, the unknown variant is a supertype of the other variants, in which case we might want a feature like sealed classes which allow subclasses to opt out of additional properties. In other cases, the unknown variant may be a special variant that is unrelated to the others.

In either case, the union should include the unknown variant, and we need a way for emitters to identify the unknown variant. There are a few options:

  1. Emitters determine which variant is a supertype of all the other variants
  2. A decorator on the union indicating the type of the unknown variant
  3. A decorator on the variant indicating that it is unknown
  4. A variant with a special name, like "Unknown"
  5. The only unnamed variant in an otherwise named union.
  6. A new keyword like "default".

The drawbacks of each:

  1. Does not work for unrelated unknown variants.
  2. Requires typing the unknown variant type twice (since it should be part of the union).
  3. Requires a new decorator.
  4. If you call a variant Unknown for other reasons, things become problematic.
  5. Doesn't work for unnamed unions, not super explicit.
  6. Requires a new keyword, difficult to name?

I think a decorator on the variant, @unknownVariant, is the best option, and in the future we can consider a keyword.

@timotheeguerin timotheeguerin added the design:needed A design request has been raised that needs a proposal label Dec 11, 2023
@timotheeguerin timotheeguerin self-assigned this Dec 11, 2023
@timotheeguerin timotheeguerin added this to the [2024] February milestone Dec 11, 2023
@markcowl markcowl modified the milestones: [2024] February, [2024] March Feb 7, 2024
@markcowl markcowl modified the milestones: [2024] March, [2024] April Mar 11, 2024
@markcowl markcowl modified the milestones: [2024] April, [2024] May Apr 6, 2024
@markcowl markcowl added design:accepted Proposal for design has been discussed and accepted. and removed design:needed A design request has been raised that needs a proposal labels Apr 10, 2024
@markcowl markcowl removed this from the [2024] May milestone Apr 10, 2024
@markcowl markcowl added this to the [2024] May milestone Apr 23, 2024
@weidongxu-microsoft
Copy link
Contributor

weidongxu-microsoft commented Apr 24, 2024

For a typical extensible string (open enum that could have more value than specified), is it now be below?

union OpenEnum extends string {
  "type1",
  "type2",
  @unknownVariant
  string,
}

I am not sure what type is the @unknownVariant suppose to be. E.g. in discriminated Union, would it be a specific type (instead of the base type)?

@disciminator("kind")
union Type extends {"kind": string} {
  Type1,
  Type2,
  @unknownVariant
  UnknownType,
}

model UnknownType {
  "kind": string,
  ...Record<unknown>,
}

(I wrote UnknownType to try to make it accept anything; this may not be the type service would write)

@timotheeguerin
Copy link
Member Author

What we have today will still work and should still work. I don't think simple primitive unions should need to have extends for emitters to generate something good. A user of course could decide to be explicit but at this point it should just be a checker validation and woudln't provide more information that the emitter couldn't figure out before.

For the 2nd part I think it could be either:

  1. Reuse the base
@disciminator("kind")
union Type extends PetBase {
  Cat
  Dog,
  @unknownVariant
  PetBase,
}

model PetBase {
  "kind": string,
}
  1. Dedicated type
@disciminator("kind")
union Type extends PetBase {
  Cat,
  Dog,
  @unknownVariant
  UnknownPet,
}

model PetBase {
  "kind": string,
}

model UnknownPet {
  "kind": string,
  ...Record<unknown>;
}

I think it is up to the spec writer to decide what they rather do. And maybe in Azure we need to agree on a standard way of doing this.

@weidongxu-microsoft
Copy link
Contributor

weidongxu-microsoft commented Apr 29, 2024

Part of SDK's concern is version resiliency.

E.g. if we have this in v1

@resource
model Resource {
  type: "type1" | "type2"
}

On paper, we are sure the "type" be only "type1" or "type2".

However, if "type3" is added to v2, it would be hard to say that v1 SDK accessing that resource won't have "type=type3" as return (the resource would be returned, the "type" is required property so some value had to be in it).

I assume the recommendation of

Union Type extends string {
  "type1",
  "type2"
}

to Azure service is based on this.

@weidongxu-microsoft
Copy link
Contributor

weidongxu-microsoft commented Apr 29, 2024

BTW, does extends support the pattern of "composition of Union values" like below?

Union OperationStatus extends string {
  "Running",
  "Succeeded",
  "Failure"
}

Union ServiceOperationStatus extends string {
  OperationStatus,
  "NotStarted"
}

@timotheeguerin
Copy link
Member Author

Just to not mix things, extends string doesn't say this union is extensible in any way it just says every variant all extends from this.

So those 2 should not be producing anything different

union Type {
  "type1",
  "type2"
}
union Type extends string {
  "type1",
  "type2"
}

an extensible union will still need to have that extra variant that define extensiblity, in the case of string it would be either of those

union Type {
  "type1",
  "type2",
  string,
}
union Type extends string {
  "type1",
  "type2",
  string
}

for this I believe yes this should work

union OperationStatus extends string {
  "Running",
  "Succeeded",
  "Failure"
}

union ServiceOperationStatus extends string {
  OperationStatus,
  "NotStarted"
}

@markcowl markcowl added this to the [2024] June milestone May 6, 2024
@markcowl
Copy link
Contributor

markcowl commented May 6, 2024

est: 5
pri: 1

@markcowl markcowl self-assigned this Jun 25, 2024
@markcowl
Copy link
Contributor

Additional Discussion

In the above discussion, the following union Pets will be considered valid:

model Pet {name: string,  tag: string}
model Cat {name: string, tag: string, hair: boolean}
model Dog extends Pet { toy: string[] }

union Pets extends Pet {cat: Cat, dog: Dog, Pet}

Some language emitters would prefer to make the inclusion of Cat in the union invalid, as it does not explicitly extend Pet, although it is assignable in TypeSpec.

To enable this extra constraint, we could add a core decorator @strictExtends(type? Type) which would emit a diagnostic if decorating a union which had one or more variants that did not explicitly extend the given type.

In the above

union Pets extends Pet {cat: Cat, dog: Dog, Pet} // valid

@strictExtends
union ReallyPets extends Pet {cat: Cat, dog: Dog, Pet} // emits diagnostic
  • Should the emitted diagnostic be a warning or error?
  • Should Azure.Core (and logically, core emitters) emit a warning diagnostic if the decorator is not present?

@bterlson
Copy link
Member

Another option: admit @willmtemple was right and use satisfies for the current meaning, and extends to mean that each variant must literally extend the given type.

@markcowl
Copy link
Contributor

  • this would require thinking about where these different meaning of extends and satisfies should be applied in the language, and whether the keyword is the right mechanism to express the difference

@timotheeguerin
Copy link
Member Author

Using satsifies vs extends to ask for an explicit relation wouldn't work due to

  1. Being inconsitent with decorators/ scalar consturctors which define the constriant with :
  2. ALready used in template constraint with extends meaning structural match which would be a significiant breaking change

This become a little separate issues from this one which just adds constrains to the union. So we should just implement the original proposal with union Foo extends constraint { ... }

Separate issue to discuss:

@markcowl markcowl modified the milestones: [2024] July, [2024] August Jul 19, 2024
@markcowl markcowl changed the title Discuss extends base type for union Implementextends base type for union Aug 19, 2024
@markcowl
Copy link
Contributor

est: 5

@markcowl markcowl added this to the [2024] October milestone Aug 19, 2024
@markcowl markcowl added the compiler:core Issues for @typespec/compiler label Aug 19, 2024
@markcowl markcowl modified the milestones: [2024] October, Backlog Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:core Issues for @typespec/compiler design:accepted Proposal for design has been discussed and accepted. triaged:core
Projects
None yet
Development

No branches or pull requests

5 participants