Skip to content

JS: Better type-tracking through Promise.all() #19412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions javascript/ql/lib/semmle/javascript/ApiGraphs.qll
Original file line number Diff line number Diff line change
Expand Up @@ -850,10 +850,10 @@ module API {
)
or
lbl = Label::promised() and
PromiseFlow::storeStep(rhs, pred, Promises::valueProp())
SharedTypeTrackingStep::storeStep(rhs, pred, Promises::valueProp())
or
lbl = Label::promisedError() and
PromiseFlow::storeStep(rhs, pred, Promises::errorProp())
SharedTypeTrackingStep::storeStep(rhs, pred, Promises::errorProp())
or
// The return-value of a getter G counts as a definition of property G
// (Ordinary methods and properties are handled as PropWrite nodes)
Expand Down Expand Up @@ -1008,11 +1008,11 @@ module API {
propDesc = ""
)
or
PromiseFlow::loadStep(pred.getALocalUse(), ref, Promises::valueProp()) and
SharedTypeTrackingStep::loadStep(pred.getALocalUse(), ref, Promises::valueProp()) and
lbl = Label::promised() and
(propDesc = Promises::valueProp() or propDesc = "")
or
PromiseFlow::loadStep(pred.getALocalUse(), ref, Promises::errorProp()) and
SharedTypeTrackingStep::loadStep(pred.getALocalUse(), ref, Promises::errorProp()) and
lbl = Label::promisedError() and
(propDesc = Promises::errorProp() or propDesc = "")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

private import javascript
private import semmle.javascript.dataflow.FlowSummary
private import semmle.javascript.dataflow.TypeTracking
private import FlowSummaryUtil

DataFlow::SourceNode promiseConstructorRef() {
Expand Down Expand Up @@ -211,12 +212,57 @@ private class PromiseReject extends SummarizedCallable {
}
}

/**
* A call to `Promise.all()`.
*/
class PromiseAllCall extends DataFlow::CallNode {
PromiseAllCall() { this = promiseConstructorRef().getAMemberCall("all") }

/** Gets the source of the input array */
DataFlow::ArrayCreationNode getInputArray() { result = this.getArgument(0).getALocalSource() }

/** Gets the `n`th element of the input array */
DataFlow::Node getNthInput(int n) { result = this.getInputArray().getElement(n) }

/** Gets a reference to the output array. */
DataFlow::SourceNode getOutputArray() {
exists(AwaitExpr await |
this.flowsToExpr(await.getOperand()) and
result = await.flow()
)
or
Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider adding a comment to explain that this fallback branch captures the Promise resolution callback when an AwaitExpr is not present, which aids future maintainability.

Suggested change
or
or
// Fallback: Captures the Promise resolution callback when an AwaitExpr is not present.

Copilot uses AI. Check for mistakes.

result = this.getAMethodCall("then").getCallback(0).getParameter(0)
}

/** Gets the `n`th output */
DataFlow::SourceNode getNthOutput(int n) {
exists(string prop |
Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider adding an inline comment clarifying that the property names read from the output array are expected to be numeric strings, which justifies the conversion using toInt().

Suggested change
exists(string prop |
exists(string prop |
// The property names read from the output array are expected to be numeric strings,
// which justifies the conversion using toInt().

Copilot uses AI. Check for mistakes.

result = this.getOutputArray().getAPropertyRead(prop) and
n = prop.toInt()
)
}
}

/**
* Helps type-tracking simple uses of `Promise.all()` such as `const [a, b] = await Promise.all([x, y])`.
*
* Due to limited access path depth, type tracking can't track things that are in a promise and an array
* at once. This generates a step directly from the input array to the output array.
*/
private class PromiseAllStep extends SharedTypeTrackingStep {
override predicate loadStep(DataFlow::Node node1, DataFlow::Node node2, string prop) {
exists(PromiseAllCall call, int n |
node1 = call.getNthInput(n) and
node2 = call.getNthOutput(n) and
prop = Promises::valueProp()
)
}
}

private class PromiseAll extends SummarizedCallable {
PromiseAll() { this = "Promise.all()" }

override DataFlow::InvokeNode getACallSimple() {
result = promiseConstructorRef().getAMemberCall("all")
}
override DataFlow::InvokeNode getACallSimple() { result instanceof PromiseAllCall }

override predicate propagatesFlow(string input, string output, boolean preservesValue) {
preservesValue = true and
Expand Down
5 changes: 5 additions & 0 deletions javascript/ql/src/change-notes/2025-04-30-promise-all.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
category: minorAnalysis
---
* Type information is now propagated more precisely through `Promise.all()` calls,
leading to more resolved calls and more sources and sinks being detected.