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

Move union/enum optimization to a compiler option #18043

Closed
Sicilica opened this issue Aug 25, 2017 · 3 comments
Closed

Move union/enum optimization to a compiler option #18043

Sicilica opened this issue Aug 25, 2017 · 3 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@Sicilica
Copy link

TypeScript Version: 2.3.4

When using union or enum types, the compiler changes the types of your local variables based on what it understands as the set of values that the variable could actually have taken on. While this proactive behavior could be helpful, it is impossible to opt-out of at present and can lead to complications and problems (which is hardly surprising, given that it literally changes the types of your variables to something other than what you declared them as).

Code

type MyEnum = "foo" | "bar" | "baz"

let myVal: MyEnum = "foo"

if (randomChance())
  myVal = "bar"

if (myVal == "baz")
  doStuff()

Expected behavior:

Compiles.

Actual behavior:

Line 8 throws an error, because the type of myVal is "foo" | "bar", which isn't compatible with type "baz".

In this case, it is true that myVal couldn't possibly have taken on the value "baz" before that point, but in more complex examples, this behavior of the compiler yields a lot of false positives. It should absolutely be possible to disable this through a compiler flag (and perhaps this stricter check should be disabled by default).

It's worth noting that this can also be avoided by changing line 3 to let myVal = "foo" as MyEnum, since then the compiler can't know what value myVal was assigned in order to do this strict check.

@RyanCavanaugh
Copy link
Member

Per usual I'll link #9998

I don't see why a compiler flag is the right fix to this. This situation typically comes up in a codebase maybe once every 50,000 lines at most and, as you note, the fix is very small and local. Turning this off with a flag would leave you open to a lot of different bugs that flow control analysis catches.

We get "bug" reports very frequently where people have nonworking code and didn't understand the compiler was finding a logic error. Duplicate switch labels, using the wrong || / &&, not realizing that a if (!x) return; statement excluded the 0 value in the enum, having wrong closing braces, etc - for the sake of what? I'd look askance at any codebase that turned on such a flag.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Aug 25, 2017
@Sicilica
Copy link
Author

Sicilica commented Aug 25, 2017

If this functionality is tightly-coupled with the rest of the flow control analysis routines, then disabling it would certainly not be worth it - in particular it makes sense that this would be tied to the rest of the type guard code.

If that's the case, though, then I feel that improving error messages in this case would be valuable enough to warrant the effort required. Saying "the types of these two expressions are incompatible" immediately after the desired type of one of those expressions was explicitly declared is anything but helpful; saying "that expression can't possible take a compatible value in this location" is clear. Of course this would only be possible in a subset of all possible occurrences (perhaps only when one side of the expression is a direct reference to a variable in the current scope), but I do believe it has merit in those cases.

If it would be impossible to change this functionality without breaking type guards and other flow control elements, then this issue should be closed - but certainly the error reporting across the compiler as a whole tends to be pretty unclear, and I'm quite confident that this situation befuddles a large number of people starting out in typescript at some point (50000 lines really isn't very much, when you're considering software that was complex enough to require type-checking in the first place). Certainly I had never personally seen #9998, although I hardly suspected my concerns were new.

EDIT:
On further thought, it's interesting that both the example from the tsc codebase in #9998 and the example I first encountered were performing parsing / lexing. Is there no way that enums (or string unions, which are basically discount enums) can be handled differently? Getting a token from some other context and thus setting the local seems to be a common occurrence in that realm. Are none of the proposed syntax for explicitly stating that a function call modifies some variable being considered any longer?

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Aug 2, 2019
@RyanCavanaugh
Copy link
Member

Given the current state of how much stuff depends on the automatic narrowing implied by the OP, there's not anything practical to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants