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

"Did you mean" spelling suggestions can offer the same thing being complained about #25564

Closed
RyanCavanaugh opened this issue Jul 10, 2018 · 9 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jul 10, 2018

TypeScript Version: 2.92

Search Terms: did you mean spelling suggestion same

Code

type NonVoid = Object | object;

type Something<T> = { test: string } & (T extends NonVoid ? {
    requiredWhenNonVoid: T
} : {
    requiredWhenNonVoid?: undefined
    });

function testFunc2<A extends NonVoid>(a: A): Something<A> {
    return {
        test: 'test',
        requiredWhenNonVoid: a
//      ~~~~~~~~~~~~~~~~~~~
    };
}

Expected behavior: Probably no error, or at least not this error suggestion

Actual behavior:

clipboard.ts:12         requiredWhenNonVoid: a
                        ~~~~~~~~~~~~~~~~~~~~~~
  TS2322: Type '{ test: string; requiredWhenNonVoid: A; }' is not assignable to type 'Something<A>'.
    Object literal may only specify known properties, but 'requiredWhenNonVoid' does not exist in type 'Something<A>'. Did you mean to write 'requiredWhenNonVoid'?

There's no spelling error here -- the requiredWhenNonVoid being cited is the same requiredWhenNonVoid being suggested 🤡

Playground Link: Link

Related Issues: Nope

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jul 10, 2018
@mhegazy mhegazy added this to the TypeScript 3.0.1 milestone Jul 10, 2018
@sandersn
Copy link
Member

The immediate problem is that spelling suggestions get suggestion targets from getPropertiesOfUnionOrIntersectionType, which results in ['test', 'requiredWhenNonVoid']. But excess property checks uses getPropertiesOfObjectType, which gives only ['test'], probably because the conditional type is not an object type.

I'm not sure whether spelling suggestions should be changed, except perhaps to add an assert that the result is not equal to the missing name. (The assertion that the missing name isn't in the candidate set is too expensive.)

@jcalz
Copy link
Contributor

jcalz commented Jul 11, 2018

an assert that the result is not equal to the missing name

So then a real misspelling suggests the correct spelling, which is then an error? Sounds fun:

return {
  test: 'test',
  requiredWhenNotVoid: a
//~~~~~~~~~~~~~~~~~~~ Error:
//Object literal may only specify known properties,
//but 'requiredWhenNotVoid' does not exist in 
//type 'Something<A>'. Did you mean to write 'requiredWhenNonVoid'?
}

Okay...

return {
  test: 'test',
  requiredWhenNonVoid: a  // fixed
//~~~~~~~~~~~~~~~~~~~ Error:
//Object literal may only specify known properties,
//but 'requiredWhenNonVoid' does not exist in 
//type 'Something<A>'. Ha ha, fooled you! 😝
}

I hate you, compiler! 🏃‍♂️ 😭

@sandersn
Copy link
Member

I figured out how to fix the excess property error at least: make excess property checks understand conditional types:

/** (in isKnownProperty) */
            else if (targetType.flags & TypeFlags.Conditional) {
                return isKnownProperty((targetType as ConditionalType).root.trueType, name, isComparingJsxAttributes) ||
                    isKnownProperty((targetType as ConditionalType).root.falseType, name, isComparingJsxAttributes);
            }

This exposes the more inscrutable bug that @RyanCavanaugh and I encountered, which is that { requiredWhenNonVoid: a } is not assignable to Something<A>. This is probably a lack of assignability rules for conditional types, but I haven't worked out whether it's inherent to the definition of conditionals or just an oversight — new types typically start with few assignability rules.

@sandersn sandersn added the Fixed A PR has been merged for this issue label Jul 11, 2018
@tjenkinson
Copy link
Contributor

Thanks for fixing the message so quickly! Any idea why the example still doesn't compile though? It should be valid right?

This came from my initial question here: https://stackoverflow.com/q/51274107/1048589

@sandersn
Copy link
Member

Basically, there's no assignability rule that directly relates object types to conditional types, and because we're inside testFunc2, we can't (I think) collapse the conditional type down to either its true branch or false branch. So we're looking at the following assignability check, as expressed in my own made-up notation:

{ arg: A } ==> A extends object ? { arg: A } : { arg: undefined } 
    where A extends object

The assignment the other way works because of a simplification rule that applies:

> A extends object ? { arg: A } : { arg: undefined } ==> { arg: A }
    where A extends object
> object extends object ? { arg: A} : {arg: undefined } ==> { arg: A }
> { arg: A } ==> { arg: A }
#t

So I know why the example doesn't compile. I don't know if the reason is valid: Should the simplification rule also apply when the target is a conditional type, or just when the source is a conditional type? I'm not sure. @ahejlsberg have you thought about this assignability case?

@tjenkinson
Copy link
Contributor

@ahejlsberg any ideas? I'm wondering if I should create a separate issue for tracking this? This causes an issue for us currently and we have to revert to some '' trickery and it would be great to not have to do this.

@ahejlsberg
Copy link
Member

ahejlsberg commented Jul 16, 2018

First some comments on the original example.

The type NonVoid is equivalent to just Object, since object is a subtype of Object. So, Object | object is just a complicated way of writing Object (or simply {}).

The conditional type used in Something<T> is distributive (for more details see #21316), which is probably not what you want. For example, Something<string | number> resolves to

({ test: string } & { requiredWhenNonVoid: string }) |
({ test: string } & { requiredWhenNonVoid: number })

I suspect something closer to the intent is

type Something<T> = { test: string } &
    (undefined extends T ? { requiredWhenNonVoid?: T } : { requiredWhenNonVoid: T });

which basically says that if undefined is assignable to T then the property is optional, otherwise it is required (and of course undefined is assignable to void). For Something<string | number> this resolves to

{ test: string } & { requiredWhenNonVoid: string | number }

Regarding the assignment compatibility error, as @sandersn points out we don't currently have a rule that makes a non-conditional type assignable to a conditional type. But we probably should. Specifically, we should say that some type S is assignable to a conditional type T if S is assignable to both the true branch and the false branch of T. For example:

type Foo<T> = T extends true ? string : "a";

function test<T>(x: Foo<T>, s: string) {
    x = "a";  // Currently an error, should be ok
    x = s;    // Error
}

With such a rule the original example would compile if the definition of Something<T> is changed to what I suggest above. (The original definition won't because it uses undefined instead of T as the type of the optional property.)

@tjenkinson
Copy link
Contributor

@ahejlsberg thanks I understand a bit better what is happening now.

I think there is actually already a ticket for this: #24929

@ahejlsberg
Copy link
Member

Actually, #24929 is a different issue in that it would require the type checker to infer conditional types from conditional statements and expressions. What we're talking about here is a simple rule that says a type S is assignable to a conditional type T when S is assignable to both branches of T regardless of the condition in T.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

6 participants