tc39 / Function-prototype-toString-revision

:fishing_pole_and_fish: ECMA-262 proposal to update Function.prototype.toString
https://tc39.github.io/Function-prototype-toString-revision
26 stars 10 forks source link

Function.prototype.toString should throw iff its target is not callable #3

Closed claudepache closed 8 years ago

claudepache commented 8 years ago

In order to be consistent with other methods defined on Function.prototype, the method should always "work" (i.e., return a string and not throw a TypeError) when the target is callable. E.g.:

  1. If IsCallable(func) is false, throw a TypeError.
  2. If..., return a string that...
  3. If..., return a string that...
  4. Else, return an implementation-defined string that should produce a SyntaxError when evaluated using eval.

(The string produced in step 4 could be typically "function () { [native code] }")

michaelficarra commented 8 years ago

Removing your step 4 is one of the goals of this proposal, but that is unrelated to your request. I will ensure that this method throws iff the thisValue is not callable.

michaelficarra commented 8 years ago

This is done.

ljharb commented 6 years ago

This change surprises me. It makes function proxies undetectable, prior to which, Array and Error were the only undetectable proxies builtins.

I think that the IsCallable change should be reverted.

claudepache commented 6 years ago

@ljharb Why? A Proxy is not supposed to be detectable, except by accident...

ljharb commented 6 years ago

Proxies are detectable for every builtin type; “not supposed to be detectable” in fact only is true when a Proxy is coupled with a membrane. It is simply not a goal of Proxy to be undetectable in isolation.

jdalton commented 6 years ago

I get being detectable but throwing doesn't really improve the dev experience here.

ljharb commented 6 years ago

I’m less concerned with throwing as i am with retaining the same detection ability as currently exists.

jdalton commented 6 years ago

If you're wanting a way to detect promises a method explicitly suited for it would be a better route than indirect inferences. For example Node is introducing require('util').types.isProxy(). There's also helpers like getPromiseDetails and even getProxyDetails which returns an array containing the original wrapped value and the handler object.

ljharb commented 6 years ago

I’d be fine with someone doing that as a separate proposal; I’d say the IsCallable change in toString should be deferred to be part of that proposal.

jdalton commented 6 years ago

I’d be fine with someone doing that as a separate proposal;

I think you're volunteering.

I’d say the IsCallable change in toString should be deferred to be part of that proposal.

I don't think one has to be tied to the other. There's currently no unified way to detect proxies so having one fall out of the handful of disparate approaches isn't a big pull.

ljharb commented 6 years ago

The main goal of the proposal is to more tightly specify the content of the string it produces. This IsCallable change is something secondary. It could probably be a separate Needs Consensus PR, but it should not be part of this proposal.

jdalton commented 6 years ago

It could probably be a separate Needs Consensus PR, but it should not be part of this proposal.

This has already been discussed. I dig the current state, since it more closely resembles other checks which test for IsCallable without the extra [[ECMAScriptCode]] internal slot check, and don't see a strong case for changing it now.

In fact, step 3 could be simplified from

If Type(func) is Object and IsCallable(func) is true,

to just

If IsCallable(func) is true,

since IsCallable checks for Type(argument) of Object and be even more consistent with other checks.

ljharb commented 6 years ago

Indeed; it was intentional, and in response to this issue. However, I don't recall it ever being discussed with the full committee (I also can't find it in the notes) - so at the very least, I think it's something that should be discussed before this proposal advances.

@michaelficarra, I'd be happy to talk about it offline with you first, but after that, would you be OK with adding it to the agenda so we can get the full committee's take on it?

jdalton commented 6 years ago

Indeed; it was intentional, and in response to this issue. However, I don't recall it ever being discussed with the full committee (I also can't find it in the notes) - so at the very least, I think it's something that should be discussed before this proposal advances.

Sure. I don't think it's a blocker, but I get it. Context is a good thing.

claudepache commented 6 years ago

FWIW, my main reason for Function.prototype.toString not throwing on proxies, is that code written with functions in mind does not accidentally stop working for certain class of functions.

jdalton commented 6 years ago

@claudepache

FWIW, my main reason for Function.prototype.toString not throwing on proxies, is that code written with functions in mind does not accidentally stop working for certain class of functions.

Right, folks generally check if something is callable by doing typeof value === "function". Since proxies carry that over they report typeof "function" but currently can't go where other typeof "function" values can.

@ljharb

I’m less concerned with throwing as i am with retaining the same detection ability as currently exists.

With this revision proxied functions are still indirectly detectable because their Function.prototype.toString.call result is now something like "function () { [native code] }" instead of what it normally would be.

ljharb commented 6 years ago

@jdalton that's only the case if i have a reference to the original function to compare; and either way, it wouldn't let me detect a proxy to a builtin function.

Also, bound functions would report the same toString, so all it'd tell me for sure is that a function was either proxied or bound or native.

jdalton commented 6 years ago

@ljharb Would taking what engines do for bound functions (e.g. binding a function a gives it a name of bound a) but extended to the native function representation work? For example in Safari

"function ProxyObject() {
    [native code]
}"

is reported for proxy functions in Function.prototype.toString.call. That way bound, proxied, native, etc can be identified by way of an indirect inference of the native code marker.

"function bound() { [native code] }"
"function proxied() { [native code] }"

This should already be allowed under this revised Function#toString proposal as implementation-dependent.

ljharb commented 6 years ago

I think that if there's a way for me to identify "proxied" (and "bound") from the toString representation, that my concern is totally satisfied. However, I'd not be comfortable just hoping that some "implementation-dependent" solutions will result out of this; I'd want it mandated at the language level.

jdalton commented 6 years ago

Cool. I can dig that. I'm in favor of less implementation-dependent bits anyways. The wiggle room is usually allowed so engines can drop full text source but, in these cases of native function representation, that isn't really an issue.

michaelficarra commented 6 years ago

@ljharb If you think we should explicitly adopt the requirement to have Function.prototype.toString brand check functions, please open a separate issue. I warn you, though, that (as you know) the committee is not fond of brand checks.

ljharb commented 6 years ago

I think that the toString proposal shouldn’t change the brand checking landscape, not the least because getting brand checking changes in otherwise is so difficult.

claudepache commented 6 years ago

For reference, the es-discuss thread that led me to open this gh issue two and a half years ago:

https://esdiscuss.org/topic/calling-tostring-on-function-proxy-throws-typeerror-exception

claudepache commented 6 years ago

Note that F.p.toString throwing a TypeError has never really been a reliable brand-checking feature: As @bzbarsky noted in the aforementioned es-discuss thread, some implementations historically considered random host objects as callable (see, e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=909656), but F.p.toString would choke on them at their discretion.

If we need to brand-check proxy functions, we should require it explicitly rather than rely on some historical accident (why on earth bound functions and proxy functions were treated differently in ES6, although both are constructed at runtime and none has an [[ECMAScriptCode]] internal slot?)

bzbarsky commented 6 years ago

There are in fact "host objects" (not really a concept anymore) that are callable in all browsers and required to be so by web compat and by the HTML spec.

Of course they report their typeof as undefined, so chances are people won't try usingF.p.toString` on them....