-
Notifications
You must be signed in to change notification settings - Fork 3
Allow implementation-defined choice of property or accessor #10
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
Conversation
|
For reference, the V8 implementation appears to be here. |
spec.emu
Outdated
| 1. If ? IsExtensible( _error_ ) is *false*, throw a *TypeError* exception. | ||
| 1. Let _name_ be an implementation-defined name. | ||
| 1. ! PrivateFieldAdd( _error_, _name_, _string_). | ||
| 1. Perform ! OrdinaryDefineOwnProperty(_error_, *"stack"*, PropertyDescriptor { [[Get]]: ? PrivateGet( _error_, _name_ ), [[Set]]: ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _string_)., [[Enumerable]]: false, [[Configurable]]: true }). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values of [[Get]] and [[Set]] must be function objects. I think what you're trying to do here is define abstract closures each with a single step and create built-in functions from those. You can use CreateBuiltinFunction for this, following the example of MakeArgGetter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all of this is actually not observable do we really need to spec it at that level? Here is my try:
let _getter_ be a function object such that _getter_.[[Call]](_error_) returns _string_ and
let _stack_ be PropertyDescriptor { [[Get]]: ? _getter_ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw. if I understand correctly currently that setter (as a function) would override itself with the stacktrace in _string_. Did you intended it to override itself with the value passed to the setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need to spec it at that level?
Yes, I think we do. Saying it is any function object with that [[Call]] behaviour doesn't go into how it's created, what realm it's in, what its [[Prototype]] is, etc., which are observable characteristics that should be specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that setter (as a function) would override itself with the stacktrace in
_string_. Did you intended it to override itself with the value passed to the setter.
Yes that is a requirement to avoid the getter / setter pair creating a covert communication channel on ordinary objects.
spec.emu
Outdated
| 1. Or: | ||
| 1. If ? IsExtensible( _error_ ) is *false*, throw a *TypeError* exception. | ||
| 1. Let _name_ be an implementation-defined name. | ||
| 1. ! PrivateFieldAdd( _error_, _name_, _string_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the above chosen name, PrivateFieldAdd may throw (because the field exists). I think you need to constrain the name further somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, I don't see any reason to use a private field here since there's no other consumers of it. I like @bakkot's suggestion of just closing over the data in both ACs.
spec.emu
Outdated
| 1. Else, | ||
| 1. Let _string_ be an implementation-defined string that represents the current stack trace. | ||
| 1. Perform ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _string_). | ||
| 1. Perform an implementation-defined choice of either: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an editorial nit, but I would prefer a step more like
Let _strategy_ be an implementation-defined choice of either ~data-property~ or ~getter~.
and then typical constructions following that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I don't know if you want to permit this choice to be made independently every time Error.captureStackTrace is called (as it would with both what you've written and my suggestion) or if you want that choice to be consistent within some unit of execution. For example, it may be a field of the Agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how having it a field of the agent change anything, unless that field is restricted from changing. Such restriction could similarly apply to algorithmic steps, no? I thought we had some similar constraints elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this already for endianness as a field of the Agent Record:
Once the value has been observed it cannot change.
The entire Agent Record should probably have the same constraint, but that's a separate conversation. If we were to do the same for algorithm steps, you'd have to be explicit about the unit of execution for which it's meant to be consistent. Putting it on the Agent (or some other value like an Environment Record) makes it clear what that unit of execution is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field on agent (for scope) that cannot change works for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't make unobservable constraints like those. I think the widest context we could constrain is an agent cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. What I'm asking is exactly about about what's observable in a realm, and making sure it remains at a minimum deterministic.
I would prefer if the choice was made solely at realm or agent creation time, but I could be convinced that the choice could be made dependent on the target (I can't imagine a use case so I'd prefer not to). I don't want it to be a random choice every invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was responding to your edit:
if the implementation is going to produce an accessor under some circumstances, I want it to always produce an accessor for those circumstances
We can't say "an implementation must always do it the same way", but we can say that it would never be observable that an implementation made different choices because those programs are being executed in separate agent clusters and thus can never communicate.
I don't want it to be a random choice every invocation.
Okay then we should probably put the field on the Agent Record like I suggested and make a guarantee analogous to this existing one for [[LittleEndian]]:
All agents within a cluster must have the same value for the [[LittleEndian]] field in their respective Agent Records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my edit was meant to relax my requirement that within an agent a different choice could be made as long as it's deterministic. Really not my preference so let's not relax this unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion of using an Agent Record field.
To clarify, the change as currently proposed in this PR does not fully reflect what v8 is doing, which is a good thing. We are one of the objectors to private properties as currently implemented by v8. To summarize our objection to accessors is that there cannot be getter/setter pairs that work to modify and read result of modifications on different objects. That means that either:
Please note these options have differences observable to the program so I don't think we should allow a choice of them to the implementation (this PR seem to currently prescribes the first one, with what looks like a shared getter). |
|
This doesn't seem like a good approach - if it can be either one, then we have an opportunity to force one choice or the other, and we should always be minimizing implementation differences. |
|
I think this is a pragmatic way to address potential future spec changes regarding error stacks, as discussed in plenary last time. If we ever want to standardize something like |
|
imo it would be better to have a willful violation in a single browser than to allow implementations this much latitude. |
Can you elaborate. I didn't mange to spot the divergence? (Or are you referring to Update: ah our setter writes to the private field. is that what you are referring to? |
spec.emu
Outdated
| 1. Let _string_ be an implementation-defined string that represents the current stack trace. | ||
| 1. Perform ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _string_). | ||
| 1. Perform an implementation-defined choice of either: | ||
| 1. Perform ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _string_).. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To bring the two cases closer together it would be nice if both cases go through the same definition primitive. E.g., we could have an implementation specific PropertyDescriptor ([[Get]] vs. [[Value]]) which is then installed in the same way (e.g., OrdinaryDefineOwnProperty) in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer all of the things that share the behaviour described in SetterThatIgnoresPrototypeProperties go through that AO. It's a very specific exception to how we usually do things in the language and it should both have fully consistent normative requirements and be discoverable. Reusing that AO helps with both goals.
Yes, I was attempting to follow Mathieu's constraint that the setter not write to the private field, in this comment #1 (comment), to avoid having to specify closures like bakkot suggested. This seemed like the smallest delta from the current behaviour that would be acceptable. I'm ok with specifying closures as long as V8 is, as they'd have to change their implementation. It currently uses the same getter and setter across instances. |
I'm sympathetic to this point of view, my initial idea was that we should just specify one behaviour. That said, I think this is pragmatic, and that it's better for web developers to have an implementation-defined choice of two behaviours that are fully specified, rather than having an implementation continue to ship unspecified behaviour, or everyone continuing to ship unspecified behaviours. |
Very much, yes. Please see https://issues.chromium.org/issues/40279506 and #1 for why this is a problem. FWIW we would like the own |
|
I've updated this with the suggestions to use an agent record field to control behaviour and closures for the getter and setter. |
spec.emu
Outdated
| </thead> | ||
| <tr> | ||
| <td>[[UseErrorCaptureStackTraceDataProperty]]</td> | ||
| <td>a Boolean</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: since this isn't a yes/no, it's an either/or, we would typically use a 2-state enum for this. Something like [[ErrorCaptureStackTraceStrategy]] with type "~data-property~ or ~accessor~".
spec.emu
Outdated
| 1. Perform ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _string_).. | ||
| 1. Else, | ||
| 1. If ? IsExtensible( _error_ ) is *false*, throw a *TypeError* exception. | ||
| 1. Let getterClosure be a new Abstract Closure with no parameters that captures _string_ and performs the following steps when called: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. Let getterClosure be a new Abstract Closure with no parameters that captures _string_ and performs the following steps when called: | |
| 1. Let _getterClosure_ be a new Abstract Closure with no parameters that captures _string_ and performs the following steps when called: |
Similarly for all the bindings below.
spec.emu
Outdated
| 1. If _useErrorCaptureStackTraceDataProperty_ is *true*, then | ||
| 1. Perform ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _string_).. | ||
| 1. Else, | ||
| 1. If ? IsExtensible( _error_ ) is *false*, throw a *TypeError* exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting nit:
| 1. If ? IsExtensible( _error_ ) is *false*, throw a *TypeError* exception. | |
| 1. If ? IsExtensible(_error_) is *false*, throw a *TypeError* exception. |
spec.emu
Outdated
| 1. Return NormalCompletion(_string_). | ||
| 1. Let getter be CreateBuiltinFunction(getterClosure, 0, "", « »). | ||
| 1. Let setterClosure be a new Abstract Closure with parameters (_value_) that captures _error_ and performs the following steps when called: | ||
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). | |
| 1. Perform ? SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). |
This AO can fail if the setter is called after the "stack" property is deleted, for example.
spec.emu
Outdated
| 1. Return NormalCompletion(_string_). | ||
| 1. Let getter be CreateBuiltinFunction(getterClosure, 0, "", « »). | ||
| 1. Let setterClosure be a new Abstract Closure with parameters (_value_) that captures _error_ and performs the following steps when called: | ||
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). | |
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, *null*, *"stack"*, _value_). |
This is just used for a SameValue test against _error_, so you just need to pass anything else.
spec.emu
Outdated
| 1. Return NormalCompletion(_string_). | ||
| 1. Let getter be CreateBuiltinFunction(getterClosure, 0, "", « »). | ||
| 1. Let setterClosure be a new Abstract Closure with parameters (_value_) that captures _error_ and performs the following steps when called: | ||
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to return something here.
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). | |
| 1. Perform ! SetterThatIgnoresPrototypeProperties(_error_, OrdinaryObjectCreate(*null*), *"stack"*, _value_). | |
| 2. Return NormalCompletion(*undefined*). |
spec.emu
Outdated
| </thead> | ||
| <tr> | ||
| <td>[[ErrorCaptureStackTraceStrategy]]</td> | ||
| <td>DATAPROPERTY or ACCESSOR</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax for spec enums is ~ on either side. Convention is to use lower-kebab-case.
| <td>DATAPROPERTY or ACCESSOR</td> | |
| <td>~data-property~ or ~accessor~</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that is what you suggested originally. I was thrown off by https://tc39.es/ecma262/#sec-enum-specification-type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep they get rendered in all caps but that's not how they look in the markup.
spec.emu
Outdated
| </table> | ||
| </emu-table> | ||
|
|
||
| <p>Once the values of [[Signifier]], [[IsLockFree1]], [[IsLockFree2]], and [[UseErrorCaptureStackTraceDataProperty]] have been observed by any agent in the agent cluster they cannot change.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <p>Once the values of [[Signifier]], [[IsLockFree1]], [[IsLockFree2]], and [[UseErrorCaptureStackTraceDataProperty]] have been observed by any agent in the agent cluster they cannot change.</p> | |
| <p>Once the values of [[Signifier]], [[IsLockFree1]], [[IsLockFree2]], and [[ErrorCaptureStackTraceStrategy]] have been observed by any agent in the agent cluster they cannot change.</p> |
Need to update this field name here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
editorially LGTM otherwise
Thank you for all the help with this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good!
Co-authored-by: Michael Ficarra <github@michael.ficarra.me>
|
FYI after a quick internal discussion, we may actually prefer the shared accessor with internal slot approach. The reason is that with the closure approach there is no way to recognize these accessors implement a "safe behavior" of only revealing a stack string, whereas with shared accessor we can identity check the accessors are the ones installed by capture stack trace. |
Would that require every object to have an internal slot for the stack string, just in case someone calls |
No but it does require checking if the object is extensible before adding the internal slot. I don't think there's a problem with adding internal slots to existing objects, but we do want to avoid mutating non extensible objects. |
|
The spec says "Unless explicitly specified otherwise, internal slots are allocated as part of the process of creating an object, Symbol, or Private Name and may not be dynamically added." so I guess we can add an internal slot to an existing object, although I'm struggling to find a place in the spec where we do this right now. |
|
@mhofman What would the setter behaviour be in this case? |
Yeah I guess I don't know how this would look like editorially. The original
I expect it would unconditionally define a |
|
I just realized that I misread the Chrome implementation, instead of a private field to store the stack trace, for the property key they use a "private" built-in symbol that's not otherwise used or accessible to user code. That seems like it could be a viable approach specification-wise as well, I'll try updating the PR accordingly. |
I don't think that's true. The spiritual equivalent to a v8 private symbol is an internal slot in the spec. Using a JS symbol in the spec would mean exposing it to user code (proxy and/or listing own props). The spec only deals in observable behaviors, we just need a way to express this editorially, even if implementations don't actually append their direct equivalent to an internal slot. |
spec.emu
Outdated
| 1. Let _getter_ be a built-in function that takes no arguments and performs the following steps when called: | ||
| 1. Return ? Get(*this* value, _captureStackSymbol_). | ||
| 1. Let _setter_ be a built-in function that takes an argument _value_ and performs the following steps when called: | ||
| 1. Perform ? DefinePropertyOrThrow(*this* value, *"stack"*, PropertyDescriptor { [[Value]]: _value_, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true }). | ||
| 1. Return NormalCompletion(*undefined*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want these accessors to be explicitly shared (same identity), I think we need them to be defined as intrinsics. Which makes this editorially interesting since they'd be optional? Maybe it's fine to unconditionally declare them with just a note mentioning that if the implementation doesn't use accessor for the ErrorCaptureStackTraceStrategy, then these accessors are never used?
This reverts commit 9bbfb59.
|
Ok, I'll revert the changes from yesterday for now. |
|
I'm going to merge this is being an explored design direction and basis for further conversation. The changes that Mathieu is suggesting seem significant enough to explore in a separate PR or through discussion at committee. |
|
#10 (comment) is still my belief, ftr. |
|
@ljharb the problem is that the shape of the violations can be more of less harmful. The web reality is that some major js engines do want an accessor here, and by accepting that, we get to specify that these are not harmful. |
|
An accessor is the obvious choice, at least for Errors. What's the argument for permitting a data property? |
I didn't spot that question. If we are already going down the painful path of adapting the spec to the diverging web reality, then it doesn't make sense to spec something that is different than said reality. I don't think I like the idea of having closures with identity. This is a costly choice as they have to be allocated and then we loose all the benefit of not eagerly creating the string. And it is also different from what a stack accessor on default error objects prototype would look like. |
This is an attempt to reflect web reality and allow an implementation-defined choice of whether to use a property (like SpiderMonkey and JSC) or accessors (like V8). Allowing an implementation-defined choice came up in a couple of conversations, including #1 (comment).
The accessor sets a private property, which is what V8 currently does. If I recall correctly, the objection to a private property came from JSC, but if we go with this direction, JSC could continue to use a data property, so hopefully this won't be problematic. If it turns out to be objectionable, we could instead use closures, as was suggested by bakkot, see #1 (comment).