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

Editorial: Reduce Annex B's monkey-patching #2952

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Oct 31, 2022

See Issue #1595 for some background. However, that talks about making many Annex B things normative, which this PR doesn't do. This PR takes pseudocode-replacements from Annex B and splices them into the corresponding points in the main body, while maintaining their normative-optional status.

This PR doesn't eliminate all Annex B monkey-patching, because I'm avoiding Annex B features that modify the grammar (due to controversy), i.e.:

  • all of B.1 Additional Syntax
  • B.3.3 FunctionDeclarations in IfStatement Statement Clauses
  • B.3.5 Initializers in ForIn Statement Heads

(It's unclear why the latter two aren't included in B.1, since they define additional syntax.)

That leaves:

  • B.3.1 Labelled Function Declarations
  • B.3.2 Block-Level Function Declarations Web Legacy Compatibility Semantics
  • B.3.4 VariableStatements in Catch Blocks
  • B.3.6 The [[IsHTMLDDA]] Internal Slot

This PR has one commit for each of those sections, plus an extra one at the start to make B.3.2 easier (which might be useful to cherry-pick, even if this PR goes nowhere). This PR is also 'based' on PR #2951, so that'll show up as a commit here until it's merged.


Here's roughly the scheme I followed when eliminating a monkey-patch of an abstract operation. (There are also early error rules that get patched, but the edits are analogous, and simpler.)

Where the status quo has:

MainlineAlg:
  1. [id="step-id"] Do the mainline thing. NOTE: This step is replaced in section (Annex B feature).

Annex B feature:
  During MainlineAlg the following steps are performed in place of step (step-id):
    1. Do the Annex B thing.

this PR changes that to:

MainlineAlg:
  1. [id="step-id"] If <ins normative-optional>the host supports (Annex B feature)</ins>, then
    1. Do the Annex B thing.
  1. Else,
    1. Do the mainline thing.

Annex B feature:
  This feature involves special semantics at step (step-id) in MainlineAlg.

(In some cases, "Do the mainline thing" is empty, which simplifies things.)

Note that the above format of the "host supports" step is a compromise to get the current ecmarkup to accept and render it. What I had in mind instead was something more like:

  1. [id="step-id" normative-optional] If the host supports (Annex B feature), then

ecmarkup could then recognize the "normative-optional" step-attribute and apply appropriate styling for that step and all its substeps.

@bakkot bakkot added the editor call to be discussed in the next editor call label Oct 31, 2022
@jmdyck
Copy link
Collaborator Author

jmdyck commented Oct 31, 2022

Note that, in the rendered spec's CSS, the normative-optional attribute forces its element to have display:block, which doesn't look great when the element isn't block-display to begin with, as is the case for my uses of <ins normative-optional> here.

I was thinking of dropping the normative-optional attribute just to get a nicer temp rendering, but now I'm not sure if I want to bother.

Anyhow, I stress that the build preview is not the final rendering; that will require some changes to ecmarkup.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 4, 2022

(force-pushed to resolve conflicts from #2901 and #2915)

@michaelficarra
Copy link
Member

We discussed this at editor call and decided this should be run by committee first so we can hear any objections. We still maintain that these changes are within editorial discretion, though.

Also, we will need to change the wording of the "feature tests" to make sure we aren't changing or obfuscating the web browser requirement.

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Nov 7, 2022
@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 7, 2022

Also, we will need to change the wording of the "feature tests" to make sure we aren't changing or obfuscating the web browser requirement.

I gather that the problem is that the wording "If the host supports [feature]" might be misread to give the host the freedom to support [feature] or not, whereas only non-browsers have that option (browsers must support it). And even though the start of Annex B makes that clear, and "[feature]" is a link to the feature description within Annex B, that might not be enough.

So one possibility would be to change the wording to something like "If the host is a web browser or the host supports [feature]".

Another might be to make the phrase "the host supports" be a link to the start of Annex B, and possibly make that a better landing site for that link.

@michaelficarra
Copy link
Member

@jmdyck Correct, and yes those are some reasonable alternative strategies. Would you like the editors to discuss it more and give you a specific recommendation for the wording, or do you want to come up with something first?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 8, 2022

Hm, not much left for me to come up with, other than elaborating on a "better landing site". Could be something like:

Each of the features defined in this annex is specified as a set of modifications to the syntax, semantics, and/or built-in properties specified in the main body of this document. In some cases, those modifications appear in this annex, and in others, they appear (specially marked) in the main body.

When a modification to semantics appears in the main body (in an algorithm or early error rule), it is guarded by the condition that "the host supports" the relevant feature. When the host is a web browser, this condition must be true. When the host is not a web browser, it is host-defined whether the condition is true or false.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 8, 2022

I now realize that the 'normative-optional' attribute isn't right for these cases, so it should probably be something new, e.g. 'annex-b'. Let me know what you'd like.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Nov 8, 2022
@bakkot
Copy link
Contributor

bakkot commented Nov 9, 2022

For the inline note, how about:

If the host is a web browser or otherwise supports [feature]

with a manual link to the "landing site" in annex B. And then the "landing site" wording you suggest as above is fine, except I'd tweak the last couple sentences as in

Each of the features defined in this annex is specified as a set of modifications to the syntax, semantics, and/or built-in properties specified in the main body of this document. In some cases, those modifications appear in this annex, and in others, they appear (specially marked) in the main body.

When a modification to semantics appears in the main body (in an algorithm or early error rule), it is guarded by the condition that "the host supports" the relevant feature. Web browsers are required to support all such features.


We're also fine with calling these normative-optional even though the "optional" part is not technically true for browsers.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 10, 2022

For the inline note, how about:

If the host is a web browser or otherwise supports [feature]

I've done this, but I'm somewhat dissatisfied with the results. In particular, in some cases (in FunctionDeclarationInstantiation,
EvalDeclarationInstantiation, and all the early errors), the "supports" condition is the first term of a conjunction, so you now get things like:

If the host is a web browser or otherwise supports [feature] and strict is false, then

and

It is a Syntax Error if [...], unless the host is a web browser or otherwise supports [feature], the source text matched by this production is not strict mode code, and the duplicate entries are only bound by FunctionDeclarations.

I'm not saying it's an A or B and C ambiguity (precedence problem), but it is a bit clunky.

with a manual link to the "landing site" in annex B.

Instead of a manual link, I added variants="otherwise supports" to the <dfn> for "the host supports".

And then the "landing site" wording you suggest as above is fine, except I'd tweak the last couple sentences as in

[...] it is guarded by the condition that "the host supports" the relevant feature.

Although now we never use that phrase. But people can (in the rendered spec) hover the term and get all the references, so not a problem, I guess.

We're also fine with calling these normative-optional even though the "optional" part is not technically true for browsers.

I'm not fine with it, though. Calling them normative-optional would imply a change in normative status, and I want to be clear that this PR doesn't do that.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 11, 2022

(force-pushed to get the updated enforce-format.yml)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 11, 2022

For the inline note, how about:

If the host is a web browser or otherwise supports [feature]

[...]

I'm not saying it's an A or B and C ambiguity (precedence problem), but it is a bit clunky.

Now that I can see the rendered version, this bothers me less.

@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2022

Let's just tweak the definition of "normative optional" to add "unless otherwise indicated", then, as in

A conforming implementation of ECMAScript may choose to implement or not implement Normative Optional subclauses, unless otherwise indicated.

@michaelficarra michaelficarra added needs consensus This needs committee consensus before it can be eligible to be merged. and removed editor call to be discussed in the next editor call labels Dec 14, 2022
@bakkot bakkot removed the needs consensus This needs committee consensus before it can be eligible to be merged. label Jan 30, 2023
@bakkot
Copy link
Contributor

bakkot commented Jan 30, 2023

We discussed this in plenary today, and while one delegate was not comfortable with it (specifically the function-in-block part) everyone else who expressed an opinion was strongly in favor, so we're going to go forward with it. Editors will still need to review for wording and so on, of course.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 10, 2023

(force-pushed to resolve merge-conflicts from PR #2877)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 10, 2023

Reminder: This PR is 'based' on PR #2951, so that PR should be resolved first.

(2951 moves one of the insertion-points for Annex B stuff, so that's easier to do before inlining than after.)

@nicolo-ribaudo
Copy link
Member

What do you think about this rendering, but with the "ANNEX B" title linking to https://tc39.es/ecma262/#sec-additional-ecmascript-features-for-web-browsers?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 7, 2023

Let's just tweak the definition of "normative optional" to add "unless otherwise indicated", then [...]

I'm not a fan of "unless otherwise indicated", but I'm not finding alternative wording that fits the context, so will add this.

And if the inlined bits of Annex B are going to be labelled normative-optional, I imagine that should apply to the whole of Annex B too. (I.e., change its <emu-annex> from normative to normative-optional.)

@ljharb
Copy link
Member

ljharb commented Apr 7, 2023

It won’t necessarily apply to everything, some things may end up being required.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 12, 2023

Force-pushed to:

  • add a commit to change the definition of "Normative Optional";
  • add a commit to bump the ecmarkup version to 16.2.0;
  • change the markup at insertion points;
  • apply fixups.

Note that there are 4 insertion-points that occur within Early Error rules, and so can't be handled via a 'normative-optional' alg-step attribute. I instead used <span normative-optional>, so we'll see how that looks. (You can find them by scanning for "unless the host").

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 12, 2023

And if the inlined bits of Annex B are going to be labelled normative-optional, I imagine that should apply to the whole of Annex B too. (I.e., change its <emu-annex> from normative to normative-optional.)

I didn't do this, because it would have rendered the whole of Annex B in the Normative Optional background color, which might have been annoying (plus nobody said yes).

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 12, 2023

(forced-pushed to get the correct ecmarkup-bump commit)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 14, 2023

(force-pushed to resolve merge conflict from PR #2995)

spec.html Outdated Show resolved Hide resolved
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. I love this!

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 23, 2024

(force-pushed to resolve merge conflicts from PR 3309)

@michaelficarra michaelficarra requested a review from a team June 26, 2024 22:32
spec.html Outdated
1. Let _lexEnv_ be _varEnv_.
1. Else,
1. [id="step-functiondeclarationinstantiation-web-compat-insertion-point", normative-optional] If the host is a web browser or otherwise supports <emu-xref href="#sec-block-level-function-declarations-web-legacy-compatibility-semantics" title></emu-xref>, then
1. For each |FunctionDeclaration| _f_ that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause|, do
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this somehow restrict scope? Otherwise it's unclear what is meant by "a |Block|, |CaseClause|, or |DefaultClause|".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Without any qualification, it seems like "a |Block|, |CaseClause|, or |DefaultClause|" could refer to any such Parse Node that the implementation is aware of, which seems too broad.

Originally (in ES6), it was scoped via "For each |FunctionDeclaration| f in varDeclarations ...", but then that was removed in commit efbfc88, which doesn't appear to be connected to a PR, but claims to fix old bug 4427. However, if you look at that bug report's suggested fix, in addition to removing "in varDeclarations", it also says to append "Contained within code", and that part didn't happen in the commit.

For discussion that prompted the bug report, see esdiscuss.

Anyhow, that's a preexisting problem, so not the main point of this PR, but I could tack on a commit if the editors want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought, I figured it deserved its own PR: #3361.

spec.html Outdated
Comment on lines 21122 to 21124
<li>
It is a Syntax Error if the LexicallyDeclaredNames of |StatementList| contains any duplicate entries.
It is a Syntax Error if the LexicallyDeclaredNames of |StatementList| contains any duplicate entries<span normative-optional>, unless IsStrict(this production) is *false*, the duplicate entries are only bound by FunctionDeclarations, and the host is a web browser or otherwise supports <emu-xref href="#sec-block-level-function-declarations-web-legacy-compatibility-semantics" title></emu-xref></span>.
</li>
Copy link
Contributor

@gibson042 gibson042 Jun 26, 2024

Choose a reason for hiding this comment

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

This renders poorly, and in particular misses the "normative optional" text: https://ci.tc39.es/preview/tc39/ecma262/sha/06e2af4a47bd749e5cbb0fb0c737221e86685a85/multipage/ecmascript-language-statements-and-declarations.html#sec-block-static-semantics-early-errors

I wonder if moving the attribute up a level would improve things.

Suggested change
<li>
It is a Syntax Error if the LexicallyDeclaredNames of |StatementList| contains any duplicate entries.
It is a Syntax Error if the LexicallyDeclaredNames of |StatementList| contains any duplicate entries<span normative-optional>, unless IsStrict(this production) is *false*, the duplicate entries are only bound by FunctionDeclarations, and the host is a web browser or otherwise supports <emu-xref href="#sec-block-level-function-declarations-web-legacy-compatibility-semantics" title></emu-xref></span>.
</li>
<li>
<div>It is a Syntax Error if the LexicallyDeclaredNames of |StatementList| contains any duplicate entries.</div>
<div normative-optional>If the host is a web browser or otherwise supports <emu-xref href="#sec-block-level-function-declarations-web-legacy-compatibility-semantics" title></emu-xref>, the above Syntax Error does not apply when IsStrict(this production) is *false* and the duplicate entries are only bound by FunctionDeclarations.</div>
</li>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This renders poorly, and in particular misses the "normative optional" text:

I agree that it renders poorly, but I'm not sure what you mean by "misses". The normative optional text is there, but the bottom of each line is slightly obscured/clipped by the padding of the line below.

ecmarkup.css probably needs a new rule for when normative-optional occurs on inline content. (Currently, that attribute is only attached to block content, specifically emu-clause elements.) E.g., if you reduce the padding from 0.5em to 0.2em, the clipping goes away. (Also, the border-left line maybe doesn't make sense for inline content.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it renders poorly, but I'm not sure what you mean by "misses". The normative optional text is there, but the bottom of each line is slightly obscured/clipped by the padding of the line below.

I mean that the literal text "NORMATIVE OPTIONAL" is absent, but Conformance indicates that it should be present:

A Normative Optional clause is denoted in this specification with the words "Normative Optional" in a coloured box, as shown below.

current

screenshot

proposed

screenshot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean that the literal text "NORMATIVE OPTIONAL" is absent

Ahh, gotcha.

spec.html Outdated
Comment on lines 22588 to 22590
<li>
It is a Syntax Error if the LexicallyDeclaredNames of |CaseBlock| contains any duplicate entries.
It is a Syntax Error if the LexicallyDeclaredNames of |CaseBlock| contains any duplicate entries<span normative-optional>, unless IsStrict(this production) is *false*, the duplicate entries are only bound by FunctionDeclarations, and the host is a web browser or otherwise supports <emu-xref href="#sec-block-level-function-declarations-web-legacy-compatibility-semantics" title></emu-xref></span>.
</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise here (<li><p>It is a Syntax Error if…</p><p normative-optional>If the host is a web browser or otherwise supports…</p></li>).

spec.html Outdated
@@ -22723,12 +22739,9 @@ <h1>Static Semantics: Early Errors</h1>
<emu-grammar>LabelledItem : FunctionDeclaration</emu-grammar>
<ul>
<li>
It is a Syntax Error if any source text is matched by this production.
It is a Syntax Error if any source text is matched by this production<span normative-optional>, unless that source text is non-strict code and the host is a web browser or otherwise supports <emu-xref href="#sec-labelled-function-declarations" title></emu-xref></span>.
Copy link
Contributor

Choose a reason for hiding this comment

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

And here (etc.).

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jun 27, 2024
…tantiation

This PR completes a small bugfix from 9 years ago.

Fixes tc39#2663.

----

History:

2015-07-17:
@bakkot identifies a problem in Annex B's
"Changes to FunctionDeclarationInstantiation":
https://esdiscuss.org/topic/block-level-function-declarations-web-legacy-compatibility-bug

To remedy this, @allenwb submits bug 4427:
https://tc39.es/archives/bugzilla/4427/
in which he recommends changing
> For each FunctionDeclaration _f_ **in _varDeclarations_** that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause|,

to
> For each FunctionDeclaration _f_ that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause| **Contained within _code_**,

(emphasis mine).

2015-10-29:
@anba submits PR tc39#141, claiming to fix bug 4427.
It deletes "in _varDeclarations_",
but doesn't add "Contained within _code_".
My guess is, this was just an oversight.

2015-11-02:
PR tc39#141 is merged to master as commit efbfc88.

2022-02-13:
@nicolo-ribaudo raises issue tc39#2663 about this,
and says he'd open a PR to fix it,
but I don't think that happened.

2024-06-26:
@gibson042 raises the problem again, in a commment on PR tc39#2952:
tc39#2952 (comment)
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Jul 11, 2024
…tantiation

This PR completes a small bugfix from 9 years ago.

Fixes tc39#2663.

----

History:

2015-07-17:
@bakkot identifies a problem in Annex B's
"Changes to FunctionDeclarationInstantiation":
https://esdiscuss.org/topic/block-level-function-declarations-web-legacy-compatibility-bug

To remedy this, @allenwb submits bug 4427:
https://tc39.es/archives/bugzilla/4427/
in which he recommends changing
> For each FunctionDeclaration _f_ **in _varDeclarations_** that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause|,

to
> For each FunctionDeclaration _f_ that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause| **Contained within _code_**,

(emphasis mine).

2015-10-29:
@anba submits PR tc39#141, claiming to fix bug 4427.
It deletes "in _varDeclarations_",
but doesn't add "Contained within _code_".
My guess is, this was just an oversight.

2015-11-02:
PR tc39#141 is merged to master as commit efbfc88.

2022-02-13:
@nicolo-ribaudo raises issue tc39#2663 about this,
and says he'd open a PR to fix it,
but I don't think that happened.

2024-06-26:
@gibson042 raises the problem again, in a commment on PR tc39#2952:
tc39#2952 (comment)
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Jul 17, 2024
…tantiation (tc39#3361)

This PR completes a small bugfix from 9 years ago.

Fixes tc39#2663.

----

History:

2015-07-17:
@bakkot identifies a problem in Annex B's
"Changes to FunctionDeclarationInstantiation":
https://esdiscuss.org/topic/block-level-function-declarations-web-legacy-compatibility-bug

To remedy this, @allenwb submits bug 4427:
https://tc39.es/archives/bugzilla/4427/
in which he recommends changing
> For each FunctionDeclaration _f_ **in _varDeclarations_** that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause|,

to
> For each FunctionDeclaration _f_ that is directly contained in the |StatementList| of a |Block|, |CaseClause|, or |DefaultClause| **Contained within _code_**,

(emphasis mine).

2015-10-29:
@anba submits PR tc39#141, claiming to fix bug 4427.
It deletes "in _varDeclarations_",
but doesn't add "Contained within _code_".
My guess is, this was just an oversight.

2015-11-02:
PR tc39#141 is merged to master as commit efbfc88.

2022-02-13:
@nicolo-ribaudo raises issue tc39#2663 about this,
and says he'd open a PR to fix it,
but I don't think that happened.

2024-06-26:
@gibson042 raises the problem again, in a commment on PR tc39#2952:
tc39#2952 (comment)
The Evaluation semantics for FunctionDeclaration
is accompanied by a Note that says
"An alternative semantics is provided in B.3.2."
However, B.3.2 is a fairly large section,
and the alternative Evaluation semantics
might be difficult to spot.

So this commit expands that Note
by adding links to the specific steps
that give the alternative Evaluation semantics
for FunctionDeclaration.
Specifically, tack on "unless otherwise indicated".

(This will allow Annex B features to be labelled normative-optional.)
…ions"

Also add a defn for "the host supports".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants