[no-shadow] Enums can false-positive ESLint's no-shadow rule

[epmatsw]

Repro

{
  "rules": {
    "no-shadow": 2
  }
}

const test = 'test';

export const enum MyEnum {
    test = 42
}

Expected Result

No errors.

Actual Result
The test enum item gets flagged by no-shadow.

Additional Info

Seems like maybe we needs an @typescript-eslint/no-shadow rule to handle TypeScript's syntax for enums?

Versions

package	version
@typescript-eslint/eslint-plugin	1.4.2
@typescript-eslint/parser	1.4.2
TypeScript	3.3.3333
ESLint	5.14.1
node	10.15.0
npm	6.7.0

[epmatsw]

I'm not sure if this should be the parser (for putting an enum item into scope?) or the plugin (it should be in scope, but we need a new TS-specific no-shadow to handle it).

[bradzacher]

we're working on implementing typescript-compliant scope analysis.
It's hard to do, but better than re-implementing all of the base eslint rules that rely on scope.

[mysticatea]

Thank you for this report.

However, this is intentional behavior because the enum member test literally shadows the upper test.

const f = 777;
enum E1 {
    f = 1, // Shadows the upper `f`.
    g = f, // = 1
}
enum E2 {
    f1 = 1,
    g = f, // This is the upper `f`.
}

Open playground

[bradzacher]

In typescript, you can think of an enum declaration as the following:
A closure which exports all variable declarations as properties on an object it returns.

It's not 100%, but it makes sense if you look at an example playground

const imOutside = 2;
const b = 2;
enum Foo {
    outer = imOutside,
    a = 1,
    b = a,
    c = b,
    // does c == Foo.b == Foo.c == 1?
    // or does c == b == 2?
}

In the above example, what does Foo.c equal? If you think of an enum as an object like you stated - then your assumption would be that Foo.c === 2.
HOWEVER, if you instead think of the enum declaration as a closure, you can tell that the b = a declaration shadows the outer const b = 2 declaration, which means that in fact Foo.c === 1.

This is why the example you provided is not correct. If I rewrite my example using objects:

const imOutside = 2;
const b = 2;
const Foo = {
    outer: imOutside,
    a: 1,
    b: a,
    c: b,
    // does c == Foo.b == Foo.c == 1?
    // or does c == b == 2?
}

Then I can ask the same question - what is the value of Foo.c? Because an object expression does not create its own scope, the answer is Foo.c === 2 (and actually typescript will throw a compile time error at you, because the variable a is not defined).

I hope this clarifies it for you. To reiterate:

• no-shadow should error out on an enum because enum properties act like variables in a new scope, so they can indeed shadow a variable in the parent scope.
• no-shadow should NOT error out on an object because object properties DO NOT act like variables in a new scope, so they CANNOT shadow a variable in the parent scope.

[oigewan]

@bradzacher sorry, tried to delete my comment before someone spent time on a response once I realized what was going on. I did eventually see that enums create a new scope. That's quite....unexpected. Thanks for taking the time, and sorry for the bother.

[mcmar]

@bradzacher Do you think it's reasonable to request an option for no-shadow to allow enums? Seems like a reasonable compromise. Right now, I'm just disabling this rule around my enums when I need to. I'll open a feature request, but not sure if I should do it here or in the core eslint library.

[bradzacher]

It's not unreasonable, no.
It would have to be in this repo, as the core lib has no concept of enums yet.
It will require the creation of an extension rule in this repo. All around it is a pretty trivial amount of work.

However, for me as an engineer; I would take this as a code smell.
If you are running into this around most enums (or rather enough enums to cause you to want to create an option to work around it), then you might need to either re-evaluate your variable naming patterns, or re-evaluate where you declare your enums (perhaps move them to a new file?).

Creating an option would create a false sense of security, and will potentially cause bugs. With that option, all it takes is one shadow usage by an engineer who doesn't know that enums have a scope, and your enum will have the wrong value.

[oigewan]

@bradzacher I agree that as-is, this is code smell since the enum keyword simply creates a scope for assignment. However, I have an idea of how to alleviate the issue. Given that you understand the language better than I do, I'm wondering if you have an opinion.

I'm thinking of writing a separate rule in the next month or two (day job is busy) that will require the values of an enum to be primitives. I would consider basing an enum's value off of a variable instead of the other way around to be a larger bit of code smell than having the property of an object use a shadowed name. In other words:

// Bad
const someString = "something";
enum Stuff {
  SomeString = someString,
}

// Good
enum Stuff {
  SomeString = "something",
}
const someString = Stuff.SomeString;

If paired with the rule I'm wanting to write, I don't think that allowing shadowed enum keys is code smell any more. Thoughts?

[bradzacher]

I would agree with you and that rule would be a good addition.
Flipping the problem on its head gives you safety when turning off no-shadow for enums.

[kg-currenxie]

I can understand that shadowing happens when you use something from the outer scope as the value, but not when it's the key.

Simplified example:

import { white } from './variables'

enum Colors {
  white = '#fff',
}

Super annoying :)

[Klemensas]

I've just hit @kg-currenxie described case, indeed it's annoying and will force me to disable the rule :(

@oigewan did you had a chance to work on the rule you've proposed? It seems like an adequate solution.

[bradzacher]

OOC how often are you hitting it that it's annoying enough to disable the rule entirely?

As mentioned in previous comments, to me that seems like a code smell that there are that many enum member name collisions.

[Klemensas]

I guess I should've phrased that differently - I meant that I won't be enabling the rule in the 1st place.

I wanted to introduce the rule into an existing react codebase of medium size. But after learning that I have to work around enum keys to have it play nice it pushes the rule over the limit of being more trouble than use.
The clashes happen mostly with imports, for example, an enum has a key of Input and there's a component named Input being imported.

IMHO, thinking about my case, the smell arguments aren't very strong. I'm not planning on using any variables for enums, I'd actually like to have the additional rule proposed by @oigewan. So I can't think of a good reason why enum member names should be all that different.
As for moving them to a different file, well that's not a good option in my case as well. The components contain types, code, and enums in one file. For me, this structure is tidy enough and works fine.

[oigewan]

I've just hit @kg-currenxie described case, indeed it's annoying and will force me to disable the rule :(

@oigewan did you had a chance to work on the rule you've proposed? It seems like an adequate solution.

I haven't as I've switched teams and otherwise been quite busy. However, I am thinking of trying to get started this weekend.

[oigewan]

@Klemensas and @bradzacher I've created a pull request with the rule described in this thread: #1898

If it gets accepted, I can do a follow-up PR which will add an option to the no-shadow rule which will ignore enum members with documentation recommending that the new rule is enabled if the no-shadow option ignoring enum members is enabled.

[JrSchild]

Sorry to chip in here. It makes sense that for variables there can be no double declaration. However for types and interfaces that should not be the case as they are not variables, this behavior seems correct if the type and enum is defined in the same file, however once they are split up in separate files this does not work anymore.

E.g. The following does not throw a no-shadow error:
file1.ts

export interface Car {
  color: string;
}

export enum Vehicles {
  Car = 'car',
}

But when importing Car it does throw an error:
file2.ts

export interface Car {
  color: string;
}

file3.ts

import { Car } from './file2';

export const car: Car = {
  color: 'red',
}

export enum Vehicles {
  Car = 'car',
}