"use strict"; function x(x=({l:/[)]/}),...l) {} print(x) asserts js debug shell on m-c changeset 50cf5bbcb180 without any CLI arguments at Assertion failure: fun->isExprClosure() == !braced,

autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 112609:247fb0614615 user: Luke Wagner date: Thu Nov 01 21:35:09 2012 -0700 summary: Bug 807228 - clean up the JSFUN_* flags situation (r=jorendorff) Luke, do you think bug 807228 caused the assertion to appear?

I don't think so; I believe the patch just fixes a bug where we were incorrectly flagging 'x' in comment 0 as a non-expression-closure (due to the previous insane JSFUN flag situation). I'm not positive I'm parsing 'x' correctly in comment 0, but I think this is just a bug in FindBody where /[)]/ is a regular expression but we are thinking it is the closing ) of the arguments list? Is that right Benjamin? (I hope I'm wrong, because otherwise it seems like we'll need to rope in the regexp parsing logic...)

Oops, I should have said "incorrectly flagging 'x' as an *expression* closure" in the first sentence.

The following changeset may have been the one that started asserting: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 99950:e080642175e6 user: Benjamin Peterson date: Fri Jul 20 20:17:38 2012 +0200 summary: Bug 761723 - Save script sources to implement Function.prototype.toString. r=jorendorff,njn,jimb,jst,Ms2ger (and this assertion later morphed to the one in this bug after Luke's changeset landing in comment 1)

(In reply to Luke Wagner [:luke] from comment #2) > (I hope I'm wrong, because otherwise it seems like we'll need to rope in the > regexp parsing logic...) The ES regexp grammar is designed so you can do a quick lex without knowing all the details. It's not as trivial as one might like, but it's not as bad as it could be.

Cool, I didn't know that.

This works and doesn't seem to break anything else. Not sure if it's actually more correct, though.

Comment on attachment 710188 [details] [diff] [review] use TSF_OPERAND flag Review of attachment 710188 [details] [diff] [review]: ----------------------------------------------------------------- Not good enough, unfortunately: function y(y=({a: 3/2, b: /[)]/, c: 2/3}), ...l) {} y.toSource(); Clearing review flag.

Comment on attachment 710188 [details] [diff] [review] use TSF_OPERAND flag Review of attachment 710188 [details] [diff] [review]: ----------------------------------------------------------------- Not good enough, unfortunately: function y(y=({a: 3/2, b: /[)]/, c: 2/3}), ...l) {} y.toSource(); Clearing review flag.

Assigning to Benjamin since he has created a patch, please change this if necessary.

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2c41bf87b4e5).

JSBugMon: Fix Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first good revision is: changeset: 125318:bf3ce88c6ea3 user: Jason Orendorff date: Sun Mar 17 20:42:36 2013 -0700 summary: Bug 846406 - Implement arrow functions. r=bhackett. Changes to Y.js r=brendan. This iteration took 130.377 seconds to run.

Jason, is the fix in comment 12 likely to have fixed this issue? Should we close as dup/wfm?

It makes no sense for my changes to FindBody to have fixed it. Looking closer.

Oh, I weakened the assertion. The code is still bogus and generates bogus output, but the assertion no longer detects it. Here's a test that detects it: "use strict"; function f(x=/[)]/) {} assertEq("" + f, 'function f(x=/[)]/) {\n"use strict";\n}'); function g(a=3/2, b=/[)]/, c=2/3) {} assertEq("" + g, 'function g(a=3/2, b=/[)]/, c=2/3) {\n"use strict";\n}');

jimb claims that you can tokenize JS using a finite amount of state; you don't have to do a full parse. It occurs to me we could just teach the TokenStream how to do this and stop using parser feedback. But what's the algorithm?

I guess it is pretty straightforward. * At the beginning of a program, / starts a regexp. * After an identifier, keyword, literal, or one of the punctuators ) ] }, / is division. * After any other punctuator, except ++ or --, / starts a regexp. * If you the previous token is ++ or --, ignore it and look at the token before that: a / 2; division a++ / 2; still division a = / 2 /; regexp a = ++/ 2 /.x; still regexp Having TokenStream do it automatically would fix this bug.

I might be missing something, but here are some fun test cases: * a = 1 <newline> ++/ * if (1) / * if (1) {} / * a = {} /

(In reply to Simon Lindholm from comment #18) > * if (1) / Oh, yeah, you're right, this is awful. jimb, this makes comment 5 a little less true, right? What do you think? To cope correctly with this case, the tokenizer would have to keep a stack of matching () [] {} characters. Something like SUPPORTED_NESTING_DEPTH bytes. :-P

(In reply to Jason Orendorff [:jorendorff] from comment #19) > Oh, yeah, you're right, this is awful. jimb, this makes comment 5 a little > less true, right? What do you think? Setting needinfo? from :jimb to help move this forward.

(In reply to Jason Orendorff [:jorendorff] from comment #19) > (In reply to Simon Lindholm from comment #18) > > * if (1) / > > Oh, yeah, you're right, this is awful. jimb, this makes comment 5 a little > less true, right? What do you think? Yeah, my hack loses in that case. :( I guess I'd better fix the idutils scanner.

Not sure what needs to be done here since it seems to have dropped off people's radar in the past 8-9 months, so re-pinging needinfo, or please feel free to push this to someone else.

I have no easy fix for this.

I propose we rip out the part of Function.p.toString()/.toSource() where we try to inject "use strict"; if it's not already present. Sure, the meaning of a strict-mode function depends on its context. So what? The same goes for every closure; we don't try to rewrite those. What do you think, Waldo?

I agree entirely.

Comment on attachment 8729627 [details] [diff] [review] Stop trying to inject "use strict"; into Function.prototype.toString() output Review of attachment 8729627 [details] [diff] [review]: ----------------------------------------------------------------- YES. FINALLY. ::: js/src/tests/ecma_5/extensions/strict-function-toSource.js @@ -7,5 @@ > -function testRunOptionStrictMode(str, arg, result) { > - var strict_inner = new Function('return typeof this == "undefined";'); > - return strict_inner; > -} > -assertEq(eval(uneval(testRunOptionStrictMode()))(), true); This test doesn't look like it *really* cares about "use strict" insertion, so it should probably stick around. But this, should change to: assertEq(testRunOptionStrictMode()(), true); assertEq(eval(uneval(testRunOptionStrictMode()))(), false); @@ -9,5 @@ > - return strict_inner; > -} > -assertEq(eval(uneval(testRunOptionStrictMode()))(), true); > - > -assertEq((new Function('x', 'return x*2;')).toSource().includes('\n"use strict"'), true); And seems like this should check for false.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7b0aae0f722d93d9bff69d5051d97fe5cf2c42 Bug 837192 - Stop trying to inject "use strict"; into Function.prototype.toString() output. r=Waldo.

This bug's landing (comment 28) left a variable with only 1 usage, inside of a MOZ_ASSERT_IF statement, which caused Werror unused-variable build errors in opt builds & closed the tree. I pushed a followup (comment 29) to fix that -- I folded the variable into its only usage site, which is an assertion.

https://hg.mozilla.org/mozilla-central/rev/5b7b0aae0f72 https://hg.mozilla.org/mozilla-central/rev/e79cc51e486c