Skip to content

JS: Allow many Array steps to be used in type-tracking #16739

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 10 commits into from
Jun 20, 2024

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Jun 12, 2024

I also spotted a mistake in Array.splice modeling, so I also fixed it up.

I think the big question is whether we actually want to have these steps part of normal type-tracking; by allowing read of array elements at any position, we might not handle examples like the one below precisely (and think the call f() could target any of the 3 functions)

const fns = [f1, f2 ,f3];
const f = fns[0];
f();

note: I didn't actually test this out, just thinking out loud

RasmusWL added 4 commits June 12, 2024 16:14
Oh how I have enjoyed working with InlineExpectationTests for these sort
of things, not worrying about all the .expected files changing because
you add a few lines in the middle of your tests :D
@github-actions github-actions bot added the JS label Jun 12, 2024
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.

Evaluation looks good. Do you want to take this out of draft?

…steps.md

Co-authored-by: Asger F <asgerf@github.com>
@RasmusWL RasmusWL marked this pull request as ready for review June 20, 2024 08:07
@RasmusWL RasmusWL requested a review from a team as a code owner June 20, 2024 08:08
@asgerf asgerf merged commit a36e393 into github:main Jun 20, 2024
asgerf added a commit to asgerf/codeql that referenced this pull request Jun 26, 2024
New library gets FN for spread arguments in a call to splice(), which
was added to the old version in this PR:
  github#16739
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants