Add match-should-case lint and fix action.
When a `match` call's patterns do not contain any symbols that reference the outer scope, then there is no reason to use `match`; that call should be replaced with `case`. This allocates diagnostic code 308 to match-should-case. In order to detect this, I had to add a loop over file.lexicals. This is often a very large table, so it could adverse performance impact. I believe it is necessary in order to distinguish between case vs match since other means seem to only be applied after macroexpansion by which time neither one exists. I've updated docs/linting.md to mention the new loop and cleaned up a few things in that file that were outdated. I've added tests for the new lint to ensure it doesn't trigger when it shouldn't.
This commit is contained in:
parent
8ef844ab90
commit
a27f42bfa4
@ -9,7 +9,7 @@ To start, you can set up all the plumbing:
|
||||
* Choose which `each` loop to put your function in, so your lint can be
|
||||
applied to right thing.
|
||||
* add `(if checks.<your-check> (table.insert diagnostics (<your-check> self file <the rest of the args>)))`
|
||||
3. Enable your lint! In `src/fennel-ls/state.fnl`, find the
|
||||
3. Enable your lint! In `src/fennel-ls/config.fnl`, find the
|
||||
`default-configuration` variable, and turn your check on by default.
|
||||
|
||||
## Writing your lint
|
||||
@ -19,19 +19,22 @@ The goal is to check whether the given arguments should emit a warning, and
|
||||
what message to show. The current loops in `check` go over every:
|
||||
* definition (Every time a new variable is bound)
|
||||
* call (Every time the user calls a function or a special. Macros don't count.)
|
||||
* lexical (Every source-tracked AST node in the whole file, before macro expansion.
|
||||
Symbols, lists, tables, and varargs will be here; numbers, strings, booleans,
|
||||
and nil will not.)
|
||||
|
||||
More loops might have been added since I wrote this document.
|
||||
|
||||
### Input arguments
|
||||
All lints give you `self` and `file`. They're mostly useful to pass to other
|
||||
All lints give you `server` and `file`. They're mostly useful to pass to other
|
||||
functions.
|
||||
* `self` is the table that represents the language server. It carries metadata
|
||||
* `server` is the table that represents the language server. It carries metadata
|
||||
and stuff around. You probably don't need to use it directly.
|
||||
* `file` is an object that represents a .fnl file. It has some useful fields.
|
||||
Check out what fields it has by looking at the end of `compiler.fnl`.
|
||||
|
||||
`file.lexical` stores the value `true` for every single list, table, or symbo
|
||||
that appears in the original file AST, and `nil` for things generated via
|
||||
`file.lexical` stores the value `true` for every single list, table, or symbol
|
||||
that appears in the original file AST, but not things generated via
|
||||
macroexpansion. Make sure that the AST you're checking is inside of
|
||||
`file.lexical`; otherwise, your lint may not be actionable or relevant, because
|
||||
the user won't be able to see or edit the code your lint is warning about.
|
||||
@ -72,6 +75,9 @@ the definitions will be:
|
||||
* `head` is the symbol that is being called. It is the same as `(. call 1)`.
|
||||
* `call` is the list that represents the call.
|
||||
|
||||
#### If your lint is linting lexical nodes
|
||||
* `ast` is the AST node
|
||||
|
||||
### Output:
|
||||
Your lint function should return `nil` if there's nothing to report, or
|
||||
return a diagnostic object representing your lint message.
|
||||
|
||||
@ -23,6 +23,7 @@ There are no global settings. They're all stored in the `server` object.
|
||||
:lints {:unused-definition (option true)
|
||||
:unknown-module-field (option true)
|
||||
:unnecessary-method (option true)
|
||||
:match-should-case (option true)
|
||||
:bad-unpack (option true)
|
||||
:var-never-set (option true)
|
||||
:op-with-no-arguments (option true)
|
||||
|
||||
@ -2,7 +2,7 @@
|
||||
Provides the function (check server file), which goes through a file and mutates
|
||||
the `file.diagnostics` field, filling it with diagnostics."
|
||||
|
||||
(local {: sym? : list? : view} (require :fennel))
|
||||
(local {: sym? : list? : table? : view} (require :fennel))
|
||||
(local analyzer (require :fennel-ls.analyzer))
|
||||
(local message (require :fennel-ls.message))
|
||||
(local utils (require :fennel-ls.utils))
|
||||
@ -133,6 +133,25 @@ the `file.diagnostics` field, filling it with diagnostics."
|
||||
#[{:range (message.ast->range server file call)
|
||||
:newText (view identity)}]))))
|
||||
|
||||
(λ match-reference? [ast references]
|
||||
(if (sym? ast) (?. references ast :target)
|
||||
(or (table? ast) (list? ast))
|
||||
(accumulate [ref false _ subast (pairs ast) &until ref]
|
||||
(match-reference? subast references))))
|
||||
|
||||
(λ match-should-case [server {: references &as file} ast]
|
||||
(when (and (list? ast)
|
||||
(sym? (. ast 1) :match)
|
||||
(not (faccumulate [ref false i 3 (length ast) 2 &until ref]
|
||||
(match-reference? (. ast i) references))))
|
||||
(diagnostic {:range (message.ast->range server file (. ast 1))
|
||||
:message "no pinned patterns; use case instead of match"
|
||||
:severity message.severity.WARN
|
||||
:code 308
|
||||
:codeDescription "match-should-case"}
|
||||
#[{:range (message.ast->range server file (. ast 1))
|
||||
:newText "case"}])))
|
||||
|
||||
(λ multival-in-middle-of-call [server file fun call arg index]
|
||||
"generally, values and unpack are signs that the user is trying to do
|
||||
something with multiple values. However, multiple values will get
|
||||
@ -176,6 +195,9 @@ the `file.diagnostics` field, filling it with diagnostics."
|
||||
(let [arg (. call index)]
|
||||
(if lints.multival-in-middle-of-call (table.insert diagnostics (multival-in-middle-of-call server file head call arg index)))))))
|
||||
|
||||
(each [ast (pairs file.lexical)]
|
||||
(if lints.match-should-case (table.insert diagnostics (match-should-case server file ast))))
|
||||
|
||||
(if lints.unknown-module-field
|
||||
(unknown-module-field server file))))
|
||||
;; (if lints.unnecessary-values
|
||||
|
||||
@ -177,6 +177,35 @@
|
||||
[] [{:code 307}])
|
||||
nil)
|
||||
|
||||
(fn test-match-should-case []
|
||||
;; OK: most basic pinning
|
||||
(check "(let [x 99] (match 99 x :yep!))" [] [{}])
|
||||
;; pinning inside where clause
|
||||
(check "(let [x 99]
|
||||
(match 98
|
||||
y (print y)
|
||||
(where x (= 0 (math.fmod x 2))) (print x)))" [] [{}])
|
||||
;; OK: nested pinning
|
||||
(check "(let [x 99]
|
||||
(match [{:x 32}]
|
||||
[{: x}] (print x)))" [] [{}])
|
||||
;; OK: values pattern
|
||||
(check "(let [x 99]
|
||||
(match 49
|
||||
(x _ 9) (print :values-ref)))" [] [{}])
|
||||
;; warn: basic no pinning
|
||||
(check "(match 91 z (print :yeah2 z))"
|
||||
[{:message "no pinned patterns; use case instead of match"
|
||||
:code 308
|
||||
:range {:start {:character 1 :line 0}
|
||||
:end {:character 6 :line 0}}}] [])
|
||||
;; warn: nested no pinning
|
||||
(check "(match [32] [lol] (print :nested-no-pin lol))"
|
||||
[{:message "no pinned patterns; use case instead of match"
|
||||
:code 308
|
||||
:range {:start {:character 1 :line 0}
|
||||
:end {:character 6 :line 0}}}] []))
|
||||
|
||||
;; TODO lints:
|
||||
;; unnecessary (do) in body position
|
||||
;; duplicate keys in kv table
|
||||
@ -198,5 +227,6 @@
|
||||
: test-unknown-module-field
|
||||
: test-unnecessary-colon
|
||||
: test-unset-var
|
||||
: test-match-should-case
|
||||
: test-unpack-into-op
|
||||
: test-unpack-in-middle}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user