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

Enable type parameter lower-bound syntax #14520

Open
jyuhuan opened this issue Mar 7, 2017 · 42 comments
Open

Enable type parameter lower-bound syntax #14520

jyuhuan opened this issue Mar 7, 2017 · 42 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@jyuhuan
Copy link

jyuhuan commented Mar 7, 2017

TypeScript Version: 2.1.1

Code

class Animal {}
class Cat extends Animal {}
class Kitten extends Cat{}

function foo<A super Kitten>(a: A) { /* */ }

Expected behavior:
The type parameter A has the type Kitten as lower-bound.

Actual behavior:
Compilation failure. The syntax is unsupported.

Discussion:
The upper-bound counterpart of the failed code works fine:

class Animal {}
class Cat extends Animal {}
class Kitten extends Cat{}

function foo<A extends Animal>(a: A) { /* */ }

People in issue #13337 have suggested to use

function foo <X extends Y, Y>(y: Y) { /* */ }

to lower-bound Y with X. But this does not cover the case where X is an actual type (instead of a type parameter).

@RyanCavanaugh
Copy link
Member

What is this useful for?

@RyanCavanaugh RyanCavanaugh added Needs More Info The issue still hasn't been fully clarified Suggestion An idea for TypeScript labels Mar 7, 2017
@jyuhuan
Copy link
Author

jyuhuan commented Mar 7, 2017

@RyanCavanaugh : In short, it mimics contravariance, just as extends mimics covariance.

We will try to sort an array of cats to see the necessity of this feature.

To do comparison-based sorting, we need a Comparator interface. For this example, we define it as follows:

interface Comparator<T> {
  compare(x: T, y: T): number
}

The following code shows that the class Cat has Animal as its super-class:

class Animal {}
class Cat extends Animal {}

Now we can write a sorting function that supports arbitrary Cat comparators as follows:

function sort(cats: Cat[], comparator: Comparator<Cat>): void {
  // Some comparison-based sorting algorithm.
  // The following line uses the comparator to compare two cats.
  comparator.compare(cats[0], cats[1]);
  // ...
}

Now, we will try to use the sort function. The first thing is to implement a CatComparator:

class CatComparator implements Comparator<Cat> {
  compare(x: Cat, y: Cat): number {
    throw new Error('Method not implemented.');
  }
}

Then we create a list of Cats,

const cats = [ new Cat(), new Cat(), new Cat() ]

Now we can call sort as follows without any problem:

sort(cats, new CatComparator());

We have not seen the need for contravariance so far.

Now, suppose we are told that someone has already implemented a comparator for Animals as follows:

class AnimalComparator implements Comparator<Animal> {
  compare(x: Animal, y: Animal): number {
    throw new Error('Method not implemented.');
  }
}

Since a Cat is also an Animal, this AnimalComparator is also able to handle Cats, because the compare function in AnimalComparator takes two Animals as input. I can just pass two Cats to it and there will be no problem.

Naturally, we would want to use AnimalComparator for sort too, i.e., call the sort function as:

sort(cats, new AnimalComparator());

However, since the following two types:

  • Comparator<Animal>
  • Comparator<Cat>

are not related from the point of view of TypeScript's type system, we cannot do that.

Therefore, I would like the sort function to look like the following

function sort<T super Cat>(cats: Cat[], comparator: Comparator<T>): void {
  // Some comparison-based sorting algorithm.
  // The following line uses the comparator to compare two cats.
  comparator.compare(cats[0], cats[1]);
  // ...
}

or as in Java,

function sort(cats: Cat[], comparator: Comparator<? super Cat>): void {
  // Some comparison-based sorting algorithm.
  // The following line uses the comparator to compare two cats.
  comparator.compare(cats[0], cats[1]);
  // ...
}

I am aware of the fact that TypeScript does not complain if I pass AnimalComparator to sort. But I would like TypeScript's type system to explicitly handle type lower-bounds. In fact, the current type system of TypeScript will let some type error closely related to this issue silently pass the compiler's check (see issue #14524).

@jklmli
Copy link

jklmli commented Mar 7, 2017

This is the best example of contravariance I've read in a long time. Props! 🙌

@btipling
Copy link

btipling commented Mar 8, 2017

As a reference point, flowtype uses - and + to indicate covariance and contravariance

class ReadOnlyMap<K, +V> {
  store: { +[k:K]: V };
  constructor(store) { this.store = store; }
  get(k: K): ?V { return this.store[k]; }
}

@HerringtonDarkholme
Copy link
Contributor

HerringtonDarkholme commented Mar 9, 2017

Hi @jyuhuan , since you probably already know this https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-function-parameters-bivariant

I'm afraid lower bound isn't that useful in current TypeScript where variance is unsound.

Indeed, there are also cases lower bound can be useful without variance. Like

interface Array<T> {
  concat<U super T>(arg: ): Array<U>
}

var a = [new Cat]
a.concat(new Animal) // inferred as Animal[]

In such case like immutable sequence container, lower bound helps TypeScript to enable pattern where new generic type is wider than original type.

Yet such usage still needs more proposal since TS also has union type. For example should strArr.concat(num) yield a Array<string | number>?

@zpdDG4gta8XKpMCd
Copy link

migrated from #14728:

currently when we see a constraint <A extends B> it means A is a subtype of B, so that

declare var a: A;
declare var b: B;
b = a; // allowed
a = b; // not allowed

consider adding a new constraint of the reversed relation: <A within B> that would mean A is a supertype of B (or in other words B is subtype of A), so that:

declare var a: A;
declare var b: B;
b = a; // not allowed
a = b; // allowed

use case

i have a BigFatClass with 50 methods and a 1 little property, now i want to run some assertions over it, if i declare these assertions like expect<T>() and toEqual<T> of the same T then toEqual asks me for 50 methods that don't matter for the test, and that's the problem

what i need it to declare expect<T>() and toEqual<U within T>() so that i could simply write:

expect(new BigFatClass()).toEqual({ value: true });

@aaronbeall
Copy link

<U within keyof T> could be confusing, since U in this case should be a super-set of T keys, not "within" the keys of T.

@zpdDG4gta8XKpMCd
Copy link

the idea is that U is always a subset, never a superset, so if so it should not be a problem

@aaronbeall
Copy link

So <U within keyof T> would be the same as <U extends keyof T>?

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Mar 21, 2017

i always forget that subtype of a union is a subset of that union, conversely a supertype of a union must be a superset, so you right that within would look like a poor choice of word to indicate that something is a superset of something else, how about A unties B or A relaxes B or loosens, frees, eases etc: https://www.powerthesaurus.org/loosen/synonyms

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Mar 21, 2017

@aaronbeall problem with Partial<T> for making a supertype of a product type, is that it only works at the top level, so it doesn't really work for my use case, consider:

type Super<T> = Partial<T>;
type Data = { nested: { value: number; }; }
const one: Super<Data> = {}; // works
const another: Super<Data> = { nested: {} }; // bummer

so i am back to hammering the expected values with the type assertions

expect(data).toEqual(<Data>{ nested: {} });

@aaronbeall
Copy link

@Aleksey-Bykov Probably doesn't make this feature any less valuable but for your case I think you can use a recursive partial type:

type DeepPartial<T> = {
  [P in keyof T]?: DeepPartial<T[P]>;
}

(This was suggested as an addition to the standard type defs, which I think would be very useful.)

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Mar 22, 2017

@aaronbeall DeepPartial almost works for my needs, except that due to having an empty object in it, it can be assigned by anything (except null and undefined), and it's a problem:

type Super<T> = DeepPartial<T>
type Data = { value: number; }
const one: Data = 5; // not allowed
const another: Super<Data> = null; // not allowed
const yetAnother: Super<Data> = 5; // allowed
const justLike: {} = 5; // <-- reason why (allowed)

@jmlopez-rod
Copy link

jmlopez-rod commented May 9, 2017

@Aleksey-Bykov Woah, you just answered my question. But now I have another one, why is this allowed?

const a1: {} = 0;
const a2: {} = '0';
const a3: {} = false;
const a4: {} = { a: 0 };

Is {} the same asany minus null and undefined?

EDIT: I tried the following DeepPartial definition in the link I provided and it seems to work.

type DeepPartial<T> = {[P in keyof T]?: T[P] | (DeepPartial<T[P]> & object); };

It is pretty late, maybe that won't work either, I'll probably think of some example that will break it once I wake up.

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus and removed Needs More Info The issue still hasn't been fully clarified labels May 9, 2017
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
`getOrElse` and `orElse` use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method body.
This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[2], but true F-bounded polymorphism hasn't been.
This means a type like `interface Option<A, B = Option<A, B>>` is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete classes[3].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#stricter-checking-for-generic-functions
[2] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-parameters-as-constraints
[3] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
getOrElse and orElse use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method body.
This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[2], but true F-bounded polymorphism hasn't been.
This means a type like `interface Option<A, B = Option<A, B>>` is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete classes[3].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#stricter-checking-for-generic-functions
[2] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-parameters-as-constraints
[3] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
getOrElse and orElse use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method body.
This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[2], but true F-bounded polymorphism hasn't been.
This means a type like interface Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete classes[3].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#stricter-checking-for-generic-functions
[2] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-parameters-as-constraints
[3] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
getOrElse and orElse use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method body.
This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[2], but true F-bounded polymorphism hasn't been.
This means a type like interface Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete classes[3].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#stricter-checking-for-generic-functions
[2] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-parameters-as-constraints
[3] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
getOrElse and orElse use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method body.
This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[2], but true F-bounded polymorphism hasn't been.
This means a type like interface Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete classes[3].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#stricter-checking-for-generic-functions
[2] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-parameters-as-constraints
[3] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
getOrElse and orElse use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method body.
This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[2], but true F-bounded polymorphism hasn't been.
This means a type like interface Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete classes[3].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#stricter-checking-for-generic-functions
[2] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#type-parameters-as-constraints
[3] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
`getOrElse` and `orElse` use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method
body.  This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[1], but true F-bounded polymorphism hasn't been.
This means a type like `interface` Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete
classes[2].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors
compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript
[2] microsoft/TypeScript#14520
jklmli added a commit to jklmli/monapt that referenced this issue Jul 5, 2017
`getOrElse` and `orElse` use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method
body.  This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[1], but true F-bounded polymorphism hasn't been.
This means a type like `interface` Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete
classes[2].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors
compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript
[2] microsoft/TypeScript#14520
@vanbujm
Copy link

vanbujm commented May 25, 2023

Is there any movement in implementing this?
It has been 6 years and it is now blocking a bunch of other PRs and it would be amazing to have some of that functionality in TS.

@cyber-barrista
Copy link

A bit of a somewhat working workaround for anyone interested 😃

type Range<Lower, Upper> = Upper & Partial<Extract<Lower, Upper>>

// test
type Big = { f1: number, f2: string, f3: string }
type Small = { f1: number }

const scoped: Range<Big, Small> = { f1: 1, f2: 'one' }
const small: Small = scoped
const big: Big = scoped // no no

adonis0302 added a commit to adonis0302/monapt that referenced this issue Sep 23, 2023
`getOrElse` and `orElse` use self-types in order to support typed upper bounds.[0]

In TypeScript 2.4, generic functions were checked more strictly[1].
This causes the implicit downward type cast to fail, so we explicitly invoke the cast in the method
body.  This workaround is backwards-compatible with TypeScript 2.3.

Bounded polymorphism has been implemented[1], but true F-bounded polymorphism hasn't been.
This means a type like `interface` Option<A, B = Option<A, B>> is invalid.

Alternatively, we can solve this with a lower type bound, but these don't work against concrete
classes[2].

---

We should also upgrade monapt's TypeScript dependency to 2.4, but there are unrelated errors
compiling the tests.

[0] microsoft/TypeScript#13337
[1] https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript
[2] microsoft/TypeScript#14520
psychedelicious added a commit to invoke-ai/InvokeAI that referenced this issue Nov 17, 2023
In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.
psychedelicious added a commit to psychedelicious/InvokeAI that referenced this issue Nov 22, 2023
In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.
psychedelicious added a commit to psychedelicious/InvokeAI that referenced this issue Nov 24, 2023
In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.
psychedelicious added a commit to psychedelicious/InvokeAI that referenced this issue Nov 25, 2023
Node authors may now create their own arbitrary/custom field types. Any pydantic model is supported.

Two notes:
1. Your field type's class name must be unique.

Suggest prefixing fields with something related to the node pack as a kind of namespace.

2. Custom field types function as connection-only fields.

For example, if your custom field has string attributes, you will not get a text input for that attribute when you give a node a field with your custom type.

This is the same behaviour as other complex fields that don't have custom UIs in the workflow editor - like, say, a string collection.

feat(ui): fix tooltips for custom types

We need to hold onto the original type of the field so they don't all just show up as "Unknown".

fix(ui): fix ts error with custom fields

feat(ui): custom field types connection validation

In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.

fix(ui): typo

feat(ui): add CustomCollection and CustomPolymorphic field types

feat(ui): add validation for CustomCollection & CustomPolymorphic types

- Update connection validation for custom types
- Use simple string parsing to determine if a field is a collection or polymorphic type.
- No longer need to keep a list of collection and polymorphic types.
- Added runtime checks in `baseinvocation.py` to ensure no fields are named in such a way that it could mess up the new parsing

chore(ui): remove errant console.log

fix(ui): rename 'nodes.currentConnectionFieldType' -> 'nodes.connectionStartFieldType'

This was confusingly named and kept tripping me up. Renamed to be consistent with the `reactflow` `ConnectionStartParams` type.

fix(ui): fix ts error

feat(nodes): add runtime check for custom field names

"Custom", "CustomCollection" and "CustomPolymorphic" are reserved field names.

chore(ui): add TODO for revising field type names

wip refactor fieldtype structured

wip refactor field types

wip refactor types

wip refactor types

fix node layout

refactor field types

chore: mypy

organisation

organisation

organisation

fix(nodes): fix field orig_required, field_kind and input statuses

feat(nodes): remove broken implementation of default_factory on InputField

Use of this could break connection validation due to the difference in node schemas required fields and invoke() required args.

Removed entirely for now. It wasn't ever actually used by the system, because all graphs always had values provided for fields where default_factory was used.

Also, pydantic is smart enough to not reuse the same object when specifying a default value - it clones the object first. So, the common pattern of `default_factory=list` is extraneous. It can just be `default=[]`.

fix(nodes): fix InputField name validation

workflow validation

validation

chore: ruff

feat(nodes): fix up baseinvocation comments

fix(ui): improve typing & logic of buildFieldInputTemplate

improved error handling in parseFieldType

fix: back compat for deprecated default_factory and UIType

feat(nodes): do not show node packs loaded log if none loaded
psychedelicious added a commit to psychedelicious/InvokeAI that referenced this issue Nov 27, 2023
Node authors may now create their own arbitrary/custom field types. Any pydantic model is supported.

Two notes:
1. Your field type's class name must be unique.

Suggest prefixing fields with something related to the node pack as a kind of namespace.

2. Custom field types function as connection-only fields.

For example, if your custom field has string attributes, you will not get a text input for that attribute when you give a node a field with your custom type.

This is the same behaviour as other complex fields that don't have custom UIs in the workflow editor - like, say, a string collection.

feat(ui): fix tooltips for custom types

We need to hold onto the original type of the field so they don't all just show up as "Unknown".

fix(ui): fix ts error with custom fields

feat(ui): custom field types connection validation

In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.

fix(ui): typo

feat(ui): add CustomCollection and CustomPolymorphic field types

feat(ui): add validation for CustomCollection & CustomPolymorphic types

- Update connection validation for custom types
- Use simple string parsing to determine if a field is a collection or polymorphic type.
- No longer need to keep a list of collection and polymorphic types.
- Added runtime checks in `baseinvocation.py` to ensure no fields are named in such a way that it could mess up the new parsing

chore(ui): remove errant console.log

fix(ui): rename 'nodes.currentConnectionFieldType' -> 'nodes.connectionStartFieldType'

This was confusingly named and kept tripping me up. Renamed to be consistent with the `reactflow` `ConnectionStartParams` type.

fix(ui): fix ts error

feat(nodes): add runtime check for custom field names

"Custom", "CustomCollection" and "CustomPolymorphic" are reserved field names.

chore(ui): add TODO for revising field type names

wip refactor fieldtype structured

wip refactor field types

wip refactor types

wip refactor types

fix node layout

refactor field types

chore: mypy

organisation

organisation

organisation

fix(nodes): fix field orig_required, field_kind and input statuses

feat(nodes): remove broken implementation of default_factory on InputField

Use of this could break connection validation due to the difference in node schemas required fields and invoke() required args.

Removed entirely for now. It wasn't ever actually used by the system, because all graphs always had values provided for fields where default_factory was used.

Also, pydantic is smart enough to not reuse the same object when specifying a default value - it clones the object first. So, the common pattern of `default_factory=list` is extraneous. It can just be `default=[]`.

fix(nodes): fix InputField name validation

workflow validation

validation

chore: ruff

feat(nodes): fix up baseinvocation comments

fix(ui): improve typing & logic of buildFieldInputTemplate

improved error handling in parseFieldType

fix: back compat for deprecated default_factory and UIType

feat(nodes): do not show node packs loaded log if none loaded

chore(ui): typegen
psychedelicious added a commit to psychedelicious/InvokeAI that referenced this issue Nov 28, 2023
Node authors may now create their own arbitrary/custom field types. Any pydantic model is supported.

Two notes:
1. Your field type's class name must be unique.

Suggest prefixing fields with something related to the node pack as a kind of namespace.

2. Custom field types function as connection-only fields.

For example, if your custom field has string attributes, you will not get a text input for that attribute when you give a node a field with your custom type.

This is the same behaviour as other complex fields that don't have custom UIs in the workflow editor - like, say, a string collection.

feat(ui): fix tooltips for custom types

We need to hold onto the original type of the field so they don't all just show up as "Unknown".

fix(ui): fix ts error with custom fields

feat(ui): custom field types connection validation

In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.

fix(ui): typo

feat(ui): add CustomCollection and CustomPolymorphic field types

feat(ui): add validation for CustomCollection & CustomPolymorphic types

- Update connection validation for custom types
- Use simple string parsing to determine if a field is a collection or polymorphic type.
- No longer need to keep a list of collection and polymorphic types.
- Added runtime checks in `baseinvocation.py` to ensure no fields are named in such a way that it could mess up the new parsing

chore(ui): remove errant console.log

fix(ui): rename 'nodes.currentConnectionFieldType' -> 'nodes.connectionStartFieldType'

This was confusingly named and kept tripping me up. Renamed to be consistent with the `reactflow` `ConnectionStartParams` type.

fix(ui): fix ts error

feat(nodes): add runtime check for custom field names

"Custom", "CustomCollection" and "CustomPolymorphic" are reserved field names.

chore(ui): add TODO for revising field type names

wip refactor fieldtype structured

wip refactor field types

wip refactor types

wip refactor types

fix node layout

refactor field types

chore: mypy

organisation

organisation

organisation

fix(nodes): fix field orig_required, field_kind and input statuses

feat(nodes): remove broken implementation of default_factory on InputField

Use of this could break connection validation due to the difference in node schemas required fields and invoke() required args.

Removed entirely for now. It wasn't ever actually used by the system, because all graphs always had values provided for fields where default_factory was used.

Also, pydantic is smart enough to not reuse the same object when specifying a default value - it clones the object first. So, the common pattern of `default_factory=list` is extraneous. It can just be `default=[]`.

fix(nodes): fix InputField name validation

workflow validation

validation

chore: ruff

feat(nodes): fix up baseinvocation comments

fix(ui): improve typing & logic of buildFieldInputTemplate

improved error handling in parseFieldType

fix: back compat for deprecated default_factory and UIType

feat(nodes): do not show node packs loaded log if none loaded

chore(ui): typegen
psychedelicious added a commit to invoke-ai/InvokeAI that referenced this issue Nov 28, 2023
Node authors may now create their own arbitrary/custom field types. Any pydantic model is supported.

Two notes:
1. Your field type's class name must be unique.

Suggest prefixing fields with something related to the node pack as a kind of namespace.

2. Custom field types function as connection-only fields.

For example, if your custom field has string attributes, you will not get a text input for that attribute when you give a node a field with your custom type.

This is the same behaviour as other complex fields that don't have custom UIs in the workflow editor - like, say, a string collection.

feat(ui): fix tooltips for custom types

We need to hold onto the original type of the field so they don't all just show up as "Unknown".

fix(ui): fix ts error with custom fields

feat(ui): custom field types connection validation

In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.

fix(ui): typo

feat(ui): add CustomCollection and CustomPolymorphic field types

feat(ui): add validation for CustomCollection & CustomPolymorphic types

- Update connection validation for custom types
- Use simple string parsing to determine if a field is a collection or polymorphic type.
- No longer need to keep a list of collection and polymorphic types.
- Added runtime checks in `baseinvocation.py` to ensure no fields are named in such a way that it could mess up the new parsing

chore(ui): remove errant console.log

fix(ui): rename 'nodes.currentConnectionFieldType' -> 'nodes.connectionStartFieldType'

This was confusingly named and kept tripping me up. Renamed to be consistent with the `reactflow` `ConnectionStartParams` type.

fix(ui): fix ts error

feat(nodes): add runtime check for custom field names

"Custom", "CustomCollection" and "CustomPolymorphic" are reserved field names.

chore(ui): add TODO for revising field type names

wip refactor fieldtype structured

wip refactor field types

wip refactor types

wip refactor types

fix node layout

refactor field types

chore: mypy

organisation

organisation

organisation

fix(nodes): fix field orig_required, field_kind and input statuses

feat(nodes): remove broken implementation of default_factory on InputField

Use of this could break connection validation due to the difference in node schemas required fields and invoke() required args.

Removed entirely for now. It wasn't ever actually used by the system, because all graphs always had values provided for fields where default_factory was used.

Also, pydantic is smart enough to not reuse the same object when specifying a default value - it clones the object first. So, the common pattern of `default_factory=list` is extraneous. It can just be `default=[]`.

fix(nodes): fix InputField name validation

workflow validation

validation

chore: ruff

feat(nodes): fix up baseinvocation comments

fix(ui): improve typing & logic of buildFieldInputTemplate

improved error handling in parseFieldType

fix: back compat for deprecated default_factory and UIType

feat(nodes): do not show node packs loaded log if none loaded

chore(ui): typegen
skunkworxdark pushed a commit to skunkworxdark/InvokeAI that referenced this issue Nov 29, 2023
In the initial commit, a custom field's original type was added to the *field templates* only as `originalType`. Custom fields' `type` property was `"Custom"`*. This allowed for type safety throughout the UI logic.

*Actually, it was `"Unknown"`, but I changed it to custom for clarity.

Connection validation logic, however, uses the *field instance* of the node/field. Like the templates, *field instances* with custom types have their `type` set to `"Custom"`, but they didn't have an `originalType` property. As a result, all custom fields could be connected to all other custom fields.

To resolve this, we need to add `originalType` to the *field instances*, then switch the validation logic to use this instead of `type`.

This ended up needing a bit of fanagling:

- If we make `originalType` a required property on field instances, existing workflows will break during connection validation, because they won't have this property. We'd need a new layer of logic to migrate the workflows, adding the new `originalType` property.

While this layer is probably needed anyways, typing `originalType` as optional is much simpler. Workflow migration logic can come layer.

(Technically, we could remove all references to field types from the workflow files, and let the templates hold all this information. This feels like a significant change and I'm reluctant to do it now.)

- Because `originalType` is optional, anywhere we care about the type of a field, we need to use it over `type`. So there are a number of `field.originalType ?? field.type` expressions. This is a bit of a gotcha, we'll need to remember this in the future.

- We use `Array.prototype.includes()` often in the workflow editor, e.g. `COLLECTION_TYPES.includes(type)`. In these cases, the const array is of type `FieldType[]`, and `type` is is `FieldType`.

Because we now support custom types, the arg `type` is now widened from `FieldType` to `string`.

This causes a TS error. This behaviour is somewhat controversial (see microsoft/TypeScript#14520). These expressions are now rewritten as `COLLECTION_TYPES.some((t) => t === type)` to satisfy TS. It's logically equivalent.
@qwertie
Copy link

qwertie commented Jan 10, 2024

This seems like the most common thing I want from TypeScript that it can't currently do. Examples:

/** Numeric text box component. `number` must be a valid value of `Num`,
    but TypeScript does not accept <number extends Num extends NFTHNum>` */
function NumericTextField<Num extends NumEx>(...): JSX.Element {...}
type NumEx = number | undefined | null | { toString():string };

/** This should say `null extends CFV` i.e. that null must be a valid value of CFV,
    but TypeScript doesn't support that kind of constraint AFAICT. */
function WithNullCheckbox<CFV = CustomFieldValue>(...) {...}

Until just now I thought the syntax should "obviously" be <constraint extends T>, e.g. number extends T means that let t: T = 7 must be legal. I realize now that this can't work:

type U = ...;
type V = ...;
// It's unclear whether the `U` or `V` is meant as the parameter name, and for 
// backward compatibility, `U` must be the name and `V` must be the constraint.
function Foo<U extends V>() {}

So I propose that when this feature is implemented, the TypeScript compiler should detect what the user is trying to do in most cases and offer a correction, e.g. if the final syntax were Foo<T includes Bound>, TypeScript could offer error messages like:

// Currently the error is "Cannot find name 'Y'" but if `X` was already defined, 
// I propose "Cannot find name 'Y'. Did you mean 'Y includes X'?"
function Foo<X extends Y>() {}

// Currently the error is "Type parameter name cannot be 'number'". I propose
// "Type parameter name cannot be 'number'. Did you mean 'T includes number'?"
function Foo<number extends T>() {}

// These don't parse, so better error messaging would need parser changes.
function Foo<{ toString(): string } extends T>() {}
function Foo<(X|Y) extends T>() {}
function Foo<X[] extends T>() {}

@Yona-Appletree
Copy link

The use case I came across for this is an "action executor" in the context of an application.

interface AppContext { database; service1; service2; serviceN; }
function executeAction<T>(actionFn: (context: AppContext, args) => T) { ... }

Naturally, you can pass in an action that takes a subset of AppContext, which is desirable (so that they can be tested, for example, without a full context), but what if I want to have an interface/type for an action?

interface Action<
  TContext, // super AppContext
  TResult
> {
  (context: TContext): void;
  name: string;
  metadata;
}

How do I constrain TContext correctly? Seems like it can't be done.

However, there is a relatively easy workaround, though kind of annoying, which is to always use a function type when you want a contravariant type parameter:

interface Action<
  TContextFn extends (context: AppContext) => void,
  TResult
> {
  (context: Parameters<TContextFn>[0]): TResult;
  name: string;
  metadata;
}

But that makes it kind of hard to explictly type Action. In my case, that's not a big deal, but it would be really nice to have a proper lower bound syntax.

@jcalz
Copy link
Contributor

jcalz commented Jul 18, 2024

This would also allow safer property writes:

interface Foo { a: true }
const foo: Foo = { a: true };

function f(o: { a: boolean }) {
  o.a = Math.random() < 0.5; // is accepted but unsafe
}
f(foo); // is accepted but unsafe

function g<T super boolean>(o: { a: T }) {  
//           ^^^^^ <--- proposed feature
  o.a = Math.random() < 0.5; // accepted because it's safe
}
g(foo); // rejected

@LPTK
Copy link

LPTK commented Jul 19, 2024

@jcalz the underlying issue here is that TS unsoundly treats mutable records as covariant. The f(foo) call itself really should be forbidden. Using a lower bound in g does not solve anything as g's type can still be instantiated to f's type and called similarly: g<boolean>(foo).

@RobertSandiford
Copy link

RobertSandiford commented Sep 8, 2024

@jcalz the underlying issue here is that TS unsoundly treats mutable records as covariant. The f(foo) call itself really should be forbidden. Using a lower bound in g does not solve anything as g's type can still be instantiated to f's type and called similarly: g<boolean>(foo).

Yes the solution is to have readonly properties with the current assignability, and mutable properties that are invariant. Perhaps a strict setting would make all properties of a received object readonly (preserving current assignability for the sake of external compatibility), highlighting unsafe code. The author could then mark the property mutable to make it invariant, or choose to override the error e.g. via cast to continue using unsafe code. #18770

@Zachiah
Copy link

Zachiah commented Sep 11, 2024

export const isInstanceOf = <I>(c: new () => I) => <T supertype I>(v: T): v is I => {
    return v instanceof c
}

This is my use case. I would like to be able to do things like:

export const isInstanceOfString = isInstanceOf(String);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests