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

Enforce TDZ within initializer of lexical declaration (refactored) #2733

Merged
merged 4 commits into from Oct 18, 2016

Conversation

jugglinmike
Copy link
Member

This patch includes work by @nicolo-ribaudo (originally submitted in gh-2701) and a refactoring of my own design.

I initially expected that this approach would reduce some of the noise
associated with explicit maintence of the standalone "definition" objects (e.g.
the calls to state.funct["(scope)"].definition.reset). While this turned out
to be true, I found that it also introduced a different kind of noise--see
the extra calls to state.funct["(scope)"].initialize.

I think the latter is preferable because it makes the source code more closely
match the spec language. Readers are more likely to know what it means to
"initialize a label" than what it means to "clear a definition."

Resolves #2637

@jugglinmike jugglinmike changed the title Tdz initializer Enforce TDZ within initializer of lexical declaration (refactored) Oct 25, 2015
@nicolo-ribaudo
Copy link
Contributor

I think the latter is preferable because it makes the source code more closely
match the spec language.

👍

I initially expected that this approach would reduce some of the noise
associated with explicit maintence of the standalone "definition" objects (e.g.
the calls to state.funct["(scope)"].definition.reset). While this turned out
to be true, I found that it also introduced a different kind of noise--see
the extra calls to state.funct["(scope)"].initialize.

You could make the addlabel function accept an initialized parameter
(whose default value is true) so that you don't need the extra calls to
state.funct["(scope)"].initialize.
e.g.

diff --git a/src/jshint.js b/src/jshint.js
index 0758967..4bb41f0 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -3570,4 +3570,5 @@ var JSHINT = (function() {
             state.funct["(scope)"].addlabel(t.id, {
               type: type,
+              initialized: false,
               token: t.token });
             names.push(t.token);
@@ -3743,4 +3744,5 @@ var JSHINT = (function() {
       state.funct["(scope)"].addlabel(this.name, {
         type: "class",
+        initialized: false,
         token: state.tokens.curr });
# ------------------------------------------------- #
# Here I omitted the diff where I removed the extra #
# calls to `state.function["(scope)"].initialize`   #
# ------------------------------------------------- #
diff --git a/src/scope-manager.js b/src/scope-manager.js
index 11e141a..767abf2 100644
--- a/src/scope-manager.js
+++ b/src/scope-manager.js
@@ -649,5 +649,5 @@ var scopeManager = function(state, predefined, exported, declared) {
         }

-        scopeManagerInst.block.add(labelName, type, token, !isexported);
+        scopeManagerInst.block.add(labelName, type, token, !isexported, opts.initialized);

       } else {
@@ -813,9 +813,9 @@ var scopeManager = function(state, predefined, exported, declared) {
        * Adds a new variable
        */
-      add: function(labelName, type, tok, unused) {
+      add: function(labelName, type, tok, unused, initialized) {
         _current["(labels)"][labelName] = {
           "(type)" : type,
           "(token)": tok,
-          "(initialized)": false,
+          "(initialized)": initialized !== false,
           "(blockscoped)": true,
           "(unused)": unused };

@jugglinmike
Copy link
Member Author

I had some similar code when I first drafted this patch, but I held off on it because of a gut reaction to the boolean trap for the add method. That could be resolved by refactoring to use an options object with well-named attributes, but I'm also (overly?) sensitive to object allocations costs in this codepaths.

It's been over two weeks, though, and I've come around on the idea of simply adding that additional boolean parameter. It's an internal method, so the usability concern is kind of minimal.

That said, I decided to implement initialized as "false by default". It's true that there are more cases in the code base where we create initialized bindings than uninitialized bindings, but I still think this is preferable. "False by default" is easier to reason about because it aligns with JavaScript's natural treatment of values (undefined coerces to false), and it also makes "pre-initialized" bindings opt-in only. My thinking is that we don't want to accidentally creating bindings this way, and an explicit API makes that less likely.

nicolo-ribaudo and others added 4 commits September 11, 2016 11:54
Conceptually, the initialization state of a given variable is a property
of that variable. Model that relationship in the code organization by
tracking variable initialization using a dedicated property on the
representation of the variable itself. Beyond improving code clarity
(the "label" objects remain the single source of truth for the state of
each variable), this also reduces memory allocation costs (no new
objects need to be created).
@coveralls
Copy link

coveralls commented Sep 11, 2016

Coverage Status

Coverage increased (+0.007%) to 97.726% when pulling 85e4804 on jugglinmike:tdz-initializer into 8accbae on jshint:master.

@jugglinmike
Copy link
Member Author

@rwaldron I've rebased this patch (the original version which conflicts with master is available at https://github.com/jugglinmike/jshint/tree/tdz-initializer-presquash for posterity).

@rwaldron
Copy link
Member

Oh very cool :)

I will try to review this tomorrow. Feel free to nudge me if I haven't reported back by noon

@jugglinmike
Copy link
Member Author

@rwaldron Nudging, as requested.

@rwaldron
Copy link
Member

@jugglinmike is this ready to land when I'm done reviewing?

@jugglinmike
Copy link
Member Author

Yes, sir.

@rwaldron
Copy link
Member

I poked and prodded this one for a bit. Nice work!

@rwaldron rwaldron merged commit 8e9d406 into jshint:master Oct 18, 2016
@jugglinmike
Copy link
Member Author

Thanks, Rick!

@jugglinmike
Copy link
Member Author

And thanks, @nicolo-ribaudo!

jugglinmike added a commit to jugglinmike/jshint that referenced this pull request Nov 13, 2016
…shint#2733)

* [[FIX]] Enforce TDZ within initializer of lexical declaration

Fixes jshint#2637

* [[FIX]] Enforce TDZ within class heritage definition

* [[FIX]] Enforce TDZ within for in/of head

Fixes jshintgh-2693

* [[CHORE]] Refactor var initialization tracking

Conceptually, the initialization state of a given variable is a property
of that variable. Model that relationship in the code organization by
tracking variable initialization using a dedicated property on the
representation of the variable itself. Beyond improving code clarity
(the "label" objects remain the single source of truth for the state of
each variable), this also reduces memory allocation costs (no new
objects need to be created).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TDZ is not enforced within initializer of lexical declaration
4 participants