fix: handle missing case for function overloads

[mitchell-merry]

Description (What)

3.3.1 throws an error on this case:

export { _foo as bar };

export async function baz() { return false; }

because when it checks the baz() line, it gets the previous node, and assumes that node.declaration is non-null. In this case (export { _foo as bar };), it is. We can just return false in this case since it is not an overload.

The error:

Oops! Something went wrong! :(
ESLint: 8.48.0
TypeError: Cannot read properties of null (reading 'type')
Occurred while linting <PATH_TO_FILE>:<LINE>
Rule: "prefer-arrow-functions/prefer-arrow-functions"
    at isOverloadedFunction (<PATH_TO_REPO>/node_modules/eslint-plugin-prefer-arrow-functions/dist/prefer-arrow-functions.js:99:42)
    at isSafeTransformation (<PATH_TO_REPO>/node_modules/eslint-plugin-prefer-arrow-functions/dist/prefer-arrow-functions.js:201:18)
    at FunctionDeclaration[parent.type!="ExportDefaultDeclaration"] (<PATH_TO_REPO>/node_modules/eslint-plugin-prefer-arrow-functions/dist/prefer-arrow-functions.js:273:21)
    at ruleErrorHandler (<PATH_TO_REPO>/node_modules/eslint/lib/linter/linter.js:1050:28)
    at <PATH_TO_REPO>/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (<PATH_TO_REPO>/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (<PATH_TO_REPO>/node_modules/eslint/lib/linter/node-event-generator.js:297:26)
    at NodeEventGenerator.applySelectors (<PATH_TO_REPO>/node_modules/eslint/lib/linter/node-event-generator.js:326:22)
    at NodeEventGenerator.enterNode (<PATH_TO_REPO>/node_modules/eslint/lib/linter/node-event-generator.js:340:14)

Justification (Why)

3.3.1 currently breaks when a module like above exists in a codebase.

How Can This Be Tested?

I added a unit test.

You can run eslint against a file with the contents above and confirm that it breaks before this change, and works after.

[mitchell-merry]

Not really sure why the build is suddenly failing, maybe you know:

Run actions/setup-node@v2
Found in cache @ /opt/hostedtoolcache/node/16.20.2/x64
/usr/local/bin/yarn --version
1.22.21
/usr/local/bin/yarn cache dir
/home/runner/.cache/yarn/v6
yarn cache is not found
7s
Run yarn install --frozen-lockfile
yarn install v1.22.21
[1/4] Resolving packages...
[2/4] Fetching packages...
error @release-it/conventional-changelog@[8](https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions/actions/runs/8047862700/job/21977992063?pr=31#step:3:9).0.[1](https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions/actions/runs/8047862700/job/21977992063?pr=31#step:4:1): The engine "node" is incompatible with this module. Expected version ">=18". Got "1[6](https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions/actions/runs/8047862700/job/21977992063?pr=31#step:4:7).20.2"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error: Process completed with exit code 1.

Definitely seems unrelated to my change.

Also, the version in package.json is 3.3.0.

[JamieMason]

Thanks @mitchell-merry, are there any other test scenarios you can think of that would be worthwhile adding as well? Not specifically related to this bug, but to help us discover other potential ones.

[JamieMason]

/cc @renato-bohler

[mitchell-merry]

Not sure, sorry. I ran this in quite a large codebase and found no issues besides this when I ran. (I'm fairly new to custom eslint plugins, so not familiar with the edge cases)

Is there a reason why the node parameter is node typed anywhere? Does ESLint not provide sufficient typing? :(

[mitchell-merry]

I suppose the node-version in the github workflow needs to be updated to version 18 to get the build working. Feel free to push to this branch if you'd like.

[JamieMason]

I suppose the node-version in the github workflow needs to be updated to version 18 to get the build working. Feel free to push to this branch if you'd like.

if you merge main I've bumped that to 18

Not sure, sorry. I ran this in quite a large codebase and found no issues besides this when I ran. (I'm fairly new to custom eslint plugins, so not familiar with the edge cases)

By this I mean, can you think of other variations of valid syntax which could be relevant (async, static, named/not-named, does/not use this etc) to add a test case for? Each test is just an example of what the code should be like before and after the plugin has been run.

Same question to @renato-bohler. The answer might be no by the way, I'm just raising it as thinking of these variations can find potential bugs really quickly that's all.

[mitchell-merry]

if you merge main I've bumped that to 18

done

By this I mean, can you think of other variations of valid syntax which could be relevant (async, static, named/not-named, does/not use this etc) to add a test case for? Each test is just an example of what the code should be like before and after the plugin has been run.

No, not at the moment.

[renato-bohler]

Huh, didn't run into this case, my bad 😅

We could also add this test to alwaysValid to verify that overloaded functions are still untransformed when we have export { foo } before it:

export { foo }; export function bar(val: number): void; export function bar(val: string | number): void {}

But the change makes sense and works for me. Thanks for catching this!

[renato-bohler]

Unsure of the impact of this change

[mitchell-merry]

I don't even see category as a field in the docs: https://eslint.org/docs/latest/extend/custom-rules#rule-structure so I'm not sure either. But I have to imagine the impact would be minimal

[mitchell-merry]

If it's possible, could we get this PR in so as to fix the latest version of the package? Thank you