Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix the error when use spread arguments twice #33069

Closed
wants to merge 2 commits into from
Closed

Fix the error when use spread arguments twice #33069

wants to merge 2 commits into from

Conversation

rchaser53
Copy link
Contributor

@rchaser53 rchaser53 commented Aug 24, 2019 •
edited by elibarzilay
Loading

This is a partial fix for #32835, but a full fix should also handle the additional test posted there.

@rchaser53
Copy link
Contributor Author

@RyanCavanaugh
Could you review my PR? And if it's ok, please merge it.
This bug is still caused using the current version.

CI is failed, but it doesn't seem to be my fault.
The other PRs are also failed and emit the same errors.
ex. https://github.com/microsoft/TypeScript/pull/36148/checks?check_run_id=385338469

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020
@elibarzilay elibarzilay force-pushed the fix-spread-arguments-twice branch from a366747 to e6c8381 Compare March 11, 2020 21:02
@elibarzilay
Copy link
Contributor

(@rchaser53 -- JFYI, I pushed a rebase to drop the big merge commit.)

elibarzilay
elibarzilay suggested changes Mar 12, 2020
View reviewed changes
@elibarzilay elibarzilay mentioned this pull request Mar 12, 2020
Closed
@elibarzilay
Copy link
Contributor

Note that I changed the PR text to keep #32835 for the additional issue there.

@rchaser53
Copy link
Contributor Author

Thank you for the review. I fixed it as possible as I can.

@rchaser53 rchaser53 requested a review from elibarzilay March 13, 2020 13:09
@rchaser53 rchaser53 force-pushed the fix-spread-arguments-twice branch from 5073b62 to 71bb928 Compare March 13, 2020 13:30
elibarzilay
elibarzilay suggested changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@elibarzilay elibarzilay left a comment

Choose a reason for hiding this comment

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

Sorry, but there are still a bunch of issues that I see here...

  1. You still have the previous behavior as a special case, which I think should not be needed if this becomes a proper fix (more on that below).

  2. You initialize totalCount by the first spread length, but I think that you should add to ut the firstSpreadIndex otherwise the code doesn't count those plain arguments that might appear before the first spread.

  3. The loop starts from firstSpreadIndex, which means that this first spread's count is added twice to the count. It should probably be firstSpreadIndex+1 instead.

  4. IIUC, this code (tries to) handle tuples, but it omits arrays, which makes it a partial solution. For example, it would be good to add these two tests:

    takeTwoOrMore(...t5);
takeTwoOrMore(...t5, ...t5);

    where both should be errors --- and as it is, the second one isn't.

    I think that the previous version was handling only arrays, so handling both should make the special-cased first part redundant too.

Since this is essentially doing only an arity count check, making the code handle both arrays and tuples should not be too difficult: it should go over all of the arguments, and for each:

  • if it's a simple tuple, add its length to the count,

  • if it's an array, then remember that the count is whatever it is "plus any number of arguments",

  • if it's a tuple with a rest array (like t4), then add the length and remember a "plus any number of arguments" as the previous case,

  • if it's neither then add 1.

This should cover all cases as well as making the first special case redundant.

@elibarzilay elibarzilay mentioned this pull request May 18, 2020
Merged
@sandersn
Copy link
Member

@rchaser53 Do you want to keep working on this? Let us know if we can help explain anything more.

@rchaser53
Copy link
Contributor Author

I cannot have the time to resolve this issue now. My main work is too busy. I'm really sorry for taking your time... I close this PR.

@rchaser53 rchaser53 closed this May 21, 2020
elibarzilay added a commit to elibarzilay/TypeScript that referenced this pull request May 27, 2020
@elibarzilay
Make hasCorrectArity handle tuples properly
3d08da4 
This completes the work that started in PR microsoft#33069, and fixes microsoft#32835.

There are probably two additional related changes that are needed to
make this more complete:

* Fix the code that composes the error message (see the first two
  `FIXME`s in `callWithSpread3.ts`).

* Fix the code that checks the argument types (second two `FIXME`s).

* There is also an error in `genericRestParameters1.ts` which changed
  but should not be an error in the first place.  Added a `FIXME` there
  too.  (Probably will work if the previous iterm is done.)

In addition, `getEffectiveCallArguments` munges the arguments in case of
a spread in the last argument which might be better to avoid.  (I think
that there are cases where it wouldn't work anyway, such as a spread of
an array followed by a spread of an empty array.)
@elibarzilay elibarzilay mentioned this pull request May 27, 2020
Merged
elibarzilay added a commit to elibarzilay/TypeScript that referenced this pull request May 27, 2020
@elibarzilay
Make hasCorrectArity handle tuples properly
f505150 
This completes the work that started in PR microsoft#33069, and fixes microsoft#32835.

There are probably two additional related changes that are needed to
make this more complete:

* Fix the code that composes the error message (see the first two
  `FIXME`s in `callWithSpread3.ts`).

* Fix the code that checks the argument types (second two `FIXME`s).

* There is also an error in `genericRestParameters1.ts` which changed
  but should not be an error in the first place.  Added a `FIXME` there
  too.  (Probably will work if the previous iterm is done.)

In addition, `getEffectiveCallArguments` munges the arguments in case of
a spread in the last argument which might be better to avoid.  (I think
that there are cases where it wouldn't work anyway, such as a spread of
an array followed by a spread of an empty array.)
elibarzilay added a commit that referenced this pull request Jun 5, 2020
@elibarzilay
Make hasCorrectArity handle tuples properly
3151e2a 
This completes the work that started in PR #33069, and fixes #32835.

There are probably two additional related changes that are needed to
make this more complete:

* Fix the code that composes the error message (see the first two
  `FIXME`s in `callWithSpread3.ts`).

* Fix the code that checks the argument types (second two `FIXME`s).

* There is also an error in `genericRestParameters1.ts` which changed
  but should not be an error in the first place.  Added a `FIXME` there
  too.  (Probably will work if the previous iterm is done.)

In addition, `getEffectiveCallArguments` munges the arguments in case of
a spread in the last argument which might be better to avoid.  (I think
that there are cases where it wouldn't work anyway, such as a spread of
an array followed by a spread of an empty array.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elibarzilay elibarzilay elibarzilay requested changes

Assignees

@elibarzilay elibarzilay

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rchaser53 @elibarzilay @sandersn