[vm] Improve const constructor coverage

The way coverage collection was implemented for const constructors was a bit of a hack. After ordinary coverage collection was complete and all ordinary source ranges had been added to the report, SourceReport::CollectConstConstructorCoverageFromScripts would iterate through all the const constructors and add extra ranges to the source report that reported them as being hit. The old ranges were still in the report, reporting the constructors as missed, but the miss was overwritten by the hit when package:coverage turned the source report into a coverage report. That hacky approach didn't work for branch coverage.

I've refactored it to gather all those const constructor hits before ordinary coverage gathering, and store the hits in the script table. Then during the ordinary coverage collection flow, when we see one of those const constructors, we mark everything inside the function as hit.

This also fixes a related bug that we hadn't noticed before, where calls inside the const constructor were treated as missed.

Note: If there are dozens of const constructors in a single script, it might be worth sorting ScriptTableEntry.const_constructor_hits and then binary searching it, but I don't think that's worth doing atm.

Bug: dart-lang/tools#513
Fixes: dart-lang/tools#513
Change-Id: Ie66a153fbeaac5b3ecfc9a28a7f8d129ab61114a
TEST=source_report_test.cc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/421720
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Liam Appelbe <[email protected]>

Author: liamappelbe

pkg/vm_service/test/get_source_report_const_coverage_test.dart

