DataFlow: overlay-informed flow includes the diff

Author: jbj

shared/dataflow/codeql/dataflow/internal/DataFlowImplStage1.qll

@@ -144,19 +144,58 @@ module MakeImplStage1<LocationSig Location, InputSig<Location> Lang> {
         exists(node)
       }
 
+      /**
+       * Holds if `node` comes from a file that was in the diff for which we
+       * are producing results.
+       */
+      overlay[global]
+      private predicate isDiffFileNode(Node node) {
+        exists(string filePath |
+          node.getLocation().hasLocationInfo(filePath, _, _, _, _) and
+          AlertFiltering::fileIsInDiff(filePath)
+        )
+      }
+
       overlay[global]
       pragma[nomagic]
       private predicate isFilteredSource(Node source) {
         Config::isSource(source, _) and
-        if Config::observeDiffInformedIncrementalMode()
-        then AlertFiltering::filterByLocation(Config::getASelectedSourceLocation(source))
+        // Data flow is always incremental in one of two ways.
+        // 1. If the configuration is diff-informed and diff information is
+        //    available, we filter to only include nodes in the diff, which
+        //    gives the smallest set of nodes.
+        // 2. If not, in global evaluation with overlay, we filter to only
+        //    include nodes from files in the overlay or the diff; flow from
+        //    other nodes will be added back later. There can be two reasons
+        //    why we are in this case:
+        //    1. This could be the primary configuration for a query that
+        //       hasn't yet become diff-informed. In that case, the
+        //       `getASelectedSourceLocation` information is probably just the
+        //       default, and it's a fairly safe overapproximation to
+        //       effectively expand to all nodes in the file (via
+        //       `isDiffFileNode`).
+        //    2. This could be a secondary configuration, like a helper
+        //       configuration for finding sources or sinks of a primary
+        //       configuration. In that case, the results of this configuration
+        //       are typically in the same file as the final alert.
+        if
+          Config::observeDiffInformedIncrementalMode() and
+          AlertFiltering::diffInformationAvailable()
+        then AlertFiltering::locationIsInDiff(Config::getASelectedSourceLocation(source))
         else (
           // If we are in base-only global evaluation, do not filter out any sources.
           not isEvaluatingInOverlay()
           or
           // If we are in global evaluation with an overlay present, restrict
-          // sources to those visible in the overlay.
+          // sources to those visible in the overlay or
           isOverlayNode(source)
+          or
+          // those from files in the diff. The diff is a subset of the overlay
+          // in the common case, but this is not guaranteed. Including the diff here
+          // ensures that we re-evaluate flow that passes from a file in the
+          // diff (but in the base), through a file in the overlay with
+          // possibly important changes, back to the base.
+          isDiffFileNode(source)
         )
       }
 
@@ -167,15 +206,17 @@ module MakeImplStage1<LocationSig Location, InputSig<Location> Lang> {
           Config::isSink(sink, _) or
           Config::isSink(sink)
         ) and
-        if Config::observeDiffInformedIncrementalMode()
-        then AlertFiltering::filterByLocation(Config::getASelectedSinkLocation(sink))
+        // See the comments in `isFilteredSource` for the reasoning behind the following.
+        if
+          Config::observeDiffInformedIncrementalMode() and
+          AlertFiltering::diffInformationAvailable()
+        then AlertFiltering::locationIsInDiff(Config::getASelectedSinkLocation(sink))
         else (
-          // If we are in base-only global evaluation, do not filter out any sinks.
           not isEvaluatingInOverlay()
           or
-          // If we are in global evaluation with an overlay present, restrict
-          // sinks to those visible in the overlay.
           isOverlayNode(sink)
+          or
+          isDiffFileNode(sink)
         )
       }
 

shared/util/codeql/util/AlertFiltering.qll

@@ -82,6 +82,21 @@ module AlertFilteringImpl<LocationSig Location> {
     )
   }
 
+  /** Holds if diff information is available in this evaluation. */
+  predicate diffInformationAvailable() {
+    restrictAlertsTo(_, _, _) or restrictAlertsToExactLocation(_, _, _, _, _)
+  }
+
+  /**
+   * Holds if diff information is available, and `filePath` is in the diff
+   * range.
+   */
+  predicate fileIsInDiff(string filePath) {
+    restrictAlertsTo(filePath, _, _)
+    or
+    restrictAlertsToExactLocation(filePath, _, _, _, _)
+  }
+
   /**
    * Holds if the given location intersects the diff range. When that is the
    * case, ensuring that alerts mentioning this location are included in the
@@ -103,8 +118,17 @@ module AlertFilteringImpl<LocationSig Location> {
    */
   bindingset[location]
   predicate filterByLocation(Location location) {
-    not restrictAlertsTo(_, _, _) and not restrictAlertsToExactLocation(_, _, _, _, _)
+    not diffInformationAvailable()
     or
+    locationIsInDiff(location)
+  }
+
+  /**
+   * Like `filterByLocation`, except that if there is no diff range, this
+   * predicate never holds.
+   */
+  bindingset[location]
+  predicate locationIsInDiff(Location location) {
     exists(string filePath |
       restrictAlertsToEntireFile(filePath) and
       location.hasLocationInfo(filePath, _, _, _, _)