From 297f6c4fa98b374f14766defad1ca7d53715fae3 Mon Sep 17 00:00:00 2001 From: XeroOl Date: Wed, 24 Jan 2024 22:18:18 -0600 Subject: [PATCH] fix set rename issue ~xerool/fennel-ls#8 Now unused variables and var-not-set lints use a different system. References are now of type: {:symbol sym :target ast :ref-type (or :read :write :mutate)} instead of ast. unused var will warn if all the references are :write references. --- src/fennel-ls/compiler.fnl | 79 ++++++++++++++++++++++------------- src/fennel-ls/diagnostics.fnl | 12 ++++-- src/fennel-ls/handlers.fnl | 4 +- src/fennel-ls/language.fnl | 15 +++---- test/diagnostic-test.fnl | 30 +++++++++++-- test/rename-test.fnl | 14 ++++++- 6 files changed, 106 insertions(+), 48 deletions(-) diff --git a/src/fennel-ls/compiler.fnl b/src/fennel-ls/compiler.fnl index 5feb4c3..4740444 100644 --- a/src/fennel-ls/compiler.fnl +++ b/src/fennel-ls/compiler.fnl @@ -3,11 +3,21 @@ This file is responsible for the low level tasks of analysis. Its main job is to recieve a file object and run all of the basic analysis that will be used later by fennel-ls.language to answer requests from the client." -(local {: sym? : list? : sequence? : table? : sym &as fennel} (require :fennel)) +(local {: sym? : list? : sequence? : table? : sym : view &as fennel} (require :fennel)) (local message (require :fennel-ls.message)) (local utils (require :fennel-ls.utils)) (local searcher (require :fennel-ls.searcher)) +(fn scope? [candidate] + ;; just checking a couple of the fields + (and + (= (type candidate) :table) + (= (type candidate.includes) :table) + (= (type candidate.macros) :table) + (= (type candidate.manglings) :table) + (= (type candidate.specials) :table) + (= (type candidate.gensyms) :table))) + ;; words surrounded by - are symbols, ;; because fennel doesn't allow 'require in a runtime file (local -require- (sym :require)) @@ -64,24 +74,34 @@ later by fennel-ls.language to answer requests from the client." require-calls {}] ; the keys are all the calls that start with `require (λ find-definition [name ?scope] - (when ?scope + (if ?scope (or (. definitions-by-scope ?scope name) (find-definition name ?scope.parent)))) - (λ reference [ast scope] - ;; Add a reference to the references - (assert (sym? ast)) + (λ reference [symbol scope ref-type] + (assert (or (= ref-type :read) (= ref-type :write) (= ref-type :mutate)) "wrong ref-type") + (assert (sym? symbol) :not-a-symbol) + (assert (scope? scope) :not-a-scope) ;; find reference - (let [name (string.match (tostring ast) "[^%.:]+")] + (let [name (string.match (tostring symbol) "[^%.:]+")] (case (find-definition (tostring name) scope) target - (do - (tset references ast target) - (table.insert target.referenced-by ast))))) + (if (. references symbol) + (do ;; already exists + (assert (= symbol (. references symbol :symbol)) (.. "the symbol should always be the same"))) + ;; (assert (= target (. references symbol :target)) (.. "different targets: " (view target) (view (. references symbol :target))))) + ;; (print "old and new" ref-type (. references symbol :ref-type))) + (let [ref {: symbol : target : ref-type}] + (tset references symbol ref) + (table.insert target.referenced-by ref)))))) (λ symbol-to-expression [ast scope ?reference?] - (when (or ?reference? (fennel.multi-sym? ast)) - (reference ast scope))) + (assert (sym? ast) "symbols only") + (reference ast scope (if ?reference? + :read + (not (multisym? ast)) + :write + :mutate))) (λ define [?definition binding scope ?opts] ;; Add a definition to the definitions @@ -124,22 +144,19 @@ later by fennel-ls.language to answer requests from the client." (recurse binding [])) (λ mutate [_?definition binding scope] - ;; for now, mutating a field counts as a reference I guess (λ recurse [binding keys] (if (sym? binding) - (let [ - ;; ;; future work may need to care about mutations - ;; _mutation - ;; {: binding - ;; :new-definition ?definition - ;; :keys (if (< 0 (length keys)) - ;; (fcollect [i 1 (length keys)] - ;; (. keys i)))} - name (string.match (tostring binding) "[^%.:]+")] - (case (find-definition (tostring name) scope) - target (if (multisym? binding) - (table.insert target.referenced-by binding) - (set target.var-set true)))) + ;; (let [;; future work may need to care about mutations + ;; _mutation + ;; {: binding + ;; :new-definition ?definition + ;; :keys (if (< 0 (length keys)) + ;; (fcollect [i 1 (length keys)] + ;; (. keys i)))}] + (when (not (multisym? binding)) + (reference binding scope :write) + (if (. references binding) + (tset (. references binding :target) :var-set true))) (= :table (type binding)) (each [k v (iter binding)] (table.insert keys k) @@ -147,7 +164,7 @@ later by fennel-ls.language to answer requests from the client." (table.remove keys)))) (recurse binding [])) - (λ destructure [to from scope {:declaration ?declaration? &as opts}] + (λ destructure [to from scope {:declaration ?declaration? : symtype &as opts}] ;; I really don't understand symtype ;; I think I need an explanation (if ?declaration? @@ -164,9 +181,9 @@ later by fennel-ls.language to answer requests from the client." (set target.fields (or target.fields {})) (tset target.fields field {:binding multisym - :definition ast - ;; referenced-by inherits from all other symbols - :referenced-by (or (?. definitions multisym :referenced-by) [])})))) + :definition ast})))) + ;; ;; referenced-by inherits from all other symbols + ;; :referenced-by (or (?. definitions multisym :referenced-by) [])})))) (λ define-function-name [ast scope] ;; add a function definition to the definitions @@ -221,7 +238,7 @@ later by fennel-ls.language to answer requests from the client." (tset require-calls ast true) ;; fennel expands multisym calls into the `:` special, so we need to reference the symbol while we still can (where [sym] (multisym? sym) (: (tostring sym) :find ":")) - (reference sym scope))) + (reference sym scope :read))) (λ recoverable? [msg] (or (= 1 (msg:find "unknown identifier")) @@ -295,6 +312,8 @@ later by fennel-ls.language to answer requests from the client." (true ?item1 ?item2) (values ?item1 ?item2) (where (or (nil err) (false err)) (not (err:find "^[^\n]-__NOT_AN_ERROR\n"))) (if (os.getenv :TESTING) + (print (.. "\nYou have crashed fennel-ls (or the fennel " component ") with the following message\n:" err + "\n\n^^^ the error message above here is the root problem\n\n")) (error (.. "\nYou have crashed fennel-ls (or the fennel " component ") with the following message\n:" err "\n\n^^^ the error message above here is the root problem\n\n")) (table.insert diagnostics diff --git a/src/fennel-ls/diagnostics.fnl b/src/fennel-ls/diagnostics.fnl index 50bb81c..3b8411c 100644 --- a/src/fennel-ls/diagnostics.fnl +++ b/src/fennel-ls/diagnostics.fnl @@ -10,8 +10,12 @@ Goes through a file and mutates the `file.diagnostics` field, filling it with di (λ unused-definition [self file] "local variable that is defined but not used" (icollect [symbol definition (pairs file.definitions) &into file.diagnostics] - (if (and (= 0 (length definition.referenced-by)) - (not= "_" (: (tostring symbol) :sub 1 1))) + (if (and (not= "_" (: (tostring symbol) :sub 1 1)) + (not (accumulate [reference false + _ ref (ipairs definition.referenced-by) + &until reference] + (or (= ref.ref-type :read) + (= ref.ref-type :mutate))))) {:range (message.ast->range self file symbol) :message (.. "unused definition: " (tostring symbol)) :severity message.severity.WARN @@ -77,9 +81,9 @@ Goes through a file and mutates the `file.diagnostics` field, filling it with di (icollect [symbol definition (pairs file.definitions) &into file.diagnostics] (if (and definition.var? (not definition.var-set)) {:range (message.ast->range self file symbol) - :message (.. "var is never set: " (tostring symbol)) + :message (.. "var is never set: " (tostring symbol) " Consider using (local) instead of (var)") :severity message.severity.WARN - :code 301 + :code 305 :codeDescription "var-never-set"}))) (λ check [self file] diff --git a/src/fennel-ls/handlers.fnl b/src/fennel-ls/handlers.fnl index 6e73d11..5ac3d43 100644 --- a/src/fennel-ls/handlers.fnl +++ b/src/fennel-ls/handlers.fnl @@ -90,7 +90,7 @@ Every time the client sends a message, it gets handled by a function in the corr symbol (language.find-nearest-definition self file symbol byte) (where (definition def-file) (not= definition.referenced-by nil)) - (let [result (icollect [_ symbol (ipairs definition.referenced-by)] + (let [result (icollect [_ {: symbol} (ipairs definition.referenced-by)] (message.range-and-uri self def-file symbol))] (if ?include-declaration? (table.insert result @@ -188,7 +188,7 @@ Every time the client sends a message, it gets handled by a function in the corr (language.find-nearest-definition self file symbol symbol.bytestart) ;; TODO we are assuming that every reference is in the same file (where (definition def-file) (not= definition.referenced-by nil)) - (let [usages (icollect [_ symbol (ipairs definition.referenced-by) + (let [usages (icollect [_ {: symbol} (ipairs definition.referenced-by) &into [{:range (message.multisym->range self def-file definition.binding 1) :newText new-name}]] (if (not (rawequal symbol definition.binding)) diff --git a/src/fennel-ls/language.fnl b/src/fennel-ls/language.fnl index 29eacc8..92e518d 100644 --- a/src/fennel-ls/language.fnl +++ b/src/fennel-ls/language.fnl @@ -2,7 +2,7 @@ The high level analysis system that does deep searches following the data provided by compiler.fnl." -(local {: sym? : list? : sequence? : varg? : sym} (require :fennel)) +(local {: sym? : list? : sequence? : varg? : sym &as fennel} (require :fennel)) (local utils (require :fennel-ls.utils)) (local state (require :fennel-ls.state)) @@ -35,15 +35,16 @@ the data provided by compiler.fnl." (stack-add-split! stack (utils.multi-sym-split symbol))) (λ search-assignment [self file assignment stack opts] - (let [{:binding _ - :definition ?definition - :keys ?keys - :fields ?fields} assignment] + (assert assignment.target (.. "WRONG TYPE" (fennel.traceback))) + (let [{:target {:binding _ + :definition ?definition + :keys ?keys + :fields ?fields}} assignment] (if (and (= 0 (length stack)) opts.stop-early?) - (values assignment file) ;; BASE CASE!! + (values assignment.target file) ;; BASE CASE!! ;; search a virtual field from :fields (and (not= 0 (length stack)) (?. ?fields (. stack (length stack)))) - (search-assignment self file (. ?fields (table.remove stack)) stack opts) + (search-assignment self file {:target (. ?fields (table.remove stack))} stack opts) (search-ast self file ?definition (stack-add-keys! stack ?keys) opts)))) (λ search-symbol [self file symbol stack opts] diff --git a/test/diagnostic-test.fnl b/test/diagnostic-test.fnl index 11e028d..4f0931d 100644 --- a/test/diagnostic-test.fnl +++ b/test/diagnostic-test.fnl @@ -88,7 +88,7 @@ [{:params {: diagnostics}}] (is (find [_ v (ipairs diagnostics)] (match v - {:message "var is never set: x" + {:code 305 :range {:start {:character 5 :line 0} :end {:character 6 :line 0}}} v)) @@ -112,7 +112,7 @@ (it "does not warn if a field is used" (let [self (create-client) responses (self:open-file! filename "(fn [a b] (set a.x 10) (fn b.f []))")] - (assert (not (?. responses 1 :params :diagnostics 1))))) + (assert (not (?. responses 1 :params :diagnostics 1)) (?. responses 1 :params :diagnostics 1 :message)))) (it "warns when using the : special when a multisym would do" (let [self (create-client)] @@ -150,20 +150,42 @@ v))) _ (error "did not match")))) - (it "warns if a var is written but not read" + (it "warns 'unused' if a var is written but not read" (let [self (create-client) responses (self:open-file! filename "(var x 1) (set x 2) (set [x] [3])")] (match responses [{:params {: diagnostics}}] (is (find [_ v (ipairs diagnostics)] (match v - {:message "unused definition: x" + {:code 301 :range {:start {:character 5 :line 0} :end {:character 6 :line 0}}} v)) "not found") _ (error "did not match")))) + (it "warns 'var-never-set' if a var is not written" + (let [self (create-client) + responses (self:open-file! filename "(var x 1) (print x)")] + (match responses + [{:params {: diagnostics}}] + (is (find [_ v (ipairs diagnostics)] + (match v + {:code 305 + :range {:start {:character 5 :line 0} + :end {:character 6 :line 0}}} + v)) + "not found") + _ (error "did not match")))) + + (it "does not warn 'var-never-set' if a var is written" + (let [self (create-client) + responses (self:open-file! filename "(var x 1) (set x 2) (print x)")] + (match responses + [{:params {: diagnostics}}] + (is.equal 0 (length diagnostics) "this code has no problems") + _ (error "did not match")))) + (it "does not warn on ampersand in destructuring" (let [self (create-client) responses (self:open-file! filename "(let [[x & y] [1 2 3]] (print x (. y 1) (. y 2)))")] diff --git a/test/rename-test.fnl b/test/rename-test.fnl index 225fac6..8c76c41 100644 --- a/test/rename-test.fnl +++ b/test/rename-test.fnl @@ -39,4 +39,16 @@ (it "renames a sym inside of lambda" (check-rename "(λ [foo] (print foo))" 0 6 :something - "(λ [something] (print something))"))) + "(λ [something] (print something))")) + + (it "renames a sym inside of set" + (check-rename "(var x 10)\n(set x 20)" 1 6 :something + "(var something 10)\n(set something 20)")) + + (it "renames a sym inside of set 2" + (check-rename "(var x 10)\n(var m 0)\n(set (m x) (values 10 20))" 2 8 :something + "(var something 10)\n(var m 0)\n(set (m something) (values 10 20))")) + + (it "renames a sym inside of macro that uses multiple times" + (check-rename "(var x 10)\n(doto x (set 20) (set 30))" 1 6 :something + "(var something 10)\n(doto something (set 20) (set 30))")))