[dot-notation] Should typescript-eslint provide a replacement for dot-notation?

[octogonz]

Repro

{
  "rules": {
     "dot-notation": "error",  // ESLint's rule
  }
}

class X { }

function createX(): X {
  const x: X = new X();
  // Error dot-notation: ["__tag"] is better written in dot notation
  x['__tag'] = 123;
  return x;
}

Additional Info

ESLint's fixer converts the above code to x.__tag = 123; which fails to compile, because __tag is not a declared member. This is bad, because before migrating a bunch of code to ESLint, you want to run the fixer to clear away any trivial problems, but we certainly don't want the fixer to introduce compiler errors.

Also, the expectations in TypeScript might be a little different:

• If __tag is a declared member of X, then the the dot-notation warning/fixer makes good sense.
• If __tag is not a declared member of X, then perhaps we should allow this notation, since it's a common idiom used by unit tests to access private members of a class.
• If the data type of x is any, then [] notation is maybe okay as well, since it conveys that we're accessing dynamic keys, not declared members.

What do you think? Should typescript-eslint provide a TypeScript-aware replacement for dot-notation? If so, should the new rule be called @typescript-eslint/dot-notation?

Versions

package	version
@typescript-eslint/eslint-plugin	2.0.0-alpha.4
@typescript-eslint/parser	1.13.0
TypeScript	3.5.3
ESLint	6.1.0

[bradzacher]

I'm not quite sure I understand why you would be breaking encapsulation by accessing private internals of a class.
I get that it's the easy way to test certain things, but to me that signifies a code smell - that something is incorrectly structured.

If the data type of x is any, then [] notation is maybe okay as well, since it conveys that we're accessing dynamic keys, not declared members.

I don't see why (x as any)['prop'] is any better than (x as any).prop. They are essentially equivalent statements. IIRC the dot notation is just shorthand for the array index notation.

If __tag is not a declared member of X, then perhaps we should allow this notation, since it's a common idiom used by unit tests to access private members of a class.

Nope - it should fail if it not a declared member. Typescript will error if it's not a declared member.
I could potentially see a case for private members, but those are declared members.

class X {
  private priv_prop = 123;
  public pub_prop = 123;
}

const x = new X();
x['doesnt_exist'] = 123; // ts error
x.doesnt_exist = 123; // ts error
x['priv_prop'] = 123;
x.priv_prop = 123; // ts error
x['pub_prop'] = 123;
x.pub_prop = 123;

repl

---

I could see the extension adding an option which goes both ways - allowPrivateClassPropertyAccessViaBracketNotation: boolean.

class X {
  private priv_prop = 123;
}

// allowPrivateClassPropertyAccessViaBracketNotation: true
x['priv_prop'] = 123; // no error

// allowPrivateClassPropertyAccessViaBracketNotation: false
x['priv_prop'] = 123; // error: you should not access private members via bracket notation as it breaks encapsulation

[octogonz]

I'm not quite sure I understand why you would be breaking encapsulation by accessing private internals of a class. I get that it's the easy way to test certain things, but to me that signifies a code smell - that something is incorrectly structured.

I'm mostly seeing this in repos that require 100% code coverage for unit tests, so people often resort to hacks to force obscure code paths to get executed. If the recommendation is to spend more time designing the classes to make their contracts more amenable to unit testing... that's noble, but a somewhat unrelated topic from adopting ESLint. :-)

Is there a better syntax for allowing unit tests to access private members? Here's what I had considered as an alternative:

type PublicMembers<T> = { [P in keyof T]: T[P]; };

class X {
  private _a: number = 123;
  private _method(): void {
  }
  public b: number = 123;
}

interface IPrivateX extends PublicMembers<X> {
  _a: number;
  _method(): void;
}

function unitTest() {
  let x: IPrivateX = new X() as any;
  console.log(x._a);
  console.log(x.b);
  x._method();
}

This is a LOT more verbose, however. And it might get messy with static members, inheritance, merged declarations, etc.

Typescript will error if it's not a declared member.

I see, you must have noImplicitAny enabled.

I don't see why (x as any)['prop'] is any better than (x as any).prop. They are essentially equivalent statements. IIRC the dot notation is just shorthand for the array index notation.

The no-explicit-any lint rule will complain about (x as any).prop.

[bradzacher]

100% code coverage is a fallacy and anyone requiring that should be given a good spoken to, IMO :)

The no-explicit-any lint rule will complain about (x as any).prop.

sorry, i was just using that as shorthand.

const x = Object.create(null); // implicitly typed as any
// there's no difference between these two
x['prop']
x.prop

---

There's no nice way to access private members, because you shouldn't be doing it! But I understand that people can't always spend the time to design the perfect abstractions so you don't access privates, esp when they're wasting a lot of their time trying to get 100% code coverage :P

Could extend the rule and add the option as described in my previous comment.

[octogonz]

100% code coverage is a fallacy and anyone requiring that should be given a good spoken to, IMO :)

I agree 100%. 👍 That bandwagon is coming, just a bit further down the road from "convert everything to TypeScript" and "adopt typescript-eslint".

[octogonz]

I chatted with some more people about this. We realized that x['prop'] is superior to (x as any)['prop'], because the first one preserves type information for the property, and it's way less verbose than the private interface thing. Everyone also agrees that accessing private members is a bad practice, but in practice unit tests need to do this. We can configure the linter to allow this for unit tests only, though.

