Rule to prevent certain keywords being used as variable names

[myitcv]

Prevents nasties like:

let undefined = 5;
let Number: Number;
function F8(undefined) {}
function G7(boolean: boolean) {}
let [ string ] = [ 5 ];
let { boolean } = { boolean: 1 };

Comments/thoughts welcomed.

[myitcv]

Not a negative diff, simply correcting ordering

[adidahiya]

looks like this would address microsoft/TypeScript#3081

[jkillian]

Is this a comprehensive list of keywords/types that tsc will let you redefine? It looks good to me, just want to make sure there isn't anything missing or extraneous

[myitcv]

@jkillian started with this to kick off discussion.

Arguably this list could include any ambient variable from lib.d.ts, like Number, String and Boolean

The idea behind this rule is to increase the likelihood of writing code that is unambiguously understood by as many as possible. You could easily chuck RegExp, Arrayand other core interfaces that have real class equivalents in there too...

But then you could probably well argue it goes too far to include event for example.

[jkillian]

Gotchya, I'm fine with it like this for this PR. Changing the list is easy and essentially backwards-compatible, so that can be discussed down the road.

[jkillian]

Looks good other than my comments above!

[jkillian]

Also, perhaps there should be test cases for reassignments? I know NaN isn't on this blacklist right now, but if it was, we would want to disallow things like:

NaN = 5;

[myitcv]

@jkillian agree that is totally bizarre but I intentionally didn't include such things in this rule because arguably you could/should make those definitions in lib.d.ts const (unless I'm missing something?)

[myitcv]

@jkillian fixed the filename issue. Good catch, but per my comment can you tell me why the build didn't fail? Seems like that entry in tsconfig.json is redundant.... hence can all of them go?

[jkillian]

@myitcv Perhaps the reason to not make them const is to reflect their JS behavior maybe? Since in JS you can theoretically redefine NaN, TS doesn't prevent you from doing that. Maybe there's some TS library out there that depends on redefining NaN haha.

[myitcv]

Perhaps the reason to not make them const is to reflect their JS behavior maybe? Since in JS you can theoretically redefine NaN

Could well be. Sounds like a candidate for another rule.... agree?

[jkillian]

As far as the build not failing, my guess is that your tests just weren't running at all (and also then not failing at all).

My inclination is that it can be in this same rule - it seems reasonable that a rule that doesn't let you declare variables with the name String also doesn't let you reassign String (although in practice it will be difficult to reassign String because you have to match the typing). What do you think?

[myitcv]

As far as the build not failing, my guess is that your tests just weren't running at all (and also then not failing at all).

Quite possibly, but the compiler/grunt should probably error if a listed file does not exist, that was more my point. Which then leads you to working out "oh my tests aren't running"...

My inclination is that it can be in this same rule

Gave this some more thought last night.

By preventing assignments like NaN = 5 we are essentially enforcing that NaN is const. Question is then why you wouldn't do the same for every ambient var in lib.d.ts?

Consider the following:

// marker 1
name = "test";

export function DoIt(): void {
    // marker 2
    let name = 5;
    console.log(name);
}

This is valid because of the declaration of name in lib.d.ts:

declare var name: string;

Couple of questions (at least) come out of this:

1. Should we prevent the assignment at marker 1? name is an ambient var like NaN
2. The declaration of name in the function is essentially shadowing the ambient declaration

The no-shadowed-variable rule does not pick up point 2 above; not sure why.

In any case, I think preventing the assignment to NaN is better achieved by using a custom lib.d.ts (or equivalent) and changing the ambient vars to ambient consts. Otherwise the rule needs to maintain a list of all the ambient vars.

The intention of this rule should I think be to prevent variables being named with known 'keywords'. The definition of 'keyword' is clearly up for debate with so many types (Date, Array, etc) you could include in this list. Hence why I've started with only those types which have primitive equivalents.

[jkillian]

Sounds good to me, I'm fine with leaving this rule as is then.

As far as the strangeness with grunt and a lack of errors, I'll try to get that changed when we move to the test system proposed in #620

[jkillian]

Can you just do two tweaks before this gets merged in?

• Add a few false positive lines to the tests just as a sanity check that the rule doesn't start going off on all variable names, prefixed variable names, etc.
• Add the rule to sample.tslint.json and the readme :)

[myitcv]

@jkillian - done. The formatting changes in sample.tslint.json are just that; correct the ordering and indentation (again would probably be a good to have this automatically formatted)