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.

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

Editorial: Narrow integer into non-negative integer in return type of ExpectedArgumentCount #3378

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

tmdghks
Copy link
Contributor

@tmdghks tmdghks commented Jul 25, 2024

I carefully assume that the return type of ExpectedArgumentCount can be narrowed into non-negative integer, which is integer in current specification.
The reason why I suggest this PR is that the return type of the ExpectedArgumentCount is integer makes type mismatching in OrdinaryFunctionCreate and SetFunctionLength.

@tmdghks tmdghks marked this pull request as draft July 25, 2024 04:29
@tmdghks tmdghks changed the title Editorial: Using non-negative integer or +infin instead of integer in return type of ExpectedArgumentCount Editorial: Using non-negative integer instead of integer in return type of ExpectedArgumentCount Jul 25, 2024
@tmdghks tmdghks force-pushed the fix-expectedargumentcount-return-type branch from 7a1a5dc to 1ff79be Compare July 25, 2024 06:05
@tmdghks tmdghks changed the title Editorial: Using non-negative integer instead of integer in return type of ExpectedArgumentCount Editorial: Narrow integer into non-negative integer in return type of ExpectedArgumentCount Jul 25, 2024
@tmdghks tmdghks marked this pull request as ready for review July 25, 2024 06:07
@tmdghks
Copy link
Contributor Author

tmdghks commented Jul 25, 2024

The following is the additional reason why I think it is possible to narrow.

First, the following cases are all the cases where ExpectedArgumentCount returns value.

  1. FormalParameters: [empty] FunctionRestParameter
    • Returns 0
  2. FormalParameters: FormalParameterList, FunctionRestParameter
    • Return non-negative integer
  3. FormalParameterList: FormalParameter:
    • Return 0 or Return 1
  4. FormalParameterList: FormalParameterList, FormalParameter
    • Return count or count + 1, and count should be non-negative.
  5. ArrowParameters: BindingIdentifier
    • Return 1
  6. ArrowParameters : CoverParenthesizedExpresionAndArrowParameterList
    • Return a non-negative integer
  7. PropertySetParameterList: FormalParameter
    • Return 0 or 1
  8. AsyncArrowBindingIdentifier: BindingIdentifier
    • Return 0 or 1

In addition, the following is the reason why the type mismatching occurs

In the step 21 of 10.2.3 OrdinaryFunctionCreate, len should have the type integer. However, in the step 22 of 10.2.3 OrdinaryFunctionCreate, SetFunctionLength get len argument as a second argument, and the type of this argument should be either non-negative integer or +∞. Therefore, if the return type of the 15.1.5 Static Semantics ExpectedArgumentCount is integer then type mismatch can occur. However, if the return type is changed into non-negative integer then we can prevent the type mismatch regarding these functions.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 30, 2024

Looks correct to me.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

Thanks!

@tmdghks
Copy link
Contributor Author

tmdghks commented Jul 30, 2024

Thank you very much for your review!

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Sep 2, 2024
@ljharb ljharb force-pushed the fix-expectedargumentcount-return-type branch from 1ff79be to d9d77ad Compare September 2, 2024 23:32
@ljharb ljharb merged commit d9d77ad into tc39:main Sep 3, 2024
8 checks passed
@tmdghks tmdghks deleted the fix-expectedargumentcount-return-type branch September 4, 2024 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants