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

Normative: move __proto__ out of annex b #2125

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Aug 5, 2020

No description provided.

@devsnek devsnek force-pushed the __proto__-shenanigans branch 3 times, most recently from 9a7d36d to 3572cef Compare August 5, 2020 00:33
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@Jack-Works
Copy link
Member

Is it still optional for non-web platforms?

@devsnek
Copy link
Member Author

devsnek commented Aug 5, 2020

@Jack-Works Object.prototype.__proto__ is normative optional due to security concerns but { __proto__: ... } is not.

@Jack-Works
Copy link
Member

I don't find what benefit we can get from the __proto__ literal 🤔

@devsnek
Copy link
Member Author

devsnek commented Aug 5, 2020

@Jack-Works we have previous consensus to move items out of annex b and into the main spec where possible.

spec.html Outdated
</emu-clause>
</emu-clause>

<emu-clause id="sec-object.prototype.__defineGetter__">
Copy link
Member

Choose a reason for hiding this comment

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

Why would we want the legacy define methods to be normative required??

The vast majority of annex b should remain normative optional, and anything that’s definitely only needed on the web imo should stay in annex b.

Copy link
Member Author

@devsnek devsnek Aug 5, 2020

Choose a reason for hiding this comment

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

🤷 no one had any reason to make these ones optional, unlike the __proto__ accessor. even quickjs and xs implement __defineGetter__ and whatnot.

Copy link
Member

Choose a reason for hiding this comment

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

#1JS

Copy link
Member

Choose a reason for hiding this comment

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

The reason - one I hold for many parts of Annex B - is that these things are bad and should never have existed, and more importantly, are not universally used.

__proto__ is used everywhere, in non-web code as well. __defineGetter_ and friends are not.

Copy link
Contributor

Choose a reason for hiding this comment

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

__defineGetter__ and friends are sufficiently widely used that any implementation intending to run JavaScript as it exists in the world needs to implement them. Writing down those things is the whole point of having a standard. I don't see the point of omitting them.

Incidentally, that's also what I understood to be consensus when this was discussed in plenary.

Copy link
Member

Choose a reason for hiding this comment

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

@ljharb I know the history of __proto__. The point I was making is that there's parts of the JavaScript standard that we now, in hindsight, consider to be mistakes (even outside of Annex B). Whether they were intentionally designed or organically became part of the standard after first becoming a de facto standard doesn't matter. It feels weird to special-case features just because they used to be in Annex B.

Choose a reason for hiding this comment

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

FWIW, I wish __proto__ was completely removed from specs, and from any engine. To date it caused more troubles than it supposed to solve, and both Object and Reflect have dedicated methods that replace it already.

The only usage I kinda think is reasonable, is for one-off literals creation, and literally nothing else: {__proto__: Shared, counter: 0} ... that's it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb so I just need to add a note that says we don't like them?

Copy link
Contributor

@ExE-Boss ExE-Boss Aug 5, 2020

Choose a reason for hiding this comment

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

The __proto__ accessor and other %Object.prototype% __‑prefixed and suffixed properties should remain in Annex B, as they have the much saner {Object,​Reflect}.{defineProperty,​getOwnPropertyDescriptor,​getPrototypeOf,​setPrototypeOf} alternatives.

In fact, in some old engines the __lookup*__ functions will return an accessor even if it’s shadowed by a data property.

Copy link
Member

Choose a reason for hiding this comment

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

@mathiasbynens that's totally true, but the entire purpose of Annex B was always "this is a special case we don't want in the actual language, and we don't want people using, but is here for web compat". In other words, I'm saying that "being in Annex B" is already being special-cased, and i don't want that indiscriminately lost.

@devsnek just like "normative optional" is a special tag, i'd want some kind of "deprecated" tag, that referenced a big scary note saying these are bad and if you use them you should feel bad, yes.

spec.html Outdated Show resolved Hide resolved
@ExE-Boss
Copy link
Contributor

This PR currently has unresolved issues and merge conflicts that prevent merging it.

Also, the lint issues have been fixed in tc39/ecmarkup#239.

@phoddie
Copy link

phoddie commented Sep 11, 2020

...even quickjs and xs implement defineGetter and whatnot...

FWIW – we held out on supporting __defineGetter__ and __proto__ in XS for a very long time. If I recall correctly, it was test262 that drove us to implement them. We would be happy to have them remain in Annex B.

@devsnek
Copy link
Member Author

devsnek commented Sep 11, 2020

in case it was not clear, they are still normative optional.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Sep 15, 2020

Since this has several merge conflicts, moves other legacy __‑prefixed properties of Object.prototype out of Annex B, instead of just the “__proto__ Property Names in Object Initializers” bit (and that’s currently done in a way that breaks JSON text, ObjectAssignmentPattern, CoverParenthesizedExpressionAndArrowParameterList and CoverCallExpressionAndAsyncArrowHead), I’ve rebased this and opened #2175.

@devsnek
Copy link
Member Author

devsnek commented Sep 15, 2020

I will update this if there is reason to do so (after we discuss it at TC39). There is no reason to open another PR.

Copy link
Member

@zeldajay zeldajay left a comment

Choose a reason for hiding this comment

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

Direct removal is not the best solution, we should seek a better one.

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Sep 21, 2020
@ljharb
Copy link
Member

ljharb commented Sep 21, 2020

Summarizing today's consensus:

  • proto syntax: required, non-"icky"
  • proto accessor: optional, "icky"
  • lookup/define methods: optional, "icky"

where "icky" conveys the existing sentiment for everything in Annex B found here:

All of the language features and behaviours specified in this annex have one or more undesirable characteristics and in the absence of legacy usage would be removed from this specification.

Additionally, there was interest expressed in noting which optional things were encouraged to be implemented when an engine's goals are interop; versus which "icky" things are discouraged for practitioners to use.

(I'll update the PR's consensus label when it matches that consensus)

@devsnek
Copy link
Member Author

devsnek commented Sep 21, 2020

Will update shortly...

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Sep 21, 2020

@devsnek Feel free to use my #2175 normative/object-proto-property-intializer branch as a base.

@phoddie
Copy link

phoddie commented Sep 21, 2020

Commenting on the summary from @ljharb, ECMA-402 contains text which appears to define its use of "normative optional" to mean "Annex B":

This specification uses blocks demarcated as Normative Optional to denote the sense of Annex B in ECMA-262. That is, normative optional sections are required when the ECMAScript host is a web browser. The content of the section is normative but optional if the ECMAScript host is not a web browser.

With this PR, TC39 would be using "normative optional" for the first time. It would be preferable to have a consistent definition of "normative optional", if possible, between ECMA-262 and ECMA-402.

@ExE-Boss
Copy link
Contributor

Alternatively, perform the move to use “normative optional” in a future PR and keep the legacy properties on %Object.prototype% in Annex B for now.

@ljharb ljharb requested a review from jmdyck July 13, 2021 18:18
ljharb added a commit to devsnek/ecma262 that referenced this pull request Jul 13, 2021
 - co-opts prose from Annex B description
 - dfn legacy and add emu-not-ref where appropriate
 - use dash in IDs
 - don't <dfn> legacy; clauses are what is legacy

Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Michael Ficarra <mficarra@shapesecurity.com>
ljharb added a commit to devsnek/ecma262 that referenced this pull request Jul 13, 2021
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Looks good. (I'll try not to think about this any more.)

spec.html Outdated Show resolved Hide resolved
@ljharb ljharb added the editor call to be discussed in the next editor call label Jul 21, 2021
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jul 22, 2021
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 22, 2021
ljharb and others added 2 commits July 21, 2021 21:55
 - co-opts prose from Annex B description
 - dfn legacy and add emu-not-ref where appropriate
 - use dash in IDs
 - don't <dfn> legacy; clauses are what is legacy

Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Michael Ficarra <mficarra@shapesecurity.com>
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
@ljharb ljharb merged commit 1d70aa7 into tc39:master Jul 22, 2021
@devsnek devsnek deleted the __proto__-shenanigans branch July 22, 2021 05:08
@devsnek
Copy link
Member Author

devsnek commented Jul 22, 2021

🎉

mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
 - co-opts prose from Annex B description
 - dfn legacy and add emu-not-ref where appropriate
 - use dash in IDs
 - don't <dfn> legacy; clauses are what is legacy

Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Michael Ficarra <mficarra@shapesecurity.com>
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Co-authored-by: Michael Dyck <jmdyck@ibiblio.org>
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. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.