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

Avoid looking up @@isConcatSpreadable for tuples. #334

Closed
acutmore opened this issue Jul 25, 2022 · 6 comments
Closed

Avoid looking up @@isConcatSpreadable for tuples. #334

acutmore opened this issue Jul 25, 2022 · 6 comments
Milestone

Comments

@acutmore
Copy link
Collaborator

Right now the spec will check @@isConcatSpreadable on Tuple.prototype when passed a tuple, before defaulting to returning true for tuples:

1. If Type(O) is not Object or Tuple, return false.
2. Let spreadable be ? GetV(O, @@isConcatSpreadable).
3. If spreadable is not undefined, return ! ToBoolean(spreadable).
4. If ! IsTuple(O), return true.
5. Return ? IsArray(O).

This means that if someone creates Tuple.prototype[Symbol.isConcatSpreadable] and returns false, it would change how Tuples behave in concat. It might make sense to do an early return for IsTuple and always return true - keeping the behavior 'static'.

cc: @michaelficarra

@rricard rricard mentioned this issue Jul 25, 2022
25 tasks
@ljharb
Copy link
Member

ljharb commented Jul 26, 2022

It would be confusing if someone did that, and Tuple concat still spreaded tuples but Array concat didnt. If both are hardcoding it, then it would still be confusing that primitive Tuple spread but boxed tuples dont.

I think they should just be looked up via the symbol.

@acutmore acutmore added this to the stage 3 milestone Sep 27, 2022
@jayaddison
Copy link

This has been resolved by 94cd6e3 (#264), I think; specifically:

       </dl>
       <emu-alg>
-        1. If Type(_O_) is not Object or Tuple, return *false*.
-        1. Let _spreadable_ be ? Get<ins>V</ins>(_O_, @@isConcatSpreadable).
-        1. If _spreadable_ is not *undefined*, return ToBoolean(_spreadable_).
-        1. <ins>If IsTuple(_O_), return *true*.</ins>
+        1. If IsTuple(_O_), return *true*.
         1. Return ? IsArray(_O_).
       </emu-alg>
     </emu-clause>

(I see that spreadable is referenced in spec pseduocode elsewhere; but @@isConcatSpreadable is no longer found in 5c9a6e1)

@jayaddison
Copy link

(I see that spreadable is referenced in spec pseduocode elsewhere; but @@isConcatSpreadable is no longer found in 5c9a6e1)

Sorry; I spoke too soon; @@isConcatSpreadable does still appear there in miscellaneous-updates.html -- but the short-circuit logic suggested is included there too (I believe -- please double-check me on this!).

@acutmore
Copy link
Collaborator Author

acutmore commented Nov 7, 2024

Sorry; I spoke too soon; @@isConcatSpreadable does still appear there in miscellaneous-updates.html

Right it still appears because the specification is written as a delta to apply to the existing specification. The important line is:

1. <ins>If IsTuple(_O_) is *true*, return *true*.</ins>
where the <ins>...</ins> is, as that is the part that this proposal is adding. The other lines are "just" the already official spec text from https://tc39.es/ecma262/multipage/indexed-collections.html#sec-isconcatspreadable. Hope that makes sense! 😃

In terms of closing this issue. On one hand, yes the spec text in this repo has been updated. But that does not necessarily also mean that this issue was discussed in plenary and got committee consensus. So sometimes an issue might remain open to reflect that the issue still needs wider discussion. I believe the last time we discussed this in committee was 2022-07 but no absolute conclusion was reached. That said it looks like this PR was merged after that meeting and I believe there was a stronger leaning towards not looking up the symbol so I'm happy to close this issue, and we can always re-open.

@acutmore acutmore closed this as completed Nov 7, 2024
@jayaddison
Copy link

Thanks again @acutmore; that's helpful to know some more of the logic behind holding issues open, too. I began reading a few of the meeting notes a day or two ago, but I'm not entirely up-to-date on all the details.

@jayaddison
Copy link

(and yep, to confirm also: that makes sense that the documents represent a patch/branch from the existing specification 👍)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants