Fix harmony sloppy block scoping dynamic redeclaration check

test/mjsunit/harmony/block-eval-var-over-let.js

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Flags: --harmony-sloppy --harmony-sloppy-let --harmony-sloppy-function --no-legacy-const
 
 // Var-let conflict in a function throws, even if the var is in an eval
 
+// Throws at the top level of a function
+assertThrows(function() {
+  let x = 1;
+  eval('var x');

[Dan Ehrenberg, 2015/11/12 23:41:57]

How about leaving in the previous tests and adding these no-intializer ones in
addition, rather than replacing the tests?

[adamk, 2015/11/12 23:45:17]

What do you feel the initializer versions add to these tests? They're really
supposed to be about redeclaration, right?

I'm happy to have more tests rather than fewer, I'd just like to understand your
thinking here.

[Dan Ehrenberg, 2015/11/12 23:53:18]

Well, my thinking is rather mechanical:
1. You found that my tests missed a case
2. My tests previously caused an exception to be thrown, which is the intended
behavior, and wasn't the case (at least for some of them) before I wrote my
patch
3. Your patch deleted my test and replaced it with a test of that other case

Seems pretty clear-cut that both have value, but maybe I'm missing something.
Maybe we don't need the full product of both tests, and one
redeclaration+initializer would be enough; is that what you're getting at?

[adamk, 2015/11/13 07:06:01]

All I was thinking was that we desugar "var x = 1" into "var x; x = 1", and the
"x = 1" part doesn't have anything to do with what the rest of this test is
testing. The fact that the "x = 1" is the only thing that was (erroneously)
causing one of these tests to pass is sort of beside the point. Though maybe
what you're saying is: why reduce test coverage? That seems fair enough.
Especially if we change out desugaring later.

[adamk, 2015/11/13 20:59:07]

After more discussion offline, I convinced Dan that the duplication here
wouldn't buy us anything.

+}, TypeError);
+
+// If the eval is in its own block scope, throws
+assertThrows(function() {
+  let y = 1;
+  { eval('var y'); }
+}, TypeError);
+
+// If the let is in its own block scope, with the eval, throws
+assertThrows(function() {
+  {
+    let x = 1;
+    eval('var x');
+  }
+}, TypeError);
+
+// Legal if the let is no longer visible
+assertDoesNotThrow(function() {
+  {
+    let x = 1;
+  }
+  eval('var x');
+});
+
+// All the same works for const:
+// Throws at the top level of a function
+assertThrows(function() {
+  const x = 1;
+  eval('var x');
+}, TypeError);
+
+// If the eval is in its own block scope, throws
+assertThrows(function() {
+  const y = 1;
+  { eval('var y'); }
+}, TypeError);
+
+// If the const is in its own block scope, with the eval, throws
+assertThrows(function() {
+  {
+    const x = 1;
+    eval('var x');
+  }
+}, TypeError);
+
+// Legal if the const is no longer visible
+assertDoesNotThrow(function() {
+  {
+    const x = 1;
+  }
+  eval('var x');
+});
+
+// In global scope
 let caught = false;
-
-// Throws at the top level of a function
 try {
-  (function() {
-    let x = 1;
-    eval('var x = 2');
-  })()
+  let z = 1;
+  eval('var z');
 } catch (e) {
   caught = true;
 }
-assertTrue(caught);
-
-// If the eval is in its own block scope, throws
-caught = false;
-try {
-  (function() {
-    let y = 1;
-    { eval('var y = 2'); }
-  })()
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
-
-// If the let is in its own block scope, with the eval, throws
-caught = false
-try {
-  (function() {
-    {
-      let x = 1;
-      eval('var x = 2');
-    }
-  })();
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
-
-// Legal if the let is no longer visible
-caught = false
-try {
-  (function() {
-    {
-      let x = 1;
-    }
-    eval('var x = 2');
-  })();
-} catch (e) {
-  caught = true;
-}
-assertFalse(caught);
-
-// All the same works for const:
-// Throws at the top level of a function
-try {
-  (function() {
-    const x = 1;
-    eval('var x = 2');
-  })();
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
-
-// If the eval is in its own block scope, throws
-caught = false;
-try {
-  (function() {
-    const y = 1;
-    { eval('var y = 2'); }
-  })();
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
-
-// If the const is in its own block scope, with the eval, throws
-caught = false
-try {
-  (function() {
-    {
-      const x = 1;
-      eval('var x = 2');
-    }
-  })();
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
-
-// Legal if the const is no longer visible
-caught = false
-try {
-  (function() {
-    {
-      const x = 1;
-    }
-    eval('var x = 2');
-  })();
-} catch (e) {
-  caught = true;
-}
-assertFalse(caught);
-
-// In global scope
-caught = false;
-try {
-  let z = 1;
-  eval('var z = 2');
-} catch (e) {
-  caught = true;
-}
 assertTrue(caught);
 
 // Let declarations beyond a function boundary don't conflict
 caught = false;
 try {
   let a = 1;
   (function() {
     eval('var a');
   })();
 } catch (e) {
   caught = true;
 }
 assertFalse(caught);
 
 // var across with doesn't conflict
 caught = false;
 try {
   (function() {
     with ({x: 1}) {
-      eval("var x = 2;");
+      eval("var x");
     }
   })();
 } catch (e) {
   caught = true;
 }
 assertFalse(caught);
 
 // var can still conflict with let across a with
 caught = false;
 try {
   (function() {
     let x;
     with ({x: 1}) {
-      eval("var x = 2;");
+      eval("var x");
     }
   })();
 } catch (e) {
   caught = true;
 }
 assertTrue(caught);
 
 // Functions declared in eval also conflict
 caught = false
 try {
@@ skipping 16 matching lines @@
     {
       let x = 1;
       eval('{ function x() {} }');
     }
   })();
 } catch (e) {
   caught = true;
 }
 // TODO(littledan): switch to assertTrue when bug is fixed
 assertTrue(caught);