Fix harmony sloppy block scoping dynamic redeclaration check

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

Index: test/mjsunit/harmony/block-eval-var-over-let.js
diff --git a/test/mjsunit/harmony/block-eval-var-over-let.js b/test/mjsunit/harmony/block-eval-var-over-let.js
index 292d073c81002e5239c525fdceb66a9e5245b392..c95123167c7f9713e0027c5f85341ec5d255bb9f 100644
--- a/test/mjsunit/harmony/block-eval-var-over-let.js
+++ b/test/mjsunit/harmony/block-eval-var-over-let.js
@@ -6,116 +6,68 @@
 
 // Var-let conflict in a function throws, even if the var is in an eval
 
-let caught = false;
-
 // Throws at the top level of a function
-try {
-  (function() {
-    let x = 1;
-    eval('var x = 2');
-  })()
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
+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
-caught = false;
-try {
-  (function() {
-    let y = 1;
-    { eval('var y = 2'); }
-  })()
-} catch (e) {
-  caught = true;
-}
-assertTrue(caught);
+assertThrows(function() {
+  let y = 1;
+  { eval('var y'); }
+}, TypeError);
 
 // 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);
+assertThrows(function() {
+  {
+    let x = 1;
+    eval('var x');
+  }
+}, TypeError);
 
 // 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);
+assertDoesNotThrow(function() {
+  {
+    let x = 1;
+  }
+  eval('var x');
+});
 
 // 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);
+assertThrows(function() {
+  const x = 1;
+  eval('var x');
+}, TypeError);
 
 // 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);
+assertThrows(function() {
+  const y = 1;
+  { eval('var y'); }
+}, TypeError);
 
 // 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);
+assertThrows(function() {
+  {
+    const x = 1;
+    eval('var x');
+  }
+}, TypeError);
 
 // 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);
+assertDoesNotThrow(function() {
+  {
+    const x = 1;
+  }
+  eval('var x');
+});
 
 // In global scope
-caught = false;
+let caught = false;
 try {
   let z = 1;
-  eval('var z = 2');
+  eval('var z');
 } catch (e) {
   caught = true;
 }
@@ -138,7 +90,7 @@ caught = false;
 try {
   (function() {
     with ({x: 1}) {
-      eval("var x = 2;");
+      eval("var x");
     }
   })();
 } catch (e) {
@@ -152,7 +104,7 @@ try {
   (function() {
     let x;
     with ({x: 1}) {
-      eval("var x = 2;");
+      eval("var x");
     }
   })();
 } catch (e) {