Skip to content

Asserting changes type of original variable inline (the change reverts in subsequent lines) #58055

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

Closed
paradoxloop opened this issue Apr 3, 2024 Β· 10 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@paradoxloop
Copy link

paradoxloop commented Apr 3, 2024 β€’

πŸ”Ž Search Terms

asserting type
as changing underlying type

πŸ•— Version & Regression Information

  • This changed between versions v4.2 and v4.3

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.4.3#code/MYewdgzgLgBAHjAvDA3gWAFAxgcgJ44BcuAFgKYA2FIOANJtjnETAIwAMmAvppgGYBXMMCgBLcDAggAtmQBiQkeLAAeACowycKGTAATCDADWZPCD4woeAA5lz8AHwAKawCcQ1wmoCUqBpIB3UShgEhgXd2tfdCxsGGAAQwgyXAJCfziYAHosmATYKBJRQ2sQUTBYNw8YYskxKhgAIjxGmAAfJrhWvQEUqBBLGzIIYFdRa1gwEFgAIzJygHM8mYo+gbAE13cAjLiczQgKcthXAVWmqYBaITJgYYhNvEurW0uk5NcxcFa3MgA3XRQQwbLYgIJgJYgASuZIUAEQXbYUCQE6sJAwKrWADciOyuUymQAegB+XHI6AwVwAJnRmLyhnwOBxsUy+wJcRJuJmrjICSMzOwPAw3EwQA

πŸ’» Code

const x = {
  'y': 'hello',
  'x': 10
}

function someFunction<T extends keyof typeof x>(prop:T) {
  switch (prop) {
    case 'y':
      // at this point prop is still "y" | "x" due to typescript not being able to narrow
      // eslint rule "no-unecessary-type-assertion" prevents narrowing ourselves
      const r1 = prop;
      //         ^?
      const r2 = prop as 'y';
      //         ^?
      break;
  }

}

πŸ™ Actual behavior

If you inspect the types of prop with and without the assertion you can see that without the assertion TS reports the type of prop to be T extends "y" | "x" but with the assertion TS reports the type of prop as "y".
image

This is a bug with TS itself - a behaviour changed in TS v4.3. In v4.2 TS correctly reports both locations as the generic - in v4.3+ it exhibits the above behaviour.

πŸ™‚ Expected behavior

The assertion should not be changing the type of the original variable.

Additional information about the issue

Discovered while debugging a eslint issue .

@MartinJohns
Copy link
Contributor

Note that the type of prop is not actually changed. If you look at the type in the next line it's still correctly T extends "y" | "x".

So it's at most an issue with the type shown in the tooltip.

@paradoxloop paradoxloop changed the title Asserting changes type of original variable Asserting changes type of original variable inline (the change reverts in subsequent lines) Apr 3, 2024
@paradoxloop
Copy link
Author

thanks @MartinJohns I have clarified the bug report title to make it clear that it is on the asserted line only.

Whether the issue is with the type shown in the tooltip or not is beyond my understanding of the typescript codebase. Downstream tools like eslint which rely on the typescript types report an error as if the type of prop is actually set to the incorrect value rather than just appearing incorrect in the tooltip.

@jcalz
Copy link
Contributor

jcalz commented Apr 3, 2024

Duplicate of #43873

@RyanCavanaugh
Copy link
Member

We need to retain basically both the T and the "y" aspects of the type here, otherwise code like this fails:

function someFunction<T extends keyof typeof x>(prop:T): T {
  switch (prop) {
    case 'y':
      return prop;

Note that

const r1: 'y' = prop;

also doesn't error.

The "correct" type is both T and "y" depending on what you need to use it for. If the contextual target is a union we treat it as "y", otherwise it's T. So neither display is "wrong", but we can only show one at a time to the user obviously.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Apr 3, 2024
@bradzacher
Copy link
Contributor

The issue is that from typescript-eslint's perspective for the rule no-unnecessary-type-assertion we inspect the left and right side of the assertion and compare the types to make a guess if they are the same type. If they're the same type then we error and flag the assertion as unnecessary.

The way we do this is in the most basic form - "are the type objects the same?" Not even doing any fancy type assignability checks - it all just boils down to if (left === right) report().

So in this particular case when we inspect the left we don't see the type T - we see the type "y". Obviously this is the exact same type as the right so we report.

There's no flags set on the left type either so we can't even guess that the left might be the generic type. The contextual type too is the same. Is there another API we can use to get the type of the left without the assertion here (eg see that it is the generic type)?

@bradzacher
Copy link
Contributor

To clarify - it's not just what you show to the user - this is not a problem of intellisense has to display one so we pick the best one via some logic.

Instead it's what the type API itself reports for the expression. When there is no assertion it reports the type T. When there is an assertion it reports the type "y".

@fatcerberus
Copy link

I would argue that, if this triggers no-unnecessary-type-assertion for prop as 'y' then that's correct - the assertion is indeed unnecessary if

const r1: 'y' = prop;

is already legal as Ryan suggests. prop will be treated as a 'y' in contexts where it matters so there's no reason to assert it to that type.

@jcalz
Copy link
Contributor

jcalz commented Apr 3, 2024

I suppose there'd be some issue if it became T & "y"?

function someFunction<T extends keyof typeof x>(prop: T) {
    if (prop === "y") {
        x[prop].toUpperCase(); // error
        const p: "y" = prop;
        x[p].toUpperCase(); // okay
        const q: T & "y" = prop; // error!
        x[q].toUpperCase(); // this would have been okay
    }
}

@paradoxloop
Copy link
Author

paradoxloop commented Apr 3, 2024 β€’

@jcalz my using of a switch in the example was probably a mistake. I don't think this is a duplicate of #43873 because the problem is not typescripts inability to narrow for extend but that it picks and choses in the same code block about whether it is narrowed (in a way that doesn't let us override without triggering reasonable eslint rules).

@RyanCavanaugh needing to keep the return type broad to enable returing T makes sense

IF statement example:
ES Lint Playground

Screenshot 2024-04-03 at 22 44 50

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

7 participants