-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
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
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). |
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.
You are right about CreateImmutableBinding breaking for relaxing allowing multiple same-named function declarations in block. I will update this PR. |
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. |
@@ -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> |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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!
Other than the minor comments above, it seems good. Semantic changes for the changelog are:
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? |
@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. |
@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|. |
There was a problem hiding this comment.
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?
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. |
@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! |
@allenwb Have you had a chance to look yet? |
on my queue. I'll try to talke a look by end of day 3/25 |
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. |
For #348
This was more change than I'd intended. Let me know if I got the language wrong anywhere.