Rule proposal: Disallow literals where enums are expected

[mohsen1]

Do not allow number literals to be used instead of enums. I could think of three places that this can happen:

• Function arguments
• Binary expressions
• Assignment expressions

enum Foo {
    ONE,
    TWO
}

function getFoo(f: Foo) {

    if (f === Foo.ONE) { // Good

    }

    if (f === 1) { // Bad

    }

    let ff: Foo;

    ff = Foo.TWO // Good
    ff = 1 // Bad

}

getFoo(Foo.ONE) // Good
getFoo(1) // Bad

Rule name

I need help with naming it according to conventions

Options

I don't think this rule needs any options. I don't see why anyone wants to use number literals for enums.

[bradzacher]

This could be expanded to just "disallow non-enum comparisons when using enum types"

So that it covers all of the cases (which are valid typescript, but error-prone):

enum FooImplicit {
    A,
    B,
}
FooImplicit.A === 0

enum FooNum {
    A = 1,
    B = 2,
}
FooNum.A === 1

enum FooStr {
    A = 'a',
    B = 'b',
}
FooStr.A === 'a'

[bradzacher]

TS 4.3 brings in some checks here that solves this for number enums!
Union Enums Cannot Be Compared to Arbitrary Numbers

[ChocolateLoverRaj]

@bradzacher @JoshuaKGoldberg if someone makes a PR for this feature, what should the name of the rule be?

[bradzacher]

It's really hard to name this. Maybe something like no-misused-enums?

Given this rule will cover assignments and comparisons I can't think of a good name which covers both cases succinctly.

[JoshuaKGoldberg]

@ChocolateLoverRaj are you interested in & have time for sending a PR?

No worries if you don't, I can send one soon if not!

[ChocolateLoverRaj]

@ChocolateLoverRaj are you interested in & have time for sending a PR?

No worries if you don't, I can send one soon if not!

Yes, i can make a PR. it's my first time doing an eslint related PR though.

[Zamiell]

@ChocolateLoverRaj Any updates on this?

[Zamiell]

It's really hard to name this. Maybe something like no-misused-enums?

How about strict-enums?

[Zamiell]

Also, I would want greater-than and less-than comparison operators to be illegal:

enum Foo {
  A,
  B,
  C,
  D,
}

// bad
if (foo < Foo.C) {}

// good
if (foo === Foo.A || foo === Foo.B) {}

[Zamiell]

I'm trying my hand at this, but I'm stuck at comparing the types.

const leftType = typeChecker.getTypeAtLocation(leftTSNode);
const rightType = typeChecker.getTypeAtLocation(rightTSNode);
const areTypesEqual = ???

What is the correct way to see if the types are equal? I'm looking for an ID field or something, but I don't see anything like that.

In the helper functions, I do see getTypeName, but its possible for two different enums to exist with the same name, right? So I presume that comparing by using the name would not be safe.

[Zamiell]

I proceeded with just using the name to compare for now.
I did more work on this today.
Everything works great, except for detecting invalid function calls, like:

function fruitFunction(fruit: Fruit) {}
fruitFunction(0); // This line should throw an ESLint warning

I'm stuck on how to get the type of the matching function argument. Specifically, on how to write the "getFunctionArgumentType" function:

CallExpression(node): void {
  const identifier = node.callee;
  const functionType = getTypeFromNode(identifier);

  for (let i = 0; i < node.arguments.length; i++) {
    const expressionArgument = node.arguments[i];
    const expressionArgumentType = getTypeFromNode(argument);
    const expressionArgumentTypeName = getTypeName(argumentType);

    const functionArgumentType = getFunctionArgumentType(functionType, i);
    const functionArgumentTypeName = getTypeName(functionArgumentType);
    
    if (expressionArgumentTypeName  !== functionArgumentTypeName) {
      context.report({ node, messageId: 'incorrectFunctionArgument' });
    }
  }
}

function getFunctionArgumentType(functionType: ts.Type, i: number) {
  // ???
}

I tried iterating over getCallSignatures, but that didn't work, because it reports that the type of (myStringEnum = MyStringEnum.Value) => void is "string" instead of "MyStringEnum".

[Zamiell]

I made more progress today. It turns out the trick to get function argument types was to use the getCallSignaturesOfType helper function.

Now I'm stuck on a new problem. I don't know how to use ESLint and/or the TypeScript API to get the constituent types of a union type. Until I can figure this out, the rule doesn't catch things like:

const fruit: Fruit | null = 0;

[JoshuaKGoldberg]

It's really hard to name this. Maybe something like no-misused-enums?

How about strict-enums?

That's a bit catch-all, maybe... strict-enum-comparisons?

comparing the types

😭 microsoft/TypeScript#9879 strikes yet again.

way to see if the types are equal

You could try looking at the declarations with symbol.getDeclarations(). ✨

getCallSignaturesOfType

For the curious, that's a small wrapper around type.getCallSignatures() that also accounts for unions and intersections: https://github.com/ajafff/tsutils/blob/03b4df1/util/type.ts#L121

constituent types of a union

type.isUnion() is type (): this is ts.UnionType -- which gives a hint that ts.UnionType has what you need.

if (type.isUnion()) {
  type.types; // UnionOrIntersectionType.types: ts.Type[]
}

---

Looks like you're making a lot of great progress. Really exciting!

[Zamiell]

That's a bit catch-all, maybe... strict-enum-comparisons?

Currently, the rule does more than look at comparisons. It looks at:

1. Declarations
2. Assignments
3. Incrementing/Decrementing
4. Comparisons
5. Function arguments

You can look through the strict-enums.test.ts to see.
It's at 1184 lines currently and growing.

[Zamiell]

You could try looking at the declarations with symbol.getDeclarations()

I actually found out that you can just use triple equals to compare types directly.
My initial tests with triple equals didn't work because I was comparing Fruit to Fruit.Apple, which are different types, so I got confused and burned a bunch of hours trying other things.

[Zamiell]

if (type.isUnion()) {

Yup, that works great, thanks.

[JoshuaKGoldberg]

Ah, we have an existing no-misused-promises rule that also does a lot more than comparisons. Maybe that's a good naming precedent?

[Zamiell]

Well, there's also an existing rule called strict-boolean-expressions.

[JoshuaKGoldberg]

[bradzacher]

For navigating unions you can use tsutils.unionTypeParts(type) which will handle both the union and non-union cases automatically.

---

For comparing two types - you can do an equality check. If the type is the same type, then TS will return a referentially equal object.

For enums because they are a "primitive" type (non object), TS will do the hard work of resolving past aliases and such for you.

EG for the following code, the resolved types (via checker.getTypeAtLocation(node)) for the variables will be the same type object.

enum Foo { A }
declare const x: Foo.A;
type T = Foo.A;
declare const y: T;

You can play around in https://ts-ast-viewer.com/ to validate this assumption.

If we were dealing with object types - things would get much harder, but enums are simple!

In a nutshell I believe TS will do the following - define a type for each of the enum members, and then define a type for the enum itself which is essentially just a special type which is equivalent to a union of all the enum member types.

---

Re: naming - maybe no-misused-enums is a good name?
Using -comparisons isn't great because ideally we should also assignments as well as comparisons:

enum Foo { A }

const x: Foo = 0; // should error
if (x === 0) {} // should error

[Zamiell]

Also note that strict-enums follows the TypeScript compiler flag convention of --strict, --strictBindCallApply, --strictFunctionTypes, --strictNullChecks, and so on.

If this code was ever submitted as a pull request to TypeScript itself, it would almost certainly be named --strictEnums.

[Zamiell]

Ok, I did more work today, and all the tests are passing now.
It uses using strict equality to compare the types, and uses the unionTypeParts helper function to decompose unions.
I'll mark the PR as not being a draft anymore.

[Zamiell]

The lint rule uncovers some bugs in this monorepo.
Thus, linting in CI fails.

Should I fix all of the bugs as part of this PR or should that be done in a separate PR?

[JoshuaKGoldberg]

Could you fix them in the PR please? If you don't have time a TODO comment would suffice. In the past we've found the typescript-eslint codebase to be surprisingly good at exposing some edge cases in PRs.

[JoshuaKGoldberg]

From #6107: I don't have the time to work on this either, so if anybody else wants to pick it up, please do!

[Zamiell]

If anyone wants it, I implemented the rule here as part of eslint-plugin-isaacscript: https://github.com/IsaacScript/isaacscript/blob/main/packages/eslint-plugin-isaacscript/docs/rules/strict-enums.md

[loicraux]

Would be nice if it could also handle switch statements... No errors are reported for instance with this code where the switch statement deals with a string and compare against an string-based enum EventKeys:

    @bound private handleKeydown(keyboardEvent: KeyboardEvent): void {
        let doStopPropagationAndPreventDefault = false;
        assertIsDefined(this.controlledPopupMenu);
        switch (keyboardEvent.key) {
            case EventKeys.SPACE:
            case EventKeys.ENTER:
            case EventKeys.DOWN:
                this.controlledPopupMenu.open();
                this.controlledPopupMenu.setFocusToFirstVisibleItem();
                doStopPropagationAndPreventDefault = true;
                break;

            case EventKeys.UP:
                this.controlledPopupMenu.open();
                this.controlledPopupMenu.setFocusToLastVisibleItem();
                doStopPropagationAndPreventDefault = true;
                break;

            default:
                break;
        }

        if (doStopPropagationAndPreventDefault) {
            keyboardEvent.stopPropagation();
            keyboardEvent.preventDefault();
        }
    }

[Zamiell]

@loicraux I've since stripped out the code in isaacscript/strict-enums that overlaps with the official no-unsafe-enum-comparison rule.

But yes, what you posted is a bug, let's track that in #7509.

[RedGuy12]

I think this should be closed now? @typescript-eslint/no-unsafe-enum-comparison exists.

[Zamiell]

@RedGuy12 It cannot be closed because the rule only checks for comparisons and doesn't check for assignment.

e.g.

enum Fruit {
  Apple,
  Banana,
}

let fruit = Fruit.Apple;
fruit = 1; // No error!

If you want to prevent this unsafe pattern in your code, use the isaacscript/strict-enums rule.