Function definition after the return statement

[eight04]

• Version: v11.10.0
• Platform: Windows 7 x64

1. Create foo.js:

   function test(foo = "foo") {
     return {bar};
     
     function bar() {
       console.log("test");
     }
   }
   
   test().bar();

2. Run c8 --reporter html node foo.js
3. Line 3, 4 are marked as uncovered:
   

It works fine if test has no default argument:

c8 v3.4.0

[eight04]

This bug also applies to async function without default arguments:

async function test(foo) {
  return {bar};
  
  function bar() {
    console.log("test");
  }
}

test().then(r => r.bar());

[bcoe]

@eight04 could you please try with Node@12.0, or a never version of 11; I believe this issue has been addressed upstream.

[eight04]

I can still reproduce this issue with Node@12.1.0 + c8@4.0.0.

[bcoe]

@schuay ping, this looks like it might trace back to some oddities in V8.

[schuay]

Are microtasks run to completion before reporting coverage? I tried this locally in d8 and indeed just running the script does not show coverage for bar. But after flushing microtasks with %PerformMicrotaskCheckpoint(); (pass --allow-natives-syntax) the counts show up correctly.

I'm not familiar with how coverage is collected in node. Do you collect after completing all microtasks?

[bcoe]

@schuay interesting; it sounds like we might need to change our implementation in Node.js to force micro tasks to complete (CC: @joyeecheung).

[bcoe]

@schuay coming back to some old issues, this still seems to have issues in Node 13:

%PerformMicrotaskCheckpoint();
function test(foo = "foo") {
  return {bar};
  
  function bar() {
    console.log("test");
  }
}

test().bar();

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |       80 |       75 |      100 |       80 |                   |
 foo.js   |       80 |       75 |      100 |       80 |               4,5 |
----------|----------|----------|----------|----------|-------------------|

[joyeecheung]

I think the question is whether we actually want to flush the microtasks at exit - in the case of e.g. uncaught excetions, this might not be desirable (because that may trigger more exceptions/rejections that are treated as exceptions by us). However we should be able to flush them at least when we are on the normal exit path.

[bcoe]

@joyeecheung I tried, on Jakob's suggestion, enabling natives syntax, and calling %PerformMicrotaskCheckpoint(); in the test script, and it's worth noting it didn't seem to do the trick.

@joyeecheung, I was thinking for some of these issues I'll perhaps open a tracking issue in V8, okay if I loop you in? or can you suggest someone else?

[joyeecheung]

Indeed, I tried this ad-hoc fix on Node.js master and it did not seem to work

diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
index b5f63b2b41..8e760063a1 100644
--- a/src/inspector_profiler.cc
+++ b/src/inspector_profiler.cc
@@ -369,6 +369,7 @@ void EndStartedProfilers(Environment* env) {
   if (connection != nullptr && !connection->ending()) {
     Debug(
         env, DebugCategory::INSPECTOR_PROFILER, "Ending coverage collection\n");
+    env->isolate()->RunMicrotasks();
     connection->End();
   }
 }

okay if I loop you in?

Sure!

[schuay]

I tried, on Jakob's suggestion, enabling natives syntax, and calling %PerformMicrotaskCheckpoint(); in the test script, and it's worth noting it didn't seem to do the trick.

As shown in comment #66 (comment)? I think you would need to call %PerformMicrotaskCheckpoint(); at the end of the script, not the beginning.

Anyways, there are no promises or asyncs in your most recent example, so likely no connection to microtasks. (Note another example above did use async functions.)

[sigurdschneider]

We've altered coverage reporting to not report whitespace as uncovered in this case: https://bugs.chromium.org/p/v8/issues/detail?id=9952

Thanks for finding this issue und posting a repro!

[schuay]

I just want to point out that there is probably some issue in c8 causing the line containing function bar() { to be reported as uncovered, even though V8 correctly reports the leading two spaces as uncovered and the bar, starting at the function keyword, as covered.

The fix Sigurd mentioned above is slightly orthogonal since it just fixes our 'report-trailling-whitespace-and-comments-after-return' optimization for functions with default-value-args.

@bcoe please double-check why c8 is reporting lines 4 and 5 as uncovered.

[bcoe]

The upstream V8 fixes have been landed in Node.js, and I have confirmed that this addresses this issue 👍

@schuay I've opened a tracking issue for the hiccup with v8-to-istanbul (I think you're right that this is an actual issue).

@sigurdschneider thanks for this fix! this addresses a pretty nasty bug.