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

Invalid type checking on union types returned getters #56149

Closed
LassazVegaz opened this issue Oct 18, 2023 · 10 comments
Closed

Invalid type checking on union types returned getters #56149

LassazVegaz opened this issue Oct 18, 2023 · 10 comments
Labels
Duplicate An existing issue was already created

Comments

@LassazVegaz
Copy link

🔎 Search Terms

typescript union types getters

🕗 Version & Regression Information

  • This is a crash
  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about "union type getters"
  • I was unable to test this on prior versions because 5.2.2

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.3.0-dev.20231018#code/C4TwDgpgBAyg9gWwgFXNAvFA3lMAnOMALigCJ9CBGUqAXygB9tcDiyKwAmG2gbgCh+AYwA2AQwDOE2IggBhcVOz8oUMVEwAGASqgBzCMCgTZqSAAoAlCXhIz0LLtV5DAVzwA7KMAAWASwkAOnUAUihODXQtKAB+Zg4SclZqOigSHAT2Vm46AVVafgLhOA8JIxMkBUlpTA8IAHcZSsUJKwE-ADMocwr5FsDe+0COSMwkqlJLZVUhEpMRCECROD1zcbBqSx1VXqqpYI0oAFkxX0C8MQ8AE0QrKAAqKEptQVVO7t3+wbRh1lGswjcKaOVQzOZwBZLFZrDhAvJ0Qr8IA

💻 Code

type SomeType = { prop: "prop1" } | { prop: "prop2" };

class SomeClass {
  a = 0;

  get someType(): SomeType {
    return this.a % 2 === 0 ? { prop: "prop1" } : { prop: "prop2" };
  }
}

const someClass = new SomeClass();
if (someClass.someType.prop === "prop1") {
  console.log("prop1");

  someClass.a = Math.random() * 10;

  // error comes in the following line
  //This comparison appears to be unintentional because the types '"prop1"' and '"prop2"' have no overlap
  if (someClass.someType.prop === "prop2") {
    console.log("prop2");
  }
}

🙁 Actual behavior

When using union types as returned the type of a getter, TypeScript shows errors that cannot be certain about. I am not very good at putting the error into words, please check the sample code given above. It is very easy to understand. Sorry for not explaining properly.

🙂 Expected behavior

TypeScript should not check union-based returned types from getters conditionally.
Still bad at explaining this ☹️ But I hope you can easily get an idea about the expected behavior by looking at the code

Additional information about the issue

No response

@MartinJohns
Copy link
Contributor

Duplicate of #9998 / #46771.

TypeScript should not check union-based returned types from getters conditionally.

TypeScript does not distinguish between getters and properties. They're both properties in the type system, so there's nothing to cause different behaviour.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Oct 18, 2023
@LassazVegaz
Copy link
Author

TypeScript does not distinguish between getters and properties.

Shouldn’t they be treated differently? Getters have functional bodies. Return value of a getter cannot be predicted by looking at previous call on that getter. Isn’t that right?

@MartinJohns
Copy link
Contributor

🤷‍♂️ Changing it to not narrow will break the code of other people. While safer, it will undoubtly create more friction and troubles in the end. Getters are very likely to always return the same type (without any assignment in between) on subsequent calls, but having a getter randomly return different types is uncommon (and honestly quite smelly).

@LassazVegaz
Copy link
Author

These days I am creating a programming language. That is where I caught this. I have added a minimal code from it to explain the use case. In the following code, I am using a getter that returns different values occasionally. It is very helpful.

export type Token =
  | {
      type: "keyword";
      value: "let" | "const";
    }
  | {
      type: "string" | "declaration";
      value: string;
    }
  | {
      type: "operator";
      value: "=" | "+";
    };

export default class Passer {
  private pos = 0;

  constructor(private readonly tokens: Token[]) {}

  private get token() {
    return this.tokens[this.pos];
  }

  private move() {
    this.pos++;
  }

  /**
   * Pass the tokens.
   */
  public pass() {
    while (this.pos < this.tokens.length) {
      if (this.token.type === "keyword") {
        if (this.token.value === "let") {
          this.move();
          if (this.token.type === "declaration") { // error shows up here
            // getting variable name
            this.move();
            if (this.token.type === "operator") {  // and here here
              this.move();
              // saving variable
              // goes like this...
            }
          }
        }
      }

      // again lots of conditional statements using this.token....

      this.move();
    }
  }
}

If I did not have the getter, I would have to write this.tokens[this.pos].type or this.tokens[this.pos].value in lots of places. It can be tedious. right? 🙂

My idea is that, no matter what the getter will return a value of type Token. I can see that TS is saying the type of token in the third error is never. Which can never be true. It is always going to be a Token. Please correct me if I am wrong. I am trying to contribute. No offense 🙂

@RyanCavanaugh
Copy link
Member

We fully understand that there is a trade-off involved here; TypeScript's own parser uses a function call token() to retrieve the local so it isn't narrowed.

@LassazVegaz
Copy link
Author

TypeScript's own parser uses a function call token() to retrieve the local so it isn't narrowed.

I didn’t quite catch that. What “local” does the token function return.

If there is a trade off how about adding another option in tsconfig to disable conditional type checking on getters? So developers can turn it on off on-demand. I would love to cooperate on that feature.

@RyanCavanaugh
Copy link
Member

Instead of

let token: Token;

// later
if (token === whatever)

we do

let _token: Token;
function token() { return _token }

// later
if (token() === whatever)

which doesn't meaningfully impact performance (we've measured to a very high degree).

how about adding another option

We're not interested in doubling the configuration space of the checker at this time.

@fatcerberus
Copy link

Also

option ... to disable conditional type checking on getters

This is not really feasible anyway, because TS doesn't generally distinguish between accessors and regular properties in type-land. { get foo() { return 42; } } is inferred as { readonly foo: number }, the information that foo is a getter is completely lost

@LassazVegaz
Copy link
Author

LassazVegaz commented Oct 20, 2023

I knew getters are treated the same way as read-only variables. I never questioned it because I hadn't come to this kind of scenario before.
Now I understand that TS treats them the same way because it is the typical use case.

Getters are very likely to always return the same type (without any assignment in between) on subsequent calls, but having a getter randomly return different types is uncommon (and honestly quite smelly).

But even TSC itself can be benefited if they are treated differently. Adding an option in tsconfig would be nice. Then it won't break the existing code and people like me and you who develop compilers/interpreters can code happily ever after 😇 But as you said,

We're not interested in doubling the configuration space of the checker at this time.

I hope this feature will be available in the future... And I love to contribute when you decide to implement it because I already contributed to it in the issues section 😁

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" 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 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants