Skip to content

Change definition of getFactoryNodeInternal #19359

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jeongsoolee09
Copy link
Contributor

@jeongsoolee09 jeongsoolee09 commented Apr 23, 2025

What this PR Contributes

Some variants of AMD module such as sap.ui.define can accept an additional parameter, making it the last parameter in some cases.

However, the additional parameter can technically be anywhere in the parameter list and shift around the exact index of
the callback argument in the parameter list as a result. Therefore, dynamically determine the index of the factory method parameter.

Note 1: There may be multiple matches since we're using _ (don't care) as the argument index.

Note 2: We could have used DataFlow::InvokeNode.getCallback if the supertype were not CallExpr, but jumping to data flow node is an overkill here.

Some variants of AMD module such as
[sap.ui.define])(https://sdk.openui5.org/api/sap.ui#methods/sap.ui.define)
can accept a boolean as its last parameter.
Therefore, explicitly state the index of the
factory method parameter as `1`.
An additional parameter may be anywhere in the
parameter list and shift around the exact index of
the callback argument in the parameter list.

So, "dynamically" determine the index by type-checking
a parameter in the parameter list.

Note 1: There may be multiple matches since we're
using `_` (don't care) as the argument index.

Note 2: We could have used DataFlow::InvokeNode.getCallback
if the supertype were not CallExpr, but jumping to
data flow node is an overkill here.
@Copilot Copilot AI review requested due to automatic review settings April 23, 2025 18:43
@jeongsoolee09 jeongsoolee09 requested a review from a team as a code owner April 23, 2025 18:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • javascript/ql/lib/semmle/javascript/AMD.qll: Language not supported

@github-actions github-actions bot added the JS label Apr 23, 2025
@jeongsoolee09 jeongsoolee09 added the no-change-note-required This PR does not need a change note label Apr 23, 2025
result = TValueNode(this.getLastArgument())
exists(Function factoryFunction | factoryFunction = this.getArgument(_) |
result = TValueNode(factoryFunction)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The restriction to Function here is too restrictive as we may need to follow some local flow steps before we get to the function (see the recursive case below). Some tests are failing because of this. Could you try simply changing getLastArgument() to getArgument(_)?

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