Don't check 'with' for undefined vars, but do record var declarations. (issue 7577048)

[usrb...@yahoo.com]

Reviewers: arv-chromium, peterhal, johnjbarton,

Message:
I've been letting a lot of patches just sit there, which is not a good
strategy with an active tree (potential merge problems), so I'm getting
this one moving again.

But it looks like nobody has touched FreeVariableChecker.js in awhile,
so I didn't have to update the patch this time.

----

I believe that this solution is probably the best you can hope to do
statically analyzing a 'with' (until we start supporting something like
structs with a fixed set of properties, typechecking, etc).

#--cut--
git checkout 5ea5341472cecb61 # last merge with branch master

dl_check() {
local f=$(basename $1)
test -e $f || curl -sO $1
openssl sha1 < $f | grep -q "= $2" && return 0
echo sha1 mismatch!
return 1
}

git checkout -b issue7577048-dont-check-with-statement

dl_check https://codereview.appspot.com/download/issue7577048_4001.diff
\
6fce87cc444dc7d8 && git apply issue7577048_4001.diff && make test
#--cut--

----

Note: there is a systemic bug in visitCatch where vars and functions
declared there aren't scoped to the enclosing function. Not that any
sane person would do that. But since 'let' desugars to try-catch,
someone might plausibly do that, with some compiler support.

I originally thought it'd be a simple fix, but it's not because a lot of
information is lost in the desugaring. In particular

try { ... } catch ([a, b, c]) { ... }

// generated
try {
...
} catch ($__0) {
var a = $__0[0], b = $__0[1], c = $__0[2];
...
}

And suddenly the bound identifiers are just normal vars. We could
special-case the handling of '$'-prefixed names somehow, but that seems
a little messy.

Actually, there are a lot of holes in this area, on reflection. These
vars aren't supposed to escape the catch, yet they can and do affect any
a, b, c in the enclosing function.

Sorry, I'll get these notes into bug reports eventually.



https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js
File src/semantics/FreeVariableChecker.js (right):

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js#newcode85
src/semantics/FreeVariableChecker.js:85: RecordOnly.prototype = this;
I tried something like

class RecordOnly extends this {
visitIdentifierExpression(tree) {}
validateScope_() {}
}

but ran into some errors that I didn't bother to investigate any
further.

https://codereview.appspot.com/7577048/diff/4001/test/feature/FreeVariableChecker/Error_With.js
File test/feature/FreeVariableChecker/Error_With.js (right):

https://codereview.appspot.com/7577048/diff/4001/test/feature/FreeVariableChecker/Error_With.js#newcode7
test/feature/FreeVariableChecker/Error_With.js:7: var x = missingVar *
2;
Just realized this test is pointless because this part never even
touches the 'with' code. But I guess this would catch the case if
the code ever uses something that disables some flag in the with and
forgets to turn it back on outside the 'with'.

https://codereview.appspot.com/7577048/diff/4001/test/feature/FreeVariableChecker/WithVarDecl.js
File test/feature/FreeVariableChecker/WithVarDecl.js (right):

https://codereview.appspot.com/7577048/diff/4001/test/feature/FreeVariableChecker/WithVarDecl.js#newcode5
test/feature/FreeVariableChecker/WithVarDecl.js:5: yW = xW * xW;
I started adding suffixes and prefixes to everything (started with
prefixes, switched to suffixes, but forgot to convert everything)
because of a bug in the tests -- global vars carry over between them. In
a previous version of this test, I had a global var 'x2' that caused
an entirely unrelated test to fail (maybe I should go fix that test;
have to go find it again).

Description:
Don't check 'with' for undefined vars, but do record declarations.

There is no general way to statically analyze a 'with' statement for
usage of undefined vars, so we only do what we can: add any declared
vars and functions to our scope.

BUG=https://code.google.com/p/traceur-compiler/issues/detail?id=207
TEST=test/feature/FreeVariableChecker/Error_With.js
test/feature/FreeVariableChecker/WithBasic.js
test/feature/FreeVariableChecker/WithVarDecl.js


Please review this at https://codereview.appspot.com/7577048/

Affected files:
M src/semantics/FreeVariableChecker.js
A test/feature/FreeVariableChecker/Error_With.js
A test/feature/FreeVariableChecker/WithBasic.js
A test/feature/FreeVariableChecker/WithVarDecl.js


Index: src/semantics/FreeVariableChecker.js
diff --git a/src/semantics/FreeVariableChecker.js
b/src/semantics/FreeVariableChecker.js
index
0e8927ff8246b2b1dfca1fa733f18d7b9da2d58b..23334f329e95bc005b82ce3513a233c0399c3bd0
100644
--- a/src/semantics/FreeVariableChecker.js
+++ b/src/semantics/FreeVariableChecker.js
@@ -77,6 +77,13 @@ export class FreeVariableChecker extends
ParseTreeVisitor {
this.reporter_ = reporter;
/** Current scope (block, program) */
this.scope_ = null;
+ // Records declarations within a ParseTree, but doesn't validate.
+ function RecordOnly() {
+ this.visitIdentifierExpression = function(tree) {};
+ this.validateScope_ = function() {};
+ }
+ RecordOnly.prototype = this;
+ this.recordOnly_ = new RecordOnly();
}

/**
@@ -196,6 +203,15 @@ export class FreeVariableChecker extends
ParseTreeVisitor {
}
}

+ visitWithStatement(tree) {
+ this.visitAny(tree.expression);
+ // We don't just 'this.visitAny(tree.body)' because unbound identifiers
+ // inside 'with' might be members of 'tree.expression'. There is no
general
+ // way to deduce which is which at compile time. However, we still
need to
+ // record any declarations within the block.
+ this.recordOnly_.visitAny(tree.body);
+ }
+
declareVariable_(tree) {
var name = getVariableName(tree);
if (name) {
Index: test/feature/FreeVariableChecker/Error_With.js
diff --git a/test/feature/FreeVariableChecker/Error_With.js
b/test/feature/FreeVariableChecker/Error_With.js
new file mode 100644
index
0000000000000000000000000000000000000000..315791eaface905616be4021f806d8bbea05986e
--- /dev/null
+++ b/test/feature/FreeVariableChecker/Error_With.js
@@ -0,0 +1,7 @@
+// Should not compile.
+// Error: missingVar is not defined
+var o = {};
+with (o) {
+ missingVar = 42;
+}
+var x = missingVar * 2;
Index: test/feature/FreeVariableChecker/WithBasic.js
diff --git a/test/feature/FreeVariableChecker/WithBasic.js
b/test/feature/FreeVariableChecker/WithBasic.js
new file mode 100644
index
0000000000000000000000000000000000000000..b7a36a466d095b12fcef3be97cf44c09d7851a22
--- /dev/null
+++ b/test/feature/FreeVariableChecker/WithBasic.js
@@ -0,0 +1,13 @@
+var o = {
+ unicorns: 42,
+ fairyGodmothers: 3
+};
+
+with (o) {
+ x = 42;
+ y = x * x;
+
+ bubbles = 1000;
+ fairyGodmotherBubbles = bubbles * fairyGodmothers;
+ unicorns = bubbles * fairyGodmothers - (fairyGodmotherBubbles -
unicorns);
+}
Index: test/feature/FreeVariableChecker/WithVarDecl.js
diff --git a/test/feature/FreeVariableChecker/WithVarDecl.js
b/test/feature/FreeVariableChecker/WithVarDecl.js
new file mode 100644
index
0000000000000000000000000000000000000000..b19157c30c32a34f8c29dda68a37620ff50fe35a
--- /dev/null
+++ b/test/feature/FreeVariableChecker/WithVarDecl.js
@@ -0,0 +1,25 @@
+var o = {xW: 0, yW: 0, propW: {xW: 0, yW: 0, funcW: null}};
+
+with (o) {
+ xW = 20;
+ yW = xW * xW;
+ function funcW(x, y) { return y / x + x; }
+ var withVar;
+}
+
+withVar = 20;
+assertEquals(40, funcW(o.xW, o.yW));
+
+with (o) {
+ with (propW) {
+ xW = 21;
+ yW = xW * xW;
+ funcW = function(f, x, y) { return f(x, y) * 100; }
+ var withVar2;
+ }
+}
+
+withVar2 = 20;
+var op = o.propW;
+assertEquals(42, funcW(op.xW, op.yW));
+assertEquals(4200, op.funcW(funcW, op.xW, op.yW));

[a...@chromium.org]

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js
File src/semantics/FreeVariableChecker.js (right):

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js#newcode85
src/semantics/FreeVariableChecker.js:85: RecordOnly.prototype = this;

On 2013/03/22 20:52:09, usrbincc wrote:
> I tried something like

> class RecordOnly extends this {
> visitIdentifierExpression(tree) {}
> validateScope_() {}
> }

> but ran into some errors that I didn't bother to investigate any
> further.

I find this pattern strange. What is the intent?

Why isn't this correct?

class RecordOnly extends ParseTreeVisitor {
visitIdentifierExpression(tree) {}
validateScope_() {}
}

https://codereview.appspot.com/7577048/

[usrb...@yahoo.com]

Got rid of the weird proxy object technique. I still think it's kind of
interesting.

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js
File src/semantics/FreeVariableChecker.js (right):

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js#newcode85
src/semantics/FreeVariableChecker.js:85: RecordOnly.prototype = this;

Basically I want to disable visitIdentifierExpression and validateScope_
inside 'with' bodies. It can't be a new object extending
ParseTreeVisitor
or FreeVariableChecker. It has to be the current FreeVariableChecker
object, because I need to update its 'scope_' member.

There are other ways to do this, but this appealed to me somehow. It's
something you can only do in a prototype-based language, or by emulating
one. I consider it similar to a union or cast in C.

One nice thing about this technique is that you only have to touch two
places in the code, instead of four. The downside is of course, that
it's a strange technique. The other downside is that it doesn't work
completely transparently:

a = {ctr: 0};
function B() {}
B.prototype = a;
b = new B();
console.log(++b.ctr, a.ctr, b.ctr);

So inside 'this.recordOnly_', 'this.scope_' never gets updated, though
the
object 'this.scope_' points to does get updated. This is okay, because
by
the time we exit 'this.recordOnly_', it should point to the same object
anyway.

On the other hand, it's not as easy to trivially verify, so maybe I'll
switch over to using the same technique as SuperTransformer, and use a
'withCount' var.

https://codereview.appspot.com/7577048/

[a...@chromium.org]

LGTM

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js
File src/semantics/FreeVariableChecker.js (right):

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableChecker.js#newcode85
src/semantics/FreeVariableChecker.js:85: RecordOnly.prototype = this;

I think what you ended up with is easier to grasp.

There are issues with the prototype based approach and the main issue is
that any write to a data property creates an instance property on the
new object and does not write through. In this case it wasn't a problem
but I can easily see it becoming a problem in the future.

https://codereview.appspot.com/7577048/diff/12001/test/feature/FreeVariableChecker/WithVarDecl.js
File test/feature/FreeVariableChecker/WithVarDecl.js (right):

https://codereview.appspot.com/7577048/diff/12001/test/feature/FreeVariableChecker/WithVarDecl.js#newcode6
test/feature/FreeVariableChecker/WithVarDecl.js:6: function funcW(x, y)

{ return y / x + x; }

This is actually not allowed in any version of ECMAScript. All script
engines allow it though so I guess it is good that we allow it too.

https://codereview.appspot.com/7577048/

[a...@chromium.org]

Committed as f6a3d21a18bea1191cf90878f95aabaea9d325df

https://codereview.appspot.com/7577048/

[usrb...@yahoo.com]

https://codereview.appspot.com/7577048/diff/12001/test/feature/FreeVariableChecker/WithVarDecl.js
File test/feature/FreeVariableChecker/WithVarDecl.js (right):

https://codereview.appspot.com/7577048/diff/12001/test/feature/FreeVariableChecker/WithVarDecl.js#newcode6
test/feature/FreeVariableChecker/WithVarDecl.js:6: function funcW(x, y)
{ return y / x + x; }

On 2013/03/24 14:30:22, arv-chromium wrote:
> This is actually not allowed in any version of ECMAScript. All script
> engines allow it though so I guess it is good that we allow it too.

If it's explicitly disallowed, maybe we should (eventually) flag it as
an error. Low priority "should", since it's not really a common pattern,
and people should be using strict mode anyway. But still.

https://codereview.appspot.com/7577048/