Repository navigation
Specific enum value not comparable with number in 5.0 #52701
Description
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
💻 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 Mateusz Burzyński (@Andarist) Anders Hejlsberg (@ahejlsberg)
Activity
jakebailey commentedon Feb 9, 2023
Julian Dominguez (@jdom) I filed this based on your TS discussion email.
fatcerberus commentedon Feb 9, 2023
fwiw the example given feels like a misuse of enums. If anything that should be a namespace instead.
Andarist commentedon Feb 9, 2023
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 commentedon Feb 9, 2023
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.Ais a different value fromEnum.B. a bit like nominal types but in value space.Andarist commentedon Feb 9, 2023
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):jakebailey commentedon Feb 9, 2023
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 commentedon Feb 9, 2023
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 commentedon Feb 9, 2023
Sure, but even with inlining - we end up comparing
numberandnumber | string. So the question is: is this something that TS considers to be safe enough to allow devs to rely on coercion here?Andarist commentedon Feb 9, 2023
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 commentedon Feb 9, 2023
I can provide details about the reason for doing this, and is like Jake Bailey (@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 commentedon Feb 9, 2023
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
getBaseTypeOfLiteralTypethe 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 commentedon Feb 9, 2023
More correctly would be to have a variant of the above which checks
EnumLiteralafter all of the other literals.jakebailey commentedon Feb 9, 2023
I've sent #52703 for discussion.
jakebailey commentedon Feb 15, 2023
Per design meeting, this is a bug and we should allow this code. Just need to get the above PR reviewed and fixed up.