Disappearing function types in constructor arguments

[mkustermann]

From the test in tests/language/function_subtype_inline2_test.dart:

class C {
  var field;
  C.c1(int this.field());
  C.c2({int this.field()});
  C.c3({int field(): null});
  C.c4({int this.field(): null});
  C.c5([int this.field()]);
  C.c6([int field() = null]);
  C.c7([int this.field() = null]);
}

  class C extends core::Object {
    field dynamic field;
    constructor c1(dynamic field) → void
      : test::C::field = field, super core::Object::•()
      ;
    constructor c2({dynamic field = null}) → void
      : test::C::field = field, super core::Object::•()
      ;
    constructor c3({dynamic field = null}) → void
      : test::C::field = null, super core::Object::•()
      ;
    constructor c4({dynamic field = null}) → void
      : test::C::field = field, super core::Object::•()
      ;
    constructor c5([dynamic field = null]) → void
      : test::C::field = field, super core::Object::•()
      ;
    constructor c6([dynamic field = null]) → void
      : test::C::field = null, super core::Object::•()
      ;
    constructor c7([dynamic field = null]) → void
      : test::C::field = field, super core::Object::•()
      ;
  }

Everything becomes dynamic.

This is one of the issues that appears in checked-mode.

[mkustermann]

I think @peter-ahe-google or @stereotype441 fixed this not too long ago.

[peter-ahe-google]

This test is still marked as failing in checked mode.

[mkustermann]

@peter-ahe-google I think that's because the buildbot running the checked mode tests is on FYI and nobody updated the status files, see build where it is passing:

FAILED: dartk-vm-checked release_x64 language/function_subtype_inline2_test
Expected: RuntimeError 
Actual: Pass
CommandOutput[vm]:

Command[vm]: DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart --dfe=out/ReleaseX64/gen/kernel-service.dart.snapshot --kernel-binaries=out/ReleaseX64/patched_sdk --enable_asserts --enable_type_checks --ignore-unrecognized-flags --packages=/b/build/slave/vm-kernel-linux-release-x64-checked-be/build/sdk/.packages /b/build/slave/vm-kernel-linux-release-x64-checked-be/build/sdk/tests/language/function_subtype_inline2_test.dart
Took 0:00:01.351702

Short reproduction command (experimental):
    python tools/test.py -m release -c dartk --checked --builder-tag no_ipv6 language/function_subtype_inline2_test

Since I'm the gardener today, I'll update status files for it and start moving it to the main waterfall. It has the same cycle time as the other builders we have there (and since kernel integration is now getting integrated into flutter, which e.g. uses checked mode, we really need to ensure we test it (and act upon new failures )).

[peter-ahe-google]

Please don't move them to the main waterfall yet. I can't keep them green without doubling my testing time.

[mkustermann]

I don't think our engineers are expected to run every single possible configuration for every CL before submitting. People should test what they think gives good coverage and let the buildbot do the exhaustive test matrix. It's not an ideal setup, but @whesse is working on getting us try bots atm.

If we have something our customers are using, we have to know when we break it. Currently we don't get notified on any breakages for checked mode issues (as well as precompilation issues).

People are pushing hard to get kernel support in the VM ready so flutter can also use it, so we really have to test it (whatever infrastructure we have available now).

At least I'll update the status files for checked mode, and maybe prepare a CL we can land if there is agreement on the builder.

[peter-ahe-google]

Please discuss this with @mraleph.

If I don't test all combinations and break something, my changes are likely to be reverted.

In my opinion, it would be better to turn off checked mode until we have less bugs in production mode.

[whesse]

There will be always be a tradeoff between trybot coverage (or local testing) completeness and resources used, so it is inevitable that some changes will get reverted when the full buildbot shows unexpected failures. If is easier to fix breakages in checked mode as they occur, rather than letting them build up, I think it is worth having more reverts happen. But just adding one checked configuration should catch most errors, and shouldn't double the testing matrix.

[peter-ahe-google]

Yes. It is a tradeoff. This is why @mraleph needs to be involved.