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

Regression in the way optional properties behave when there's an error in the property value #61382

Open
devanshj opened this issue Mar 9, 2025 · 7 comments Β· May be fixed by #61383
Open

Regression in the way optional properties behave when there's an error in the property value #61382

devanshj opened this issue Mar 9, 2025 · 7 comments Β· May be fixed by #61383

Comments

@devanshj
Copy link

devanshj commented Mar 9, 2025

πŸ”Ž Search Terms

optional properties, self types

πŸ•— Version & Regression Information

This changed between versions 5.3.3 and 5.4.5

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.8.2#code/CYUwxgNghgTiAEYD2A7AzgF3gMyUgjAFwBQ88APACLwgAeGIKwa8AYnvlQHxcAUo2AJYpBGQakLxKASngBeLlOLEMATwAOCdgSoAaeACFY8+AAUYSdXvgAiAEawbPeaXgBveA5gkyv9-ABtAGl4YXgAaxBVJGxDWABdHz9koxhg+Jp6RmZPPAgQKBR4AH44tKCMyRsAURgLb09jNAALJABXCGBPBDs8gpQbeFc-AF9XMeJQSFgEZHQsXCQAJiSqTIYmFm0l7j4BYVFxFEkZeUVKZTVNNjwdyn1Uk3NLa3tHZzlXDy9ipL8PYKhIqRaKxVKJYbJMipdLrbIsXpIfKFEplWFVWr1SReeAtdqdbq5JH9QaQshjcmXDQIZ5WAAq+iCik+ZBCdA2ORBMXgdNRdPR8BQIAAbiAYMpFvheN9YJIPAANKoMTCDEbwEbSYgAei1UKhAD1ihLbtLGg0FUqQCr1erNTq9clDcogA

πŸ’» Code

declare const foo1:
  <D extends Foo1<D>>(definition: D) => D

type Foo1<D, Bar = Prop<D, "bar">> =
  { bar:
      { [K in keyof Bar]:
          Bar[K] extends boolean ? Bar[K] : "Error: bar should be boolean" 
      }
  }

declare const foo2:
  <D extends Foo2<D>>(definition: D) => D

type Foo2<D, Bar = Prop<D, "bar">> =
  { bar?:
      { [K in keyof Bar]:
          Bar[K] extends boolean ? Bar[K] : "Error: bar should be boolean" 
      }
  }

type Prop<T, K> =
  K extends keyof T ? T[K] : never

foo1({ bar: { X: "test" } })
//            ^? "Error: bar should be boolean"

foo2({ bar: { X: "test" } })
//            ^? "test"

πŸ™ Actual behavior

x in foo2 has type "test"

πŸ™‚ Expected behavior

x in foo2 should have type "Error: bar should be boolean" (ie it should behave same as foo1)

Additional information about the issue

I have a project that uses twoslash to test "custom errors" like above (see here for an example) and in the later TypeScript version those tests are failing even though the types are correct and have no change in them. The type error in the quickinfo is same but the inferred type changes across versions. And twoslash returns the inferred type not the type error message. It would be really nice if we can revert back to the previous behavior.

@devanshj devanshj changed the title Regression in the way optional properties behave when there's an error Regression in the way optional properties behave when there's an error in the property value Mar 9, 2025
@devanshj
Copy link
Author

devanshj commented Mar 9, 2025

Here's a fourslash test...

/// <reference path='fourslash.ts' />

// @strict: true
////declare const foo:
////  <D extends Foo<D>>(definition: D) => D
////
////type Foo<D, Bar = Prop<D, "bar">> =
////  { bar?:
////      { [K in keyof Bar]:
////          Bar[K] extends boolean ? Bar[K] : "Error: bar should be boolean" 
////      }
////  }
////
////type Prop<T, K> =
////  K extends keyof T ? T[K] : never
////
////foo({ bar: { /**/X: "test" } })

verify.quickInfoAt("", '(property) X: "Error: bar should be boolean"');

Bisected it to 02f9ddf (#56318) (this is my first time using git bisect so I could be wrong)

@Andarist you broke my code :P

@Andarist
Copy link
Contributor

Andarist commented Mar 9, 2025

I was just reading your issue before you pinged me and I thought that it totally looks like something I could change behavior of πŸ˜‰

@devanshj
Copy link
Author

devanshj commented Mar 9, 2025

Haha please do it!

@Andarist
Copy link
Contributor

Andarist commented Mar 9, 2025

I meant to write "something I could have changed behavior of" - but nevertheless, I'll see why this has changed and if something can be done about it

@Andarist
Copy link
Contributor

Andarist commented Mar 9, 2025

Simpler repro (TS playground):

function test1(arg: { prop: "foo" }) {}
test1({ prop: "bar" });
//      ^? (property) prop: "foo"

function test2(arg: { prop: "foo" } | undefined) {}
test2({ prop: "bar" });
//      ^? (property) prop: "bar"

I wouldn't really say this is a bug though. The behavior has changed, sure - but this related to an implementation detail that shouldn't be depended on. When a code has an error there is not always a good answer as to what should be displayed in situations like this. I'll still investigate this further though to understand the nature of this change better, maybe I'll put up some PR changing it, maybe not πŸ˜‰

@devanshj
Copy link
Author

devanshj commented Mar 9, 2025

It is a bug if you see what the type of arg gets inferred to...

function test1<T extends { prop: "foo" }>(arg: T): T { return arg; }
let t1 = test1({ prop: "bar" });
//               ^? "foo"
  t1;
//^? { prop: "foo" }

function test2<T extends { prop: "foo" } | undefined>(arg: T): T { return arg }
let t2 = test2({ prop: "bar" });
//               ^? "bar"
  t2;
// ^? { prop: "foo" } | undefined

Here the type of prop in test2 (and in test1) is inferred as "foo" but the quickinfo doesn't reflect that.

Also I'm not really dependent on it, the type error is same in both of those so it's not really a problem... just that when combined with @ts-expect-error the type error vanishes and one is only left with the type which twoslash returns... so it becomes harder to test it.

@Andarist
Copy link
Contributor

Andarist commented Mar 9, 2025

There are two types here though - otherwise you wouldn't get the error in the first place. In reality, the prop has the "bar" type here. Even in the case that seems to work as you expect it to. It's just that there is some extra logic there, purely in the services layer, that makes it to prefer - at times - the property symbol of the contextual type. If you'd call checker.getTypeAtLocation on both of those you'd uniformly get "bar" and not "foo". In some sense, the quick info just tells us a sweet little lie here πŸ˜‰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants