1149001 - Remove some SpiderMonkey tests for nonstandard let blocks

Status: RESOLVED DUPLICATE of bug 1023609

(Core :: JavaScript Engine, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: remove-let-tests.patch

This patch removes some test files and individual test cases of let blocks and expressions.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: add-block-scopes.patch

Change some SpiderMonkey tests to use new {} block scopes instead of nonstandard let blocks.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: replace-let-with-functions.patch

Change some SpiderMonkey tests to use arrow function IIFEs instead of nonstandard let blocks. Some of these tests are fuzzer jit-tests and won't cover the same code paths using IIFEs instead of let blocks. If you think the modified tests are no longer useful, I can just delete them instead.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: replace-let-with-with.patch

Change some SpiderMonkey tests to use `with` blocks instead of nonstandard let blocks where the `with` blocks are syntactically convenient replacements.

Some of these tests are fuzzer jit-tests and won't cover the same code paths or  will deoptimize the JIT when using `with` blocks. If you think the modified tests are no longer useful, I can just delete them instead.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #0)
> This patch removes some test files and individual test cases of let blocks
> and expressions.

Also, should I wait to remove these let block tests until after SpiderMonkey's let block support has been removed (in bug 1023609)?

Comment 5

[Chris Peterson [:cpeterson]]

btw, I intended to remove support for let blocks and expressions at the same time, but I see that for loop heads appear to be desugared to let blocks. I'll try to first remove just let expressions so I will keep all the let block tests for now.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8585253 [details] [diff] [review]
remove-let-tests.patch

Review of attachment 8585253 [details] [diff] [review]:
-----------------------------------------------------------------

Wondrous joy

::: js/src/jit-test/tests/debug/Environment-callee-01.js
@@ -20,5 @@
>  }
>  
>  check('debugger;', 'object', null);
>  check('with({}) { debugger; };', 'with', null);
> -check('let (x=1) { debugger; };', 'declarative', null);

For just this test we should replace these with let decls: '{ let x = 1; debugger; }' here

@@ -43,5 @@
>  
>  g.eval('function m() { debugger; yield true; }');
>  check('m().next();', 'declarative', gw.makeDebuggeeValue(g.m));
>  
> -g.eval('function n() { let (x = 1) { debugger; } }');

'function n() { let x = 1; debugger; }' here

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8585255 [details] [diff] [review]
add-block-scopes.patch

Review of attachment 8585255 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/testAliasedLet.js
@@ +1,2 @@
>  function f() {
> +    let x, y, z;

I don't think these outer decls are necessary.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8585259 [details] [diff] [review]
replace-let-with-functions.patch

Review of attachment 8585259 [details] [diff] [review]:
-----------------------------------------------------------------

Why were these changed to lambdas instead of just let decls?

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Chris Peterson [:cpeterson] from comment #4)
> (In reply to Chris Peterson [:cpeterson] from comment #0)
> > This patch removes some test files and individual test cases of let blocks
> > and expressions.
> 
> Also, should I wait to remove these let block tests until after
> SpiderMonkey's let block support has been removed (in bug 1023609)?

Shouldn't it be the other way around? These tests will fail if they aren't fixed/removed yet the actual language construct is removed.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8585261 [details] [diff] [review]
replace-let-with-with.patch

Review of attachment 8585261 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is a good change. I say remove the fuzz tests and change the non-fuzz tests to use let decls.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Shu-yu Guo [:shu] from comment #8)
> Why were these changed to lambdas instead of just let decls?

I guess let decls would have been easier for replacing let blocks, but the let expression tests needed an expression. I can switch to let decls if you prefer.

Comment 12

[Shu-yu Guo [:shu]]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> (In reply to Shu-yu Guo [:shu] from comment #8)
> > Why were these changed to lambdas instead of just let decls?
> 
> I guess let decls would have been easier for replacing let blocks, but the
> let expression tests needed an expression. I can switch to let decls if you
> prefer.

If you would, thanks. Lambdas would be testing something completely different than before. I'd just remove the fuzz tests and only change those that aren't obviously fuzz tests.

Comment 13

[Chris Peterson [:cpeterson]]

I posted a simpler patch to just remove tests of let expressions in bug 1023609 comment 1.