Whitelist option for `no-unbound-method` #4472

piotrgajow · 2019-01-20T21:22:37Z

PR checklist

Addresses an existing issue: no-unbound-method in Jasmine spy have been called test #3755
Enhancement
- Includes tests
Documentation update

Overview of change:

Added configuration option to whitelist methods and operators for no-unbound-method rule, so that it will not report errors in cases such as:

expect(x.method).toHaveBeenCalled()
typeof x.method === 'function'

Is there anything you'd like reviewers to focus on?

Please verify that options for this rule are backwards compatible.

CHANGELOG.md entry:

[new-rule-option] Added whitelist option to no-unbound-method

palantirtech · 2019-01-20T21:22:38Z

Thanks for your interest in palantir/tslint, @piotrgajow! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

JoshuaKGoldberg

Exciting!

src/rules/noUnboundMethodRule.ts

piotrgajow · 2019-01-23T12:04:07Z

I see that some tests are failing... Can anyone tell me what is the difference between test2.1, test2.4 and for example test2.7?

JoshuaKGoldberg · 2019-01-25T03:32:47Z

Hi @piotrgajow! The difference is which TypeScript version the tests are running against. TSLint supports all the versions listed in the tests, down to 2.1. It'd be a breaking change to drop support, even though it's a little annoying how some APIs have changed or added since then.

Suggestion: try installing TypeScript 2.1 locally and compile against it? It could be there's an API change that is missed. Feel free to tag me if that doesn't catch it and I can take a deeper look! This'd be a really great PR to get in.

piotrgajow · 2019-01-31T21:34:01Z

Hi @JoshuaKGoldberg, I have tried installing Typescript 2.1/2.4 locally, but then I didn't manage to run the tests - it fails during compilation phase with:

$ yarn compile:test && yarn test:rules
yarn run v1.10.1
$ tsc -p test
src/formatterLoader.ts(73,17): error TS1005: '(' expected.
src/formatterLoader.ts(89,13): error TS1005: '(' expected.
src/language/utils.ts(128,49): error TS1005: ',' expected.
src/language/utils.ts(128,53): error TS1005: ',' expected.
src/ruleLoader.ts(122,13): error TS1005: '(' expected.
src/rules/fileNameCasingRule.ts(62,13): error TS1005: '(' expected.
src/rules/noImplicitDependenciesRule.ts(159,17): error TS1005: '(' expected.
src/runner.ts(149,28): error TS1109: Expression expected.
src/runner.ts(149,34): error TS1134: Variable declaration expected.
src/utils.ts(292,13): error TS1005: '(' expected.
error Command failed with exit code 2.

But these files are not strictly related to changes from this PR...
Could you maybe direct me somehow?
Maybe I did something wrong?

I have installed typescipt with version 2.1, and then removed strictFunctionTypes and strictPropertyInitialization from tsconfig.json as compiler complained that these options are unknown.

JoshuaKGoldberg

Found a couple edge cases 😉

JoshuaKGoldberg · 2019-02-02T14:40:58Z

src/rules/noUnboundMethodRule.ts

+ return allowTypeof;
+ }
+ if (isCallExpression(node.parent)) {
+ const expression = node.parent.expression as ts.Identifier;


as ts.Identifier

It's actually possible for node.parent.expression to not be an identifier. You'll want to add test cases for:

(condition ? expectA : expectB)(c.method); (await someObject)(c.method); (await someMethod())(c.method);

I don't think we should worry about trying to allow these even if expectA and expectB are whitelisted. Suggestion: just don't run this check if the parent's expression isn't an identifier.

Ok, changed as you suggested. I maybe might include it in some future PR

JoshuaKGoldberg · 2019-02-02T14:41:49Z

src/rules/noUnboundMethodRule.ts

+ }
+ if (isCallExpression(node.parent)) {
+ const expression = node.parent.expression as ts.Identifier;
+ const callingMethodName = expression.escapedText as string;


expression.escapedText

Looks like escapedText is what's giving you trouble in in TS<=2.4. Switch to .text and they seem to work fine: https://circleci.com/gh/JoshuaKGoldberg/tslint/1267

Yes, that was the issue. Thanks!

JoshuaKGoldberg · 2019-02-02T14:43:40Z

AH, that's fun. I guess you'd have to compile in newer TS so it would understand your syntax, then run using a lower version TS. See comments in PR for resolution! 😊

- Ignore case when parent of call expression is not an identifier

piotrgajow · 2019-02-03T23:36:37Z

It seems that I finally managed to fix all of the issues 😃 .

JoshuaKGoldberg

🎊, thanks @piotrgajow!

This ended up being a tricker option to add than it looked, so +1 for getting this through! Very excited to not have to disable the rule in tests. 😄

fixing no-side-effect-code, deprecated, no-inferred-empty-object-type, no-unnecessary-type-assertion, restrict-plus-operands tslint errors. expect is added to the whitelist for no-unbound-method, because expect doesn't call function, see palantir/tslint#4472 for more details.

piotrgajow added 6 commits January 10, 2019 09:39

Add tests for unbound method whitelist

f3bfb0b

Extend no-unbound-method parameters

98c5864

Merge whitelist parameters into one

d584708

Handle whitelisting typeof expression

b67bdfb

Handle method reference in whitelisted function call

1e3c935

Update rule description and examples

91cc507

JoshuaKGoldberg suggested changes Jan 22, 2019

View reviewed changes

JoshuaKGoldberg added the PR: waiting for author label Jan 22, 2019

piotrgajow added 6 commits January 23, 2019 11:45

Fix rule description

541568e

Use is* type guard from tsutils

9cc9647

Change parameter types and parsing

b42389b

Fix error with addint elements to set

f27072b

Move handling of typeof to separate option

d51bf19

Fix linter issue

f378058

JoshuaKGoldberg suggested changes Feb 2, 2019

View reviewed changes

piotrgajow added 3 commits February 4, 2019 00:19

Use expression.text instead of expression.escapedText

71761ae

Fix for edge case

bdb09d3

- Ignore case when parent of call expression is not an identifier

Fix linter issue

7794931

JoshuaKGoldberg approved these changes Feb 3, 2019

View reviewed changes

JoshuaKGoldberg removed the PR: waiting for author label Feb 3, 2019

JoshuaKGoldberg merged commit 39201ac into palantir:master Feb 3, 2019

This was referenced Feb 3, 2019

no-unbound-method in Jasmine spy have been called test #3755

Closed

No-unbound-method warn when tested 'toHaveBeenCalled' in test #4500

Closed

Margar1ta mentioned this pull request Oct 10, 2019

chore(tabs): fixing tslint errors positive-js/mosaic#298

Merged

snyk-bot mentioned this pull request Oct 11, 2019

[Snyk] Upgrade tslint from 5.11.0 to 5.20.0 Narshe1412/nuarca-sql#21

Merged

snyk-bot mentioned this pull request Oct 26, 2019

[Snyk] Upgrade tslint from 5.12.1 to 5.20.0 ajesse11x/caprover#5

Open

snebjorn mentioned this pull request Apr 24, 2020

[unbound-method] False positive for Uncalled Function Checks typescript-eslint/typescript-eslint#1934

Closed

This was referenced Apr 1, 2021

API: Add option keep media to DELETE /notes/{note} hedgedoc/hedgedoc#1094

Merged

Split ESLint config for tests and code? hedgedoc/hedgedoc#1098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Whitelist option for `no-unbound-method` #4472

Whitelist option for `no-unbound-method` #4472

piotrgajow commented Jan 20, 2019

palantirtech commented Jan 20, 2019

JoshuaKGoldberg left a comment

piotrgajow commented Jan 23, 2019

JoshuaKGoldberg commented Jan 25, 2019

piotrgajow commented Jan 31, 2019

JoshuaKGoldberg left a comment

JoshuaKGoldberg Feb 2, 2019

piotrgajow Feb 3, 2019

JoshuaKGoldberg Feb 2, 2019

piotrgajow Feb 3, 2019

JoshuaKGoldberg commented Feb 2, 2019

piotrgajow commented Feb 3, 2019

JoshuaKGoldberg left a comment

Whitelist option for no-unbound-method #4472

Whitelist option for no-unbound-method #4472

Conversation

piotrgajow commented Jan 20, 2019

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Jan 20, 2019

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

piotrgajow commented Jan 23, 2019

JoshuaKGoldberg commented Jan 25, 2019

piotrgajow commented Jan 31, 2019

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 2, 2019

Choose a reason for hiding this comment

piotrgajow Feb 3, 2019

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 2, 2019

Choose a reason for hiding this comment

piotrgajow Feb 3, 2019

Choose a reason for hiding this comment

JoshuaKGoldberg commented Feb 2, 2019

piotrgajow commented Feb 3, 2019

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Whitelist option for `no-unbound-method` #4472

Whitelist option for `no-unbound-method` #4472