Whitelist option for no-unbound-method (#4472)

* Add tests for unbound method whitelist

* Extend no-unbound-method parameters

* Merge whitelist parameters into one

* Handle whitelisting typeof expression

* Handle method reference in whitelisted function call

* Update rule description and examples

* Fix rule description

* Use is* type guard from tsutils

* Change parameter types and parsing

* Fix error with addint elements to set

* Move handling of typeof to separate option

* Fix linter issue

* Use expression.text instead of expression.escapedText

* Fix for edge case

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

* Fix linter issue

src/rules/noUnboundMethodRule.ts

@@ -15,28 +15,78 @@
  * limitations under the License.
  */
 
-import { hasModifier, isPropertyAccessExpression } from "tsutils";
+import {
+ hasModifier,
+ isCallExpression,
+ isIdentifier,
+ isPropertyAccessExpression,
+ isTypeOfExpression,
+} from "tsutils";
 import * as ts from "typescript";
 
 import * as Lint from "../index";
 
 const OPTION_IGNORE_STATIC = "ignore-static";
+const OPTION_WHITELIST = "whitelist";
+const OPTION_ALLOW_TYPEOF = "allow-typeof";
+
+const OPTION_WHITELIST_EXAMPLE = [
+ true,
+ {
+ [OPTION_IGNORE_STATIC]: true,
+ [OPTION_WHITELIST]: ["expect"],
+ [OPTION_ALLOW_TYPEOF]: true,
+ },
+];
 
 interface Options {
+ allowTypeof: boolean;
  ignoreStatic: boolean;
+ whitelist: Set<string>;
+}
+
+interface OptionsInput {
+ [OPTION_ALLOW_TYPEOF]?: boolean;
+ [OPTION_IGNORE_STATIC]?: boolean;
+ [OPTION_WHITELIST]?: string[];
 }
 
 export class Rule extends Lint.Rules.TypedRule {
  /* tslint:disable:object-literal-sort-keys */
  public static metadata: Lint.IRuleMetadata = {
  ruleName: "no-unbound-method",
  description: "Warns when a method is used outside of a method call.",
- optionsDescription: `You may optionally pass "${OPTION_IGNORE_STATIC}" to ignore static methods.`,
+ optionsDescription: Lint.Utils.dedent`
+ You may additionally pass "${OPTION_IGNORE_STATIC}" to ignore static methods, or an options object.
+ 
+ The object may have three properties:
+ 
+ * "${OPTION_IGNORE_STATIC}" - to ignore static methods.
+ * "${OPTION_ALLOW_TYPEOF}" - ignore methods referenced in a typeof expression.
+ * "${OPTION_WHITELIST}" - ignore method references in parameters of specifed function calls.
+ 
+ `,
  options: {
- type: "string",
- enum: [OPTION_IGNORE_STATIC],
+ anyOf: [
+ {
+ type: "string",
+ enum: [OPTION_IGNORE_STATIC],
+ },
+ {
+ type: "object",
+ properties: {
+ [OPTION_ALLOW_TYPEOF]: { type: "boolean" },
+ [OPTION_IGNORE_STATIC]: { type: "boolean" },
+ [OPTION_WHITELIST]: {
+ type: "array",
+ items: { type: "string" },
+ minLength: 1,
+ },
+ },
+ },
+ ],
  },
- optionExamples: [true, [true, OPTION_IGNORE_STATIC]],
+ optionExamples: [true, [true, OPTION_IGNORE_STATIC], OPTION_WHITELIST_EXAMPLE],
  rationale: Lint.Utils.dedent`
  Class functions don't preserve the class scope when passed as standalone variables.
  For example, this code will log the global scope (\`window\`/\`global\`), not the class instance:
@@ -88,20 +138,46 @@ export class Rule extends Lint.Rules.TypedRule {
  return this.applyWithFunction(
  sourceFile,
  walk,
- {
- ignoreStatic: this.ruleArguments.indexOf(OPTION_IGNORE_STATIC) !== -1,
- },
+ parseArguments(this.ruleArguments),
  program.getTypeChecker(),
  );
  }
 }
 
+function parseArguments(args: Array<string | OptionsInput>): Options {
+ const options: Options = {
+ allowTypeof: false,
+ ignoreStatic: false,
+ whitelist: new Set(),
+ };
+
+ for (const arg of args) {
+ if (typeof arg === "string") {
+ if (arg === OPTION_IGNORE_STATIC) {
+ options.ignoreStatic = true;
+ }
+ } else {
+ options.allowTypeof = arg[OPTION_ALLOW_TYPEOF] || false;
+ options.ignoreStatic = arg[OPTION_IGNORE_STATIC] || false;
+ options.whitelist = new Set(arg[OPTION_WHITELIST]);
+ }
+ }
+
+ return options;
+}
+
 function walk(ctx: Lint.WalkContext<Options>, tc: ts.TypeChecker) {
  return ts.forEachChild(ctx.sourceFile, function cb(node): void {
  if (isPropertyAccessExpression(node) && !isSafeUse(node)) {
  const symbol = tc.getSymbolAtLocation(node);
  const declaration = symbol === undefined ? undefined : symbol.valueDeclaration;
- if (declaration !== undefined && isMethod(declaration, ctx.options.ignoreStatic)) {
+
+ const isMethodAccess =
+ declaration !== undefined && isMethod(declaration, ctx.options.ignoreStatic);
+ const shouldBeReported =
+ isMethodAccess &&
+ !isWhitelisted(node, ctx.options.whitelist, ctx.options.allowTypeof);
+ if (shouldBeReported) {
  ctx.addFailureAtNode(node, Rule.FAILURE_STRING);
  }
  }
@@ -151,3 +227,15 @@ function isSafeUse(node: ts.Node): boolean {
  return false;
  }
 }
+
+function isWhitelisted(node: ts.Node, whitelist: Set<string>, allowTypeof: boolean): boolean {
+ if (isTypeOfExpression(node.parent)) {
+ return allowTypeof;
+ }
+ if (isCallExpression(node.parent) && isIdentifier(node.parent.expression)) {
+ const expression = node.parent.expression;
+ const callingMethodName = expression.text;
+ return whitelist.has(callingMethodName);
+ }
+ return false;
+}

test/rules/no-unbound-method/whitelist/test.tsx.lint

@@ -0,0 +1,72 @@
+class C {
+ method(x: number) {}
+ property: () => void;
+ template(strs: TemplateStringsArray, x: any) {}
+}
+
+const c = new C();
+[0].forEach(c.method);
+ ~~~~~~~~ [0]
+[0].forEach(x => c.method(x));
+[0].forEach(c.property);
+
+c.template;
+~~~~~~~~~~ [0]
+c.template`foo${0}`;
+String.raw`${c.template}`;
+ ~~~~~~~~~~ [0]
+
+expect(c.method).toHaveBeenCalled();
+typeof c.method;
+
+test(c.method);
+ ~~~~~~~~ [0]
+
+interface I {
+ foo(): void;
+ bar: () => void;
+}
+declare var i: I;
+i.foo;
+~~~~~ [0]
+i.bar;
+
+c.method === i.foo;
+
+// OK in condition
+c.method ? 1 : 2;
+1 ? c.method : c.method;
+ ~~~~~~~~ [0]
+ ~~~~~~~~ [0]
+if (c.method) {}
+while (c.method) {}
+do {} while (c.method);
+for (c.method; c.method; c.method) {}
+
+
+[0].forEach(c.method || i.foo);
+ ~~~~~~~~ [0]
+ ~~~~~ [0]
+[0].forEach(c.method.bind(c));
+
+<button onClick={c.method}>Click me!</button>;
+ ~~~~~~~~ [0]
+
+class Validators {
+ static required() {
+ return null;
+ }
+ static compose(...args: Function[]) {}
+}
+
+Validators.compose(Validators.required);
+ ~~~~~~~~~~~~~~~~~~~ [0]
+
+(condition ? expectA : expectB)(c.method);
+ ~~~~~~~~ [0]
+(await someObject)(c.method);
+ ~~~~~~~~ [0]
+(await someMethod())(c.method);
+ ~~~~~~~~ [0]
+
+[0]: Avoid referencing unbound methods which may cause unintentional scoping of 'this'.

test/rules/no-unbound-method/whitelist/tsconfig.json

@@ -0,0 +1,5 @@
+{
+ "compilerOptions": {
+ "module": "commonjs"
+ }
+}

test/rules/no-unbound-method/whitelist/tslint.json

@@ -0,0 +1,8 @@
+{
+ "linterOptions": {
+ "typeCheck": true
+ },
+ "rules": {
+ "no-unbound-method": [true, { "whitelist": ["expect"], "allow-typeof": true }]
+ }
+}