Skip to content

Design Meeting Notes, 7/15/2022 #49927

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
DanielRosenwasser opened this issue Jul 15, 2022 · 4 comments
Closed

Design Meeting Notes, 7/15/2022 #49927

DanielRosenwasser opened this issue Jul 15, 2022 · 4 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

Enabling Computations on String Enums

#49809

  • Seems strange to allow const foo: string = "some string".

    • The : string annotation should "spoil" it.
    • What about a union of literals?
    • Must evaluate to exactly one string literal type.
  • Aside: wtf?

    enum E {
       VALUE = "SOME_STRING",
    }
    
    enum F {
       VALUE = E.VALUE
    }
    
    // WORKS!?
    const value: number = F.VALUE;
  • Is this an aside? Is it irrelevant?

    • Well, you really don't want to open the hole further.

      const s = "some string";
      enum F {
         VALUE = s
      }
      
      // !?
      const value: number = F.VALUE;
  • Most people don't create a lot of indirect computations between enums and constants.

  • We really have two enums: "classic numeric" and union.

    • Today, any computation opts you into classic numeric enums.
      • Classic numeric enums are assignable to/from number, are subtypes of number, are not assignable to/from most other enums.
    • This feature would imply that any union enum can contain computations.
  • Usually you want a "classic numeric" enum to support bit-flags. The most common computation you might have is a left-shift.

    • Maybe rules between strings and numbers don't need to be the same.
    enum SomeStringEnum {
      Value = "hello!",
    }
    
    enum Flags {
      None = 0,
      One = 1 << 0,
      Uhh = SomeStringEnum.Value
    }
  • How would you model operations other than concatenation?

    • Could in theory understand function signatures.
    • But the reason you can do inlining between enums is because you can "guarantee" the values. If you resolve emitted values based on function return (e.g. an initializer that calls <T>(x: T) => Uppercase<T>), then the types and values can diverge, and you have divergences between a non-type resolving compiler (e.g. Babel) and TypeScript itself.
  • Two options for fixing classic enums.

    • One is to change the checking behavior of individual string members (use-site restrictions).
    • The other is to disallow string enums from classic/computed enums (declaration-side restrictions).
  • What would be the user fix?

    • declaration site break requires extracting members out to their own enum
    • use-site break only happens when assigning to number. Same fix, but you might never have encountered it.
  • We don't like the idea of needing deeper checks to determine the "kind" of enum that you have (vs. looking at the initializers syntactically).

  • Are people really using strings in classic/numeric enums?

    • We see it with some regularity.
  • Strictness flag?

  • Conclusions

    • Enums are a mess - want to think about this some more.
    • We want to "fix" the behavior today for strings in computed enums - want to see how much we would break and decide from there.
    • Difference between direct and indirect computations that are hard to reason about.
@Zamiell
Copy link
Contributor

Zamiell commented Jul 17, 2022

A "strictEnums" compiler flag has been on my wish list for years, so I'm overjoyed to see this issue!

Would this hypothetical flag prevent the following pattern? (I'm hoping for "yes".)

enum Vegetable {
  Lettuce = 'lettuce',
  Carrot = 'carrot',
}

declare const vegetable: Vegetable;

// Bad
if (vegetable === 'lettuce') {
  // The TypeScript compiler currently allows this.
}

// Good
if (vegetable === Vegetable.Lettuce) {
}

This pattern is dangerous. Say that I swapped the enum values of Lettuce and Carrot by using the F2 (rename) feature of VSCode. After this, the code block would be doing an entirely different thing than the author first intended. And the TypeScript compiler would never alert me to the error, because "lettuce" is still a "valid" value.

I'd rather that the TypeScript compiler forced me to change vegetable === 'lettuce' to vegetable === Vegetable.Lettuce from the get-go, such that the code becomes future-safe from any enum values arbitrarily changing. (At least in the world where I have the strictEnums compiler flag turned on.)

As for number enums, they have this same problem too, but of course it's much worse, because you can actually assign non-valid values at compile-time. I'd hope that a hypothetical strict flag would address all of the issues that this ESLint rule does.

@aghArdeshir
Copy link
Contributor

Didn't understand most of mentioned notes in the bullet list 😅
I believe most of it was discussed implicitly verbal/unwritten.

But anyway thanks for sharing, considering and implementing this thing 👍

@DanielRosenwasser
Copy link
Member Author

@Zamiell I don't believe so - the tightening we're considering is around the fact that "classic"/numeric enums are usable in numeric arithmetic operations and are always assignable to number. That "assignable to number" part is a pretty big hole, but it's to allow assigning the result of arithmetic between enum members back to the enum member itself.

export enum Permissions {
  None = 0,
  Read = 1 << 0,
  Write = 1 << 1,
  Execute = 1 << 2,
}

// Debatable whether this should be allowed.
// Today, it is.
let p: Permissions = Permissions.Read | Permissions.Write;

The "wtf" we have in the notes refers to the fact that a "classic numeric" enum with computed members would usually disallow a string member. Here's an example where an inline string is disallowed:

export enum Permissions {
  None = 0,
  Read = 1 << 0,
  Write = 1 << 1,
  Execute = 1 << 2,
  
  // Errors - good! ✅
  UnrelatedMember = "what?",
}

And here's an example where a const value is disallowed:

const unrelated = "what?";

export enum Permissions {
  None = 0,
  Read = 1 << 0,
  Write = 1 << 1,
  Execute = 1 << 2,
  
  // This is also disallowed ✅, although the error message is kind of confusing...
  UnrelatedMember = unrelated,
}

But you can defeat the check through a layer of indirection by referencing an enum.

export enum Unrelated {
  Unrelated = "what?",
}

export enum Permissions {
  None = 0,
  Read = 1 << 0,
  Write = 1 << 1,
  Execute = 1 << 2,
  
  // What? This *is* allowed? 🤔
  UnrelatedMember = Unrelated.Unrelated,
}

// And therefore, this is allowed!? 😬
let p: number = Permissions.UnrelatedMember;

// Runtime: 💥
p.toExponential();

So it's unclear what the flag would do (maybe @RyanCavanaugh has thoughts). We could tighten the declarations to not allow strings in these cases to avoid breaking existing code unless under --strict.

We could also have the flag disallow the default arithmetic behavior as well. Maybe we'd have to come up with a way to mark how you intend the enum to be used.

// Possibly now disallowed unless `Permissions` is declared as a "bitflags" enum or something.
let p: Permissions = Permissions.Read | Permissions.Write;

The more I see these examples, the more I'm convinced that we should try to fix the string behavior without a flag if we can. @RyanCavanaugh mentioned that it feels like we see code like this with some regularity, but I still don't have a great sense of this.

@DanielRosenwasser
Copy link
Member Author

@aghArdeshir sorry if there's anything unclear. The notes are taken while the conversation is relatively fast (and while I often participate). I'll admit that likely makes things less clear, but hopefully my last comment clarifies some of the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

4 participants