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: Fix Annex B FiB to only assign function Objects out of blocks and allow redeclarations. #400

Closed
wants to merge 2 commits into from

Conversation

syg
Copy link
Contributor

@syg syg commented Feb 20, 2016

For #348

This was more change than I'd intended. Let me know if I got the language wrong anywhere.

@syg syg changed the title Fix Annex B FiB to only assign function Objects out of blocks. Normative: Fix Annex B FiB to only assign function Objects out of blocks. Feb 20, 2016
@bterlson bterlson added the normative change Affects behavior required to correctly evaluate some ECMAScript source text label Feb 20, 2016
@littledan
Copy link
Member

I'd really like to know more about the extent of the web breakage before making a change like this. Are there any known issues beyond the Rackspace case?

As for the spec text, I'm wondering how this line

  1. Perform ! fibRec.CreateImmutableBinding(fn, true).

behaves when there are multiple function declarations in the same block with the same name. We seem to have ample evidence that it's not web-compatible to prohibit that case (though other parts of the spec text continue to prohibit it).

@syg
Copy link
Contributor Author

syg commented Feb 22, 2016

I'd really like to know more about the extent of the web breakage before making a change like this. Are there any known issues beyond the Rackspace case?

No other known cases so far. I am in favor of making this change to Annex B as no implementation has ever assigned non-functions outside of blocks, and to spec such a behavior misses the intent of Annex B approaching intersection semantics of various browser implementations.

We seem to have ample evidence that it's not web-compatible to prohibit that case (though other parts of the spec text continue to prohibit it).

You are right about CreateImmutableBinding breaking for relaxing allowing multiple same-named function declarations in block. I will update this PR.

@bterlson
Copy link
Member

I will review this more closely tomorrow. I believe this is a change we should take prior to the snapshot. It has always been the intention for these Annex B semantics to reflect web reality and its nonsense to standardize something no one has ever done or will or should do (ie. assigning non-function values outside of a block). Compat is a risk, but this change makes all of annex b strictly less risky from a compat perspective afaict.

@syg syg changed the title Normative: Fix Annex B FiB to only assign function Objects out of blocks. Normative: Fix Annex B FiB to only assign function Objects out of blocks and allow redeclarations. Feb 25, 2016
@@ -36557,27 +36557,138 @@
<p>The first use case is interoperable with the semantics of |Block| level function declarations provided by ECMAScript 2015. Any pre-existing ECMAScript code that employs that use case will operate using the Block level function declarations semantics defined by clauses 9, 13, and 14 of this specification.</p>
<p>ECMAScript 2015 interoperability for the second and third use cases requires the following extensions to the clause <emu-xref href="#sec-ordinary-and-exotic-objects-behaviours"></emu-xref>, clause <emu-xref href="#sec-ecmascript-language-functions-and-classes"></emu-xref>, clause <emu-xref href="#sec-eval-x"></emu-xref> and clause <emu-xref href="#sec-globaldeclarationinstantiation"></emu-xref> semantics.</p>
<p>If an ECMAScript implementation has a mechanism for reporting diagnostic warning messages, a warning should be produced when code contains a |FunctionDeclaration| for which these compatibility semantics are applied and introduce observable differences from non-compatibility semantics. For example, if a var binding is not introduced because its introduction would create an early error, a warning message should not be produced.</p>
<emu-annex id="sec-web-compat-block-environment-records">
<h1>Block Environment Records</h1>
<p>A block Environment Record is a declarative Environment Record that is used to represent block scopes. Functions declared inside the block are stored in a separate Environment Record in the [[FunctionsInBlock]] internal slot in addition to in the block environment record. This helps ensure that only function Objects are assigned to outer environments for web legacy compatibility.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Probably capital B, ie. "Block Environment Record".

Copy link
Member

Choose a reason for hiding this comment

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

Also might be worth a dfn!

@bterlson
Copy link
Member

Other than the minor comments above, it seems good.

Semantic changes for the changelog are:

  1. Duplicate function declarations in blocks are allowed in sloppy mode.
  2. Only function declaration bindings are hoisted out of blocks, as opposed to all declarations in blocks.

In all cases, browsers agree on this semantics and these have always been in the interoperable intersection semantics.

Anything else you think we should call out?

@syg
Copy link
Contributor Author

syg commented Feb 26, 2016

@bterlson Your changelog seems good to me. I can't think of anything else that needs to be called out.

A quick pass by @littledan would be appreciated since he's co-championing at least the duplicate declarations change.

Will push fixes later today.

@bterlson
Copy link
Member

@littledan @syg sounds good. Just note that we're down to the wire - I'll be snapshotting on Monday!

@@ -36546,7 +36546,7 @@
<p>A function is declared and possibly used within a single block but also referenced within subsequent blocks.</p>
<ul>
<li>
A |FunctionDeclaration| whose |BindingIdentifier| is the name _f_ occurs exactly once within the function code of an enclosing function _g_ and that declaration is nested within a |Block|.
One or more |FunctionDeclaration| whose |BindingIdentifier| is the name _f_ occur exactly once within the function code of an enclosing function _g_ and that declaration is nested within a |Block|.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also remove 'exactly once' here?

@anba
Copy link
Contributor

anba commented Feb 27, 2016

It's probably cleaner to use a different data structure than environment records for [[FunctionsInBlock]], because actually we just need a <Identifier → Function> mapping. But that's not a reason to block this PR.

@syg
Copy link
Contributor Author

syg commented Feb 27, 2016

@anba Good catch on early errors for switch. Also much appreciated for not only pointing out a problem but also providing a fixed version of BlockDeclarationInstantiation!

@littledan
Copy link
Member

@allenwb Does this patch do what you would prefer that we do, rather than #453 ?

@syg
Copy link
Contributor Author

syg commented Mar 24, 2016

@allenwb Have you had a chance to look yet?

@allenwb
Copy link
Member

allenwb commented Mar 24, 2016

on my queue. I'll try to talke a look by end of day 3/25

@syg
Copy link
Contributor Author

syg commented Mar 30, 2016

Consensus is that duplicate functions should go in, preventing leaks of arbitrary values out of blocks via Annex B var-assignment should not.

Closing this in favor of taking @littledan's #453, which only contains the duplicate functions change.

@syg syg closed this Mar 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants