Flutter engine const finder tests fail with NNBD flag flip

[jakemac53]

See the log here https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8865278440977658192/+/steps/test_engine/0/stdout, (here is the test itself).

These consts here appear to be missing from the dill file, or at least aren't visited by the visitor any longer (the ones numbers 7, 8, and 9). Note that these are const constructor invocations nested inside a non-const constructor invocation, so maybe the visitor doesn't properly crawl those in NNBD mode somehow?

[jakemac53]

Part of #43872

[johnniwinther]

Is this only observed when non-nullable is enabled by default?

[jakemac53]

That is only an assumption, based on it being red for the flag flip CL.

[johnniwinther]

@alexmarkov This tests runs frontend server with --tfa so the problem might be the tree-shaking.

[jakemac53]

Ah, I wonder if just adding a reference to the target in the toString method of the IgnoreMe class would resolve this?

[alexmarkov]

@jakemac53 How can I reproduce this problem? I tried getting and building the latest flutter engine, and then running

out/host_debug_unopt/dart-sdk/bin/dart --enable-experiment=non-nullable flutter/tools/const_finder/test/const_finder_test.dart out/host_debug_unopt/gen/frontend_server.dart.snapshot out/host_debug_unopt/flutter_patched_sdk

but the test works. Do I need to patch additional changes?

[jakemac53]

This fails with the flag flip cl here https://dart-review.googlesource.com/c/sdk/+/168220

[alexmarkov]

I patched that CL into third_party/dart in the engine build, but still wasn't able to reproduce the problem.
Using engine flutter/engine@53d5d68.

[jakemac53]

Ya, I also haven't been able to repro locally. It seems like possibly my earlier attempt to fix these bots did actually fix it but the bot didn't pick up those changes maybe? I can't really explain why that would happen though it landed a while ago.

[johnniwinther]

If I run frontend_server on this reduced version of the test I do see a difference:

// @dart=2.7

class IgnoreMe {
  const IgnoreMe([this.target]);

  final Target target;
}

class Target {
  const Target(this.stringValue, this.intValue, this.targetValue);

  final String stringValue;
  final int intValue;
  final Target targetValue;

  void hit() {
    print('$stringValue $intValue');
  }
}

main() {
  final IgnoreMe ignoreMe2 = IgnoreMe(const Target('8', 8, null));
}

When running with --tfa:

dart pkg/frontend_server/bin/frontend_server_starter.dart --sdk-root=out/ReleaseX64/ --platform=vm_platform_strong.dill --target=vm --aot --tfa --output-dill=out.dill pkg\front_end\testcases\nnbd\issue43938.dart

the generated main method is:

  static method main() → dynamic {
    final iss::IgnoreMe* ignoreMe2 = new iss::IgnoreMe::•();
  }

When running without --tfa:

dart pkg\frontend_server\bin\frontend_server_starter.dart --sdk-root=out/ReleaseX64/ --platform=vm_platform_strong.dill --target=vm --aot --output-dill=out.dill pkg\front_end\testcases\nnbd\issue43938.dart

the generated main method is:

  static method main() → dynamic {
    final iss::IgnoreMe* ignoreMe2 = new iss::IgnoreMe::•(#C3272);
  }

Notice constant expression #C3272 passed to the constructor in the second case.

[alexmarkov]

This difference could be caused by signature shaking. It was recently reworked in 23ee425 (cc @askeksa-google). We should probably make sure that targetValue is used in this test to avoid tree shaking.

It is not clear how this is related to null safety...