If an if or do statement always exits, following statements are dead.

If an if or do statement always exits, following statements are dead.

BUG=https://github.com/dart-lang/sdk/issues/16420
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/d7833e1c1f418bf602d0b9faec7bc868daa55cde

[srawlins, 2016-05-04 17:00:54]

srawlins@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[srawlins, 2016-05-04 17:01:22]

I've performed a small performance analysis. This does not seem to slow down
warning analysis.

https://gist.github.com/srawlins/13edfecd9deea6149c8b3ac004e47dc1

[Brian Wilkerson, 2016-05-04 17:28:19]

I'm open to counter arguments if you disagree with me.

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:2071: bool statementExits(Statement
statement) {
With the exception of the handling of the break and continue statements, it
looks like all of this is already handled by the ExitDetector. Could this be
replaced by the following?

if (statement is BreakStatement) {
  return statement.label == null;
} else if (statement is BreakStatement) {
  return statement.label == null;
}
return ExitDetector.exits(statement);

I'd prefer to keep as much of this logic as possible in the ExitDetector. (In
fact, I don't know why the break and continue statements aren't handled by it.

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/hint_code_test.dart (right):

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/hint_code_test.dart:622: void
test_deadCode_statementAfterExitingIf_function() {
If ExitDetector worked, then most of these tests could be moved to be unit tests
for ExitDetector.

[srawlins, 2016-05-19 18:36:44]

Thanks Brian! Simplified, and two failing_ tests are now not failing!

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:2071: bool statementExits(Statement
statement) {
On 2016/05/04 17:28:19, Brian Wilkerson wrote:
> With the exception of the handling of the break and continue statements, it
> looks like all of this is already handled by the ExitDetector. Could this be
> replaced by the following?
> 
> if (statement is BreakStatement) {
>   return statement.label == null;
> } else if (statement is BreakStatement) {
>   return statement.label == null;
> }
> return ExitDetector.exits(statement);
> 
> I'd prefer to keep as much of this logic as possible in the ExitDetector. (In
> fact, I don't know why the break and continue statements aren't handled by it.

Done! Dramatically simpler!

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/hint_code_test.dart (right):

https://codereview.chromium.org/1943443002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/hint_code_test.dart:622: void
test_deadCode_statementAfterExitingIf_function() {
On 2016/05/04 17:28:19, Brian Wilkerson wrote:
> If ExitDetector worked, then most of these tests could be moved to be unit
tests
> for ExitDetector.

You're right. Dramatically reduced tests!

[Brian Wilkerson, 2016-05-20 18:04:28]

lgtm after test method names are fixed

https://codereview.chromium.org/1943443002/diff/60001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/hint_code_test.dart (right):

https://codereview.chromium.org/1943443002/diff/60001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/hint_code_test.dart:24: void
deadCode_statementAfterRethrow() {
nit: These method names need to start with "test_" in order to be recognized by
our "reflectiveTest" support.

[srawlins, 2016-05-23 20:07:53]

https://codereview.chromium.org/1943443002/diff/60001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/hint_code_test.dart (right):

https://codereview.chromium.org/1943443002/diff/60001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/hint_code_test.dart:24: void
deadCode_statementAfterRethrow() {
On 2016/05/20 18:04:28, Brian Wilkerson wrote:
> nit: These method names need to start with "test_" in order to be recognized
by
> our "reflectiveTest" support.

Done.

[Brian Wilkerson, 2016-05-23 20:09:21]

lgtm

[srawlins, 2016-05-23 20:20:53]

Description was changed from

==========
If an if or do statement always exits, following statements are dead.

BUG=https://github.com/dart-lang/sdk/issues/16420
==========

to

==========
If an if or do statement always exits, following statements are dead.

BUG=https://github.com/dart-lang/sdk/issues/16420
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d7833e1c1f418bf602d0b9faec7bc868daa55cde
==========

[srawlins, 2016-05-23 20:20:55]

Committed patchset #5 (id:80001) manually as
d7833e1c1f418bf602d0b9faec7bc868daa55cde (presubmit successful).

[sra1, 2016-05-23 21:40:46]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2016-05-23 21:40:47]

This CL causes false positives in DDC's presumbit.
Can it be reverted until it is fixed?

Example: the 'if' after the loop at [A] is flagged as dead.


  JS.Expression _emitNullSafe(Expression node) {
    // Desugar ?. sequence by passing a sequence of callbacks that applies
    // each operation in sequence:
    //
    //     obj?.foo()?.bar
    // -->
    //     nullSafe(obj, _ => _.foo(), _ => _.bar);
    //
    // This pattern has the benefit of preserving order, as well as minimizing
    // code expansion: each `?.` becomes `, _ => _`, plus one helper call.
    //
    // TODO(jmesserly): we could desugar with MetaLet instead, which may
    // lead to higher performing code, but at the cost of readability.
    var tail = <JS.Expression>[];
    for (;;) {
      var op = _getOperator(node);
      if (op != null && op.lexeme == '?.') {
        var nodeTarget = _getTarget(node);
        if (!isNullable(nodeTarget)) {
          node = _stripNullAwareOp(node, nodeTarget);
          break;
        }

        var param =
            _createTemporary('_', nodeTarget.staticType, nullable: false);
        var baseNode = _stripNullAwareOp(node, param);
        tail.add(new JS.ArrowFun([_visit(param)], _visit(baseNode)));
        node = nodeTarget;
      } else {
        break;
      }
    }
    if (tail.isEmpty) return _visit(node);  // [A]
    return js.call('dart.nullSafe(#, #)', [_visit(node), tail.reversed]);
  }

[srawlins, 2016-05-24 13:57:27]

On 2016/05/23 21:40:47, sra1 wrote:
> This CL causes false positives in DDC's presumbit.
> Can it be reverted until it is fixed?
> 
> Example: the 'if' after the loop at [A] is flagged as dead.
> 
> 
>   JS.Expression _emitNullSafe(Expression node) {
>     // Desugar ?. sequence by passing a sequence of callbacks that applies
>     // each operation in sequence:
>     //
>     //     obj?.foo()?.bar
>     // -->
>     //     nullSafe(obj, _ => _.foo(), _ => _.bar);
>     //
>     // This pattern has the benefit of preserving order, as well as minimizing
>     // code expansion: each `?.` becomes `, _ => _`, plus one helper call.
>     //
>     // TODO(jmesserly): we could desugar with MetaLet instead, which may
>     // lead to higher performing code, but at the cost of readability.
>     var tail = <JS.Expression>[];
>     for (;;) {
>       var op = _getOperator(node);
>       if (op != null && op.lexeme == '?.') {
>         var nodeTarget = _getTarget(node);
>         if (!isNullable(nodeTarget)) {
>           node = _stripNullAwareOp(node, nodeTarget);
>           break;
>         }
> 
>         var param =
>             _createTemporary('_', nodeTarget.staticType, nullable: false);
>         var baseNode = _stripNullAwareOp(node, param);
>         tail.add(new JS.ArrowFun([_visit(param)], _visit(baseNode)));
>         node = nodeTarget;
>       } else {
>         break;
>       }
>     }
>     if (tail.isEmpty) return _visit(node);  // [A]
>     return js.call('dart.nullSafe(#, #)', [_visit(node), tail.reversed]);
>   }

Sorry, Stephen! This has been reverted.

The bug you caught was landed in: https://codereview.chromium.org/2002353003/

I intend to land this CL again today, in:
https://codereview.chromium.org/2007933002/