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

Allow iterating through enum variants with for of #42457

Open
5 tasks done
Timmmm opened this issue Jan 22, 2021 · 7 comments
Open
5 tasks done

Allow iterating through enum variants with for of #42457

Timmmm opened this issue Jan 22, 2021 · 7 comments
Labels
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

Comments

@Timmmm
Copy link

Timmmm commented Jan 22, 2021

Suggestion

πŸ” Search Terms

enum, iterate, variants

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

Note that "is this a runtime feature?" doesn't really apply in this case since enums are already a runtime feature (maybe the only one?). This suggestion just modifies the compilation output slightly.

⭐ Suggestion

The Typescript code for an enum...

enum Colour {
  Red,
  Green,
  Blue,
}

...generates Javascript like this:

var Colour;
(function (Colour) {
    Colour[Colour["Red"] = 0] = "Red";
    Colour[Colour["Green"] = 1] = "Green";
    Colour[Colour["Blue"] = 2] = "Blue";
})(Colour || (Colour = {}));

Unfortunately there's no nice way to iterate over the enum values. The prevailing suggestion on these two very highly upvoted StackOverflow questions is to use for in and then filter out the reverse mapping entries using typeof. That sucks:

  • It's very hacky. Probably really buggy.
  • The types are wrong - you get string or number instead of Colour.
  • It can't handle duplicated enum variants, e.g. enum { Zero = 0, None = 0, ...}, although I'm not 100% sure how duplicates should be handled anyway.

I propose that the above enum code should compile to something like this:

var Colour;
(function (Colour) {
    Colour[Colour["Red"] = 0] = "Red";
    Colour[Colour["Green"] = 1] = "Green";
    Colour[Colour["Blue"] = 2] = "Blue";
    Colour[Symbol.iterator] = function* () {
      yield 0;
      yield 1;
      yield 2;
    };
})(Colour || (Colour = {}));

This would allow you to write:

function paintColour(c: Colour) {...}

for (const c of Colour) {
  paintColour(c);
}

πŸ“ƒ Motivating Example / Use Cases

Probably the most common use is generating UI from enums (e.g. a colour drop-down), but this is very general purpose. The StackOverflow questions linked above demonstrate how much people want this. We've come across this twice already in our code - once for UI and once for collecting data per enum variant, e.g.:

enum Category {
  Code,
  Data,
  Constant,
  Variable,
  Temporary,
  ... lots more ...
}

const totals: Map<Category, number> = new Map();

// Add a 0 entry for all entries.
for (const cat of Category) {
  totals.set(cat, 0);
}

for (const variable of ...) {
  totals.set(totals.get(variable.category) + variable.size);
}

Note there is a related but slightly different bug here: #39627 but it's more about how for in does unexpected things than about making for of work. I don't think it really matters since you should never ever use for in anyway.

@MartinJohns
Copy link
Contributor

MartinJohns commented Jan 22, 2021

This would create much larger output for enums, when it's just used by a few. And it would be kinda weird for flagged enums.

Workarounds are trivial, for example with module merging. Or just with some small helper code: for (const c of Object.values(Colour).filter(x => !isNaN(+x))) { .. } Or wrapped in a function.

IMO enums generate the minimum amount of code needed for the functionality, and that's good.

@Timmmm
Copy link
Author

Timmmm commented Jan 22, 2021

module merging

That requires duplicating the entire enum by hand. Obviously a bad solution (check the emoji responses to that comment!).

Object.values(Colour).filter(x => !isNaN(+x))) { .. }

Yes I mentioned that code like this is currently the best option but it's a pretty bad option. This issue is about making something better.

enums generate the minimum amount of code needed for the functionality

const enums do, but standard enums clearly have additional functionality beyond the minimum, e.g. reverse mappings.

This would create much larger output for enums

This is your only valid point and the only real downside to this proposal, but I think the small increase in code size is worth it.

@Timmmm
Copy link
Author

Timmmm commented Jan 22, 2021

Notes for people that might try to implement it (maybe me). I think there are two things to do to implement it.

  1. Change the code that emits the Javascript to include the Symbol.iterator method. That code is here.
  2. Change the code that checks the iterator type of a value. I think the relevant function is getIteratedTypeOrElementType() (or maybe getIterationTypesOfIterable()?) in checker.ts (took me a while to find because that file is so huge it doesn't get indexed by grep.app!). Note that it calls reportTypeNotIterableError() which uses reports the error Diagnostics.Type_0_must_have_a_Symbol_iterator_method_that_returns_an_iterator which is the one you currently get.

@RyanCavanaugh
Copy link
Member

There's an active TC39 proposal for enums (https://github.com/rbuckton/proposal-enum) so we would want to hold off on changing any runtime semantics of enums until that got through

@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 Jan 22, 2021
@Timmmm
Copy link
Author

Timmmm commented Jan 23, 2021

There's an active TC39 proposal for enums

Thanks for the link. I got a bit carried away and had a go at implementing the iterator part of that proposal (same as mine but returns [string, TheEnum][] instead of just TheEnum[] which I think is better). I have no idea what I'm doing but I got something that sort of works! The following Typescript code

enum Test {
  Foo,
  Bar,
  Baz,
}

for (const member of Test) {
  const x: string = member[0];
  const y: Test = member[1];
  console.log(x, y);
}

const members: [string, Test][] = [...Test];
console.log(members);

compiles with no errors to this Javascript

var Test;
(function (Test) {
    Test[Test["Foo"] = 0] = "Foo";
    Test[Test["Bar"] = 1] = "Bar";
    Test[Test["Baz"] = 2] = "Baz";
    Test[Symbol.iterator] = function* () { yield ["Foo", 0]; yield ["Bar", 1]; yield ["Baz", 2]; };
})(Test || (Test = {}));
for (const member of Test) {
    const x = member[0];
    const y = member[1];
    console.log(x, y);
}
const members = [...Test];
console.log(members);

which produces this output

Foo 0
Bar 1
Baz 2
[ [ 'Foo', 0 ], [ 'Bar', 1 ], [ 'Baz', 2 ] ]

It might be better to use function* () { *[["Foo", 0], ...]; } but that's easy to change. I couldn't make it work with type aliases (type AlsoTest = Test;) but given checker.ts is 40k lines of code I feel like that's not bad!

By the way editing these enormous files is a right pain - it absolutely rinses my laptop, and it makes it really hard to find stuff. All of the scrollbar annotations in VSCode are useless! Would be nice if they were just a little smaller. I'm sure you have had very long discussions about that!

so we would want to hold off on changing any runtime semantics of enums until that got through

I feel like it would be worth implementing just this part of the proposal now anyway, for the following reasons:

  1. It's really annoying, and has been annoying for ages. 600 votes on StackOverflow. In the words of a colleague: 'he amount of times i've thought "ah that's got to be a thing" about this exact problem before half an hour later accepting that it's not!', or in the words of another colleague trying to figure out how to do it: "Any ideas? Driving myself mad here…"

  2. It's not a big change.

  3. The linked proposal seems to be inactive (3 years with no changes). It seems like the author works on the Typescript team (maybe you're their boss?) so I guess you can say "get back to work!" at any point... but still. Are we going to wait another 3 years?

  4. That Symbol.iterator part of the spec seems very unlikely to change. Hopefully this can just be a partial implementation of the draft and then when/if it is ratified it won't need to change.

  5. That proposal looks like it is a slight breaking change to Typescript enums anyway, e.g. you (very sensibly) can't have mixed number/string enums.

@RyanCavanaugh
Copy link
Member

This remains uncommitted and we're not able to take a PR for it at this time.

  1. Not every problem needs a first-class language answer
  2. Doesn't matter
  3. We waited 5 years for optional chaining and it was absolutely the right decision. Language evolution is slow on purpose
  4. "Very unlikely" is not "Certain" and we do not take risks on that aspect
  5. We're not super interested in making bad situations worse πŸ˜‰

@Timmmm
Copy link
Author

Timmmm commented Mar 2, 2021

There's a reasonable workaround for some situations that's worth noting:

const FooVariants = ["a", "b", "c"] as const;
type Foo = typeof FooVariants[number];

Foo gets deduced as "a" | "b" | "c". It doesn't give you symbolic names like Foo.a and it doesn't really work for number-based enums but when it does work it's pretty useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants