Change Request: Some tweaks to getFunctionHeadLoc

[kirkwaiblinger]

ESLint version

main

What problem do you want to solve?

getFunctionHeadLoc is an excellent utility for making reports on function nodes less noisy than just highlighting the whole thing. Love it!

There are a few things about the report locations, though, that I would change, ranging from outright buggy IMO to more opinion-based.

What do you think is the correct solution?

I'd like to make the following changes to the defined behavior for getFunctionHeadLoc.

1. Never underline trailing whitespace. It just looks like a bug.
2. Don't end on an = sign either when solving 1.
3. Don't use => as the loc for arrow functions (async or not). This is a really difficult one, since there is no good spot. But I think that highlighting from the start of the arrow fn through the end of the parameter list is a significantly better loc than just =>, even though it's a bit noisier. There's no right answer here, though, so I'll happily drop this point if it doesn't have consensus.

Examples of changes looking at the JSDoc for getFunctionHeadLoc

      *
      * - `function foo() {}`
      *    ^^^^^^^^^^^^
+     * - `function foo         () {}`
-     *    ^^^^^^^^^^^^^^^^^^^^^ <-- what the existing impl would do
+     *    ^^^^^^^^^^^^ <-- proposed
      * - `(function foo() {})`
      *     ^^^^^^^^^^^^
      * - `(function() {})`
      *     ^^^^^^^^
+     * - `(function  \n\n \/\* comment \*\/ () {})`
-     *     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ <-- existing
+     *     ^^^^^^^^ <-- proposed
      * - `function* foo() {}`
      *    ^^^^^^^^^^^^^
      * - `(function* foo() {})`
      *     ^^^^^^^^^^^^^
      * - `(function*() {})`
      *     ^^^^^^^^^
+     * - `(function*   () {})`
-     *     ^^^^^^^^^^^^ <-- existing 
+     *     ^^^^^^^^^
      * - `() => {}`
-     *       ^^
+     *    ^^ <-- This is more controversial, since it's clearly WAI.
      * - `async () => {}`
-     *             ^^
+     *    ^^^^^^^^ <-- Same as previous
      * - `({ foo: function foo() {} })`
      *       ^^^^^^^^^^^^^^^^^
      * - `({ foo: function() {} })`
@@ -1909,9 +1917,9 @@ module.exports = {
      * - `class A { foo = function() {} }`
      *              ^^^^^^^^^^^^^^
      * - `class A { static foo = function() {} }`
-     *              ^^^^^^^^^^^^^^^^^^^^^ <-- This one is actually fine by me; just highlighting for consistency with the next one
+     *              ^^^^^^^^^^
      * - `class A { foo = (a, b) => {} }`
-     *              ^^^^^^ <-- It's very surprising to me that a trailing space was intended to be underlined. It just looks bad.
+     *              ^^^
+.    *              ^^^^^^^^^^^^ <-- Or, through the end of the parameter list, like my proposal for standalone arrow functions

Participation

• [X] I am willing to submit a pull request for this change.

Additional comments

Alluded to item 1. above in https://github.com/eslint/eslint/issues/18339, but I now think these should be considered as completely separate issues (in part because I don't think that require-await should use getFunctionHeadLoc at all)

[nzakas]

Thanks for the suggestion. We generally don't accept proposals for changing internal logic on their own. To proceed, we'd need you to outline what the problems are with the current approach from the end-user perspective. That means finding all the rules that use this function and showing before/after of the highlighted areas.

[kirkwaiblinger]

Closing since I am feeling lazy, and the arrow function part of this seems DOA at this point anyway a la #18339 😄

[eslint-github-bot[bot]]

It looks like there wasn't enough information for us to know how to help you, so we're closing the issue.

Thanks for your understanding.