Wrong coverage in mixins

[tsinis]

Hey Dart Team!

Coverage calculation has changed since 2.18 and is no longer calculating coverage on mixins. Please check by yourself at https://github.com/tsinis/mixins_tests_bug

Just run tests with the coverage flag and you will find out that the CoverageBug mixin is not covered even though it's the same as CoverageOk class. It was introduced in Dart 2.18. Thanks!

• Dart SDK version: 2.18.0 (stable)
• MacOSX (12.5.1 (21G83)) with M1 CPU

[devoncarew]

cc @liamappelbe

[liamappelbe]

Something weird is going on here. I've confirmed that this bug is new in 2.18, but it only happens if I run the test via dart test. If I run the test script directly, dart test/main_test.dart, coverage works correctly. Might be something to do with the logic that decides whether a function should be included in the report? I'm still investigating.

[liamappelbe]

Looks like the wrongCoverage method is missing its source positions, but only when run through package:test. I also tried running it through Isolate.spawnUri, and it is covered correctly.

@jakemac53 How exactly does package:test compile VM tests? Does it pass any specific flags to the kernel isolate? It seems that as of Dart 2.18, mixin methods compiled by package:test are missing source positions, so we can't gather coverage for them.

[jakemac53]

https://github.com/dart-lang/test/blob/b0a39cc64807152e95075a8a2b7d216c4bf0b05b/pkgs/test_core/lib/src/runner/vm/platform.dart#L154 is the default strategy for spawning isolates, it ends up using frontend server which is spawned with this configuration https://github.com/dart-lang/test/blob/b0a39cc64807152e95075a8a2b7d216c4bf0b05b/pkgs/test_core/lib/src/runner/vm/test_compiler.dart#L161.

There hasn't been any change to how it works in a while that I am aware of.

@tsinis Another thing you could try out of curiosity is passing --use-data-isolate-strategy which won't precompile the test to a dill file and bypasses using frontend server entirely.

[liamappelbe]

I've managed to reproduce the bug without going through dart test. When I compile the test to a dill using FrontendServerClient, it reproduces the bug. When I compile the test to a dill using dart compile, it doesn't reproduce the bug. So I dumped the kernel files.

Bug free dill produced by dart compile:

