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

Typeguard for union not working after adding a property to the superclass of one of the union members #35953

Closed
G-Rath opened this issue Jan 2, 2020 · 8 comments · Fixed by #49625
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@G-Rath
Copy link

G-Rath commented Jan 2, 2020

This issue appeared in eslint-plugin-jest after updating @typescript-eslint to 2.13.0 (from 2.12.0).

TypeScript Version: 3.7.x-dev.201xxxxx

Search Terms:
typeguard, union
Code

export declare enum AST_NODE_TYPES {
  Literal = 'Literal',
  TemplateLiteral = 'TemplateLiteral',
}

export type TSNode = StringLiteral | TemplateLiteral;

export interface BaseNode {
  stuff: null;
}
export interface LiteralBase extends BaseNode {
  raw: string;
  value: boolean | number | RegExp | string | null;
  regex?: {
    pattern: string;
    flags: string;
  };
}

export interface StringLiteral extends LiteralBase {
  type: AST_NODE_TYPES.Literal;
  value: string;
}
export interface TemplateLiteral extends BaseNode {
  type: AST_NODE_TYPES.TemplateLiteral;
  quasis: string[];
}

interface MyStringLiteral<Value extends string = string> extends StringLiteral {
  value: Value;
}

interface MyTemplateLiteral<Value extends string = string>
  extends TemplateLiteral {
  quasis: [Value];
}

type MyStringNode<S extends string = string> =
  | MyStringLiteral<S>
  | MyTemplateLiteral<S>;

declare function isMyStringNode<V extends string>(
  node: TSNode,
  specifics?: V,
): node is MyStringNode<V>;

function foo(argument: TSNode) {
  if (isMyStringNode(argument)) {
    /*
        Expected: argument.type is Literal|TemplateLiteral
        Actual  : argument.type is Literal

        This condition will always return 'false' since the types
        'AST_NODE_TYPES.Literal' and 
        'AST_NODE_TYPES.TemplateLiteral' have no overlap.
    */
    if (argument.type === AST_NODE_TYPES.TemplateLiteral) {
    }
  }
}

This is a real life reproduction - it can be made smaller by removing the generics, BaseNode, etc, but then it becomes counterproductive as you can argue the solution to be "just use StringLiteral directly".

Expected behavior:
No errors

Actual behavior:

