Specific enum value not comparable with number in 5.0

[jakebailey]

Bug Report

🔎 Search Terms

operator cannot be applied enum number comparison

🕗 Version & Regression Information

• This changed between versions 4.9 and 5.0

⏯ Playground Link

Playground Link

💻 Code

const enum Constants {
    DefaultName = "foo",
    DefaultTimeout = 1000,
    MaxItems = 25,
}

declare var someValue: number

if (someValue > Constants.MaxItems) {
    someValue = Constants.MaxItems;
}

🙁 Actual behavior

Operator '>' cannot be applied to types 'number' and 'Constants'.(2365)

Removing the string-ish value from Constants stops the error.

🙂 Expected behavior

No error; even though Constants shouldn't be comparable thanks to #52048, this seems like it should work because it's only referencing a specific value.

/cc @Andarist @ahejlsberg

[jakebailey]

@jdom I filed this based on your TS discussion email.

[fatcerberus]

fwiw the example given feels like a misuse of enums. If anything that should be a namespace instead.

[Andarist]

I don't quite understand TS enums (I don't use them in my own code) - so I have no idea if this should work or not. If it should, I can create a PR patching my change to allow this.

[fatcerberus]

Generally the purpose of enums is that you have a set of symbolic constants whose exact value is unimportant (except maybe for debugging); all that’s important is that Enum.A is a different value from Enum.B. a bit like nominal types but in value space.

[Andarist]

The fact that this enum has mixed constant (?) values (string | number~) makes me believe that this code is misusing the relation operator. With my change this code errors as well (and that was "caught" and rechecked at the PR stage):

declare const val: string | number
declare var someValue: number

if (someValue > val) { // errors
    someValue = 100;
}

[jakebailey]

I generally agree that these "constant bags" are probably a misuse of enums, though I suspect in this case it's being used for the inlining provided by const-ness.

[jakebailey]

makes me believe that this code is misusing the relation operator

I would disagree; this code example references a specific enum member which is a number and is interchangable with the literal 1000. It seems to me like this is a bad interaction of #52048 and #50528 where the freshness of this enum reference is being lost in the comparison check.

[Andarist]

Sure, but even with inlining - we end up comparing number and number | string. So the question is: is this something that TS considers to be safe enough to allow devs to rely on coercion here?

[Andarist]

I would disagree; this code example references a specific enum member which is a number and is interchangable with the literal 1000. It seems to me like this is a bad interaction of #52048 and #50528 where the freshness of this enum reference is being lost in the comparison check.

Oh, I see what you are saying here - ye. I guess that perhaps this might be a bug worth fixing then. I would love to have some extra confirmation before I jump into fixing this though.

[jdom]

I can provide details about the reason for doing this, and is like @jakebailey mentioned: it's because it allows me to define some constants on the top of the file that I just reuse in my code, and will automatically be inlined in the generated javascript. Perhaps there's a better way to have such inlining without using const enum, but I have yet to find it (open to alternatives).

[jakebailey]

In this case, I think this might just be something we didn't have a test for, so wasn't caught. The error goes away if we don't getBaseTypeOfLiteralType the two sides before comparing them, as that function takes enum literals and converts them to their enum's base type, rather than considering the literal itself. It seems like it might be worthwhile in this case to have a variant which instead considers that specific literal's type.

But generally speaking, having a grab bag of random unrelated constants in an enum is not a super good idea; if you split them apart into smaller, related pieces, you would avoid this.

(I have further const enum inlining opinions but they're sorta moot and hypocritical given TS itself makes heavy use of them exactly for the perf of inlining...)

[jakebailey]

The error goes away if we don't getBaseTypeOfLiteralType the two sides before comparing them

More correctly would be to have a variant of the above which checks EnumLiteral after all of the other literals.

[jakebailey]

I've sent #52703 for discussion.

[jakebailey]

Per design meeting, this is a bug and we should allow this code. Just need to get the above PR reviewed and fixed up.