main = mai::main;
library from "file:///usr/local/google/home/liama/dart-sdk/mixins_tests_bug/test/main_test_fe.dart" as mai {

[0]  import "package:mixins_tests_bug/main.dart";

[52]  abstract class _MixinTest&Object&CoverageBug extends core::Object implements main::CoverageBug /*isAnonymousMixin,isEliminatedMixin*/  {
[27]    field core::bool boolean = [-1] [-1] false /* from file:///usr/local/google/home/liama/dart-sdk/mixins_tests_bug/lib/main.dart */;
[-1]    synthetic constructor •() → mai::_MixinTest&Object&CoverageBug
      : [-1] super core::Object::• [-1]()
[-1]      ;
[52]    method /* from file:///usr/local/google/home/liama/dart-sdk/mixins_tests_bug/lib/main.dart */ wrongCoverage() → core::bool {
[-1]      [74] [74] core::print [-1]([-1] "starting method");
[127]      [127] final core::int x = [-1] [-1] 2;
[142]      [142] core::int y = [-1] [-1] 3;
[-1]      [153] [153] [-1] [-1] this.{main::CoverageBug::boolean} = [-1] [-1]![167]([167] [165] [165] #C1 =={core::num::==}{(core::Object) → core::bool} [172] [172] [170] [170] x.{core::num::+}[-1]([174] y){(core::num) → core::int});
[-1]      [182] [182] core::print [-1]([-1] "triggered all those lines in test for coverage");
[244]      return [251] [251] [-1] [-1] this.{main::CoverageBug::boolean}{core::bool};
    }
  }
[52]  class MixinTest extends mai::_MixinTest&Object&CoverageBug {
[52]    synthetic constructor •() → mai::MixinTest
      : [-1] super mai::_MixinTest&Object&CoverageBug::• [-1]()
[-1]      ;
  }
[88]  static method main() → void {
[105]    [105] final mai::MixinTest mixinBugTest = [120] [120] new mai::MixinTest::• [-1]();
[-1]    [135] [135] core::print [-1]([154] [141] [141] mixinBugTest.{mai::_MixinTest&Object&CoverageBug::boolean}{core::bool});
[-1]    [166] [166] core::print [-1]([185] [172] [172] mixinBugTest.{mai::_MixinTest&Object&CoverageBug::wrongCoverage}[-1](){() → core::bool});
[-1]    [205] [205] core::print [-1]([224] [211] [211] mixinBugTest.{mai::_MixinTest&Object&CoverageBug::boolean}{core::bool});
[-1]    [236] [236] core::print [-1]([242] main::foo [-1]());
[-1]    [252] [252] core::print [-1]([-1] "Testing done");
  }
}
library from "package:mixins_tests_bug/main.dart" as main {

[6]  class CoverageBug extends core::Object {
[27]    field core::bool boolean = [-1] [-1] false;
[6]    synthetic constructor •() → main::CoverageBug
      : [-1] super core::Object::• [-1]()
[-1]      ;
[52]    method wrongCoverage() → core::bool {
[-1]      [74] [74] core::print [-1]([-1] "starting method");
[127]      [127] final core::int x = [-1] [-1] 2;
[142]      [142] core::int y = [-1] [-1] 3;
[-1]      [153] [153] [-1] [-1] this.{main::CoverageBug::boolean} = [-1] [-1]![167]([167] [165] [165] #C1 =={core::num::==}{(core::Object) → core::bool} [172] [172] [170] [170] x.{core::num::+}[-1]([174] y){(core::num) → core::int});
[-1]      [182] [182] core::print [-1]([-1] "triggered all those lines in test for coverage");
[244]      return [251] [251] [-1] [-1] this.{main::CoverageBug::boolean}{core::bool};
    }
  }
[267]  static method foo() → dynamic
[276]    return [-1] [-1] 1234;
}
constants  {
  #C1 = 1
}

Buggy dill produced by FrontendServerClient:

main = mai::main;
library from "package:mixins_tests_bug/main.dart" as main {

[6]  class CoverageBug extends core::Object {
[27]    field core::bool boolean = [-1] [-1] false;
[6]    synthetic constructor •() → main::CoverageBug
      : [-1] super core::Object::• [-1]()
[-1]      ;
[52]    method wrongCoverage() → core::bool {
[-1]      [74] [74] core::print [-1]([-1] "starting method");
[127]      [127] final core::int x = [-1] [-1] 2;
[142]      [142] core::int y = [-1] [-1] 3;
[-1]      [153] [153] [-1] [-1] this.{main::CoverageBug::boolean} = [-1] [-1]![167]([167] [165] [165] #C1 =={core::num::==}{(core::Object) → core::bool} [172] [172] [170] [170] x.{core::num::+}[-1]([174] y){(core::num) → core::int});
[-1]      [182] [182] core::print [-1]([-1] "triggered all those lines in test for coverage");
[244]      return [251] [251] [-1] [-1] this.{main::CoverageBug::boolean}{core::bool};
    }
  }
[267]  static method foo() → dynamic
[276]    return [-1] [-1] 1234;
}
library from "file:///usr/local/google/home/liama/dart-sdk/mixins_tests_bug/test/main_test_fe.dart" as mai {

[0]  import "package:mixins_tests_bug/main.dart";

[52]  abstract class _MixinTest&Object&CoverageBug extends core::Object implements main::CoverageBug /*isAnonymousMixin,isEliminatedMixin*/  {
[27]    field core::bool boolean = [-1] [-1] false /* from file:///usr/local/google/home/liama/dart-sdk/mixins_tests_bug/lib/main.dart */;
[-1]    synthetic constructor •() → mai::_MixinTest&Object&CoverageBug
      : [-1] super core::Object::• [-1]()
[-1]      ;
[52]    method /* from file:///usr/local/google/home/liama/dart-sdk/mixins_tests_bug/lib/main.dart */ wrongCoverage() → core::bool {
[-1]      [74] [74] core::print [-1]([-1] "starting method");
[127]      [127] final core::int x = [-1] [-1] 2;
[142]      [142] core::int y = [-1] [-1] 3;
[-1]      [153] [153] [-1] [-1] this.{main::CoverageBug::boolean} = [-1] [-1]![167]([167] [165] [165] #C1 =={core::num::==}{(core::Object) → core::bool} [172] [172] [170] [170] x.{core::num::+}[-1]([174] y){(core::num) → core::int});
[-1]      [182] [182] core::print [-1]([-1] "triggered all those lines in test for coverage");
[244]      return [251] [251] [-1] [-1] this.{main::CoverageBug::boolean}{core::bool};
    }
  }
[52]  class MixinTest extends mai::_MixinTest&Object&CoverageBug {
[52]    synthetic constructor •() → mai::MixinTest
      : [-1] super mai::_MixinTest&Object&CoverageBug::• [-1]()
[-1]      ;
  }
[88]  static method main() → void {
[105]    [105] final mai::MixinTest mixinBugTest = [120] [120] new mai::MixinTest::• [-1]();
[-1]    [135] [135] core::print [-1]([154] [141] [141] mixinBugTest.{mai::_MixinTest&Object&CoverageBug::boolean}{core::bool});
[-1]    [166] [166] core::print [-1]([185] [172] [172] mixinBugTest.{mai::_MixinTest&Object&CoverageBug::wrongCoverage}[-1](){() → core::bool});
[-1]    [205] [205] core::print [-1]([224] [211] [211] mixinBugTest.{mai::_MixinTest&Object&CoverageBug::boolean}{core::bool});
[-1]    [236] [236] core::print [-1]([242] main::foo [-1]());
[-1]    [252] [252] core::print [-1]([-1] "Testing done");
  }
}
constants  {
  #C1 = 1
}

These are identical, except that the order of the 2 library chunks is swapped. In both cases, wrongCoverage does have a script index. I've double checked the logs, and the difference at coverage collection time is still that the buggy dill's version of wrongCoverage has no script index.

@alexmarkov Is it possible that the different ordering could cause wrongCoverage to be missing its script index, or is this a red herring and dump_kernel.dart just doesn't report all the information in the dill? In the buggy one, the declaration of CoverageBug.wrongCoverage comes first, then the library that declares class MixinTest with CoverageBug {} comes after. Is it possible that the declaration of MixinTest overwrites the script index of CoverageBug.wrongCoverage somehow?

[alexmarkov]

We have some logic for creating and caching PatchClass objects in KernelLoader::ClassForScriptAt. PatchClass object is used instead of Class when member has a script which is different from the script of the class (this situation happens in mixin case). Depending on the order of loaded libraries KernelLoader::ClassForScriptAt may create different number of PatchClass instances (as it only caches 1 instance per script index, overwriting a previously cached PatchClass).

Note that SourceReport has its own notion of script indices, different from kernel. So the bug could be somewhere between SourceReport::CollectAllScripts, Library::LoadedScripts and kernel loader. Consider printing scripts as they are discovered in Library::LoadedScripts() and check the difference.

[mZadorskii]

This issue is blocking us from updating to latest flutter. Thanks for fixing it quick!
I hope it will be merged / hotfixed asap.

[a-siva]

A cherry pick for this was created and released in a stable hot fix, closing this issue.