Update attribute setter/getters in various ways #217

domenic · 2016-11-01T19:21:38Z

Fixes getters on the global when no explicit this is passed, similarly to what was already done for operations in a2b4599. Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=29421.
Make promise getters reject in case of an error. Disallow promise types on attributes that will generate setters. Fixes Should promise-returning attributes reject in case of wrong this value? #186 and fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25048.
Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27905 by more clearly specifying the target (as was done previously for operations).
Updated to use modern ES Get/Set abstract operations instead of [[Get]] and [[Put]].
Also made minor fixes to the operations definition, e.g. wrapping it in <div algorithm> and remembering to actually return F.

Preview | Diff w/ current ED | Diff w/ base

bzbarsky · 2016-11-08T20:40:32Z

index.bs

@@ -1385,6 +1385,10 @@ ignored or an exception is thrown.
    };
 </pre>

+The type of the attribute that is not [=read only=] must not be a [=promise type=]. Additionally,


"The type of an attribute"?

bzbarsky · 2016-11-08T20:43:10Z

index.bs

+                    <!-- https://www.w3.org/Bugs/Public/show_bug.cgi?id=18547#c9 -->
+                1.  Otherwise, set |O| to the <emu-val>this</emu-val> value.
+                1.  If |O| is a [=platform object=], then [=perform a security check=], passing |O|,
+                    |realm|, |id|, and "method".


"getter", not "method", right?

bzbarsky · 2016-11-08T20:44:51Z

index.bs

+
+        1.  If |attribute|'s type is a [=promise type=], then:
+            1.  Let |reject| be the initial value of [=%Promise%=].reject.
+            1.  Return the result of calling |reject| with [=%Promise%=] as the


Did we ever decide how this should interact with [SameObject]? Seems to me like we should just disallow [SameObject] on Promise-returning attributes, or change the definition of [SameObject] or something; the current definition of [SameObject] just would not match this new behavior.

I'm still not 100% sure we want to make this behavior change, for what it's worth. What do UAs do in practice?

Fixed [SameObject] to reflect this.

In practice UAs are waiting for us to make a decision here; they throw the error per the current spec but are uncomfortable with doing so.

Fixed [SameObject] to reflect this.

I would really rather we just disallowed using [SameObject] on Promise-valued attributes if we take this route, given the weird behavior. If we do allow it, UAs have to start doing something weird themselves, because they can no longer trust [SameObject] annotations...

In particular, it's not obvious that these changes to [SameObject] are compatible with #212

they throw the error per the current spec but are uncomfortable with doing so.

I don't think Gecko is uncomfortable. It sounds like Blink is? Do you have references for the other UAs, by any chance?

I would really rather we just disallowed using [SameObject] on Promise-valued attributes if we take this route, given the weird behavior.

I really don't think it's that weird. It's about the exception case. You don't require the same exception objects be thrown each time, so similarly, you shouldn't require the same promise object be returned each time.

Do you have references for the other UAs, by any chance?

WebKit: #186

I really don't think it's that weird. It's about the exception case.

The problem is that unlike sync functions, which have a return value and a possible exception which are exposed to the caller in different ways, Promise-returning functions use a return value to represent both things. This makes [SameObject] a pretty confusing thing to start with: is it referring to the Promise object returned, or to its resolution value in the non-exception case, for example? If we really reason by analogy to sync functions, it should be referring to the resolution value... but in practice people and the spec use it to mean the actual Promise return value. Which doesn't even have an explicit equivalent in the sync function case in JS; it would be a continuation in languages that have such things. So any reasoning by analogy here is broken right now anyway.

It's worth taking a step back for a second and asking what the point of [SameObject] really is and why we have it at all, then trying to figure out what it means to map that concept to promise-returning getters. Assuming that's really possible.

WebKit: #186

Thank you. And Edge?

It's worth taking a step back for a second and asking what the point of [SameObject] really is and why we have it at all, then trying to figure out what it means to map that concept to promise-returning getters. Assuming that's really possible.

Right. I perhaps am not the best person for this, since I don't like these documentation-only extended attributes. But I tried to put myself in the shoes of someone who does, and an interpretation of "returns the same object in non-exceptional cases" seemed like a good thing to document.

Thank you. And Edge?

I haven't seen any preferences from them.

OK. Well, as another point of view, with my JIT implementor hat on, after this change all Promise-returning cases would be non-exceptional cases (because from a JIT's point of view they are), so my JIT would have to know that [SameObject] doesn't mean "the same object" in Promise-returning cases and hence can't be optimized the same way as other [SameObject] annotations...

bzbarsky · 2016-11-08T20:47:19Z

index.bs

+                <!-- https://www.w3.org/Bugs/Public/show_bug.cgi?id=18547#c9 -->
+            1.  Otherwise, set |O| to the <emu-val>this</emu-val> value.
+            1.  If |O| is a [=platform object=], then [=perform a security check=], passing |O|,
+                |realm|, |id|, and "method".


"setter", not "method".

bzbarsky · 2016-11-08T20:54:43Z

index.bs

-1.  Perform [=!=] [=DefinePropertyOrThrow=](|F|, "length",
-    PropertyDescriptor<span class="prop-desc">{\[[Value]]: |length|, \[[Writable]]: <emu-val>false</emu-val>, \[[Enumerable]]: <emu-val>false</emu-val>, \[[Configurable]]: <emu-val>true</emu-val>}</span>).
+<div algorithm>
+    To <dfn id="dfn-create-operation-function" lt="creating an operation function">create an operation function</dfn>,


Wish this reindenting were a separate commit....

Separated out into #232, but this PR for now is on top of that so it's included here.

tobie · 2016-11-19T11:18:27Z

index.bs

@@ -1374,6 +1374,10 @@ ignored or an exception is thrown.
    };
 </pre>

+The type of an attribute that is not [=read only=] must not be a [=promise type=]. Additionally,
+the type of an attribute with any of the [=extended attributes=] [{{LenientThis}}],
+[{{PutForwards}}], [{{Replaceable}}], or [{{SameObject}}] must not be a [=promise type=].


I thought there were requirements for [{{SameObject}}] promise type attributes. E.g. for https://drafts.csswg.org/css-font-loading/#font-face-set-ready, no?

Yes, but @bzbarsky prefers we don't use them, as discussed upthread.

- Fixes getters on the global when no explicit `this` is passed, similarly to what was already done for operations in a2b4599. Closes https://www.w3.org/Bugs/Public/show_bug.cgi?id=29421. - Make promise getters reject in case of an error. Disallow promise types on attributes that will generate setters. Fixes #186 and fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25048. As part of this, disallowed [SameObject] for promise-returning attributes, since different promises will be returned if the getter is used incorrectly. - Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=27905 by more clearly specifying the target (as was done previously for operations). - Updated to use modern ES Get/Set abstract operations instead of [[Get]] and [[Put]]. - Also made minor fixes to the operations definition, e.g. wrapping it in <div algorithm> and remembering to actually return F.

tobie · 2017-01-19T07:50:43Z

index.bs

@@ -1377,6 +1377,10 @@ ignored or an exception is thrown.
    };
 </pre>

+The type of an attribute that is not [=read only=] must not be a [=promise type=]. Additionally,
+the type of an attribute with any of the [=extended attributes=] [{{LenientThis}}],
+[{{PutForwards}}], [{{Replaceable}}], or [{{SameObject}}] must not be a [=promise type=].


I think the below means the same thing and is clearer:

Attributes whose type is [=promise type|promise=] must be [=read only=]. Additionally, they cannot have any of the [=extended attributes=] [{{LenientThis}}], [{{PutForwards}}], [{{Replaceable}}], or [{{SameObject}}].

tobie · 2017-01-19T08:31:05Z

index.bs

+        the object that is the location of the property; and
+    *   |S| is the [=attribute setter=] created given the attribute, the interface (or the interface
+        it's being mixed in to, if the interface is actually a mixin), and the [=relevant Realm=] of
+        the object that is the location of the property.


I'm aware this is purposefully copied from the text in the operations binding, but: s/the object that is the location of the property/the object that contains the property/ would read better.

Maybe change it everywhere? Or not at all for now also works. :)

I'll go with not changing it for now as this area is related to the mixin mess and will get straightened out when that is.

tobie · 2017-01-19T14:09:10Z

index.bs


 Note: Although there is only a single property for an IDL attribute, since
 accessor property getters and setters are passed a <emu-val>this</emu-val>
 value for the object on which property corresponding to the IDL attribute is
 accessed, they are able to expose instance-specific data.

-Note: Note that attempting to assign to a property corresponding to a
+Note: Attempting to assign to a property corresponding to a


Can't/shouldn't we express that normatively?

I'm pretty sure it is. This was just a drive-by fix to not repeat "Note" twice.

For my own education, where is that behavior defined normatively?

tobie · 2017-01-19T14:09:54Z

index.bs

+            1.  Let |R| be the result of performing the actions listed in the
+                description of |attribute| that occur on getting (or those listed in the description
+                of the inherited attribute, if this attribute is declared to
+                [=inherit its getter=]), on |O| if |O| is not <emu-val>null</emu-val>.


I find it hard to parse the meaning of the hanging conditional. I assume it means you carry out the getting in both case, but once it's carried with the |O| as this and the other one with undefined as this?

Well, there's no this formally (that's https://www.w3.org/Bugs/Public/show_bug.cgi?id=27301). The idea is not to provide anything at all; providing undefined isn't really correct.

tobie · 2017-01-19T14:10:12Z

index.bs

+    1.  Assert: |attribute|'s type is not a [=promise type=].
+    1.  Let |steps| be the following series of steps:
+        1.  If no arguments were passed, then
+        1.  Let |V| be the value of the first argument passed.


There's something unclear in the above two steps (either there's no argument or there is one).

tobie · 2017-01-19T14:10:17Z

index.bs

+        1.  Let |V| be the value of the first argument passed.
+        1.  Let |id| be |attribute|'s [=identifier=].
+        1.  Let |O| be <emu-val>null</emu-val>.
+        1.  If |attribute| is not a [=static attribute=]:


If |attribute| is a [=regular attribute=]:

tobie · 2017-01-19T14:10:25Z

index.bs

+        1.  Let |id| be |attribute|'s [=identifier=].
+        1.  Let |O| be <emu-val>null</emu-val>.
+        1.  If |attribute| is not a [=static attribute=]:
+            1.  If the <emu-val>this</emu-val> value is <emu-val>null</emu-val> or


If find all of the substeps of this step 5 super difficult to parse. I don't doubt that the algorithm is correct, but I really struggle to understand its intent. I can't imagine modifying something in here without accidentally breaking stuff.

Here's a proposed alternative (which is slightly less DRY, but hopefully a bit clearer). I added an optional note (prefixed with ?) to clarify the exit point of the substep. Maybe that helps (or is over the top—I'm not sure).

1. If |attribute| is a [=regular attribute=]: 1. If the <emu-val>this</emu-val> value is <emu-val>null</emu-val> or <emu-val>undefined</emu-val>, set |O| to |realm|'s [=Realm/global object=]. (This will subsequently cause a <emu-val>TypeError</emu-val> in a few steps, if the global object does not implement |target| and [{{LenientThis}}] is not specified.)  1. Otherwise, set |O| to the <emu-val>this</emu-val> value. 1. If |O| is a [=platform object=], then [=perform a security check=], passing |O|, |id|, and "setter". 1. Let |validThis| be `true` if |O| is a [=platform object=] that implements the interface |target|, or `false` otherwise. 1. If |validThis| is `true`, then: 1. If |attribute| is declared with a [{{LenientSetter}}] extended attribute, then: 1. Return <emu-val>undefined</emu-val>. 1. If |attribute| is declared with a [{{PutForwards}}] extended attribute, then: 1. Let |Q| be [=?=] [=Get=](|O|, |id|). 1. If [=Type=](|Q|) is not Object, then [=ECMAScript/throw=] a <emu-val>TypeError</emu-val>. 1. Let |forwardId| be the identifier argument of the [{{PutForwards}}] extended attribute. 1. Perform [=?=] [=Set=](|Q|, |forwardId|, |V|). 1. Return <emu-val>undefined</emu-val>. 1. If |attribute| is declared with the [{{Replaceable}}] extended attribute, then: 1. Perform [=?=] [=CreateDataProperty=](|O|, |id|, |V|). 1. Return <emu-val>undefined</emu-val>. ? Note: setters who are not special-cased here will ? continue being defined in the parent steps ? along with static attributes. 1. Otherwise if |attribute| was specified with the [{{LenientThis}}] [=extended attribute=], 1. If |attribute| is declared with the [{{Replaceable}}] extended attribute, then perform [=?=] [=CreateDataProperty=](|O|, |id|, |V|). 1. Return <emu-val>undefined</emu-val>. 1. Otherwise, [=ECMAScript/throw=] a <emu-val>TypeError</emu-val>.

Hmm. I don't really find that clearer; I like how you just check the conditions one after another in the existing version. Let's leave it for a follow-up, as we'd also want to change the analogous steps for operations.

tobie · 2017-01-19T14:10:32Z

index.bs

+            1.  Otherwise, set |O| to the <emu-val>this</emu-val> value.
+            1.  If |O| is a [=platform object=], then [=perform a security check=], passing |O|,
+                |id|, and "setter".
+            1.  Let |validThis| be <emu-val>true</emu-val> if |O| is a [=platform object=] that


Shouldn't the booleans here really be just `true` ans `false`? These are not JS values, they're infra booleans, no?

That's probably more correct I guess. I was thinking this is more like an ES spec algorithm where booleans are emu-val'ed, but this seems like a better place to lean toward web spec conventions than ES.

tobie · 2017-01-19T14:10:46Z

index.bs


-    The value of the <emu-val>Function</emu-val> object’s “length”
-    property is the <emu-val>Number</emu-val> value <emu-val>1</emu-val>.
+            <dl class="switch">


Why are we using a switch here rather than a regular if..otherwise clause?

Not sure. We do it a lot, it seems. I guess because it's determining a value?

tobie · 2017-01-19T14:10:57Z

index.bs

-    property is a <emu-val>String</emu-val> whose value is the
-    concatenation of “set ” and the identifier of the attribute.
+        1.  Perform the actions listed in the description of |attribute| that occur on setting, on
+            |O| if |O| is not <emu-val>null</emu-val>.


Same comments as for the hanging conditional in the getter, here.

I still think this is pretty clear as-is, but we could do a different version perhaps as a follow-up if you have a proposal.

tobie · 2017-01-19T14:22:34Z

A few nits and a few questions. LGTM otherwise.

bzbarsky · 2017-01-20T19:41:22Z

I'm really sorry for the horrible lag here. I finally had a chance to read through these changes, and they look great. There is one preexisting issue that I filed as #271

bzbarsky · 2017-01-20T19:42:43Z

Oh, and filed https://bugs.chromium.org/p/chromium/issues/detail?id=683310 for Blink not doing PutForwards per spec.

bzbarsky · 2017-01-20T19:46:20Z

And https://bugzilla.mozilla.org/show_bug.cgi?id=1332713 on updating Gecko to these changes.

bzbarsky · 2017-01-20T20:02:45Z

Also filed spec bugs on existing [SameObject] promises:

bzbarsky · 2017-01-20T22:54:07Z

And w3c/testharness.js#232 on idlharness no longer matching the spec now. And filed w3c/testharness.js#233 on what's left to do there.

annevk · 2017-01-31T08:37:15Z

It seems https://fetch.spec.whatwg.org/ was missed. It uses [SameObject] for trailers, a feature that isn't implemented yet.

annevk · 2017-01-31T08:38:13Z

Filed whatwg/fetch#473.

bzbarsky · 2017-01-31T08:49:46Z

Right, I only filed bugs on the things I found by disallowing [SameObject] in Gecko's IDL and seeing what failed...

tobie force-pushed the getters-setters branch 2 times, most recently from 474b1a9 to b0bb293 Compare November 1, 2016 22:26

domenic assigned bzbarsky Nov 3, 2016

domenic mentioned this pull request Nov 3, 2016

Getters on a global without an explicit this should throw TypeError (… w3c/testharness.js#183

Closed

bzbarsky reviewed Nov 8, 2016

View reviewed changes

domenic mentioned this pull request Nov 8, 2016

Minor fixups to creating an operation Function #232

Merged

domenic force-pushed the getters-setters branch 5 times, most recently from f978d87 to 69e9c85 Compare November 10, 2016 19:16

tobie reviewed Nov 19, 2016

View reviewed changes

domenic force-pushed the getters-setters branch from 69e9c85 to 60fe543 Compare January 10, 2017 17:35

domenic mentioned this pull request Jan 10, 2017

Add support for attributes on namespaces #263

Closed

domenic force-pushed the getters-setters branch from 60fe543 to 69d629d Compare January 10, 2017 18:41

domenic mentioned this pull request Jan 10, 2017

Attributes on namespaces #264

Merged

tobie reviewed Jan 19, 2017

View reviewed changes

tobie approved these changes Jan 19, 2017

View reviewed changes

Address review comments

9962a71

tobie merged commit 4c9c298 into gh-pages Jan 20, 2017

tobie mentioned this pull request Jan 20, 2017

Require tests for PR/issues? #268

Closed

annevk mentioned this pull request Jan 31, 2017

Cannot use [SameObject] for trailers whatwg/fetch#473

Closed

bzbarsky mentioned this pull request Feb 9, 2017

idlharness.js checks that attribute getters on a global do not require an explicit 'this' w3c/testharness.js#182

Closed

annevk deleted the getters-setters branch August 23, 2017 09:45

annevk mentioned this pull request Aug 23, 2017

[SameObject]/[NewObject] on Promise-type attributes should be allowed #71

Open

wpt-issue-mover mentioned this pull request Aug 31, 2017

idlharness.js checks that attribute getters on a global do not require an explicit 'this' web-platform-tests/wpt#7170

Open

Ms2ger mentioned this pull request Jul 2, 2019

Looking for some clarity on Promise<void> #737

Closed

Update attribute setter/getters in various ways #217

Update attribute setter/getters in various ways #217

Conversation

domenic commented Nov 1, 2016 • edited by tobie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie commented Jan 19, 2017

bzbarsky commented Jan 20, 2017

bzbarsky commented Jan 20, 2017

bzbarsky commented Jan 20, 2017

bzbarsky commented Jan 20, 2017

bzbarsky commented Jan 20, 2017

annevk commented Jan 31, 2017

annevk commented Jan 31, 2017

bzbarsky commented Jan 31, 2017

domenic commented Nov 1, 2016 •

edited by tobie

Loading