no-reserved-keywords not finding "catch" in all places

[ChrisMBarr]

I have this code:

spyOn(customerService, "getCustomer").and.returnValue({
  then: (): {catch: () => void} => {
    return {
      catch: (): void => { return; }
    };
  }
});

Running with

• TSLint version: 5.7.0
• TypeScript version: 2.5.3
• Running TSLint via: gulp

Actual behavior

It only reports a violation in the typedef

Expected behavior

I would expect it to also report a violation in the return statement below where the actual object property is named catch, in addition to the typedef above.

[HamletDRC]

@ChrisMBarr I fixed this in the attached PR, but I am not sure we actually want to do this.

In your example, we flag "catch" in the parameter name as banned term because this is a type definition that you the programmer have written. We do not flag the 2nd catch (the property name at line 6) because you only wrote 'catch' there because someone previously declared that you had to have a parameter named that. The error to fix is at line 4, not line 6.

If you look at the attached PR, you see many changes are needed in order to add a bunch of suppressions because TSLint is publishing interfaces with the word 'type' in it, or JQuery is making an interface with some other banned keyword.

I would not actually fix this one. The rule currently bans you from writing these keywords in type declarations. The rule currently allows you to use a banned keyword when you're implementing someone else's type declaration.

what do you think @loicraux @JoshuaKGoldberg @ian-craig ?

[ChrisMBarr]

Ok, what you've said makes sense. However, if the goal is not to use a reserved JS keyword in TS code I'd say that only flagging violations in typedefs is somewhat useless since that's not real "code" - meaning it never makes it to the transpiled output.

To me, it makes sense to flag the reserved keyword in both instances so that it's not used in real code, and also in the typedefs since that might help you catch this early as you typically write typedefs before you write the actual code and you get a change to name a better naming choice earlier.

If I write:

interface IMyThing {
  var: string;
  type: string;
  catch: (err: string) => void;
}

const someObj: IMyThing = {
  var: 'myNum',
  type: 'number',
  catch: (err: string) => console.log(err)
};

I'd expect to see violations on the interface and the usage since both use the same property name we've deemed to be non-ideal and possibly misleading.

For systems already in place, renaming properties is probably difficult (like in your attached PR). However, in places were it's easier to change, having one violation per usage might help give you an idea of how widely spread this mis-named property is, and that would be encouragement for renaming in.

Here's how I'd re-write my above example

interface IMyThing {
  variableName: string;
  variableType: string;
  onError: (err: string) => void;
}

const someObj: IMyThing = {
  variableName: 'myNum',
  variableType: 'number',
  onError: (err: string) => console.log(err)
};

[JoshuaKGoldberg]

How about an opt-in config option to add the extra checks? Agreed both that you should be allowed to use these if you need, but that it's better to avoid them.

[ChrisMBarr]

I think that way makes the most sense. The default doesn't work for everyone, so let it be configured!

[JoshuaKGoldberg]

☠️ It's time! ☠️

Per #876, this repository is no longer accepting feature pull requests. TSLint is being deprecated and we recommend you switch to https://typescript-eslint.io.

Thanks for open sourcing with us, everyone!