DataFlow: Run overlay-informed if not diff-informed #19857
Conversation
To ensure good performance, always run data flow overlay-informed unless the configuration has opted in to being diff-informed. This change affects only databases with an overlay and therefore has no immediate production consequences.
There was a problem hiding this comment.
Pull Request Overview
This PR defaults data flow to overlay-informed mode unless diff-informed is explicitly enabled, affecting only databases with overlays.
- Adds
isOverlayNodepredicate and updates source/sink filters in Stage 1 to respect overlay evaluation. - Introduces a default
isEvaluatingInOverlaypredicate in the core DataFlow signature. - Provides a Java-specific override of
isEvaluatingInOverlayin the Java implementation.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| shared/dataflow/codeql/dataflow/internal/DataFlowImplStage1.qll | Adds isOverlayNode and updates isFilteredSource/isFilteredSink to conditionally filter by overlay. |
| shared/dataflow/codeql/dataflow/DataFlow.qll | Declares default isEvaluatingInOverlay predicate stub for overlay mode detection. |
| java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImplSpecific.qll | Imports Java Overlay support and overrides isEvaluatingInOverlay. |
Comments suppressed due to low confidence (1)
java/ql/lib/semmle/code/java/dataflow/internal/DataFlowImplSpecific.qll:34
- The override for
isEvaluatingInOverlaydoesn't match the signature (missing()). It should be declared aspredicate isEvaluatingInOverlay() { isOverlay() }to properly override the default.
predicate isEvaluatingInOverlay = isOverlay/0;
| private predicate isOverlayNode(Node node) { | ||
| isEvaluatingInOverlay() and | ||
| // Any local node is an overlay node if we are evaluating in overlay mode | ||
| node = node |
There was a problem hiding this comment.
The predicate isOverlayNode uses a tautology (node = node), so it doesn't actually distinguish overlay-visible nodes. Replace it with a predicate that checks the node's overlay locality (e.g., isLocalNode(node)).
Copilot uses AI. Check for mistakes.
| * overlay-only local evaluation. When called from a global predicate, this | ||
| * predicate holds if we are evaluating globally with overlay and base both | ||
| * visible. | ||
| */ |
There was a problem hiding this comment.
The default isEvaluatingInOverlay predicate lacks the required overlay[local] annotation, so overlay-local evaluations won't see it. Add overlay[local] to its definition.
| */ | |
| */ | |
| overlay[local] |
Copilot uses AI. Check for mistakes.
3c2c871 to
0ee6a78
Compare
| private predicate isOverlayNode(Node node) { | ||
| isEvaluatingInOverlay() and | ||
| // Any local node is an overlay node if we are evaluating in overlay mode | ||
| node = node |
There was a problem hiding this comment.
The canonical way to write this is exists(node).
| * This predicate needs to be `overlay[local?]`, either directly or | ||
| * through annotations from an outer scope. If `Node` is global for the | ||
| * language under analysis, then every node is considered an overlay | ||
| * node, which means there will effectively be no overlay-based | ||
| * filtering of sources and sinks. | ||
| */ | ||
| private predicate isOverlayNode(Node node) { |
There was a problem hiding this comment.
Why not give this predicate an explicit overlay annotation now?
There was a problem hiding this comment.
The compiler gives a warning that it's redundant because the file module is also annotated overlay[local?].
To ensure good performance, always run data flow overlay-informed unless the configuration has opted in to being diff-informed. This change affects only databases with an overlay and therefore has no immediate production consequences.
This is my attempt to implement what @aschackmull sketched yesterday, on top of @kaspersv's branches.