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

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

[usrbincc, 2013-03-22 20:52:09]

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/FreeVariableCh...
File src/semantics/FreeVariableChecker.js (right):

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableCh...
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/FreeVariableChe...
File test/feature/FreeVariableChecker/Error_With.js (right):

https://codereview.appspot.com/7577048/diff/4001/test/feature/FreeVariableChe...
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/FreeVariableChe...
File test/feature/FreeVariableChecker/WithVarDecl.js (right):

https://codereview.appspot.com/7577048/diff/4001/test/feature/FreeVariableChe...
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).

[arv, 2013-03-22 22:01:04]

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

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableCh...
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_() {}
}

[usrbincc, 2013-03-23 20:46:00]

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/FreeVariableCh...
File src/semantics/FreeVariableChecker.js (right):

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableCh...
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.

[arv, 2013-03-24 14:30:22]

LGTM

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

https://codereview.appspot.com/7577048/diff/4001/src/semantics/FreeVariableCh...
src/semantics/FreeVariableChecker.js:85: RecordOnly.prototype = this;
On 2013/03/23 20:46:00, usrbincc wrote:
> 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.

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/FreeVariableCh...
File test/feature/FreeVariableChecker/WithVarDecl.js (right):

https://codereview.appspot.com/7577048/diff/12001/test/feature/FreeVariableCh...
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.

[arv, 2013-03-24 14:32:41]

Committed as f6a3d21a18bea1191cf90878f95aabaea9d325df

[usrbincc, 2013-03-25 18:09:38]

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

https://codereview.appspot.com/7577048/diff/12001/test/feature/FreeVariableCh...
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.