Merge pull request #1 from sutrkiller/features/state-and-props-access…

…-within-setstate

Features/state and props access within setstate

.gitignore

@@ -1,5 +1,9 @@
 # See http://help.github.com/ignore-files/ for more about ignoring files.
 
+# TSLint generated
+*.js
+*.js.map
+
 # dependencies
 node_modules
 

src/Sample.tsx

@@ -1,8 +1,32 @@
+import {isNullOrUndefined} from "util";
 class Class  {
+    private state = {
+        flag: true,
+        doNotAccessMeInSetState: () => null,
+    };
+
+    private props = {
+        doNotAccessMeInSetState: true,
+    };
+
     constructor() {
+        // Use functional setState instead
+        this.setState({obj: 123});
         this.setState({obj: 123});
         this.setState({obj: 123}, () => "callback");
 
+        // Do not access this.props nor this.state in setState
+        this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState}));
+        this.setState((_) => ({obj: 123, flag: !this.state.flag}));
+        this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}), () => "callback");
+
+        this.setState(({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}), () => "callback");
+
+        // This will probably be a false negative
+        const x = { flag: this.state.flag };
+        this.setState(x);
+
+        // These are okay
         this.setState((_) => ({obj: 123}));
         this.setState((prevState) => ({obj: prevState.obj}));
         this.setState((prevState, props) => ({obj: prevState.obj, prop: props.obj}));

src/rules/reactSetStateUsageOptions.ts

@@ -0,0 +1,13 @@
+export const OPTION_UPDATER_ONLY = "updater-only";
+
+export interface IOptions {
+    readonly updaterOnly: boolean;
+}
+
+export function parseOptions(ruleArguments: any[]): IOptions {
+    const updaterOnly = ruleArguments[0] as string;
+
+    return {
+        updaterOnly: updaterOnly === OPTION_UPDATER_ONLY,
+    };
+}

src/rules/reactSetStateUsageRule.ts

@@ -1,16 +1,26 @@
-import * as Lint from "tslint";
-import {isCallExpression, isPropertyAccessExpression} from "tsutils";
 import * as ts from "typescript";
+import * as Lint from "tslint";
+import { isObjectLiteralExpression } from "tsutils";
 
-const OPTION_UPDATER_ONLY = "updater-only";
+import {
+    IOptions,
+    OPTION_UPDATER_ONLY,
+    parseOptions,
+} from "./reactSetStateUsageOptions";
+import {
+    getFirstSetStateAncestor,
+    isThisPropertyAccess,
+    isThisSetState,
+    removeParentheses,
+} from "../utils/syntaxWalkerUtils";
 
-interface IOptions {
-    readonly updaterOnly: boolean;
-}
+const FAILURE_STRING = "Do not pass an object into setState. Use functional setState updater instead.";
+const FAILURE_STRING_UPDATER_ONLY = `Do not use callback parameter in setState. Use componentDidUpdate method instead (\"${OPTION_UPDATER_ONLY}\" switch).`;
+const getFailureStringForAccessedMember = (accessedMember: string) => `Do not access 'this.${accessedMember}' in setState. Use arguments from callback function instead.`;
 
 export class Rule extends Lint.Rules.AbstractRule {
     public static metadata: Lint.IRuleMetadata = {
-        description: "Requires the setState function to be called with function as the first argument.",
+        description: "Requires the setState function to be called with function as the first argument and without 'this.props' nor 'this.state' access within the function.",
         optionExamples: [true],
         options: {
             items: [
@@ -29,36 +39,52 @@ export class Rule extends Lint.Rules.AbstractRule {
         typescriptOnly: false,
     };
 
-    public static FAILURE_STRING = "Use functional setState instead of passing an object.";
-    public static FAILURE_STRING_UPDATER_ONLY = "Do not use callback parameter \"updater-only\" switch";
-
     public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
         const options = parseOptions(this.ruleArguments);
         return this.applyWithFunction(sourceFile, walk, options);
     }
+}
+
+function walk(ctx: Lint.WalkContext<IOptions>) {
+    const { sourceFile, options: { updaterOnly } } = ctx;
+
+    function cb(node: ts.Node): void {
+        if (isThisSetState(node)) {
+            inspectSetStateCall(node, ctx, updaterOnly);
+        }
+        else if (isThisState(node) || isThisProps(node)) {
+            inspectThisPropsOrStateContext(node, ctx);
+        }
+
+        return ts.forEachChild(node, cb);
+    }
 
+    return ts.forEachChild(sourceFile, cb);
 }
 
-function parseOptions(ruleArguments: any[]): IOptions {
-    const updaterOnly = ruleArguments[0] as string;
+function inspectSetStateCall(node: ts.CallExpression, ctx: Lint.WalkContext<IOptions>, updaterOnly: boolean) {
+    const { 0: updaterArgument, 1: callbackArgument, length: argumentsCount } = node.arguments;
 
-    return {
-        updaterOnly: updaterOnly === OPTION_UPDATER_ONLY,
-    };
+    // Forbid object literal
+    const bareUpdaterArgument = removeParentheses(updaterArgument);
+    if (isObjectLiteralExpression(bareUpdaterArgument)) {
+        ctx.addFailureAtNode(updaterArgument, FAILURE_STRING);
+    }
+
+    // Forbid second argument if updaterOnly flag set
+    if (updaterOnly && argumentsCount > 1) {
+        ctx.addFailureAtNode(callbackArgument, FAILURE_STRING_UPDATER_ONLY);
+    }
 }
 
-function walk(ctx: Lint.WalkContext<IOptions>) {
-    const {options: {updaterOnly}} = ctx;
-    return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void {
-        if (isCallExpression(node) && isPropertyAccessExpression(node.expression) &&
-            node.expression.name.text === "setState") {
-            if (node.arguments.some((arg) => arg.kind === ts.SyntaxKind.ObjectLiteralExpression)) {
-                ctx.addFailureAtNode(node.arguments[0], Rule.FAILURE_STRING);
-            }
-            if (updaterOnly && node.arguments.length > 1) {
-                ctx.addFailureAtNode(node.arguments[1], Rule.FAILURE_STRING_UPDATER_ONLY);
-            }
-        }
-        return ts.forEachChild(node, cb);
-    });
+function inspectThisPropsOrStateContext(node: ts.PropertyAccessExpression, ctx: Lint.WalkContext<IOptions>) {
+    const setStateCall = getFirstSetStateAncestor(node.parent);
+
+    if (setStateCall) {
+        ctx.addFailureAtNode(node, getFailureStringForAccessedMember(node.name.text));
+    }
 }
+
+const isThisState = (node: ts.Node): node is ts.PropertyAccessExpression => isThisPropertyAccess(node) && node.name.text === "state";
+
+const isThisProps = (node: ts.Node): node is ts.PropertyAccessExpression => isThisPropertyAccess(node) && node.name.text === "props";

src/utils/syntaxKindUtils.ts

@@ -0,0 +1,3 @@
+import * as ts from "typescript";
+
+export const isThisKeyword = (expression: ts.Expression) => expression.kind === ts.SyntaxKind.ThisKeyword;

src/utils/syntaxWalkerUtils.ts

@@ -0,0 +1,33 @@
+import * as ts from "typescript";
+import {
+    isCallExpression,
+    isPropertyAccessExpression,
+    isParenthesizedExpression,
+} from "tsutils";
+
+import { isThisKeyword } from "./syntaxKindUtils";
+
+export const isThisPropertyAccess = (node: ts.Node): node is ts.PropertyAccessExpression =>
+    isPropertyAccessExpression(node) && isThisKeyword(node.expression);
+
+export const isThisSetState = (node: ts.Node): node is ts.CallExpression =>
+    isCallExpression(node)
+    && isPropertyAccessExpression(node.expression)
+    && isThisKeyword(node.expression.expression)
+    && node.expression.name.text === "setState";
+
+export function getFirstSetStateAncestor(node: ts.Node): ts.CallExpression {
+    if (node.kind === ts.SyntaxKind.SourceFile) {
+        return null;
+    }
+
+    if (isThisSetState(node)) {
+        return node as ts.CallExpression;
+    }
+
+    return getFirstSetStateAncestor(node.parent);
+}
+
+export const removeParentheses = (node: ts.Node) => isParenthesizedExpression(node)
+    ? removeParentheses(node.expression)
+    : node;

test/rules/react-set-state-usage/default/test.ts.lint

@@ -1,7 +1,7 @@
 this.setState({obj: 123});
-              ~~~~~~~~~~   [Use functional setState instead of passing an object.]
+              ~~~~~~~~~~   [Do not pass an object into setState. Use functional setState updater instead.]
 this.setState({obj: 123}, () => "callback");
-              ~~~~~~~~~~   [Use functional setState instead of passing an object.]
+              ~~~~~~~~~~   [Do not pass an object into setState. Use functional setState updater instead.]
 
 this.setState((_) => ({obj: 123}));
 this.setState((prevState) => ({obj: prevState.obj}));
@@ -12,3 +12,23 @@ this.setState((_) => ({obj: 123}), () => "callback");
 this.setState((prevState) => ({obj: prevState.obj}), () => "callback");
 this.setState((prevState, props) => ({obj: prevState.obj, prop: props.obj}), () => "callback");
 this.setState((_, props) => ({prop: props.obj}), () => "callback");
+
+this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState}));
+                                       ~~~~~~~~~~  [Do not access 'this.props' in setState. Use arguments from callback function instead.]
+
+this.setState((_) => ({obj: 123, prop: this.state.doNotAccessMeInSetState}));
+                                       ~~~~~~~~~~  [Do not access 'this.state' in setState. Use arguments from callback function instead.]
+
+this.setState((_) => ({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState}));
+                                       ~~~~~~~~~~  [Do not access 'this.props' in setState. Use arguments from callback function instead.]
+                                                                                  ~~~~~~~~~~  [Do not access 'this.state' in setState. Use arguments from callback function instead.]
+
+this.setState(({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState})));
+              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  [Do not pass an object into setState. Use functional setState updater instead.]
+                                ~~~~~~~~~~  [Do not access 'this.props' in setState. Use arguments from callback function instead.]
+                                                                           ~~~~~~~~~~  [Do not access 'this.state' in setState. Use arguments from callback function instead.]
+
+this.setState(((({obj: 123, prop: this.props.doNotAccessMeInSetState, state: this.state.doNotAccessMeInSetState})))));
+              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  [Do not pass an object into setState. Use functional setState updater instead.]
+                                  ~~~~~~~~~~  [Do not access 'this.props' in setState. Use arguments from callback function instead.]
+                                                                             ~~~~~~~~~~  [Do not access 'this.state' in setState. Use arguments from callback function instead.]