fix readability regressions from previous block-scope CL

fix readability regressions from previous block-scope CL

also reverts the simple incremental compile code, it wasn't aware of our own compiler's version.

also fixes a warning from analyzer

R=leafp@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/06f19055b7e09838e63d548dbe016a1191fb290a

[Jennifer Messerly, 2015-09-09 20:35:04]

jmesserly@google.com changed reviewers:
+ leafp@google.com

[Leaf, 2015-09-09 21:29:45]

lgtm, assuming the answer to my question/comment is yes.

https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2936: new
JS.Block(_visitList(node.body.statements) as List<JS.Statement>));
Will JS be ok with this hoisting if either the exception parameter or the stack
trace parameter share the same name as a variable declared in the list of
statements being hoisted out?

[Jennifer Messerly, 2015-09-09 21:48:39]

Thoughts?

https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2936: new
JS.Block(_visitList(node.body.statements) as List<JS.Statement>));
On 2015/09/09 21:29:45, Leaf wrote:
> Will JS be ok with this hoisting if either the exception parameter or the
stack
> trace parameter share the same name as a variable declared in the list of
> statements being hoisted out?

JS wouldn't care, unless we end up desugaring (_catchParameter != null or
node.stackTraceParameter != null) ... but in that case, we have a duplicate
declaration.

If Dart really supports having a local variable that shadows and the exception
and/or stack trace (ugh, why?!?!) then we'll need to fix it, or make it an error
(because that seems like pretty useless behavior) :|.

Assuming that behavior is specified, we've had this bug forever though, until I
"fixed" it unintentionally last Friday. So I'd be happiest to land this and then
investigate that issue in a follow up, if that works for you. I think the fix
will be unrelated: to avoid a large readability regression, we'll need to do
some smarts to detect the conflict specifically, and that's unlike the other
places. Or, we need to change how variable declaration is generated in this case
to make it be an assignment.

[Leaf, 2015-09-09 21:57:14]

Just tried it, dartanalyzer and the vm both reject shadowing there, so I think
we're ok.  LGTM.

On 2015/09/09 21:48:39, John Messerly wrote:
> Thoughts?
> 
>
https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
> File lib/src/codegen/js_codegen.dart (right):
> 
>
https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
> lib/src/codegen/js_codegen.dart:2936: new
> JS.Block(_visitList(node.body.statements) as List<JS.Statement>));
> On 2015/09/09 21:29:45, Leaf wrote:
> > Will JS be ok with this hoisting if either the exception parameter or the
> stack
> > trace parameter share the same name as a variable declared in the list of
> > statements being hoisted out?
> 
> JS wouldn't care, unless we end up desugaring (_catchParameter != null or
> node.stackTraceParameter != null) ... but in that case, we have a duplicate
> declaration.
> 
> If Dart really supports having a local variable that shadows and the exception
> and/or stack trace (ugh, why?!?!) then we'll need to fix it, or make it an
error
> (because that seems like pretty useless behavior) :|.
> 
> Assuming that behavior is specified, we've had this bug forever though, until
I
> "fixed" it unintentionally last Friday. So I'd be happiest to land this and
then
> investigate that issue in a follow up, if that works for you. I think the fix
> will be unrelated: to avoid a large readability regression, we'll need to do
> some smarts to detect the conflict specifically, and that's unlike the other
> places. Or, we need to change how variable declaration is generated in this
case
> to make it be an assignment.

[Jennifer Messerly, 2015-09-09 22:02:57]

https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1305413006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2936: new
JS.Block(_visitList(node.body.statements) as List<JS.Statement>));
On 2015/09/09 21:48:39, John Messerly wrote:
> On 2015/09/09 21:29:45, Leaf wrote:
> > Will JS be ok with this hoisting if either the exception parameter or the
> stack
> > trace parameter share the same name as a variable declared in the list of
> > statements being hoisted out?
> 
> JS wouldn't care, unless we end up desugaring (_catchParameter != null or
> node.stackTraceParameter != null) ... but in that case, we have a duplicate
> declaration.
> 
> If Dart really supports having a local variable that shadows and the exception
> and/or stack trace (ugh, why?!?!) then we'll need to fix it, or make it an
error
> (because that seems like pretty useless behavior) :|.
> 
> Assuming that behavior is specified, we've had this bug forever though, until
I
> "fixed" it unintentionally last Friday. So I'd be happiest to land this and
then
> investigate that issue in a follow up, if that works for you. I think the fix
> will be unrelated: to avoid a large readability regression, we'll need to do
> some smarts to detect the conflict specifically, and that's unlike the other
> places. Or, we need to change how variable declaration is generated in this
case
> to make it be an assignment.

confirmed that it's spec'd that way.

[Jennifer Messerly, 2015-09-09 22:52:21]

Committed patchset #1 (id:1) manually as
06f19055b7e09838e63d548dbe016a1191fb290a (presubmit successful).