Skip to content

Enumerating an enum: Wrong thing is allowed and right thing is not with with noImplicityAny #39627

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

Open
Manish3177 opened this issue Jul 16, 2020 · 3 comments
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@Manish3177
Copy link

Consider this code:

enum Test1 {
One = 1,
Two = 2
}
enum Test2 {
One = "one",
Two = 2
}
enum Test3 {
One = "one",
Two = "two"
}
for (const key in Test1) {
const aValue = Test1[key];
console.log(aValue);
}

This code compiles but because in an enum a reverse value->key map is also set up for numeric values, the outcome is not what one would expect: 1, 2. It's: One, Two, 1, 2 instead. Similarly, replacing Test1 with Test2 in the for..in loop compiles but gives unexpected result: "Two", "one", 2. However, with Test3, for which the result would've been correct: "one", "two", there's a compile error with the noImplicitAny compiler option:

Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'typeof Test3'.
No index signature with a parameter of type 'string' was found on type 'typeof Test3'.

This seems backwards of what the behavior should be: With Test1 and Test2, the compile should fail and it should succeed with Test3. If not that, the compile should either fail or succeed with all 3. FYI, the reason I noticed this was because converting an all numeric enum to all string enum caused compile failure for some other code that was using "for..in" which originally had a logical flaw which the compiler didn't catch but it's flagging it now when the logical flaw has (inadvertently) been fixed.

It seems like there are at least a couple of independent problems here.

  1. With enums that have at least one numeric member (Test1 and Test2), the inferred type of loop variable (key) should ideally be the union of all keys and numeric values or at least string | number, not just string.

  2. Indexing enums by a string (that cannot be narrowed down specific literals representing keys of an enum) should consistently return any. It may acceptable to return union type of all possible values: number for Test1, string|number for Test2 and string for Test3 and not string for Test1 and Test2 and any for Test3.

  3. In addition to the issues above, a for..in over an enum should somehow be flagged and fail to compile regardless of noImplicitAny flag because it only yields the expected result in the enum has all string values but even with all string-enums, that would be fragile as adding a single numeric value to the enum will cause compile failures at that point. It should just not be allowed at least by default.

Issue #18409 seems related but not quite the same.

@DanielRosenwasser
Copy link
Member

Indexing enums by a string (that cannot be narrowed down specific literals representing keys of an enum) should consistently return any.

In addition to the issues above, a for..in over an enum should somehow be flagged and fail to compile regardless of noImplicitAny flag because it only yields the expected result in the enum has all string values but even with all string-enums,

I don't know about any, but I am legitimately surprised that we give back the type string for aValue. Maybe @RyanCavanaugh knows more on this.

the inferred type of loop variable (key) should ideally be the union of all keys and numeric values or at least string | number, not just string.

This is just not true - try printing out typeof each time.

enum Test2 {
  One = "one",
  Two = 2
}

for (const key in Test2) {
  console.log(typeof key);
}

Further, keyof always describes the lower-bound of values, so it's not fit to describe something like Object.keys which might have more values at runtime. There are lots of related issues on this issue tracker for that.

@Manish3177
Copy link
Author

Manish3177 commented Jul 17, 2020

This is just not true - try printing out typeof each time.

I wasn't taking about the actual runtime typeof key. That will indeed match what it actually is. I was referring to what the VS IDE shows if you hover over key in that for loop. Trying the following test:

const k1: string = prompt() || ""; // force TypeScript to not infer a specific literal type
const v1 = Test2[k1]; // doesn't compile even for Test2
const k2 = "One";
const v2 = Test2[k2]; // compiles fine

Shows that although the IDE shows it as string, the compiler is aware that the loop variable isn't just any string.

I think ultimately if there's one thing that should be fixed it's really the third thing: Fail the compile for for...in on an enum. It doesn't matter how that's flagged. Other observations / suggestions are there in case they help identify a deeper issue in type inference.

@RyanCavanaugh
Copy link
Member

This is the intended behavior, or at least the designed behavior.

The iterating variable in a for / in loop is correctly typed as string, but you can use it to index a thing with a numeric index signature as long as it's the thing you're foring over. This is so that "normal" array loops of the form

const arr: boolean[] = [true, false, true];
for (const i in arr) {
  console.log(arr[i]);
}

work as expected.

This interacts a bit weirdly with the reverse lookup on enums, but broadly speaking it's what we want to happen.

"Mixed" enums like Test2 are a disaster and we should have banned them when we had a chance; I would recommend never writing something like this.

@RyanCavanaugh RyanCavanaugh added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants