Skip to content

[unbound-method] support an allowlist #2951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
3 tasks done
G-Rath opened this issue Jan 20, 2021 · 8 comments · Fixed by jest-community/eslint-plugin-jest#765
Closed
3 tasks done

[unbound-method] support an allowlist #2951

G-Rath opened this issue Jan 20, 2021 · 8 comments · Fixed by jest-community/eslint-plugin-jest#765
Labels
awaiting response Issues waiting for a reply from the OP or another party external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@G-Rath
Copy link
Contributor

G-Rath commented Jan 20, 2021

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/unbound-method": ["error", { "allowlist": ["expect"] }]
  }
}
import { mocked } from 'ts-jest/utils';

const mockAxios = mocked(axios, true);
expect(mockAxios.get).toHaveBeenCalledTimes(totalPages);

Expected Result

No ESLint Error.

Actual Result

An ESLint Error.

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.14.0
@typescript-eslint/parser 4.14.0
TypeScript 4.1.2
ESLint 7.15.0
node 14.2.2

I know this is technically a duplicate of #1776, but that's locked so I can't comment on that and I think this would be a useful addition that I'm happy to implement myself.

@bradzacher in response to your comment on the original issue:

Disabling the rule completely for test files means its completely disabled, whereas the allowlist lets us keep it enabled - we can also use it with overrides to target specific files (i.e just our test files).

While it's true we can use disable comments, they tend to stick out and for this rule in particular it just results in a lot of eslint-disable comments since its for a common testing pattern.

Finally, we can give it some nice context with a comment if we're using any config aside from json:

/** @type {import('eslint').Linter.Config} */
const config = {
  overrides: [
    {
      files: ['test/**'],
      extends: ['ackama/jest'],
      rules: {
        '@typescript-eslint/unbound-method': [
          'error',
          {
            allow: [
              // expect is used for test assertions, which require us to pass
              // seemingly unbound methods when testing if mocks were called
              'expect'
            ]
          }
        ]
      }
    }
  ]
};

module.exports = config;
@G-Rath G-Rath added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jan 20, 2021
@bradzacher
Copy link
Member

The problem here is that the rule is actually right for some cases - expect will actually call a passed function under certain circumstances.
For example expect(clazz.method).toThrow() will call the passed function with an incorrect this context.

Which is the inherent danger of an allowlist like this - it's contextless and can easily cause footguns.
IMO it's better to have a tighter API with some false-positives which a user can assess and disable as appropriate, than a looser API with some false-negatives that are invisible and then cause runtime errors. These are terrible erode user trust in the linting ecosystem as a whole, not just one lint rule ("if this rule missed this case, what other rules are missing cases...")

Don't get me wrong - I 100% understand that it's clunky and cumbersome to manually disable the rule around the place, but it's just a really hard problem to solve, and is an inherent problem with the dynamic nature of JS.

Do we need a solution? Yes.
As much as I prefer false-positives, they also erode user's trust, but in a different way ("I know this rule over-reports, so I can just ignore it.").

Do I have a good solution? Nope.
allowlists are not good because whilst we can say "hey make sure you limit the scope of your allowlist to ensure you don't cause false-negatives", there's no way to enforce that, which is a huge foot-gun and an API design smell. On the one hand - sure let users shoot themselves, they'll learn. But on the other hand, they'll raise an issue here when they encounter it (and usually with a barely filled out issue).


Perhaps a better solution is to fork this rule into something like eslint-plugin-jest, and teach it how to deeply analyse expect.
I.e. it could be taught that expect(clazz.method).toBeTruthy() is safe, but expect(clazz.method).toThrow() is unsafe.
Then a user could enable that version in test files (limiting the scope of it's logic), and enable our version everywhere else.

Unsure... Just a random thought.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Jan 20, 2021
@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 20, 2021

Perhaps a better solution is to fork this rule into something like eslint-plugin-jest,

I had the exact same though minutes after hitting "submit" 😅

Do you have any documentation or examples on how we could easily hook into the type info for our rules over at eslint-plugin-jest? I thought I remembered seeing some at some point but can't find it now (it might have been on a PR).

@bradzacher
Copy link
Member

For that idea - it could actually be as simple as forking the rule and adding some additional checks using ESTree AST to look for jest patterns and then just prevent the unbound-method logic from running.

eg:

if (isExpectToThrow(node) || isExpectNotToThrow(node)) {
  checkUnboundMethod();
} else {
  return;
}

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 22, 2021

So I've started forking the rule, but having trouble forking the tests - would you mind having a look?

Initially I got Parsers provided as strings to RuleTester must be absolute paths, which I resolved by using require.resolve to pass the parser property.

That then gave me The file does not match your project config: estree.ts which I resolved by adding files: ["estree.ts"] to fixtures/tsconfig.json.

Finally, I'm now having a couple of tests fail 😬

So far the majority of our setup seems to be the same, except for Leara. It could be because of version differences, but I'd have thought that'd make for some pretty flakey tests if patch difference versions in packages like @types/node had this much effect.

@rgant

This comment has been minimized.

@bradzacher
Copy link
Member

Initially I got Parsers provided as strings to RuleTester must be absolute paths, which I resolved by using require.resolve to pass the parser property.

That then gave me The file does not match your project config: estree.ts which I resolved by adding files: ["estree.ts"] to fixtures/tsconfig.json.

You want to use ESLintUtils.RuleTester instead of TSESLint.RuleTester.
The former includes utils and config to help do tests.
The later is just a typed version of the class and nothing more.

They're separate so that people don't get our stuff forced on them.

Finally, I'm now having a couple of tests fail 😬

I would have thought that if you just fork the rule and make no changes, it should all "just work".

@bradzacher bradzacher added external This issue is with another package, not typescript-eslint itself wontfix This will not be worked on labels Feb 18, 2021
@G-Rath
Copy link
Contributor Author

G-Rath commented Feb 18, 2021

I would have thought that if you just fork the rule and make no changes, it should all "just work".

As would I, but sadly no dice so far - I'll look into using ESLintUtils.RuleTester and see if that helps.

@G-Rath
Copy link
Contributor Author

G-Rath commented Mar 13, 2021

(Slightly surprised my PR actually closed this issue - I'm guessing probably because I was the author of both)

@bradzacher I've merged the new rule in eslint-plugin-jest - once we've lived with it for a bit to weed out any bugs, I'll make a PR updating unbound-methods docs to link the two together :)

bradzacher pushed a commit that referenced this issue Mar 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants