Merge pull request github#12792 from asgerf/js/redux-model-perf

asgerf · web-flow · commit b819f552039d · 2023-04-12T14:09:59.000+02:00
JS: add getForwardingFunction and use to sharpen useSelector model
diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll
@@ -347,6 +347,46 @@ module API {
       result = this.getASuccessor(Label::promisedError())
     }
 
+    /**
+     * Gets a node representing a function that is a wrapper around the function represented by this node.
+     *
+     * Concretely, a function that forwards all its parameters to a call to `f` and returns the result of that call
+     * is considered a wrapper around `f`.
+     *
+     * Examples:
+     * ```js
+     * function f(x) {
+     *   return g(x); // f = g.getForwardingFunction()
+     * }
+     *
+     * function doExec(x) {
+     *   console.log(x);
+     *   return exec(x); // doExec = exec.getForwardingFunction()
+     * }
+     *
+     * function doEither(x, y) {
+     *   if (x > y) {
+     *     return foo(x, y); // doEither = foo.getForwardingFunction()
+     *   } else {
+     *     return bar(x, y); // doEither = bar.getForwardingFunction()
+     *   }
+     * }
+     *
+     * function wrapWithLogging(f) {
+     *   return (x) => {
+     *     console.log(x);
+     *     return f(x); // f.getForwardingFunction() = anonymous arrow function
+     *   }
+     * }
+     * wrapWithLogging(g); // g.getForwardingFunction() = wrapWithLogging(g)
+     * ```
+     */
+    cached
+    Node getForwardingFunction() {
+      Stages::ApiStage::ref() and
+      result = this.getASuccessor(Label::forwardingFunction())
+    }
+
     /**
      * Gets any class that has this value as a decorator.
      *
@@ -901,6 +941,9 @@ module API {
           or
           lbl = Label::return() and
           ref = pred.getAnInvocation()
+          or
+          lbl = Label::forwardingFunction() and
+          DataFlow::functionForwardingStep(pred.getALocalUse(), ref)
         )
         or
         exists(DataFlow::Node def, DataFlow::FunctionNode fn |
@@ -1431,6 +1474,9 @@ module API {
     /** Gets the `return` edge label. */
     LabelReturn return() { any() }
 
+    /** Gets the label representing a function wrapper that forwards to an underlying function. */
+    LabelForwardingFunction forwardingFunction() { any() }
+
     /** Gets the `promised` edge label connecting a promise to its contained value. */
     LabelPromised promised() { any() }
 
@@ -1483,6 +1529,7 @@ module API {
         MkLabelDecoratedClass() or
         MkLabelDecoratedMember() or
         MkLabelDecoratedParameter() or
+        MkLabelForwardingFunction() or
         MkLabelEntryPoint(API::EntryPoint e)
 
       /** A label for an entry-point. */
@@ -1566,6 +1613,11 @@ module API {
         override string toString() { result = "getReceiver()" }
       }
 
+      /** A label for a function that is a wrapper around another function. */
+      class LabelForwardingFunction extends ApiLabel, MkLabelForwardingFunction {
+        override string toString() { result = "getForwardingFunction()" }
+      }
+
       /** A label for a class decorated by the current value. */
       class LabelDecoratedClass extends ApiLabel, MkLabelDecoratedClass {
         override string toString() { result = "getADecoratedClass()" }
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Redux.qll b/javascript/ql/lib/semmle/javascript/frameworks/Redux.qll
@@ -985,7 +985,9 @@ module Redux {
    */
   private module ReactRedux {
     /** Gets an API node referring to the `useSelector` function. */
-    API::Node useSelector() { result = API::moduleImport("react-redux").getMember("useSelector") }
+    API::Node useSelector() {
+      result = API::moduleImport("react-redux").getMember("useSelector").getForwardingFunction*()
+    }
 
     /**
      * A step out of a `useSelector` call, such as from `state.x` to the result of `useSelector(state => state.x)`.
@@ -994,7 +996,7 @@ module Redux {
       override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
         exists(API::CallNode call |
           call = useSelector().getACall() and
-          pred = call.getParameter(0).getReturn().asSink() and
+          pred = call.getCallback(0).getReturnNode() and
           succ = call
         )
       }
@@ -1231,4 +1233,9 @@ module Redux {
       }
     }
   }
+
+  /** For testing only. */
+  module Internal {
+    predicate getRootStateAccessPath = rootStateAccessPath/1;
+  }
 }
diff --git a/javascript/ql/lib/semmle/javascript/internal/CachedStages.qll b/javascript/ql/lib/semmle/javascript/internal/CachedStages.qll
@@ -277,6 +277,7 @@ module Stages {
             .getUnknownMember()
             .getInstance()
             .getReceiver()
+            .getForwardingFunction()
             .getPromisedError()
             .getADecoratedClass()
             .getADecoratedMember()
diff --git a/javascript/ql/test/library-tests/frameworks/Redux/accessPaths.js b/javascript/ql/test/library-tests/frameworks/Redux/accessPaths.js
@@ -0,0 +1,15 @@
+import { useSelector } from 'react-redux';
+
+function useSelectorWrapped(fn) {
+    return useSelector(fn);
+}
+
+function MyComponent(props) {
+    const x1 = useSelectorWrapped(state => state.x1);
+    const x2 = useSelectorWrapped(state => state.x2);
+    const x3 = useSelectorWrapped(state => state.x3);
+    const x4 = useSelectorWrapped(state => state.x4);
+    const x5 = useSelectorWrapped(state => state.x5);
+
+    return <span>X</span>;
+}
diff --git a/javascript/ql/test/library-tests/frameworks/Redux/test.expected b/javascript/ql/test/library-tests/frameworks/Redux/test.expected
@@ -113,9 +113,11 @@ taintFlow
 | react-redux.jsx:69:31:69:38 | source() | react-redux.jsx:76:10:76:36 | props.p ... Action3 |
 | react-redux.jsx:70:30:70:37 | source() | react-redux.jsx:77:10:77:28 | props.propFromAsync |
 reactComponentRef
+| accessPaths.js:7:1:15:1 | functio ... pan>;\\n} | accessPaths.js:7:1:15:1 | functio ... pan>;\\n} |
 | react-redux.jsx:64:1:80:1 | functio ... r}}/>\\n} | react-redux.jsx:64:1:80:1 | functio ... r}}/>\\n} |
 | react-redux.jsx:64:1:80:1 | functio ... r}}/>\\n} | react-redux.jsx:94:28:94:84 | connect ... ponent) |
 | react-redux.jsx:64:1:80:1 | functio ... r}}/>\\n} | react-redux.jsx:97:12:97:12 | c |
+ambiguousAccessPath
 getAffectedStateAccessPath
 | react-redux.jsx:12:33:17:9 | (state, ...       } | toolkit |
 | react-redux.jsx:18:41:23:9 | (state, ...       } | toolkit |
@@ -163,3 +165,24 @@ reducerToStateStep
 | react-redux.jsx:35:45:35:58 | action.payload | react-redux.jsx:86:31:86:54 | state.m ... alValue |
 | react-redux.jsx:39:42:39:55 | action.payload | react-redux.jsx:87:32:87:56 | state.m ... lValue2 |
 | react-redux.jsx:44:27:46:14 | [1, 2,  ...      }) | react-redux.jsx:88:32:88:56 | state.m ... lValue3 |
+getRootStateAccessPath
+| manual | react-redux.jsx:86:31:86:42 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("manual") |
+| manual | react-redux.jsx:87:32:87:43 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("manual") |
+| manual | react-redux.jsx:88:32:88:43 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("manual") |
+| manual.manualValue | react-redux.jsx:86:31:86:54 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("manual").getMember("manualValue") |
+| manual.manualValue2 | react-redux.jsx:87:32:87:56 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("manual").getMember("manualValue2") |
+| manual.manualValue3 | react-redux.jsx:88:32:88:56 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("manual").getMember("manualValue3") |
+| toolkit | react-redux.jsx:84:32:84:44 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("toolkit") |
+| toolkit | react-redux.jsx:85:24:85:36 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("toolkit") |
+| toolkit.asyncValue | react-redux.jsx:85:24:85:47 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("toolkit").getMember("asyncValue") |
+| toolkit.value | react-redux.jsx:84:32:84:50 | use entryPoint("react-redux-connect").getParameter(0).getParameter(0).getMember("toolkit").getMember("value") |
+| x1 | accessPaths.js:8:16:8:52 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getForwardingFunction().getReturn() |
+| x1 | accessPaths.js:8:44:8:51 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getParameter(0).getParameter(0).getMember("x1") |
+| x2 | accessPaths.js:9:16:9:52 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getForwardingFunction().getReturn() |
+| x2 | accessPaths.js:9:44:9:51 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getParameter(0).getParameter(0).getMember("x2") |
+| x3 | accessPaths.js:10:16:10:52 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getForwardingFunction().getReturn() |
+| x3 | accessPaths.js:10:44:10:51 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getParameter(0).getParameter(0).getMember("x3") |
+| x4 | accessPaths.js:11:16:11:52 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getForwardingFunction().getReturn() |
+| x4 | accessPaths.js:11:44:11:51 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getParameter(0).getParameter(0).getMember("x4") |
+| x5 | accessPaths.js:12:16:12:52 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getForwardingFunction().getReturn() |
+| x5 | accessPaths.js:12:44:12:51 | use moduleImport("react-redux").getMember("exports").getMember("useSelector").getParameter(0).getParameter(0).getMember("x5") |
diff --git a/javascript/ql/test/library-tests/frameworks/Redux/test.ql b/javascript/ql/test/library-tests/frameworks/Redux/test.ql
@@ -63,3 +63,10 @@ query predicate taintFlow(DataFlow::Node source, DataFlow::Node sink) {
 query DataFlow::SourceNode reactComponentRef(ReactComponent component) {
   result = component.getAComponentCreatorReference()
 }
+
+query predicate ambiguousAccessPath(API::Node node, string path) {
+  count(string accessPath | Redux::Internal::getRootStateAccessPath(accessPath) = node) > 1 and
+  Redux::Internal::getRootStateAccessPath(path) = node
+}
+
+query predicate getRootStateAccessPath = Redux::Internal::getRootStateAccessPath/1;

Original file line number	Diff line number	Diff line change
`@@ -985,7 +985,9 @@ module Redux {`
`985`	`985`	`*/`
`986`	`986`	`private module ReactRedux {`
`987`	`987`	/** Gets an API node referring to the `useSelector` function. */
`988`		`- API::Node useSelector() { result = API::moduleImport("react-redux").getMember("useSelector") }`
	`988`	`+ API::Node useSelector() {`
	`989`	`+ result = API::moduleImport("react-redux").getMember("useSelector").getForwardingFunction*()`
	`990`	`+ }`
`989`	`991`
`990`	`992`	`/**`
`991`	`993`	* A step out of a `useSelector` call, such as from `state.x` to the result of `useSelector(state => state.x)`.
`@@ -994,7 +996,7 @@ module Redux {`
`994`	`996`	`override predicate step(DataFlow::Node pred, DataFlow::Node succ) {`
`995`	`997`	`exists(API::CallNode call \|`
`996`	`998`	`call = useSelector().getACall() and`
`997`		`- pred = call.getParameter(0).getReturn().asSink() and`
	`999`	`+ pred = call.getCallback(0).getReturnNode() and`
`998`	`1000`	`succ = call`
`999`	`1001`	`)`
`1000`	`1002`	`}`
`@@ -1231,4 +1233,9 @@ module Redux {`
`1231`	`1233`	`}`
`1232`	`1234`	`}`
`1233`	`1235`	`}`
	`1236`	`+`
	`1237`	`+ /** For testing only. */`
	`1238`	`+ module Internal {`
	`1239`	`+ predicate getRootStateAccessPath = rootStateAccessPath/1;`
	`1240`	`+ }`
`1234`	`1241`	`}`