Add invalid-fn-name linter

[tomdl89]

Fixes #2159 paritially

Please answer the following questions and leave the below in as part of your PR.

• [✓] I have read the developer documentation.

• [✓] This PR corresponds to an issue with a clear problem statement.
  New linter idea: threaded-fn-form #2159

• [✓] This PR contains a test to prevent against future regressions

• [✓] I have updated the CHANGELOG.md file with a description of the addressed issue.

This, effectively supersedes #2160, although as noted in the test file, there is still room for a linter which catches bound vars threaded into fn name position.

[tomdl89]

I noticed that the % arg needed adding to the bindings, as that isn't done by expand fn. I also noticed that#() in ->> forms needs expanding too, and fn* needed adding to checks, in order to surface unsued-value lints which otherwise don't show. So a bit of scope drift, but all related.

[tomdl89]

During this PR, I have spotted this false positive, which is also fixed:

n.b. hello is not bound, but doesn't need to be. the warning says it's an unresolved symbol

[tomdl89]

Merged master back in so it's easier to see it can be merged. Also updated the error message to be a bit more accurate. I think the only question that needs answering before it can be merged is if it's ok to set the default config for this to be :error or :off? @borkdude lmk if there's anything else you'd like to see before merging. Thanks!

[borkdude]

Hey @tomdl89 ! I appreciate the effort, but I think the PR fixes more than one issue and partially fixes the main issue it was written for? If this PR is only for checking that the name of a fn should be simple symbol (unqualified), then I think there is a much simpler PR possible.

[tomdl89]

Hey @borkdude yes the PR definitely has some scope creep, no doubt. But I'm fairly sure it isn't otherwise over-engineered.

The only bit of code that checks for invalid fns directly is invalid-fn-name? and reg-invalid-fn-name! (and the supporting vector-node? helper fn).

The rest of the code is expanding function reader literals (i.e. #(...) ) before the threading macros are expanded (they weren't before, which means some linting couldn't happen). I can split this out if you would prefer?

That, and tests to support the above work.

Some project prefer separate PRs, but I felt that it was connected enough to warrant one PR. Let me know what you'd like and I'll get on it :)

[borkdude]

Yes, let's split it up into tinier issues. With the expanding of fn literals, please give a test case that would otherwise fail with the current order.

[borkdude]

@tomdl89 I quickly bashed together a PR to show how I would have done it:

#2243

Happy to make you co-author of the PR if it gets merged. Feedback welcome.

[tomdl89]

Ah, cool. I just extracted mine too. Lemme compare the approaches...

[tomdl89]

So, your version is arguably a bit neater, but it does miss things like this:

(fn {:foo :bar} [x] (+ x 42))

which mine catches, because it explicitly says the first arg has to be a symbol, arg vec or fn arity:

({:file "<stdin>",
  :row 1,
  :col 1,
  :level :error,
  :message "First arg of fn should be a symbol, params vector or arity clause"})

otherwise, good catch on needing to lint it for fns and defns. I'll add that.

[tomdl89]

I'll close this to reduce clutter. Can spin out the threading-macro-related functionality in a later PR.