@bradzacher I am willing to implement your proposed allowPrivateClassPropertyAccessViaBracketNotation idea.

Hmm... is there an example of an extended rule that adds an option to the baseRule.meta.schema? I'm not sure how to do that.

[octogonz]

is there an example of an extended rule that adds an option to the baseRule.meta.schema?

I found an example in no-magic-numbers.ts.

[bradzacher]

FYI - these are all the extension rules we have.

typescript-eslint/packages/eslint-plugin/tools/generate-configs.ts

Lines 21 to 32 in 39e41b5

	const BASE_RULES_TO_BE_OVERRIDDEN = new Set([
	'camelcase',
	'func-call-spacing',
	'indent',
	'no-array-constructor',
	'no-extra-parens',
	'no-magic-numbers',
	'no-unused-vars',
	'no-use-before-define',
	'no-useless-constructor',
	'semi',
	]);

I've been meaning to write docs on how to do it, but it's not a hugely common thing for people to do.

[octogonz]

@bradzacher I started working on this rule, but I cannot figure out how to access the compiler's semantic analysis. For example:

class MyClass {
  private _a: number = 123;
  public b(): void {
    log(this._a.toString());
  }
}
const c: MyClass = new MyClass();
log(c['_a'].toString());

Given the MemberExpression for c['_a'], how do I find the class declaration for c and test whether _a is a private member or not?

[bradzacher]

I'm a bit fuzzy on exactly how it works. It's been a month since I touched typecheck accessing rule code.
The best way to figure it out would be to look at our other rules which use type information.
If you're using vscode, the debugger should be setup to work automatically, just hit go. This is the best way to explore what the compiler returns. All you need to do is have one test case.

IIRC, this is something along the lines of what you need to do

const parserServices = utils.getParserServices(context);
const typeChecker = program.getTypeChecker();

{ 
  SomeNode(node) {
    const tsNode = parserServices.esTreeNodeToTSNodeMap.get<ts.SomeNodeType>(node);

	// i can't remember which one you need to use, sorry
    const type = typeChecker.getTypeAtLocation(tsNode);
    const declaration = util.getDeclaration(typeChecker, tsNode);
  },
}

[octogonz]

Awesome, this is what I needed. I'm super familiar with the compiler API, so I can probably figure it out from here.

However, I realized that because we already enforce the _ prefix for all private members, we can workaround this using "allowPattern": "^_". Thus I'm going to table this PR for the moment and focus on #790 which is a higher priority.

[octogonz]

While migrating files, I'm encountering a bunch of warnings for this scenario:

interface IFileInfo { 
  name: string;
}

interface ILookupTable {
  [filename: string]: IFileInfo;
}

const table: ILookupTable = { };

table['.gitignore'] = { name: 'first' };

// Error dot-notation: ["LICENSE"] is better written in dot notation.
table['LICENSE'] = { name: 'second' };

// Error dot-notation: ["make"] is better written in dot notation.
table['make'] = { name: 'third' };

for (const key of Object.keys(table)) {
  console.log(table[key].name);
}

In this example, it would be weird to write table.make.name because that key is a dynamic value that is usually determined at runtime. Some of these keys may contain characters that are not allowed in a JavaScript identifier. The table['make'].name form above more clearly communicates that 'make' is a dictionary key, whereas name is a declared member.

Proposal: The new @typescript-eslint/dot-notation rule should accept table['make'] because it is referring to an ts.IndexSignatureDeclaration. Wheres ['name'] would be rejected because it refers to a ts.PropertySignature. The table.make form should be allowed-but-not-required, though, since there are legitimate cases where type algebrists use ts.IndexSignatureDeclaration in keyof expressions that describe declared members.

This seems pretty uncontroversial. However, I could make it configurable via a setting, if there are any TypeScript-minded developers out there who actually prefer this sort of inconsistency:

// Does anyone prefer this inconsistent form?
table['.gitignore'] = { name: 'first' };
table.LICENSE = { name: 'second' };
table.make = { name: 'third' };

What do you think?

[bradzacher]

Could potentially add an option for it, if you want to do the work to implement it.

I'm not sure how much use it would see outside of your codebase - I haven't seen the use of Record<string, any> when you explicitly know the keys.

[octogonz]

I'm not sure how much use it would see outside of your codebase - I haven't seen the use of Record<string, any> when you explicitly know the keys.

I encountered it in unit tests that were adding hardcoded entries to the table.

Could potentially add an option for it, if you want to do the work to implement it.

Yeah, I'm willing to implement this when I get some time.

[LuminescentMoon]

dot-notation taking into account TS's index signatures would be particular useful in projects where a JS minifier that performs symbol renaming is used. May want to add an option to enforce square-bracket notation for accessing properties on global objects (and objects nested in global objects), and enforce dot-notation otherwise.

See: https://developers.google.com/closure/compiler/docs/api-tutorial3#dangers

Closure compiler only renames property access by dot notation but not square-bracket notation, which has many implications.

[bradzacher]

@LuminescentMoon - that sounds like something you don't need type information for, so it would work fine as an addition to the base eslint rule.

ESLint has scope analysis built in by default, so it should be able to tell the difference between a global and an imported variable.

[bradzacher]

This has been implemented: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/dot-notation.md#options