Typescript-eslint: [prefer readonly] readonly versus this[property]

[kmdrGroch]

Repro

{
  "rules": {
    "@typescript-eslint/prefer-readonly": "error"
  }
}

interface Config {
   num: 40
}
const config = {
  num: 40
}

class myClass {
  private num: number; // 1
  constructor() {
    this.num= config.num;
  }
  public configure(conf: Partial<Config>) {
    const configKeys = Object.keys(config);
    for (const attr in conf) {
      if (configKeys.includes(attr)) {
        this[attr as keyof Config] = conf[attr as keyof Config] || 0; // 2
      }
    }
  }
}

Expected Result

Get no error

Actual Result

"Member 'num' is never reassigned; mark it as readonly." // 1

after adding readonly i get TS error:
"Cannot assign to 'num' because it is a read-only property." // 2

Versions

| package | version |
| ---------------------------------- | ------- |
| @typescript-eslint/eslint-plugin | 2.11.0 |
| @typescript-eslint/parser | 2.11.0 |
| TypeScript | 3.7.3 |
| ESLint | 6.7.2 |
| node | 13.3.0 |
| npm | 6.13.3 |

[bradzacher]

It's impossible for static analysis to detect this without unrolling the loops and analysing every possible value and branch.

This is an edge case which you'll have to use an eslint-disable comment.

Generally it's considered bad practice to do this[prop] on classes, esp when assigning.
It's a serious security hole (someone can easily spoof your types and pass in { configure: () => maliciouslyLogUserData() }, and silently override your implementations.

Consider storing your configurable options separately on a class property that is a map/object.

The best that we can do is detect the this[prop] = ... statement, and disable the rule on the class.
Happy to accept a PR to do that.

[vanya829]

Hi @bradzacher . I want to work on this issue. Just to clarify what should be done:
Skip check if a class has a dynamic property assignment

Should we add this functionality as an optional or it will be enabled in all cases?

[bradzacher]

Thinking more about this, I don't think that's a good course of action.
I think that this is just an edge-case that an eslint-disable comment is for.

An eslint disable comment is a much better fit because it makes it clear you're doing something dynamic and sketchy with the class.
If we just disable the rule on a class, then it creates a weird situation where the rule will just stop reporting for some users, and as we cannot message to say that it was disabled, it will just confuse users.

Also, detecting this is actually really hard to do properly due to having to do manual scope analysis and such.