@@ -87,6 +87,7 @@ final tests = <IsolateTest>[
       final coverage = await service.getSourceReport(
         isolateId,
         [SourceReportKind.kCoverage],
+        forceCompile: true,
       );
       hits = getHitsFor(coverage, filename);
       final lines = <int>{};
@@ -106,6 +107,7 @@ final tests = <IsolateTest>[
         isolateId,
         [SourceReportKind.kCoverage],
         scriptId: foundScript.id!,
+        forceCompile: true,
       );
       final localHits = getHitsFor(coverage, filename);
       expect(localHits.length, hits.length);

runtime/vm/source_report.cc

@@ -331,6 +331,7 @@ intptr_t SourceReport::GetTokenPosOrLine(const Script& script,
 }
 
 void SourceReport::PrintCoverageData(JSONObject* jsobj,
+                                     intptr_t script_index,
                                      const Function& function,
                                      const Code& code,
                                      bool report_branch_coverage) {
@@ -340,6 +341,17 @@ void SourceReport::PrintCoverageData(JSONObject* jsobj,
 
   const Script& script = Script::Handle(zone(), function.script());
 
+  bool const_constructor_hit = false;
+  if (function.IsConstructor() && function.is_const()) {
+    for (TokenPosition hit :
+         script_table_entries_[script_index]->const_constructor_hits) {
+      if (hit == begin_pos) {
+        const_constructor_hit = true;
+        break;
+      }
+    }
+  }
+
   const int kCoverageNone = 0;
   const int kCoverageMiss = 1;
   const int kCoverageHit = 2;
@@ -351,7 +363,7 @@ void SourceReport::PrintCoverageData(JSONObject* jsobj,
     coverage[i] = kCoverageNone;
   }
 
-  if (function.WasExecuted()) {
+  if (function.WasExecuted() || const_constructor_hit) {
     coverage[0] = kCoverageHit;
   } else {
     coverage[0] = kCoverageMiss;
@@ -382,7 +394,7 @@ void SourceReport::PrintCoverageData(JSONObject* jsobj,
       if (is_branch_coverage == report_branch_coverage) {
         const bool was_executed =
             Smi::Value(Smi::RawCast(coverage_array.At(i + 1))) != 0;
-        update_coverage(token_pos, was_executed);
+        update_coverage(token_pos, was_executed || const_constructor_hit);
       }
     }
   }
@@ -564,10 +576,12 @@ void SourceReport::VisitFunction(JSONArray* jsarr,
     PrintCallSitesData(&range, func, code);
   }
   if (IsReportRequested(kCoverage)) {
-    PrintCoverageData(&range, func, code, /* report_branch_coverage */ false);
+    PrintCoverageData(&range, script_index, func, code,
+                      /* report_branch_coverage */ false);
   }
   if (IsReportRequested(kBranchCoverage)) {
-    PrintCoverageData(&range, func, code, /* report_branch_coverage */ true);
+    PrintCoverageData(&range, script_index, func, code,
+                      /* report_branch_coverage */ true);
   }
   if (IsReportRequested(kPossibleBreakpoints)) {
     PrintPossibleBreakpointsData(&range, func, code);
@@ -678,6 +692,18 @@ void SourceReport::PrintJSON(JSONStream* js,
   {
     JSONArray ranges(&report, "ranges");
 
+    // Output constant coverage if coverage is requested.
+    if (IsReportRequested(kCoverage) || IsReportRequested(kBranchCoverage)) {
+      // Find all scripts. We need to go though all scripts because a script
+      // (even one we don't want) can add coverage to another library (i.e.
+      // potentially one we want).
+      DirectChainedHashMap<ScriptTableTrait> local_script_table;
+      GrowableArray<ScriptTableEntry*> local_script_table_entries;
+      CollectAllScripts(&local_script_table, &local_script_table_entries);
+      CollectConstConstructorCoverageFromScripts(&local_script_table_entries);
+      CleanupCollectedScripts(&local_script_table, &local_script_table_entries);
+    }
+
     const GrowableObjectArray& libs = GrowableObjectArray::Handle(
         zone(), thread()->isolate_group()->object_store()->libraries());
 
@@ -692,19 +718,6 @@ void SourceReport::PrintJSON(JSONStream* js,
 
     // Visit all closures for this isolate.
     VisitClosures(&ranges);
-
-    // Output constant coverage if coverage is requested.
-    if (IsReportRequested(kCoverage)) {
-      // Find all scripts. We need to go though all scripts because a script
-      // (even one we don't want) can add coverage to another library (i.e.
-      // potentially one we want).
-      DirectChainedHashMap<ScriptTableTrait> local_script_table;
-      GrowableArray<ScriptTableEntry*> local_script_table_entries;
-      CollectAllScripts(&local_script_table, &local_script_table_entries);
-      CollectConstConstructorCoverageFromScripts(&local_script_table_entries,
-                                                 &ranges);
-      CleanupCollectedScripts(&local_script_table, &local_script_table_entries);
-    }
   }
 
   // Print the script table.
@@ -756,8 +769,7 @@ void SourceReport::CleanupCollectedScripts(
 }
 
 void SourceReport::CollectConstConstructorCoverageFromScripts(
-    GrowableArray<ScriptTableEntry*>* local_script_table_entries,
-    JSONArray* ranges) {
+    GrowableArray<ScriptTableEntry*>* local_script_table_entries) {
   // Now output the wanted constant coverage.
   for (intptr_t i = 0; i < local_script_table_entries->length(); i++) {
     const Script* script = local_script_table_entries->At(i)->script;
@@ -770,7 +782,6 @@ void SourceReport::CollectConstConstructorCoverageFromScripts(
           Array::Handle(script->CollectConstConstructorCoverageFrom());
       intptr_t constructors_count = constructors.Length();
       Function& constructor = Function::Handle(zone());
-      Code& code = Code::Handle(zone());
       for (intptr_t i = 0; i < constructors_count; i++) {
         constructor ^= constructors.At(i);
         // Check if we want coverage for this constructor.
@@ -782,25 +793,9 @@ void SourceReport::CollectConstConstructorCoverageFromScripts(
         if (script_index < 0) {
           continue;
         }
-        code ^= constructor.unoptimized_code();
-        const TokenPosition begin_pos = constructor.token_pos();
-        const TokenPosition end_pos = constructor.end_token_pos();
-        JSONObject range(ranges);
-        range.AddProperty("scriptIndex", script_index);
-        range.AddProperty("compiled",
-                          !code.IsNull());  // Does this make a difference?
-        range.AddProperty("startPos", begin_pos);
-        range.AddProperty("endPos", end_pos);
 
-        JSONObject cov(&range, "coverage");
-        {
-          JSONArray hits(&cov, "hits");
-          hits.AddValue(GetTokenPosOrLine(scriptRef, begin_pos));
-        }
-        {
-          JSONArray misses(&cov, "misses");
-          // No misses
-        }
+        script_table_entries_[script_index]->const_constructor_hits.Add(
+            constructor.token_pos());
       }
     }
   }

runtime/vm/source_report.h

@@ -88,6 +88,7 @@ class SourceReport {
                           const Function& func,
                           const Code& code);
   void PrintCoverageData(JSONObject* jsobj,
+                         intptr_t script_index,
                          const Function& func,
                          const Code& code,
                          bool report_branch_coverage);
@@ -108,13 +109,15 @@ class SourceReport {
                   CompileMode compile_mode);
   void VisitLibrary(JSONArray* jsarr, const Library& lib);
   void VisitClosures(JSONArray* jsarr);
+
   // An entry in the script table.
   struct ScriptTableEntry {
     ScriptTableEntry() : key(nullptr), index(-1), script(nullptr) {}
 
     const String* key;
     intptr_t index;
     const Script* script;
+    GrowableArray<TokenPosition> const_constructor_hits;
   };
 
   // Needed for DirectChainedHashMap.
@@ -143,8 +146,7 @@ class SourceReport {
       GrowableArray<ScriptTableEntry*>* local_script_table_entries);
 
   void CollectConstConstructorCoverageFromScripts(
-      GrowableArray<ScriptTableEntry*>* local_script_table_entries,
-      JSONArray* ranges);
+      GrowableArray<ScriptTableEntry*>* local_script_table_entries);
 
   intptr_t report_set_;
   CompileMode compile_mode_;

runtime/vm/source_report_test.cc

@@ -1587,6 +1587,129 @@ main() {
       buffer);
 }
 
+ISOLATE_UNIT_TEST_CASE(SourceReport_Coverage_constConstructor) {
+  // WARNING: This MUST be big enough for the serialized JSON string.
+  const int kBufferSize = 1024;
+  char buffer[kBufferSize];
+  const char* kScript = R"(
+abstract class SuperClass {
+  const SuperClass();
+}
+
+class Class extends SuperClass {
+  const Class() :
+      super();
+  int method() => 123;
+}
+
+main() {
+  const Class().method();
+}
+)";
+
+  Library& lib = Library::Handle();
+  const bool old_branch_coverage = IsolateGroup::Current()->branch_coverage();
+  IsolateGroup::Current()->set_branch_coverage(true);
+  lib ^= ExecuteScript(kScript);
+  IsolateGroup::Current()->set_branch_coverage(old_branch_coverage);
+  ASSERT(!lib.IsNull());
+  const Script& script =
+      Script::Handle(lib.LookupScript(String::Handle(String::New("test-lib"))));
+
+  SourceReport report(SourceReport::kCoverage, SourceReport::kForceCompile);
+  JSONStream js;
+  report.PrintJSON(&js, script);
+  const char* json_str = js.ToCString();
+  ASSERT(strlen(json_str) < kBufferSize);
+  ElideJSONSubstring("classes", json_str, buffer);
+  ElideJSONSubstring("libraries", buffer, buffer);
+  EXPECT_STREQ(
+      "{\"type\":\"SourceReport\",\"ranges\":["
+
+      // The super class constructor is hit.
+      "{\"scriptIndex\":0,\"startPos\":31,\"endPos\":49,\"compiled\":true,"
+      "\"coverage\":{\"hits\":[31],\"misses\":[]}},"
+
+      // The class constructor is hit, as well as the super() call.
+      "{\"scriptIndex\":0,\"startPos\":89,\"endPos\":118,\"compiled\":true,"
+      "\"coverage\":{\"hits\":[89,111],\"misses\":[]}},"
+
+      // The method is called.
+      "{\"scriptIndex\":0,\"startPos\":122,\"endPos\":141,\"compiled\":true,"
+      "\"coverage\":{\"hits\":[122],\"misses\":[]}},"
+
+      // Main is hit.
+      "{\"scriptIndex\":0,\"startPos\":146,\"endPos\":181,\"compiled\":true,"
+      "\"coverage\":{\"hits\":[146,171],\"misses\":[]}}],"
+
+      // Only one script in the script table.
+      "\"scripts\":[{\"type\":\"@Script\",\"fixedId\":true,"
+      "\"id\":\"\",\"uri\":\"file:\\/\\/\\/test-lib\",\"_kind\":\"kernel\"}]}",
+      buffer);
+}
+
+ISOLATE_UNIT_TEST_CASE(SourceReport_BranchCoverage_constConstructor) {
+  // WARNING: This MUST be big enough for the serialized JSON string.
+  const int kBufferSize = 1024;
+  char buffer[kBufferSize];
+  const char* kScript = R"(
+abstract class SuperClass {
+  const SuperClass();
+}
+
+class Class extends SuperClass {
+  const Class() :
+      super();
+  int method() => 123;
+}
+
+main() {
+  const Class().method();
+}
+)";
+
+  Library& lib = Library::Handle();
+  const bool old_branch_coverage = IsolateGroup::Current()->branch_coverage();
+  IsolateGroup::Current()->set_branch_coverage(true);
+  lib ^= ExecuteScript(kScript);
+  IsolateGroup::Current()->set_branch_coverage(old_branch_coverage);
+  ASSERT(!lib.IsNull());
+  const Script& script =
+      Script::Handle(lib.LookupScript(String::Handle(String::New("test-lib"))));
+
+  SourceReport report(SourceReport::kBranchCoverage,
+                      SourceReport::kForceCompile);
+  JSONStream js;
+  report.PrintJSON(&js, script);
+  const char* json_str = js.ToCString();
+  ASSERT(strlen(json_str) < kBufferSize);
+  ElideJSONSubstring("classes", json_str, buffer);
+  ElideJSONSubstring("libraries", buffer, buffer);
+  EXPECT_STREQ(
+      "{\"type\":\"SourceReport\",\"ranges\":["
+
+      // The super class constructor is hit.
+      "{\"scriptIndex\":0,\"startPos\":31,\"endPos\":49,\"compiled\":true,"
+      "\"branchCoverage\":{\"hits\":[31],\"misses\":[]}},"
+
+      // The class constructor is hit.
+      "{\"scriptIndex\":0,\"startPos\":89,\"endPos\":118,\"compiled\":true,"
+      "\"branchCoverage\":{\"hits\":[89],\"misses\":[]}},"
+
+      // The method is called.
+      "{\"scriptIndex\":0,\"startPos\":122,\"endPos\":141,\"compiled\":true,"
+      "\"branchCoverage\":{\"hits\":[122],\"misses\":[]}},"
+
+      // Main is hit.
+      "{\"scriptIndex\":0,\"startPos\":146,\"endPos\":181,\"compiled\":true,"
+      "\"branchCoverage\":{\"hits\":[146],\"misses\":[]}}],"
+
+      // Only one script in the script table.
+      "\"scripts\":[{\"type\":\"@Script\",\"fixedId\":true,\"id\":\"\",\""
+      "uri\":\"file:\\/\\/\\/test-lib\",\"_kind\":\"kernel\"}]}",
+      buffer);
+}
+
 #endif  // !PRODUCT
 
 }  // namespace dart