Repository: jshint/jshint
Pull request #3618 (2022-06-12 20:00:42)

[[FIX]] Tolerate late definition of async function
We used to mark `async function` as unreachable instead of treating it as any other late function definition. This change makes jshint react to `async` keyword in the same way as it reacted to `function` keyword.

jugglinmike: Thanks for the patch! This is definitely an oversight, and I'm grateful you've stepped up to fix it.

Since `async` was never formally reserved, it can be used as an identifier, and that makes it tricky to deal with. For a concrete example, here's a test that ought to pass:

```js
  "async identifier": function (test) {
    var src = [
      "(function() {",
      "  return f(4);",
      "  async();",
      "}());"
    ];

    TestRun(test)
      .addError(3, 3, "Unreachable 'async' after 'return'.")
      .test(src, { esversion: 8, latedef: "nofunc" });

    test.done();
  }
```

Now, disambiguating `async` in a lookahead (that is, *before* formally parsing it) is pretty tough. The good news is that I think we might be able to ignore all that complexity because it mostly comes from accounting for arrow functions, and we're only conscered with function declarations here. As far as I know, a relatively straightforward heuristic that considers the next two tokens ought to be enough (though we probably have to take care about newlines).

Does that sound right to you? Would you be up for addressing it?
pirxpilot: How about now?
I have to admit I did not spend much/any time looking at the jshint code, so I may be doing something stupid here. Also - are you sure we need to worry about the newlines? I do skip them but I am thinking newlines between `async` and `function` just mean that `async` is parsed as an identifier. Or do you mean some other newlines?
jugglinmike: Much closer! Your expectation about the grammar is correct: a newline between `async` and `function` will cause `async` to be interpreted as an identifier. In that case, we want to preserve this warning, since that identifier (confusing as it may be) represents unreachable code.

However, the latest iteration of suppresses the warning when it encounters the sequence "`async` [[newline]] `function`". Here's a test that I believe ought to pass:

```js
  "async asi": function (test) {
    var src = [
      "(function() {",
      "  return f(4);",
      "  async",
      "  function f(p) {",
      "    return p + 1;",
      "  }",
      "}());"
    ];

    TestRun(test)
      .addError(3, 3, "Unreachable 'async' after 'return'.")
      .test(src, { esversion: 8, latedef: "nofunc", asi: true, expr: true });

    test.done();
  },
```

(I added in the `asi` and `expr` linting options to silence the warnings that JSHint would produce by default. I'm hoping that makes the intent of the test more clear.)

Does that make sense to you? If so, I think we may be able to simplify the implementation--we only want to suppress the warning if we see `async` followed immediately by `function`.
pirxpilot: I added the recommended test and removed the code that used to skip `newline`. Let me know what you think.

Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 4d599b0b..480a21b8 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -1850,8 +1850,9 @@ var JSHINT = (function() {
       if (t.reach) {
         return;
       }
+
       if (t.id !== "(endline)") {
-        if (t.id === "function") {
+        if (isFunction(t, i)) {
           if (state.option.latedef === true) {
             warning("W026", t);
           }
@@ -1862,6 +1863,16 @@ var JSHINT = (function() {
         break;
       }
     }
+
+    function isFunction(t, i) {
+      if (t.id === "function") {
+        return true;
+      }
+      if (t.id === "async") {
+        t = peek(i);
+        return t.id === "function";
+      }
+    }
   }
 
   /**


Test patch:
diff --git a/tests/unit/parser.js b/tests/unit/parser.js
index 30d50b1f..5cfbef0e 100644
--- a/tests/unit/parser.js
+++ b/tests/unit/parser.js
@@ -8268,7 +8268,53 @@ exports.unreachable = {
       .test(src);
 
     test.done();
-  }
+  },
+  'async function': function (test) {
+    var src = [
+      "(function() {",
+      "  return f(4);",
+      "  async function f(p) {",
+      "    return p + 1;",
+      "  }",
+      "}());"
+    ];
+
+    TestRun(test)
+      .test(src, { esversion: 8, latedef: 'nofunc' });
+
+    test.done();
+  },
+  "async identifier": function (test) {
+    var src = [
+      "(function() {",
+      "  return f(4);",
+      "  async();",
+      "}());"
+    ];
+
+    TestRun(test)
+      .addError(3, 3, "Unreachable 'async' after 'return'.")
+      .test(src, { esversion: 8, latedef: "nofunc" });
+
+    test.done();
+  },
+   "async asi": function (test) {
+    var src = [
+      "(function() {",
+      "  return f(4);",
+      "  async",
+      "  function f(p) {",
+      "    return p + 1;",
+      "  }",
+      "}());"
+    ];
+
+    TestRun(test)
+      .addError(3, 3, "Unreachable 'async' after 'return'.")
+      .test(src, { esversion: 8, latedef: "nofunc", asi: true, expr: true });
+
+    test.done();
+  },
 };
 
 exports["test for 'break' in switch case + curly braces"] = function (test) {