Enum improvements

[jbondc]

Started to look at implementing #1003 and struggled understanding enums.
This patch addresses the following issues:

• Make ambiant & const enums report similar errors (allow any constant number expression). fixes Const enum expression in Ambient enum declaration behaves differently between spec and compiler #2790
• If an invalid enum constant expression is found, continue incrementing with the last valid initialized value.
• If an enum expression references another enum member, then emit a reference to the other value.

Note that I'm still confused about what 'constant' means for an enum #538
Thinking that enum should allow for NumericLiteral, it's an enum<number>.

You could have a more restricted/subset enum:

enum<int32> a {
  a, // 0
  b, = 1.1 // error, initializer must be a constant expression of type 'int32'
  c, = -1, // ok
  d, // 1
}

Or if you don't want signed:

enum<uint32> b {
  a, // 0
  b, = 1.1 // error, initializer must be a constant expression of type 'uint32'
  c, = -1, // error, initializer must be a constant expression of type 'uint32'
  d, // 1
}

[msftclas]

Hi @jbondc, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[DanielRosenwasser]

Hi @jbondc, thanks for the PR. While I'm not sure if @vladima was working on it at the time, in the future please be mindful of the assignee of an issue if there is one, and ask if it would be okay to pick the issue up.

I have a few questions about your work though:

If an invalid enum constant expression is found, continue incrementing with the last valid initialized value.

Is there any reason for this behavior change? I don't see the previous behavior as an actual issue, and I'd rather not introduce new behavior for the sake of what one might feel is slightly more correct. Both are just as broken.

If an enum expression references another enum member, then emit a reference to the other value.

Sorry, from what I understand this is the current behavior, could you please elaborate?

You could have a more restricted/subset enum

For something like this, it would warrant formal proposal beforehand.

[DanielRosenwasser]

If an enum expression references another enum member, then emit a reference to the other value.

I see what you mean now - can you write a test case like the following so that we can see the effect this PR would have?

tests/cases/conformance/enumsMembersWithInitializersThatReferenceLaterMembers01.ts

enum E1 {
    a = c,
    b = 2,
    c = 3,
}

declare enum E2 {
    a = c,
    b = 2,
    c = 3,
}

const enum E3 {
    a = c,
    b = 2,
    c = 3,
}

declare const enum E4 {
    a = c,
    b = 2,
    c = 3,
}

enum E5 {
    a = c,
    b,
    c,
}

declare enum E6 {
    a = c,
    b,
    c,
}

const enum E7 {
    a = c,
    b,
    c,
}

declare const enum E8 {
    a = c,
    b,
    c,
}

[jbondc]

Is there any reason for this behavior change? I don't see the previous behavior as an actual issue, and I'd rather not introduce new behavior for the sake of what one might feel is slightly more correct. Both are just as broken.

Pretty much. It felt slightly more correct because no errors are generated on the nodes that are undefined:

My expectation would have been to see red lines on d,e,f
By continuing to increment/assign values, it works with less errors.

[jbondc]

Good catch, I'm thinking:

var E5;
(function (E5) {
    E5[E5["b"] = 0] = "b";
    E5[E5["c"] = 1] = "c";
    E5["a"] = E5.c;
})(E5 || (E5 = {}));

Would be the emit if a member references another member before it's emitted, does that seem right? Alternative is it's an error to reference an enum member before it's declared.

[jbondc]

For something like this, it would warrant formal proposal beforehand.

Proposal #3105 though will likely make refinements in the upcoming weeks.

[CyrusNajmabadi]

@jbondc "Why no errors after...".

Because inundating users with errors that all stem from an initial bad issue is not good compiler behavior. We try to avoid this sort of cascading when we can.

[jbondc]

While I'm not sure if @vladima was working on it at the time, in the future please be mindful of the assignee of an issue if there is one, and ask if it would be okay to pick the issue up.

Noted, wasn't my intent to work on enums. Stumbled on it as it overlaps with number literal types, at least how I'd plan to implement it.

[DanielRosenwasser]

👎 this breaks existing reverse mapping behavior.

[jbondc]

E9[E9.A] // "A"
E9[E9.B] // "A"

That's what I would expect, this pattern works well with string enums #3192

Don't think the current behavior is more correct where the last declared member overrides the mapping:

E9[E9.A] // "B"
E9[E9.B] // "B"

[jbondc]

If there's a BC concern I guess I could emit:

     E9[E9["A"] = 0] = "A";
     E9["B"] = E9.A;
     E9[0] = "B";

[DanielRosenwasser]

That's what I would expect, this pattern works well with string enums

But string enums weren't officially approved at all.

Don't think the current behavior is more correct where the last declared member overrides the mapping

Perhaps for most cases, but this is still a breaking change; the compiler did not give an error in these scenarios.

Can you explain why this change was originally necessary?

[jbondc]

It's a simpler model to work with (that the original declaration doesn't change).

// lib.ts
enum  foo {
a = 1
b = 2
}

// thirdparty.ts
enum foo {
c = 1
}

Why is 'thirdparty.ts' allowed to modify the reverse mapping?

About string enums, I've implemented it here:
master...jbondc:setTypes-v2

There's some wonkyness still but overall think it will scale well with literals/set type guards.

[jbondc]

Oops missing file here:
jbondc@5991931

Need to cleanup & pass tests but maybe will clarify what I'm trying to do and how it relates to enums.

[jbondc]

@DanielRosenwasser To follow up on the reverse mapping issue, here's a recent issue/use case within the compiler:

    export const enum SymbolConstraints {
        notWritable = 1,
        notMutable,
        Immutable = notWritable | notMutable,
        //ConstantBinding = notWritable,
    }

When debugging, I would normally see:

  *** infered type
  Type id=4122 ValueSetMember=67108864 NumberSet=2097152 Number=4
    Symbol name=valuesNumber id=15743 ConstEnum=128 -- constraints:  notMutable=2 notWritable=1

But if you uncomment 'ConstantBinding', I now get:

  *** infered type
  Type id=4122 ValueSetMember=67108864 NumberSet=2097152 Number=4
    Symbol name=valuesNumber id=15743 ConstEnum=128 -- constraints:  notMutable=2 ConstantBinding=1

That's clearly not my intent and quite annoying behavior.

[jbondc]

Or just this one:

  Node kind=7 (FirstLiteralToken) pos=1296

What I really expect is

  Node kind=7 (NumericLiteral) pos=1296