fix(eslint-plugin): [prefer-function-type] handle `this` type #1783

anikethsaha · 2020-03-23T10:37:27Z

added a no check when return type is this for interface declarations
added tests

typescript-eslint · 2020-03-23T10:37:30Z

Thanks for the PR, @anikethsaha!

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.

bradzacher

What about cases like

interface Interface {(): this | string}
interface Interface {(): Array<this>}
interface Interface {(arg: this): void}
interface Interface {(arg: this | string): void}
interface Interface {(arg: Array<this>): void}

In addition to the other comment, this PR should handle all cases where a callable interface uses this this type.

packages/eslint-plugin/tests/rules/prefer-function-type.test.ts

anikethsaha · 2020-03-23T17:14:57Z

interface Interface {(): this | string} // valid 
interface Interface {(): Array<this>} // not sure, I will add a test for this to check 
interface Interface {(arg: this): void} // it will be invalid !
interface Interface {(arg: this | string): void}  // it will be invalid !
interface Interface {(arg: Array<this>): void}  // it will be invalid !

interface Interface {(arg: this): void} // it will be invalid !
interface Interface {(arg: this | string): void}  // it will be invalid !
interface Interface {(arg: Array<this>): void}  // it will be invalid !

Should I add support for parameters as well ? or they should be kept as invalid ?

bradzacher · 2020-03-23T17:18:50Z

With this fix, we will want to update the fixer so it does not output semantically invalid code.
One of the core rules in eslint is that fixers should not knowingly break the build.
In typescript, this is not always possible, because some changes can have a butterfly-effect types-wise - we allow some level of "unsafety" in the fixers.

But in this case it's pretty clear-cut: a type declaration cannot reference the this type. So no matter the position, it should not fix it.

anikethsaha · 2020-03-23T17:23:15Z

But in this case it's pretty clear-cut: a type declaration cannot reference the this type. So no matter the position, it should not fix it.

so, what I understood, we have to disable the fixer for this related code ?

bradzacher · 2020-03-23T19:36:32Z

There are essentially two options here.

When the rule detects an interface that use the this type, it can either:

completely ignore it. This is the "simplest" solution, but also kinda sucks because it can lead to stylistic inconsistencies in the codebase.
conditionally ignore it, gated by an option. This would probably be my preferred option, as it allows people to keep their codebase consistent.
- the fixer would need to be removed for the specific case.
- A new message ID would probably make sense as well, because the message could suggest converting to a recursive type:
  interface Foo { (): this } ====> type Foo = () => Foo.
- The rule could provide a "suggestion" fixer to do this conversion for the user.
  - Suggestion fixers are like normal fixers, except they can only be applied manually via the IDE. They're a way to provide a fix for an error, but force the user to make a decision.

anikethsaha · 2020-03-23T19:41:55Z

ok, that make sense,

just one query though,

A new message ID would probably make sense as well, because the message could suggest converting to a recursive type:
interface Foo { (): this } ====> type Foo = () => Foo.

interface Foo { () : this | string } ?

bradzacher · 2020-03-23T20:14:30Z

whenever this is encountered in the type, it can be replaced with the name of the type.

interface Interface1a {(): this}
type Interface1b = {(): Interface1b}

interface Interface2a {(): this | string}
type Interface2b = {(): Interface2b | string}

interface Interface3a {(): Array<this>}
type Interface3b = {(): Array<Interface3b>}

interface Interface4a {(arg: this): void}
type Interface4b = {(arg: Interface4b): void}

interface Interface5a {(arg: this | string): void}
type Interface5b = {(arg: Interface5b | string): void}

interface Interface6a {(arg: Array<this>): void}
type Interface6b = {(arg: Array<Interface6b>): void}

repl

anikethsaha · 2020-03-23T20:36:53Z

ok cool, I will change it

anikethsaha · 2020-03-24T20:39:36Z

packages/eslint-plugin/tests/rules/prefer-function-type.test.ts

+      output: `type Interface = () => Interface`,
+    },
+    {
+      code: 'interface Interface {(): Object<this>}',


@bradzacher this ( generic types ) will fail as the latest commit is still WIP.

anikethsaha · 2020-03-24T20:47:27Z

@bradzacher the latest commit is still WIP

As per the review I made changes in the fixer.

Can you please check if you are fine with how its being checked ( getReturnType function and its being implemented ),
for changing the message ID with, we need to return it from the renderSuggestion method as well cause there only we are doing the call to the recursion thing. Can you suggest the id or message
There are few overlapping issue while comparing which is why I think the CI is failing, should I turn off the rule there ?
If you have a better suggestions to implement this, it would be awesome. Like if there is any utility
function to replace the string in a better way for the type like here , here

bradzacher · 2020-03-26T00:56:44Z

packages/eslint-plugin/src/rules/prefer-function-type.ts

+      if (node.type === AST_NODE_TYPES.TSThisType) {
+        return node.range;
+      }


There are few overlapping issue while comparing which is why I think the CI is failing, should I turn off the rule there ?

This is failing the typecheck because it can never be true.
Your argument is of type TSESTree.TypeAnnotation, which is a container node.

See my other comment for a suggestion of how to structure the function.

bradzacher · 2020-03-26T00:58:06Z

packages/eslint-plugin/src/rules/prefer-function-type.ts

+    function getReturnType(
+      node: TSESTree.TSTypeAnnotation,
+    ): Array<number> | null {
+      if (!possibleReturnThisTypeHolders.has(node.type)) {
+        return null;
+      }
+
+      if (node.type === AST_NODE_TYPES.TSThisType) {
+        return node.range;
+      }
+
+      if (node.type === AST_NODE_TYPES.TSTypeAnnotation) {
+        if (node.typeAnnotation.type === AST_NODE_TYPES.TSThisType) {
+          return node.typeAnnotation.range;
+        }
+      }
+
+      if (node.type === AST_NODE_TYPES.TSFunctionType) {
+        return getReturnType(node?.returnType);
+      }
+
+      return null;
+    }


Suggested change

function getReturnType(

node: TSESTree.TSTypeAnnotation,

): Array<number> | null {

if (!possibleReturnThisTypeHolders.has(node.type)) {

return null;

}

if (node.type === AST_NODE_TYPES.TSThisType) {

return node.range;

}

if (node.type === AST_NODE_TYPES.TSTypeAnnotation) {

if (node.typeAnnotation.type === AST_NODE_TYPES.TSThisType) {

return node.typeAnnotation.range;

}

}

if (node.type === AST_NODE_TYPES.TSFunctionType) {

return getReturnType(node?.returnType);

}

return null;

}

function getReturnType(

node: TSESTree.TSTypeAnnotation,

): Array<number> | null {

if (node.typeAnnotation.type === AST_NODE_TYPES.TSThisType) {

return node.range;

}

if (node.typeAnnotation.type === AST_NODE_TYPES.TSFunctionType) {

return getReturnType(node?.returnType);

}

// etc

return null;

}

I tried this

function getReturnType( node: TSESTree.TSTypeAnnotation, ): Array<number> | null { if (!possibleReturnThisTypeHolders.has(node.type)) { return null; } if (node.typeAnnotation.type === AST_NODE_TYPES.TSThisType) { return node.range; } if (node.typeAnnotation.type === AST_NODE_TYPES.TSTypeAnnotation) { // <--- type error if (node.typeAnnotation.type === AST_NODE_TYPES.TSThisType) { // <--- type error return node.typeAnnotation.range; } } if (node.typeAnnotation.type === AST_NODE_TYPES.TSFunctionType) { return getReturnType(node?.returnType); } return null; }

Still no luck, even tests are failing cause of this !

bradzacher · 2020-03-27T16:14:34Z

Okay. So looking at this, it's going to be a bit of a pain to recurse through all of the various permutations of valid positions for the TSThisType.

A better way to do this would be something like the following rough code.
The idea is that we could use ESLint selectors to do the heavy lifting for us.

let tsThisTypes: TSESTree.TSThisType[] | null;

{
  'TSInterfaceDeclaration:enter'(): void {
    tsThisTypes = [];
  },
  'TSInterfaceDeclaration:exit'(node: TSESTree.TSInterfaceDeclaration): void {
    if (!hasOneSupertype(node) && node.body.body.length === 1) {
      checkMember(node.body.body[0], node);
    }
    tsThisTypes = null;
  },

  'TSTypeLiteral[members.length = 1]:enter'(): void {
    tsThisTypes = [];

  },
  'TSTypeLiteral[members.length = 1]:exit'(node: TSESTree.TSTypeLiteral): void {
    checkMember(node.members[0], node);
    tsThisTypes = null;
  },

  TSThisType(node): void {
    tsThisTypes?.push(node);
  },
};

// then in the fixer for interface
for (const thisType of (tsThisTypes ?? [])) {
  fixer.replaceText(thisType, interfaceName);
}

// in the reporter for TSTypeLiteral
if (tsThisTypes && tsThisTypes.length > 0) {
  // cannot handle this types because we don't know the name of the type literal
  // so we can't generate a recursive type
  return;
}

anikethsaha · 2020-06-24T08:50:49Z

@bradzacher I am sorry as I couldn't able to complete this PR.

I may look at this in the future if no one is taking.

Thanks

fix: no check for this return type (fix typescript-eslint#1756)

8207dca

bradzacher requested changes Mar 23, 2020

View reviewed changes

packages/eslint-plugin/tests/rules/prefer-function-type.test.ts Show resolved Hide resolved

bradzacher added the bug Something isn't working label Mar 23, 2020

Merge remote-tracking branch 'upstream/master' into fixes-1756

409b7df

bradzacher changed the title ~~fix: no check for this return type (fix #1756)~~ fix(eslint-plugin): [prefer-function-type] handle this type Mar 24, 2020

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Mar 24, 2020

chore: fixed the fixer for this (refs typescript-eslint#1783)

364ed4e

anikethsaha commented Mar 24, 2020

View reviewed changes

bradzacher reviewed Mar 26, 2020

View reviewed changes

bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jun 24, 2020

bradzacher closed this Jun 24, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [prefer-function-type] handle `this` type #1783

fix(eslint-plugin): [prefer-function-type] handle `this` type #1783

anikethsaha commented Mar 23, 2020

typescript-eslint bot commented Mar 23, 2020

bradzacher left a comment •

edited

Loading

anikethsaha commented Mar 23, 2020

bradzacher commented Mar 23, 2020

anikethsaha commented Mar 23, 2020

bradzacher commented Mar 23, 2020

anikethsaha commented Mar 23, 2020

bradzacher commented Mar 23, 2020

anikethsaha commented Mar 23, 2020

anikethsaha Mar 24, 2020

anikethsaha commented Mar 24, 2020

bradzacher Mar 26, 2020

bradzacher Mar 26, 2020

anikethsaha Mar 27, 2020

bradzacher commented Mar 27, 2020

anikethsaha commented Jun 24, 2020

fix(eslint-plugin): [prefer-function-type] handle this type #1783

fix(eslint-plugin): [prefer-function-type] handle this type #1783

Conversation

anikethsaha commented Mar 23, 2020

typescript-eslint bot commented Mar 23, 2020

bradzacher left a comment • edited Loading

Choose a reason for hiding this comment

anikethsaha commented Mar 23, 2020

bradzacher commented Mar 23, 2020

anikethsaha commented Mar 23, 2020

bradzacher commented Mar 23, 2020

anikethsaha commented Mar 23, 2020

bradzacher commented Mar 23, 2020

anikethsaha commented Mar 23, 2020

anikethsaha Mar 24, 2020

Choose a reason for hiding this comment

anikethsaha commented Mar 24, 2020

bradzacher Mar 26, 2020

Choose a reason for hiding this comment

bradzacher Mar 26, 2020

Choose a reason for hiding this comment

anikethsaha Mar 27, 2020

Choose a reason for hiding this comment

bradzacher commented Mar 27, 2020

anikethsaha commented Jun 24, 2020

fix(eslint-plugin): [prefer-function-type] handle `this` type #1783

fix(eslint-plugin): [prefer-function-type] handle `this` type #1783

bradzacher left a comment •

edited

Loading