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

Implement undefined-like exotic objects #673

Merged
merged 10 commits into from
Aug 18, 2017
Merged

Implement undefined-like exotic objects #673

merged 10 commits into from
Aug 18, 2017

Conversation

bterlson
Copy link
Member

Closes #668. Still not convinced we want this in 262 but here it is so we can discuss!

spec.html Outdated
@@ -4074,6 +4074,8 @@
1. Return the result of performing Strict Equality Comparison _x_ === _y_.
1. If _x_ is *null* and _y_ is *undefined*, return *true*.
1. If _x_ is *undefined* and _y_ is *null*, return *true*.
1. If _x_ is an undefined-like exotic object and _y_ *null*, *undefined*, or an undefined-like exotic object, return *true*.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and y null

Missing verb ("is").

@littledan
Copy link
Member

LGTM

spec.html Outdated
@@ -4072,8 +4072,7 @@
<emu-alg>
1. If Type(_x_) is the same as Type(_y_), then
1. Return the result of performing Strict Equality Comparison _x_ === _y_.
1. If _x_ is *null* and _y_ is *undefined*, return *true*.
1. If _x_ is *undefined* and _y_ is *null*, return *true*.
1. If _x_ is either *null*, *undefined*, or an undefined-like exotic object and _y_ is either *null*, *undefined*, or an undefined-like exotic object, return *true*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this benefit from a new abstract operation that emulates == null, and encapsulates all three of these comparisons?

Copy link
Contributor

@claudepache claudepache Aug 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed semantics seems bad to me, because it means that document.all’s from distinct frames will be reputed equal. Not unrelatedly, it breaks the invariant x === y iff typeof x == typeof y && x == y. (nevermind) We should optimise for sanity rather than conciseness:

  1. If x is undefined/null and y is undefined/null, return true.
  2. If x is a ULEO and y is undefined/null, return true.
  3. If x is undefined/null and y is a ULEO, return true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking if IsNullish(x) and IsNullish(y), return true. and define IsNullish as "if x is null, undefined, or an ULEO, return true. else, return false".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb In case it was not clear, an abstract operation doesn’t change my “document.all’s from distinct frames appear equal” issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claudepache Oh I commented on this before on IRC. Because of Step 1. different objects are always regarded as different, because they have the same type and the strict equality operation doesn't care about "undefined-like".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of Step 1. different objects are always regarded as different, because they have the same type and the strict equality operation doesn't care about "undefined-like".

Ah, ok. However, I'm not sure it is good to sacrifice clarity for conciseness. The formulation of the current spec text:

  1. If x is null and y is undefined, return true.
  2. If x is undefined and y is null, return true.

has the advantage of not interfering with step 1. So, just adding:

  1. If x is an undefined-like exotic object and y is undefined or null, return true.
  2. If x is undefined or null and y is an undefined-like exotic object, return true.

seems clearer to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree very much that this is superior given the confusion. Thanks!!

@chicoxyzzy
Copy link
Member

chicoxyzzy commented Aug 17, 2016

Isn't it better to use something like (currently inactive) proposal Typed Objects (which makes operators overloading possible) for this type of things?

nevermind

spec.html Outdated
Object (undefined-like exotic object)
</td>
<td>
`"undefined"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That row must go before the “Object (implements [[Call]]): "function"” row, because document.all is callable. :-(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, thanks :-P

@bterlson bterlson added the needs consensus This needs committee consensus before it can be eligible to be merged. label Aug 18, 2016
@bterlson
Copy link
Member Author

Fixed the issues @claudepache raised.

@bterlson
Copy link
Member Author

Review feedback: HTML Document Dot All Exotic Object.
Put in Annex B.

spec.html Outdated
<h1>Undefined-like Exotic Objects</h1>
<p>An <dfn>undefined-like exotic object</dfn> is an exotic object that behaves like undefined in the <emu-xref href="#sec-toboolean">ToBoolean</emu-xref> and <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> abstract operations and when used as an operand for the <emu-xref href="#sec-typeof-operator">typeof operator</emu-xref>. Undefined-like exotic objects have all the ordinary object internal methods and slots.</p>
<emu-note>
<p>Undefined-like exotic objects are never created by this specification. However, the <a href="https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all">document.all object</a> in web browsers is a host-created undefined-like exotic object that exists for web compatibility purposes. There are no other known examples of this type of object.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: document.all<code>document.all</code>

@annevk
Copy link
Member

annevk commented Aug 12, 2017

It seems the "needs consensus" label can be removed?

@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Aug 12, 2017
@littledan
Copy link
Member

Well, we have consensus for something, but not for this patch. The consensus is described in this comment.

@annevk
Copy link
Member

annevk commented Aug 16, 2017

@bterlson could you maybe up the priority for this a bit? There's a plan to touch HTMLAllCollection again (to move over legacycaller from IDL to HTML) and it'd be nice to fix this at the same time.

Also, will the <dfn> that's being added get an ID or would that need to be added manually? It'd probably be useful to have something to point to directly.

@bterlson
Copy link
Member Author

@annevk I will work on this today. Was really hoping we could get away without this but alas. It'll be kinda finicky to move to Annex B (mostly in turning algorithm changes into Annex B mods) but I should be able to complete it today for your review.

The dfn will link to the parent clause ID without its own ID. Would that work for you?

@annevk
Copy link
Member

annevk commented Aug 16, 2017

Yeah, that's probably good enough. Still need to figure out exactly how the layering will work.

@bterlson
Copy link
Member Author

Rebased, renamed, moved to Annex B with diff pointers. @annevk care to review?

spec.html Outdated
<h1>Changes to Abstract Equality Comparison</h1>
<p>The following steps are inserted after step 3 of the <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> algorithm:</p>
<emu-alg>
1. If _x_ is an undefined-like exotic object and _y_ is either *null* or *undefined*, return *true*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML document dot all exotic object

@bterlson
Copy link
Member Author

After talking with @domenic, I made some changes. Biggie is instead of defining a separate object, I simply say that there exists a internal slot of a particular name and the annex B algorithms check for this internal slot rather than doing a "type check". LMK your thoughts. /cc @annevk

spec.html Outdated
<emu-annex id="sec-html-document-dot-all-exotic-object-aec">
<h1>Changes to Abstract Equality Comparison</h1>
<p>The following steps are inserted after step 3 of the <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> algorithm:</p>
<emu-alg start=4>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I thought this did something but it doesn't. Guess I'll delete it.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely, just some nits.

spec.html Outdated
@@ -39904,6 +39904,60 @@ <h1 id="ao-issuperreference">IsSuperReference ( _V_ )</h1>
1. Return ? ForIn/OfBodyEvaluation(|BindingIdentifier|, |Statement|, _keyResult_, ~enumerate~, ~varBinding~, _labelSet_).
</emu-alg>
</emu-annex>

<emu-annex id="sec-ishtmldda-internal-slot">
<h1>The [[isHTMLDDA]] Internal Slot</h1>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uppercase to match other internal slot names?

spec.html Outdated

<emu-annex id="sec-ishtmldda-internal-slot">
<h1>The [[isHTMLDDA]] Internal Slot</h1>
<p>An <dfn>[[isHTMLDDA]] internal slot</dfn> may exist on implementation-defined exotic objects. Objects with an [[isHTMLDDA]] intenral slot behave like undefined in the <emu-xref href="#sec-toboolean">ToBoolean</emu-xref> and <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> abstract operations and when used as an operand for the <emu-xref href="#sec-typeof-operator">typeof operator</emu-xref>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "intenrel"

typeof still not codeified. Actually just using title="" attribute might work better? For most of these xrefs? Maybe even it's not required at all for ToBoolean?

@bterlson
Copy link
Member Author

@domenic ok nits should be fixed

spec.html Outdated

<emu-annex id="sec-IsHTMLDDA-internal-slot">
<h1>The [[IsHTMLDDA]] Internal Slot</h1>
<p>An <dfn>[[IsHTMLDDA]] internal slot</dfn> may exist on implementation-defined exotic objects. Objects with an [[IsHTMLDDA]] internal slot behave like undefined in the <emu-xref href="#sec-toboolean">ToBoolean</emu-xref> and <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> abstract operations and when used as an operand for the <emu-xref href="#sec-typeof-operator">`typeof` operator</emu-xref>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*undefined* I think?

@bterlson
Copy link
Member Author

@annevk seem ok to you? Seems like I can't re-request a review.

spec.html Outdated

<emu-annex id="sec-IsHTMLDDA-internal-slot">
<h1>The [[IsHTMLDDA]] Internal Slot</h1>
<p>An <dfn>[[IsHTMLDDA]] internal slot</dfn> may exist on implementation-defined exotic objects. Objects with an [[IsHTMLDDA]] internal slot behave like *undefined* in the <emu-xref href="#sec-toboolean">ToBoolean</emu-xref> and <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> abstract operations and when used as an operand for the <emu-xref href="#sec-typeof-operator">`typeof` operator</emu-xref>.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be “object(s)” rather than “exotic object(s)” (here and everywhere below).

Although the behaviour described in this section is very “exotic” according the common meaning of that word, it does not make an exotic object in the sense of the definition given by the spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still fair, since HTMLAllCollection is an exotic object (due to custom [[Call]] and some other stuff) and it can only be used there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @claudepache is right - the object may be exotic depending on host choices, but it's confusing to say its exotic when it doesn't define anything exotic. Might trigger someone to go sleuthing for hours trying to find the missing exoticness. I'll fix it.

spec.html Outdated
<h1>The [[IsHTMLDDA]] Internal Slot</h1>
<p>An <dfn>[[IsHTMLDDA]] internal slot</dfn> may exist on implementation-defined exotic objects. Objects with an [[IsHTMLDDA]] internal slot behave like *undefined* in the <emu-xref href="#sec-toboolean">ToBoolean</emu-xref> and <emu-xref href="#sec-abstract-equality-comparison">Abstract Equality Comparison</emu-xref> abstract operations and when used as an operand for the <emu-xref href="#sec-typeof-operator">`typeof` operator</emu-xref>.</p>
<emu-note>
<p>Objects with an [[IsHTMLDDA]] internal slot are never created by this specification. However, the <a href="https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-all"><code>document.all</code> object</a> in web browsers is a host-created exotic object with this slot that exists for web compatibility purposes. There are no other known examples of this type of object and implementations should not create any with the exception of `document.all`.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to point to HTMLAllCollection here? document.all is a property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why not must not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think document.all is better. There's nothing special about HTMLAllCollection itself, really. Its instances are the weird ones. And a good way of collectively referring to all such instances is probably "document.all".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk I only went with "should" because this is a note and it seems odd to use strong language in a non-normative section.

@annevk
Copy link
Member

annevk commented Aug 18, 2017

@bterlson thanks, it does.

@bakkot bakkot added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Aug 18, 2017
@bterlson bterlson merged commit a3e06cb into tc39:master Aug 18, 2017
@domenic
Copy link
Member

domenic commented Aug 18, 2017

Woohoo, thank you!!

@Jack-Works
Copy link
Member

To let it evaluate to false in every case is harmful to web security. I have a suggestion that keeps the web compatibility meanwhile reduce the harm of falsy document.all.

tc39/proposal-preserve-virtualizability#2 (comment)

@claudepache
Copy link
Contributor

@Jack-Works See 12.3.6.1 step 6 for how direct eval is specced. If you want to do the same thing for document.all, you need to add similar logic in the runtime semantics of:

  • the typeof operator;
  • operators based on boolean logic: ||, &&, ? :, !;
  • operator allowing comparison with null: ==;
  • statements making use of boolean logic: if, while, do/while, for.

(Fortunately, the newer ?. and ?? operators do not have special treatment of document.all.)

@Jack-Works
Copy link
Member

Yes, but it will no longer be a threat to the security.

@claudepache
Copy link
Contributor

In fact, I’m not exactly sure that the approach I had in mind in my previous comment would work or would be the best. But more to the point:

The current PR was specifically to sanction what browsers and the HTML spec were already implementing. That is, they felt it was better that the ES spec provided the necessary hook for the HTML spec, rather than the HTML spec monkey-patched the ES spec.

What you are proposing, is modifying the semantics of document.all in a nontrivial way, which implies implementation work and BC concern. If you feel that it is important, it should be a separate issue, and probably indeed a full-fledged proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. needs editorial changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

document.all willful violation