if (argument.type === AST_NODE_TYPES.TemplateLiteral) { is marked as an error, as TS believes argument.type can only be AST_NODE_TYPES.Literal (aka it appears to discard the alternative in the union).

This behaviour is tied to the existence of the value: string property on StringLiteral: if you remove that property, everything works as expected (which is what changed between 2.12.0 & 2.13.0: previously Literal was just an interface w/ type - now it's a union of <Type>Literal interfaces; one for each of the values of LiteralBase#value)

Playground Link: https://www.typescriptlang.org/play/?ts=3.8.0-dev.20191231&ssl=1&ssc=1&pln=60&pc=2#code/KYDwDg9gTgLgBAE2AYwDYEMrDsAdgVwFs4BBAZQBUB9AOQHkARAUSooE0AFJsuAbwFgAUHDgAZAJYxgUdKjgBeOAHIJUmaiUAaISIrBCYDFNXTZC5XoNHgJ9VqEBfIUNCRYcGAE8w2CmRoQSOZkMFDiuADmtmYAPnCWhujGkqaoANzOgq7Q8OFqAGboyNgAQugAzsABQQLCcOUw+Pn5AFxwBKjpji7gOXB50oXFYinqZZU4IFK4COVw41WB2LUiMgDubQ1hkRl1AG6y+MBtAEYQEKjA6LhwcQSEJ9K3cABKwBFM4M9b4RHPHV06lgIqAAPxtFYiOBgJJqXCbUK-XZQuD5DARcoI7YRZFwBy7JyCHpuXK4ApFbAhbHROSgaazEZqWQLPg6DzeY6kSi0RgsdhcMgAOhpuIOqCOWKR3SyvXcAygQ18+kSySZtKmeAZC2qyzZXh8bXI1HozFYnG4goS1hFbIAjvgKuJMfVEZEANoAXQJmXlirgAFlPFTfjSADwANUO2DpmrmP0i5njEQAfJN6XNg5Eaaz9lG2pHxcBvUTBL6KQHPFakjZRrII1G07GXdjE66U2yYzM5lXVakcyJ7Y7nW6C0cvdKhPrsIHMxEdaGeJ2GUnW9jU-I2XEZ22w2Rk5uKz2a2qF8ndkIkGhMNh8vhcMgYOIIDcndvsfPw42u83fsmABRsrgSxtH4OraHU5Q+Mg4j5OIyDlOCcDhuBACUbRAUEToVrOH5npkt73o+z6oucf6YBERB4DAIH+EsKH9v0+RwH+r5Bm2OpkVAFGEFRKH0ZCIgAPQAFRsiiIifFBUgIG05GUWSgpTv0cw0jER40mJ4kkA+DpyHAslcfJMCKRyymMqkmTiVCFAABZYcgz4IJIT43Gs4idHAshrOgnhzFgjRQDcSiFKglRKPU4TDDANnYFO5SaSiShGjypr8haNLhdcCBwAlUJJdyJp8uaQrqbWGhwDZ6B7NgQFwBA1VQBgYCCppwmCZpMHMXJPEKUp8j9Vyxq8maAqWsq1plfxmmEiIhIOEAA

Related Issues:

This is probably a duplicate of #31156, but I've created a new issue as I believe this is a more subtle (ideally fixable) issue

@G-Rath
Copy link
Author

G-Rath commented Jan 2, 2020

@bradzacher: you might be interested in seeing this. I don't think there's much @typescript-eslint can do, as the issue seems to be the introduction of value: string on StringLiteral - so it's not even possible for ts-estree to implement the generic itself.

@AnyhowStep: thanks for the help distilling this into the playground reproduction, and helping double-check my sanity :) I think the reason that value causes the error is that it makes it a tagged type, thus leading into the territory that your original issue was in and so unlikely to be fixable :(

@RyanCavanaugh
Copy link
Member

Did you mean quasis: string[] instead of [string] by chance? If not, buckle up, because you're in for a ride.

interface Alpha {
  kind: "A"
}
interface Beta {
  kind: "B"
}
interface MyAlpha extends Alpha {
  // Add nothing
}
interface MyBeta extends Beta {
  // New optional member
  y?: any;
}

declare function isMine(arg: any): arg is MyAlpha | MyBeta;
function fn(arg: Alpha | Beta) {
  if (isMine(arg)) {
    // TS says this is "A"
    arg.kind;
  }
}

What's going on here?

TypeScript says that if you have a UDTG and apply it to some union, then the primary intent of that is to probably to do some filtering operation like this:

interface Alive { a: any }
interface Plant extends Alive { p: any }
interface Animal extends Alive { m: any }
interface Cat extends Animal { c: any }
interface House { h: any }
declare function isAlive(x: any): x is Alive;
function something(obj: Animal | Plant | House) {
  if (isAlive(obj)) {
    obj; // obj: Animal | Plant
  }
}

If the checked-for type is a subtype of the output of that process, we also narrow to exactly that:

declare function isCat(x: any): x is Cat;
function something(obj: Animal | Plant | House) {
  if (isCat(obj)) {
    obj; // obj: Cat
  }
}

Note that I said "subtype", not "assignable" to. In the OP example, MyTemplateLiteral is assignable to TemplateLiteral, but is not a subtype of it, due to the tuple member (was this intentional?). However, MyStringLiteral is a subtype of (and assignable to) its based type StringLiteral, because its value type is a subtype of its corresponding base type member's.

So in your case, TypeScript saw a list of types in a union, filtered them to those that were subtypes, saw that at least one fit, and then didn't bother checking for assignability of the remaining constituents of the union:

interface Plant { kind: "plant", names: string[] }
interface Animal { kind: "animal", age: number }
// Subtyping relation to Animal
interface Cat extends Animal { age: number }
// Assignability relation to Plant
interface Tree extends Plant { names: [string] }

declare function isCatOrTree(x: any): x is Cat | Tree;
function something(obj: Animal | Plant) {
  if (isCatOrTree(obj)) {
    // obj: Animal
    obj;
  }
}

This is an important bit of logic, because many things are an assignability target but not a subtype target. For example, if you just hack up the compiler to always use the assignability relation, you can easily introduce new errors:

interface OptionsBag {
  a?: string;
  b?: string;
  c?: string;
  name: string;
}
interface Thing {
  name: string;
  other?: string;
}
declare function isThing(x: any): x is Thing;
function something(arg: Thing | OptionsBag) {
  if (isThing(arg)) {
    // doesn't narrow 'arg', error here
    // because 'other' doesn't exist on OptionsBag
    arg.other;
  }
}

This has been extremely delicately tweaked over the years to try to produce the most intuitive behavior in other cases, so I'm not sure what the right answer is at this point. Any change is going to be a breaking change for other code. For your specific case, I think the only great answer would either be a type assertion, or to fine-tune your interfaces such that they uniformly do or don't have different assignability/subtyping relationships to their parent types.

@RyanCavanaugh
Copy link
Member

@ahejlsberg any additional thoughts? I don't see much room for improvement, unfortunately

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jan 15, 2020
@G-Rath
Copy link
Author

G-Rath commented Jan 15, 2020

@RyanCavanaugh thanks for such a fantastic write up - I'd be lying if I claimed to understand it completely, but I think I got the bulk of it well enough.

Sadly, the tuple member was intentional, but not strictly required as is: I'm trying to represent that we know for sure the value of the first element in that tuple; if there was a way to represent "array of strings, of which the first value is T" that'd be fine, but I suspect even if that could be done it might not be a subtype?

(btw in case it matters, the actual type of the elements in the tuple is TSESTree.TemplateElement & { value: { raw: Value; cooked: Value } }.)

I'm not sure I understand why the removing of value: string from StringLiteral resolves the problem however; isn't Value a valid subtype of boolean | number | RegExp | string | null? (the type of value inherited from LiteralBase?

Regardless, thanks again for the detailed response - I'll have to have a think about the best way to tackle in our codebase :)


Just as a toss away thought:

I guess a solution could be useful to have some form of as-typeguard a la function x(v: any): v as y that would make casts slightly safer by typing them to a single point & some logic, but I doubt this is a big enough problem to warrant adding a new feature.

@bradzacher
Copy link
Contributor

It's moments like this I realise I know a lot less about typescript than I think I do...


if there was a way to represent "array of strings, of which the first value is T"

type TMyTuple<TFirst extends string> = [TFirst, ...string[]]

// errors
const bad1: TMyTuple<'something'> = []; // Property '0' is missing in type '[]' but required in type 'TMyTuple<"something">'.
const bad2: TMyTuple<'something'> = ['something', 1]; // Property '1' is incompatible with rest element type. Type 'number' is not assignable to type 'string'.

// working
const good1: TMyTuple<'something'> = ['something'];
const good2: TMyTuple<'something'> = ['something', 'a', 'b'];

const first = good1[0]; // typeof === 'something'
const second = good1[1]; // typeof === string

repl

@G-Rath
Copy link
Author

G-Rath commented Jan 15, 2020

type TMyTuple = [TFirst, ...string[]]

I feel very silly, b/c I tried this quickly but missed out the [] and so for some reason jumped to "it only works with generics" instead of diving deeper 😂

Thanks for not letting me get away with that :)

Sadly, as I suspected [V, ...string[][] is not a subtype of string[], and so doesn't solve the issue :(

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Jan 15, 2020

@RyanCavanaugh If I understand what I'm reading,

  • Subtype implies assignable, but assignable does not imply subtype?

  • Does this mean conditional types only check "assignability" and not "subtype" relations?

    A extends B ? "y" : "n" checks if A is "assignable" to B but not if it is a "subtype".

  • Does this mean there is no way to check "subtype" relations from within the type system? (Like a "subtype" version of conditional types)

  • Type guards narrow based on subtype relations, but not assignability?

  • Type guards narrow first on subtype relations, assignability second?

    Since we can narrow Plant to Tree, but not Animal|Plant to Cat|Tree.
    Hence, your advice,

    fine-tune your interfaces such that they uniformly do or don't have different assignability/subtyping relationships to their parent types.

@AnyhowStep
Copy link
Contributor

Just answering some of my questions,

https://www.typescriptlang.org/docs/handbook/advanced-types.html#conditional-types

T extends U ? X : Y

The type above means when T is assignable to U the type is X, otherwise the type is Y.

https://www.typescriptlang.org/docs/handbook/type-compatibility.html#subtype-vs-assignment

So far, we’ve used “compatible”, which is not a term defined in the language spec. In TypeScript, there are two kinds of compatibility: subtype and assignment. These differ only in that assignment extends subtype compatibility with rules to allow assignment to and from any, and to and from enum with corresponding numeric values.

Different places in the language use one of the two compatibility mechanisms, depending on the situation. For practical purposes, type compatibility is dictated by assignment compatibility, even in the cases of the implements and extends clauses.

For more information, see the TypeScript spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
4 participants