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

Motivation beyond defensiveness against mutation of globals? #8

Closed
bakkot opened this issue Sep 30, 2021 · 9 comments
Closed

Motivation beyond defensiveness against mutation of globals? #8

bakkot opened this issue Sep 30, 2021 · 9 comments
Labels
question Further information is requested

Comments

@bakkot
Copy link

bakkot commented Sep 30, 2021

The readme gives as the sole motivation for this proposal the desire to be defensive against mutation of Function.prototype.

I don't find this motivation nearly compelling enough to justify new syntax. If you want to run in an untrusted environment already need to be saving any other prototype methods, as the readme notes, even given this syntax:

// Our own trusted code, running before any adversary.
const { slice } = Array.prototype;

Once you're doing that, the additional overhead of saving the various Function.prototype methods is negligible. That is, code which wants to be defensive can (and indeed generally does) already write

const { bind, call } = Function.prototype;
const uncurryThis = bind.bind(call);
const slice = uncurryThis(Array.prototype.slice);

// [...]

delete Array.prototype.slice;
delete Function.prototype.call;

// [...]

slice([0, 1, 2], 1, 2); // works fine

So it seems to me that this syntax does relatively little to improve this relatively obscure use case. As such, it doesn't seem to me to even approach the bar for adding new syntax, at least not on the strength of the motivations given in the readme. Are there other motivations?

@js-choi
Copy link
Collaborator

js-choi commented Sep 30, 2021

Regarding if this improves defense against global mutation: Yes, the #7 pull request, which I have just merged, adds discussion about two ways this improves defense (first, performance by avoiding wrapping with bound functions; second, developer ergonomics). The ergonomics problem could be debatable, but the performance problem is pretty clear (see nodejs/node#38248). (CC: @ljharb, @bmeck.)

Regarding if there are other use cases: Yes, there is are “generic methods” and “individually importable methods” use cases (#6). I plan to write a section about this within the next few weeks. Generic methods are already an idiomatic pattern in JavaScript (e.g., Array.from), and being able to apply them more ergonomically would be good. (bind and/or call might be two of the most common built-in methods in the entire language, after all.)

@js-choi js-choi added the question Further information is requested label Sep 30, 2021
@bakkot
Copy link
Author

bakkot commented Sep 30, 2021

The ergonomics problem could be debatable, but the performance problem is pretty clear (see nodejs/node#38248).

That performance problem holds in the current implementation of V8, nowhere else - not even necessarily in V8 in the future. I would not regard that as a reason for adding new syntax. (V8 optimizes common patterns; they haven't optimized this one because it's not common. It's not like it's fundamentally impossible to optimize. But if it's not common enough to optimize in V8, it's surely not common enough to warrant new syntax.)

Generic methods are already an idiomatic pattern in JavaScript (e.g., Array.from)

Array.from is intended to be used by subclasses of Array, not on random other objects. I strongly disagree with the claim that they are already idiomatic in JavaScript.

@js-choi
Copy link
Collaborator

js-choi commented Sep 30, 2021

After further discussion from Matrix, it’s clear that, in order to have any chance at Stage 1, I need to drastically change the focus of the explainer. I need to focus less on global protection and more on Function.prototype.call and bind being generally common, as well as tree-shakable, individually importable methods being desirable.


That performance problem holds in the current implementation of V8, nowhere else - not even necessarily in V8 in the future. I would not regard that as a reason for adding new syntax. (V8 optimizes common patterns; they haven't optimized this one because it's not common. It's not like it's fundamentally impossible to optimize. But if it's not common enough to optimize in V8, it's surely not common enough to warrant new syntax.)

(Further clarification on this performance problem being specific to V8 would be appreciated from anyone on the Node team. If it is indeed optimized by other engines, then I should remove the section discussing that “problem”. CC: @ljharb, @aduh95, whomever else available.)

js-choi added a commit that referenced this issue Sep 30, 2021
@bmeck
Copy link
Member

bmeck commented Oct 1, 2021

@bakkot V8 does actually try to hot path these coding patterns at the behest of node and perf E.G. https://bugs.chromium.org/p/v8/issues/detail?id=9702 also not all engines optimize this so I'm not sure I understand the claim that it only affects V8.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2021

Do any engines optimize this?

@bmeck
Copy link
Member

bmeck commented Oct 1, 2021

@ljharb V8 tries to, I couldn't find any other engine that inlines by unwrapping the bound function after some fiddling in the console.

@bakkot
Copy link
Author

bakkot commented Oct 1, 2021

also not all engines optimize this so I'm not sure I understand the claim that it only affects V8.

I meant that Node is built on V8 and so its performance issues only give you information about what is optimized in V8 rather than in any other engine.

@js-choi
Copy link
Collaborator

js-choi commented Oct 6, 2021

I’ve revamped the explainer to now focus on the commonality and clunkiness of .bind, .call, and .apply. Hopefully, after this change, the explainer makes a more compelling case.

@js-choi
Copy link
Collaborator

js-choi commented Oct 6, 2021

@bakkot: I’m going to close this, since the explainer got completely revamped…but I’d be very interested to know if you think the explainer has become more compelling than it had been before. Cheers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants