Skip to content

JS: Handle spread/rest in API graphs #19108

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 7 commits into from
Apr 1, 2025

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Mar 25, 2025

Makes sure API graphs can look through spread/rest parameters. An important situation where this occurs is in default constructors, where spread/rest are inserted automatically.

For example, the class below

import { ApolloServer } from "@apollo/server"

class CustomApollo extends ApolloServer {}

is desugared into this:

class CustomApollo extends ApolloServer {
  constructor(...args) { super(...args) }
}

We have a model that looks for arguments passed to the ApolloServer constructor,

["@apollo/server", "Member[ApolloServer,ApolloServerBase].Argument[0].AnyMember.AnyMember.AnyMember.Parameter[1]", "remote"]

This ought to pick up on calls to new CustomApollo because the arguments are forwarded to ApolloServer:

new CustomApollo({ ... })

But evaluation got stuck at Argument[0] because of two issues:

  • the super call targeting the ApolloServer constructor was not being considered at all, and
  • the rest parameter and spread argument ...args were not understood by API graphs

This PR fixes the above two issues, and fixes the FN from the test case.

@asgerf asgerf added the JS label Mar 25, 2025
@asgerf asgerf force-pushed the js/api-graph-spread-rest branch from fa40ba8 to fa8e886 Compare March 27, 2025 19:34
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Mar 28, 2025
@asgerf asgerf force-pushed the js/api-graph-spread-rest branch from 3a30bc5 to b834ffe Compare March 28, 2025 08:15
@asgerf asgerf marked this pull request as ready for review March 28, 2025 08:33
@asgerf asgerf requested a review from a team as a code owner March 28, 2025 08:33
erik-krogh
erik-krogh previously approved these changes Mar 31, 2025
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments.

I feel like the change to invocation(...) could have been it's own commit with a separate test.

/** The argument index at which the spread argument appears. */
int getIndex() { result = index }

override string toString() { result = "getSpreadArgument(" + index + ")" }
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no getSpreadArgument() method on API::Node, which is what the toString() usually refers to.

I think it's fine here, but maybe add a small comment that this method is only used internally. (I'm counting the Label:: module as internal).

or
exists(Function fun, DataFlow::InvokeNode invoke, int argIndex, Parameter rest |
fun.getRestParameter() = rest and
rest.flow() = pred and
Copy link
Contributor

Choose a reason for hiding this comment

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

.getALocalUse() on pred?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed. It would be equivalent to getALocalSource() on the rest parameter, but parameters are already a source.

pred = trackUseNode(use, _, bound, "") and
invoke = pred.getAnInvocation().asExpr() and
i = firstSpreadIndex(invoke) and
spreadArray = invoke.getArgument(i - bound).(SpreadElement).getOperand().flow()
Copy link
Contributor

Choose a reason for hiding this comment

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

I having a hard time figuring out whether the arithmetic here is correct.

Could you add a test with some bound arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, there was indeed a bug here. Added a test and made the code easier to read.

@@ -860,6 +865,15 @@ module API {
.getStaticMember(name, DataFlow::MemberKind::getter())
.getAReturn()
)
or
exists(Function fun, DataFlow::InvokeNode invoke, int argIndex, Parameter rest |
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is modelling the parameters at (or after) the rest parameter, and modelling each entry as an array store into the spread argument.
And this is then only used for the getParameter method to be able to retrieve the correct parameter when there are spread arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added a comment to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is then only used for the getParameter method to be able to retrieve the correct parameter when there are spread arguments?

Ah, actually no, it's more than that. If a library model expects an argument to contain array elements, this will allow the model to find those array elements when the array was created by passing arguments into a rest parameter.

@asgerf asgerf merged commit 887942e into github:main Apr 1, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants