feat(eslint-plugin): new rule method-signature-style #1685

phaux · 2020-03-06T01:36:54Z

typescript-eslint · 2020-03-06T01:36:57Z

Thanks for the PR, @phaux!

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

haven't reviewed the code, but a few quick comments on the documentation

bradzacher · 2020-03-06T02:04:46Z

packages/eslint-plugin/docs/rules/method-signature-style.md

+```
+
+A method and a function property of the same type behave differently.
+Methods are always bivariant in their argument, while function properties are contravariant in their argument under `strictFunctionTypes`. See the reasoning behind that [here](https://github.com/microsoft/TypeScript/pull/18654).


Would be good to clarify the functionality with/without the flag, and better define bivariance and contravariance, as most users wouldn't know what those mean (I always forget and have to google it 😅).

With strictFunctionTypes turned off, both method signatures and function types are considered bivariant in their argument types (meaning that all arguments can be the same, supertypes, or subtypes).
However, with strictFunctionTypes turned on, method signatures remain bivariant, but function types become contravariant in their argument types (meaning that all arguments must be the same type, or a supertype).
See the reasoning behind that in the typescript PR for the compiler option.

This difference creates a possible safety hole when using method types instead of function types.

I didn't want to overwhelm the user with too much information. I changed this part to be more concise and only nudge the user towards using the more correct option. I think your explanation goes too much into detail. Tell me what you think.

That original paragraph was from TypeScript's FAQ. If the user wants to learn more they still can click the link.

packages/eslint-plugin/docs/rules/method-signature-style.md

bradzacher

mostly LGTM right now.

One change for the fixer.
Thanks!

bradzacher · 2020-03-23T05:25:13Z

packages/eslint-plugin/src/rules/method-signature-style.ts

+    function getMethodParams(
+      node: TSESTree.TSMethodSignature | TSESTree.TSFunctionType,
+    ): string {
+      let params = node.params.map(node => sourceCode.getText(node)).join(', ');
+      params = `(${params})`;
+      if (node.typeParameters != null) {
+        const typeParams = sourceCode.getText(node.typeParameters);
+        params = `${typeParams}${params}`;
+      }
+      return params;
+    }


This will remove all comments inside the type.
For the most part I don't have problems with a fixer removing comments in weird places if it makes the fixer simpler and easier to maintain.

But one place we should accept and support comments is around parameters, eg:

method(/* leading comment */arg: string): void

So I would suggest converting this to something like:

grab first open paren token

grab last end paren token

grab source code between the two

That way we preserve the "expected" comments

I just did that, but it only works if there's at least one argument. If the argument list is (/* comment */) then the comment is lost. The problem is that there's no way to find the paren tokens because the method node's "params" property is just an empty array. This is contrary to "typeParams" which is a separate AST node so I can just getText it even if it's empty.

bradzacher

LGTM - thanks for doing this

codecov · 2020-04-03T04:01:37Z

Codecov Report

Merging #1685 into master will decrease coverage by 0.02%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #1685      +/-   ##
==========================================
- Coverage   94.73%   94.71%   -0.03%     
==========================================
  Files         160      161       +1     
  Lines        7300     7338      +38     
  Branches     2092     2101       +9     
==========================================
+ Hits         6916     6950      +34     
- Misses        164      165       +1     
- Partials      220      223       +3

Flag	Coverage Δ
#unittest	`94.71% <89.47%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
.../eslint-plugin/src/rules/method-signature-style.ts	`89.47% <89.47%> (ø)`

bradzacher · 2020-04-03T04:17:56Z

Note - there was a linting change recently which has caused CI failures for your PR.
Please have a look and I can merge it afterward! Thanks

phaux · 2020-04-03T21:22:14Z

The problem is that the rule @typescript-eslint/internal/plugin-test-formatting crashes ESLint. It has something to do with a comment in interface Test { 'f!': </* a */>(/* b */ x: any /* c */) => void } in tests.

$ eslint . --ext .js,.ts
Error: Comment "a" was not printed. Please report this error!
Occurred while linting /home/runner/work/typescript-eslint/typescript-eslint/packages/eslint-plugin/tests/rules/method-signature-style.test.ts:10
    at /home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:13223:13
    at Array.forEach (<anonymous>)
    at ensureAllCommentsPrinted (/home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:13221:15)
    at coreFormat (/home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:13270:3)
    at format (/home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:13510:73)
    at formatWithCursor (/home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:13526:12)
    at /home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:44207:15
    at Object.format (/home/runner/work/typescript-eslint/typescript-eslint/node_modules/prettier/index.js:44226:12)
    at prettierFormat (/home/runner/work/typescript-eslint/typescript-eslint/packages/eslint-plugin-internal/dist/rules/plugin-test-formatting.js:120:35)
    at checkTemplateLiteral (/home/runner/work/typescript-eslint/typescript-eslint/packages/eslint-plugin-internal/dist/rules/plugin-test-formatting.js:284:31)
error Command failed with exit code 2.

bradzacher · 2020-04-03T21:26:51Z

Ahh fooey. This is an unhandled case in prettier that's fixed in v2.0, but we haven't upgraded to v2 yet (https://prettier.io/blog/2020/03/21/2.0.0.html#fix-printing-of-comments-in-empty-type-parameters-7729httpsgithubcomprettierprettierpull7729-by-sosukesuzukihttpsgithubcomsosukesuzuki)

You can get around this for now by adding the noFormat template tag to that test. This will instruct the lint rule to not run prettier on that test snippet.

example:

typescript-eslint/packages/eslint-plugin/tests/rules/no-unsafe-return.test.ts

Lines 77 to 82 in 188b689

    
                 code: noFormat` 
        
           function foo() { return (1 as any); } 
        
           function foo() { return Object.create(null); } 
        
           const foo = () => { return (1 as any) }; 
        
           const foo = () => Object.create(null); 
        
                 `,

bradzacher

thanks!

boris-petrov · 2020-04-06T18:02:47Z

Thanks for this nice check but how are we supposed to handle overloaded methods? I.e.:

interface T {
  func: (arg: string) => number;
  func: (arg1: number, arg2: object) => number;
}

This is not valid (duplicate key error by TypeScript).

bradzacher · 2020-04-06T18:12:05Z

You can do it via an intersection type:

interface T {
  func:
    & ((arg: string) => number)
    & ((arg1: number, arg2: object) => number);
}

repl

Under the hood this is essentially how TS does overloads.

phaux added 2 commits March 5, 2020 23:57

feat(eslint-plugin): method-signature-style

fd93d53

feat(eslint-plugin): [method-sig-style] add fixers

995a423

docs(eslint-plugin): [method-sig-style] add docs

be1067c

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Mar 6, 2020

test(eslint-plugin): [method-sig-style] more tests

cca3d68

bradzacher reviewed Mar 6, 2020

View reviewed changes

docs(eslint-plugin): docs

f42efe0

phaux requested a review from bradzacher March 20, 2020 16:49

Merge branch 'master' into method-signature-style

130781b

bradzacher requested changes Mar 23, 2020

View reviewed changes

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

feat(eslint-plugin): preserve comments in method fixer

291f2b0

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

bradzacher previously approved these changes Apr 3, 2020

View reviewed changes

Merge branch 'master' into method-signature-style

b9f505d

bradzacher added 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge awaiting response Issues waiting for a reply from the OP or another party labels Apr 3, 2020

Merge branch 'master' into method-signature-style

932ae62

style(eslint-plugin): add workaround for failing lint

f355e4e

phaux dismissed bradzacher’s stale review via f355e4e April 3, 2020 21:38

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 3, 2020

bradzacher approved these changes Apr 3, 2020

View reviewed changes

bradzacher merged commit c49d771 into typescript-eslint:master Apr 3, 2020

This was referenced Apr 9, 2020

[Snyk] Upgrade @typescript-eslint/parser from 2.26.0 to 2.27.0 bifravst/eslint-config-typescript#19

Merged

[Snyk] Upgrade @typescript-eslint/eslint-plugin from 2.26.0 to 2.27.0 bifravst/eslint-config-typescript#20

Merged

snyk-bot mentioned this pull request Apr 19, 2020

[Snyk] Upgrade @typescript-eslint/parser from 2.27.0 to 2.28.0 freaktechnik/eslint-plugin-array-func#54

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): new rule method-signature-style #1685

feat(eslint-plugin): new rule method-signature-style #1685

phaux commented Mar 6, 2020

typescript-eslint bot commented Mar 6, 2020

bradzacher left a comment

bradzacher Mar 6, 2020

phaux Mar 8, 2020

bradzacher left a comment

bradzacher Mar 23, 2020

phaux Mar 23, 2020

bradzacher left a comment

codecov bot commented Apr 3, 2020 •

edited

Loading

bradzacher commented Apr 3, 2020

phaux commented Apr 3, 2020

bradzacher commented Apr 3, 2020 •

edited

Loading

bradzacher left a comment

boris-petrov commented Apr 6, 2020

bradzacher commented Apr 6, 2020 •

edited

Loading

feat(eslint-plugin): new rule method-signature-style #1685

feat(eslint-plugin): new rule method-signature-style #1685

Conversation

phaux commented Mar 6, 2020

typescript-eslint bot commented Mar 6, 2020

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Mar 6, 2020

Choose a reason for hiding this comment

phaux Mar 8, 2020

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Mar 23, 2020

Choose a reason for hiding this comment

phaux Mar 23, 2020

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2020 • edited Loading

Codecov Report

bradzacher commented Apr 3, 2020

phaux commented Apr 3, 2020

bradzacher commented Apr 3, 2020 • edited Loading

bradzacher left a comment

Choose a reason for hiding this comment

boris-petrov commented Apr 6, 2020

bradzacher commented Apr 6, 2020 • edited Loading

codecov bot commented Apr 3, 2020 •

edited

Loading

bradzacher commented Apr 3, 2020 •

edited

Loading

bradzacher commented Apr 6, 2020 •

edited

Loading