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

Dynamic ToPrimitive handling of Tuple exotic objects differs from Records #350

Open
acutmore opened this issue Aug 15, 2022 · 12 comments
Open
Milestone

Comments

@acutmore
Copy link
Collaborator

Spit out from #349

why does ToPrimitive have a fast path for Records, but not also for Tuples? (i understand that the lack of a Record prototype is why it's needed for Records)

Yes the lack of prototype on Record exotic objects and it being frozen requires ToPrimitive to explicitly handle them in some way otherwise it is a TypeError. #320

Tuple exotic objects having a prototype can follow the more dynamic look up and follow the same pattern as existing primitive wrappers. This means that the AO can be hooked by adding Symbol.toPrimitive to Tuple.prototype and it follows the OrdinaryToPrimitive logic of switching preference between Tuple.prototype.toString or Tuple.prototype.valueOf based on the hint.

Perhaps there is appetite for setting a new precedent for new primitives to not be dynamic here, or perhaps only for Tuple to differ here to align with Records?

@ljharb
Copy link
Member

ljharb commented Aug 15, 2022

I think we should minimize dynamism whenever possible, and there should be a fast path for Tuple here just like there is for Record.

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

rricard commented Sep 27, 2022

It looks like that the main reason we introduced a special branching for Records is because records do not have a ToPrimitive or valueOf method. Tuples do through their prototype so this is unneeded for them.

If we introduce special branching for Tuples then we would need to follow that special branching for future new primitives. In this case we don't think that Tuple is special enough to warrant special treatment.

@ljharb
Copy link
Member

ljharb commented Sep 27, 2022

I think every primitive, old or new, is special enough to warrant this kind of fast path.

@rricard
Copy link
Member

rricard commented Sep 28, 2022

For old primitives this would be a behavioural change since overriding @@toPrimitive today would change their behaviour when passed into this AO. Adding a fast path would remove this "overridability". So old primitives are not in scope here.

Now for R&T we want them as not special as possible so we would like to avoid the "fast path" if possible and we can with Tuple, but for record, as said earlier and added as a spec note, we have to balance the fact Records don't have prototypes to latch on.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2022

The fast path seems preferable to me, not something to avoid - i think it’s a mistake that existing primitives don’t have that path (and one we can’t change, certainly), and I’d like to see new primitives have it.

@acutmore
Copy link
Collaborator Author

and I’d like to see new primitives have it.

This sounds like something that can be brought to plenary. Either as part of a R&T item or separate. What do you think?

@ljharb
Copy link
Member

ljharb commented Sep 28, 2022

I think that since there must be a fast path for Records, and Records and Tuples should be consistent with each other when possible, it's part of this proposal.

@rricard
Copy link
Member

rricard commented Sep 30, 2022

I'm not sure "Records and Tuples should be consistent with each other when possible" is necessarily the goal in these semantics. I more err on the side of "Records and Tuples should be consistent with other primitives when possible" as we have been regularly been asked to do by various committee stakeholders. If we want to keep R&T consistent with each other we could alternatively pull out ToPrimitive support on records.

@ljharb
Copy link
Member

ljharb commented Sep 30, 2022

That would widen the gap with both of those consistencies, so it’d be strictly worse.

@rricard
Copy link
Member

rricard commented Sep 30, 2022

Ok, let's scratch that option then.

That leaves us with either doing nothing or adding Tuple as a fast path. I honestly am neutral but I'd like to hear from other delegates what they think of this since this will influence future primitives...

@brad4d
Copy link

brad4d commented Oct 3, 2022

I'd like to put this in terms of what would be observably different for users.

Current behavior today for, say, a string.

const stringPrimitive = 'string value';
const stringWrapper = new String(stringPrimitive);

String(stringWrapper); // 'string value'
Number(stringWrapper); // NaN
stringWrapper.valueOf(); // 'string value'
stringWrapper.toString(); // 'string value'

// override ToPrimitive AO behavior
stringWrapper[Symbol.toPrimitive] = () => "42";

// The primitive-creating methods are affected
String(stringWrapper); // '42'
Number(stringWrapper); // 42

// ... but these are not
stringWrapper.valueOf(); // 'string value'
stringWrapper.toString(); // 'string value'

This is definitely weird behavior and seems like a potential for bugs.
I expect that changing the behavior of a wrapper object was not an intended use of Symbol.toPrimitive.

I think it's reasonable for us to do the fast path logic for both Record and Tuple in this proposal.

It seems like there should be a separate editorial normative change to add fast paths for everything.
@ljharb would you be a good person to do that? You're in the editors group, right?

If that change lands before this proposal hits stage 3, great.
If not the presence of the fast paths this proposal adds could serve as a reminder that we should consider making the other primitive wrappers behave the same way.
Maybe we could even update the note we already have in ToPrimitive to include such a suggestion?

@ljharb
Copy link
Member

ljharb commented Oct 3, 2022

@brad4d no, i'm not an editor anymore - but any delegate can put up a needs-consensus PR.

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

4 participants