-
Notifications
You must be signed in to change notification settings - Fork 67
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
Are wrapped functions supposed to have "name" and "length" instance properties? #338
Comments
I think copy those two properties on wrapping is good. We should add it to the spec. |
When I interpreted the spec for the JSC implementation I explicitly removed these fields from the wrapper. This in turn has some sort of surprising performance downsides in JSC that would require additional optimization work there. So to weigh in from an implementer perspective, it be nice to have these included. |
I too see no compelling reason to omit them, and prefer for wrapped functions to have them. |
The wrapped exotic has a "reference" to the target function in the other realm, so the specs currently don't use the name or length. I can add steps to add these If that's desired. It seems like from @philomates' feedback, this helps with performance, so I'm in for it! |
#339 has a proposed solution, but introduces observability getting the target's length and name. I tried to make the steps close to what happens in |
An alternative to #339 would be the following, as suggested by @rwaldron:
This would not require (re)using any abstraction and won't add any prefix or handling of NaN of Infinity values for length, but it also seems fine. I'm ok one with this solution or #339. Maybe this one would be helpful on performance, I guess? |
It seems much better to me to mirror the logic in |
We debated about this in champion group a while ago, and at the time we decided not to copy those to avoid leaking any information from the other realm. /cc @erights I recall two main questions: 1) why should be copy them? 2) would this confuse developers to think this function is the actual function instead of the wrapper. However, in retrospective, I don't have strong objections to prevent the copy of these two properties. |
Why it will leak any meaningful information across realms?
Different engines might have different internal implementations, maybe we can leave that host defined? |
@caridy imo because one of the big justifications for this weird function-wrapping things is "it's like function bind", and function bind copies them, so this should too. |
That's the worst of all outcomes. :) Let's decide to have or not have them (I'm still inclined towards have), but there is zero reason to leave this host-defined. |
I would like to think it’s a rule that whether or not + in what order object internal methods are invoked on user objects can’t be left host-definable. That seems pretty different from data-exposure-only cases like error stacks and source text. |
In case it helps contextualize spec decisions, here is a quick JSC sketch that (mostly) implements the proposed change. Performance-wise in JSC this change doesn't speed anything up, but I'd guess that it isn't a big deal outside of micro-benchmarks also note that, as far as I can tell, |
I'm leaning towards not adding the prefix. In the bound functions, you can "compare" it with the target function as they generally co-exist in the same Realm. The name supports a distinction from what is a bound function of the other with that given name. function fn() { /*...*/ }
const other = fn.bind(/*...*/);
other.name; // "bound fn" The name property above somehow helps knowing other is a bound version of For the wrapped exotics, you don't have the target function available neither an identifier reference of the same realm for it. There is no actual use case for having a "wrapped" prefix and so it doesn't add much value to the function. There are other problems, one standing out is giving the code a language so far only used in specs. Today we define "wrapped" exotics but it can be and mean anything, such as the exotic can be eventually renamed. Exposing that name to mandatory code definitions seems unnecessary. Another nit pick: this name definition would create a way to identify wrapped exotic functions, summed up to the fact the name property can modified (configurable) and make any assertions over an existing prefix pretty loose. I believe @caridy and @rwaldron are onboard with this position and we have yet to discuss this at a SES meeting. @ljharb I know you've been supportive of the consistency with the |
I don't see any issue with exposing the term. It's already somewhat possible to distinguish wrapped functions (when you have access to the original one) because you can configure the I don't think the motivation for the |
From today's meeting:
This has consensus |
I don't see where they are set in the spec draft. Are they intended to have "name" and "length" of the target function, like how a bound function has the "name" and "length" of the underlying target?
The text was updated successfully, but these errors were encountered: