Skip to content

Conversation

kaeluka
Copy link

@kaeluka kaeluka commented Apr 26, 2022

This adds a flow step that has been missed for flow that goes from an argument to a ...rest parameter, eg:

f('x', 1, 2);
//        |
// ...    |
//        +-----------------+
function f(x, ...rest) { // |  
  console.log(rest[1]);  // |
//            ^^^^^^^       |
//                 |        |
//                 +--------+
}

This helps find one of the example I posted in #8846.

I'll trigger an experiment to see what other results this will get. Could easily turn up things.

@github-actions github-actions bot added the JS label Apr 26, 2022
@kaeluka kaeluka changed the title Add flow step to rest parameters JS: Add flow step to rest parameters Apr 26, 2022
@kaeluka kaeluka changed the title JS: Add flow step to rest parameters JS: Add flow step to ...rest parameters Apr 26, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

@kaeluka kaeluka marked this pull request as ready for review April 27, 2022 14:05
@kaeluka kaeluka requested a review from a team as a code owner April 27, 2022 14:05
@kaeluka
Copy link
Author

kaeluka commented Apr 27, 2022

@asgerf, as you already know the context of this PR, would you be able to review this? 🙇

@kaeluka kaeluka added the no-change-note-required This PR does not need a change note label Apr 27, 2022
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM provided the tests pass

Co-authored-by: Asger F <asgerf@github.com>
@kaeluka kaeluka merged commit f4104e2 into github:main Apr 28, 2022
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