feat(eslint-plugin): [no-unsafe-call][no-unsafe-member-access] improve report messages for this for noImplicitThis

[JounQin]

close #3197

[typescript-eslint]

Thanks for the PR, @JounQin!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #3199 (cee1fb1) into master (62dfcc6) will decrease coverage by 0.02%.
The diff coverage is 88.67%.

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
- Coverage   92.91%   92.89%   -0.03%     
==========================================
  Files         316      317       +1     
  Lines       10854    10905      +51     
  Branches     3069     3085      +16     
==========================================
+ Hits        10085    10130      +45     
- Misses        342      343       +1     
- Partials      427      432       +5     

Flag	Coverage Δ
unittest	92.89% <88.67%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ckages/eslint-plugin/src/util/getThisExpression.ts	84.61% <84.61%> (ø)
packages/eslint-plugin/src/rules/no-unsafe-call.ts	95.23% <88.88%> (-4.77%)	⬇️
...es/eslint-plugin/src/rules/no-unsafe-assignment.ts	92.96% <90.00%> (-0.26%)	⬇️
...eslint-plugin/src/rules/no-unsafe-member-access.ts	97.82% <90.00%> (-2.18%)	⬇️
...ckages/eslint-plugin/src/rules/no-unsafe-return.ts	98.24% <90.90%> (-1.76%)	⬇️

[bradzacher]

we can actually tell if the config option is turned on!
We should only report this message if the option is turned off

Here's an example of another rule checking if strictNullChecks is turned on:

typescript-eslint/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

Lines 114 to 117 in 9273441

	const isStrictNullChecks = tsutils.isStrictCompilerOptionEnabled(
	compilerOptions,
	'strictNullChecks',
	);

---

I think if we're going explicitly display this then we should be sure that this is the cause and not something else.

eg we should only display the specific message if:

• the "thing" is a member expression on any
• noImplicitThis is turned off
• this is of type any

If any of these conditions is false - then we should use the normal message as we can tell that noImplicitThis is not the cause.

[JounQin]

@bradzacher Thanks! I'll add the option check. And the other two conditions has been checked, isn't it?

[bradzacher]

Depends on the rule.

For example, no unsafe call - this.foo() will report if EITHER this or this.foo are any

And this only compounds the more members you add ofc.

For no unsafe member - this.foo.bar will again report if this or this.foo are any.

[JounQin]

I'm still confused, I've already added checking whether the caller/member parent is this at https://github.com/typescript-eslint/typescript-eslint/pull/3199/files#diff-57d22a396b83ef3e522e1cb9ce9ed841ebc305e7978abaf79996180f83a5dc87R54-R55 and https://github.com/typescript-eslint/typescript-eslint/pull/3199/files#diff-95b4d02562218eb4ed05f07944d416c25edb4861ac3b215e31db7ca924c97920R73 in this PR, what should I check further?

[bradzacher]

you did check if it was an expression around this.

For no-unsafe-call, it will error if the callee is any.

Which means that it will error in this case: this.foo(). But what if this is well defined, but foo is any?
i.e. this case:

class Foo {
  foo: any;

  method() {
    this.foo();
  }
}

In this case saying "try turning on noImplicitAny" is wrong - it's not the this that's any - it's this.foo.

OTOH, it is correct to say "try turning on noImplicitAny" for this case:

const Foo = {
  method() {
    this.foo(); // `this` is `any` with `noImplicitAny` turned off
  },
}

For no-unsafe-member-access, it's similar.

Eg imagine this.foo.bar - if this is any, then it makes sense to say "try turning on noImplicitAny", but if this.foo is any, then it doesn't make sense to say that.

Does that make sense?

[JounQin]

OK, maybe I get your point now. this type should be any for the specific messages, right?

[bradzacher]

this type should be any for the specific messages, right?

Correct!

To triple clarify for you:

To save me copy-pasting these strings, the messageIds:

• unsafeCall = 'Unsafe call of an `any` typed value.';

• unsafeCallThis = [
    'Unsafe call of an `any` typed value. `this` is typed as `any`.',
    'You can try to fix this by turning on the `noImplicitThis` compiler option, or adding a `this` parameter to the function.',
   ].join('\n');
  

• unsafeMemberExpression = 'Unsafe member access {{property}} on an `any` value.'

• unsafeThisMemberExpression = [
    'Unsafe member access {{property}} on an `any` value. `this` is typed as `any`.',
    'You can try to fix this by turning on the `noImplicitThis` compiler option, or adding a `this` parameter to the function.',
  ].join('\n');
  

class Foo {
  bar: any;

  method() {
    this.bar();      // no-unsafe-call: unsafeCall

    this.bar.baz()   // no-unsafe-call: unsafeCall
                     // no-unsafe-member-access: unsafeMemberExpression {property: baz}

    this.bar.baz.bam // no-unsafe-member-access: unsafeMemberExpression {property: baz}
  }
}

// noImplicitThis: false
const Obj = {
  method() {
    this.bar();      // no-unsafe-call: unsafeCallThis
                     // no-unsafe-member-access: unsafeThisMemberExpression {property: bar}

    this.bar.baz()   // no-unsafe-call: unsafeCallThis
                     // no-unsafe-member-access: unsafeThisMemberExpression {property: bar}

    this.bar.baz.bam // no-unsafe-member-access: unsafeThisMemberExpression {property: bar}
  },
};

With noImplicitThis: true, the last example would OFC be full of TypeScript errors, so we don't need to specifically worry about how we report on it.

There is this one special case which would exist with noImplicitThis: true, but it's pretty stupid and very much a user-error - so I don't think we need to handle it with a custom message:

function anyThisFn(this: any) {
  this.bar();      // no-unsafe-call: unsafeCallThis
                   // no-unsafe-member-access: unsafeThisMemberExpression {property: bar}

  this.bar.baz()   // no-unsafe-call: unsafeCallThis
                   // no-unsafe-member-access: unsafeThisMemberExpression {property: bar}

  this.bar.baz.bam // no-unsafe-member-access: unsafeThisMemberExpression {property: bar}
};

[JounQin]

@bradzacher How about current implementation?

[bradzacher]

almost there! Just a few comments.
Thanks for your work on this!

[bradzacher]

LGTM - thanks for your contribution!