Phil Hagelberg: 1 Add match-should-case lint and fix action. 4 files changed, 65 insertions(+), 6 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~xerool/fennel-ls/patches/55281/mbox | git am -3Learn more about email & git
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. --- docs/linting.md | 16 +++++++++++----- src/fennel-ls/config.fnl | 1 + src/fennel-ls/lint.fnl | 24 +++++++++++++++++++++++- test/lint.fnl | 30 ++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 6 deletions(-) diff --git a/docs/linting.md b/docs/linting.md index 470f053..4e365c2 100644 --- a/docs/linting.md +++ b/docs/linting.md @@ -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. diff --git a/src/fennel-ls/config.fnl b/src/fennel-ls/config.fnl index 42ad462..9ede9e3 100644 --- a/src/fennel-ls/config.fnl +++ b/src/fennel-ls/config.fnl @@ -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) diff --git a/src/fennel-ls/lint.fnl b/src/fennel-ls/lint.fnl index 8c13751..f154f15 100644 --- a/src/fennel-ls/lint.fnl +++ b/src/fennel-ls/lint.fnl @@ -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 diff --git a/test/lint.fnl b/test/lint.fnl index 055c89b..333f25e 100644 --- a/test/lint.fnl +++ b/test/lint.fnl @@ -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} -- 2.39.5