JS: Add overlay annotations #20424
JS: Add overlay annotations #20424
Conversation
0f6a98c to
831b225
Compare
a3235f5 to
5d28d57
Compare
3070570 to
84e7101
Compare
Locally these seem to get rid of the compilation warnings, but of course CI is the true arbiter here.
In this case we actually want magic to apply, but was prevented by locality.
Use manual recursion instead.
25f4afb to
c7341f2
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds overlay annotations to make the AST, CFG, SSA, and DataFlow::Node layers local in the JavaScript CodeQL library, improving incremental evaluation performance by 21% while maintaining 100% accuracy.
Key changes:
- Addition of
overlay[local]andoverlay[global]annotations throughout the codebase - Refactoring of several predicates for better performance
- Introduction of helper predicates to optimize evaluation
Reviewed Changes
Copilot reviewed 110 out of 110 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Multiple .qll files |
Added overlay[local] module; annotations to make modules local |
Multiple .qll files |
Added overlay[global], overlay[caller], and overlay[caller?] annotations to specific predicates |
DuplicateProperty.ql |
Refactored duplicate detection logic with new helper predicates |
ForOfLoops.qll |
Extracted array index content sets into helper predicates |
BarrierGuards.qll |
Refactored logical operator handling into unified predicate |
CallGraphs.qll |
Extracted object usage check into separate predicate |
LazyCache.qll |
Changed from .flow() to DataFlow::valueNode() |
Regexp.qll |
Added helper predicates for type checking |
Expr.qll |
Refactored enclosing statement logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Holds if there is a step from `pred` to `succ` through a call to an identity function. | ||
| */ | ||
| overlay[local] | ||
| cached | ||
| predicate identityFunctionStep(DataFlow::Node pred, DataFlow::CallNode succ) { | ||
| exists(DataFlow::GlobalVarRefNode global | | ||
| global.getName() = "Object" and | ||
| succ.(DataFlow::MethodCallNode).calls(global, ["freeze", "seal"]) and | ||
| pred = succ.getArgument(0) | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Just curious: why was this predicate moved out of the CachedSteps module?
There was a problem hiding this comment.
Good question. It doesn't seem to make a difference anymore.
| or | ||
| exists(any(DataFlow::ClassNode cls)) | ||
| or | ||
| // exists(any(DataFlow::ClassNode cls)) // Depends on AnalyzedNode |
There was a problem hiding this comment.
Given the "DON'T USE!" comment on this predicate, I assume it's a deliberate choice to leave this commented-out, but just mentioning it in case you had intended to come back and fix/uncomment it.
There was a problem hiding this comment.
This was just an oversight. I'll remove the out-commented code.
| or | ||
| yaml_locations(node, loc) | ||
| or | ||
| xmllocations(node, loc) |
There was a problem hiding this comment.
@kaspersv wrote the XML discard predicate for Java such that:
- it could cope both with the current non-overlay-aware XML extraction, which re-extracts all XML files, and a future version that is overlay-aware.
- it doesn't discard (XML)
@files or@xmlnamespaces.
I suspect we need something similar here, for XML, YAML, and JSON.
There was a problem hiding this comment.
Interesting.
The set of JSON, YAML, and HTML files to extract are picked out by the JS auto builder, so they respect overlay-changes. HTML reuses the XML dbscheme.
The JS auto-builder doesn't call the XML extractor unless some LGTM_ environment variables are explicitly set, but it seems worth fixing just in case.
There was a problem hiding this comment.
Actually the amount of code duplication needed to make that work is bothering me. Would you mind if we do this in a follow-up PR? I'd like to share the discard predicates via synchronized-files, but it's a bit much to roll into this PR.
Adds overlay annotations to make the
AST,CFG,SSA, andDataFlow::Nodelayers local, along with a lot of other predicates spread across the codebase that also needed to become local.Evaluations compared to baseline, both sides with overlay and diff-informed. This shows the effects of this PR.
Evaluations of incremental versus non-incremental, both at the head of this PR. This shows the effects of incremental in general, including contributions from prior work.
Couple notes about the evaluations:
The result count accuracy is 100%, but the tuple count accuracy is less in a few cases, due to some missing discarding of JSON elements. Currently re-running with updated discard predicates.. Fixed now.