[dart2js] Unreachable code after `return null`

[parlough]

Not a big deal, but in the new DartPad frontend I noticed warnings about unreachable code. Something along the lines of:

return null
return new ...,

Maybe the return null was an early exit behind a condition that was determined to always occur in an earlier pass?

I don't have a super simple reproduction yet, but I was able to reduce it to:

import 'package:flutter/cupertino.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const CupertinoApp(
      title: 'dart2js reproduction',
      home: CupertinoPageScaffold(
        navigationBar: CupertinoNavigationBar(
          middle: Text('dart2js reproduction'),
        ),
        child: Text('Hi web compiler engineers'),
      ),
    );
  }
}

With flutter build web on main at https://github.com/dart-lang/sdk/commit/b6647fd9ab2d3308a5894cc0e70d7da20d413082.

[rakudrama]

Are you seeing warnings from the browser about unreachable code in the generated JavaScript? If so, which browser? We don't generally aim to satisfy 'lints' coming from the browser.

dart2js might occasionally generate this kind of code because the simplifier phase (which runs 'top down') is run several times, including after the last dead code elimination phase (which runs 'bottom up'). It would be unusual to see dead code like this since the earlier runs of the simplifier phase usually 'decide' the condition guarding first return.

It is beneficial to do dead code elimination as part of simplification (we do a limited amount here). Perhaps we should be more aggressive about dead code elimination during simplification, or always run dead code elimination after the last simplification.

[parlough]

Thanks for the explanation!

Are you seeing warnings from the browser about unreachable code in the generated JavaScript? If so, which browser? We don't generally aim to satisfy 'lints' coming from the browser.

Firefox warns about it: "unreachable code after return statement". The current version of https://preview.dartpad.dev/ should trigger it as an example.

I agree it's not import to eliminate the warning, and really the code causing this should have been eliminated but wasn't due to a separate issue (https://github.com/flutter/flutter/issues/142633).

I still thought I'd call it out in case its expected to be able to handle this case. Especially since users might find warnings like this surprising and not matching their understanding of the optimizations that dart2js provides.

If you think it's not important to address or there's another issue tracking this, feel free to close this issue :)

It is beneficial to do dead code elimination as part of simplification... Perhaps we should be more aggressive about dead code elimination during simplification, or always run dead code elimination after the last simplification.

It'd be interesting to at least to see the impact of an extra DCE pass after the last simplification, both on size and compilation times for a large app. Slight size reductions and increased user confidence are always great!