From b5f4625d4ade117141426dd1a21f0f61ced8ae2a Mon Sep 17 00:00:00 2001 From: vemv Date: Sat, 1 May 2021 20:17:44 +0200 Subject: [PATCH] Make the `:suspicious-test` linter omittable (#371) Supports a specific `clojure.test/are` pattern. --- cases/testcases/are_true/green.clj | 25 +++++++ cases/testcases/are_true/red_one.clj | 7 ++ cases/testcases/are_true/red_three.clj | 26 ++++++++ cases/testcases/are_true/red_two.clj | 9 +++ changes.md | 2 + resource/eastwood/config/clojure.clj | 8 +++ src/eastwood/linters/misc.clj | 2 +- src/eastwood/linters/typos.clj | 92 ++++++++++++++------------ src/eastwood/util.clj | 33 +++++---- test/eastwood/analyze_ns_test.clj | 5 +- test/eastwood/lint_test.clj | 45 ++++++++++--- test/eastwood/linter_executor_test.clj | 8 +-- 12 files changed, 187 insertions(+), 75 deletions(-) create mode 100644 cases/testcases/are_true/green.clj create mode 100644 cases/testcases/are_true/red_one.clj create mode 100644 cases/testcases/are_true/red_three.clj create mode 100644 cases/testcases/are_true/red_two.clj diff --git a/cases/testcases/are_true/green.clj b/cases/testcases/are_true/green.clj new file mode 100644 index 00000000..2b6bf2ee --- /dev/null +++ b/cases/testcases/are_true/green.clj @@ -0,0 +1,25 @@ +(ns testcases.are-true.green + (:require + [clojure.test :refer [are deftest is join-fixtures testing use-fixtures]])) + +(deftest foo + (are [a b expected] (testing [a b] + (is (= expected + (+ a b))) + true) + 1 2 3)) + +(deftest bar + (are [a b expected] (do + (is (= expected + (+ a b))) + true) + 1 2 3)) + +(deftest baz + (are [a b expected] (let [x [a b]] + (is (= expected + (+ a b)) + (pr-str x)) + true) + 1 2 3)) diff --git a/cases/testcases/are_true/red_one.clj b/cases/testcases/are_true/red_one.clj new file mode 100644 index 00000000..11c9f03a --- /dev/null +++ b/cases/testcases/are_true/red_one.clj @@ -0,0 +1,7 @@ +(ns testcases.are-true.red-one + (:require + [clojure.test :refer [are deftest is join-fixtures testing use-fixtures]])) + +(deftest foo + (are [a b expected] true + 1 2 3)) diff --git a/cases/testcases/are_true/red_three.clj b/cases/testcases/are_true/red_three.clj new file mode 100644 index 00000000..495a96d0 --- /dev/null +++ b/cases/testcases/are_true/red_three.clj @@ -0,0 +1,26 @@ +(ns testcases.are-true.red-three + "Uses `::true` instead of `true` (the `:qualifier` value) so this corpus will fail Eastwood." + (:require + [clojure.test :refer [are deftest is join-fixtures testing use-fixtures]])) + +(deftest foo + (are [a b expected] (testing [a b] + (is (= expected + (+ a b))) + ::true) + 1 2 3)) + +(deftest bar + (are [a b expected] (do + (is (= expected + (+ a b))) + ::true) + 1 2 3)) + +(deftest baz + (are [a b expected] (let [x [a b]] + (is (= expected + (+ a b)) + (pr-str x)) + ::true) + 1 2 3)) diff --git a/cases/testcases/are_true/red_two.clj b/cases/testcases/are_true/red_two.clj new file mode 100644 index 00000000..eff0822c --- /dev/null +++ b/cases/testcases/are_true/red_two.clj @@ -0,0 +1,9 @@ +(ns testcases.are-true.red-two + (:require + [clojure.test :refer [are deftest is join-fixtures testing use-fixtures]])) + +(deftest foo + (are [a b expected] (do + (is true) + true) + 1 2 3)) diff --git a/changes.md b/changes.md index cf0c61e9..7561f0a3 100644 --- a/changes.md +++ b/changes.md @@ -6,6 +6,8 @@ * Now the `:wrong-tag` linter can also be configured via the [`disable-warning`](https://github.com/jonase/eastwood#eastwood-config-files) mechanism. * Related: the `disable-warnings` that Eastwood ships by default now prevent false positives against the [speced.def](https://github.com/nedap/speced.def) lib. +* Now the `:suspicious-test` linter can also be configured via the [`disable-warning`](https://github.com/jonase/eastwood#eastwood-config-files) mechanism. + * Relatedly, a certain pattern of usage of the `clojure.test/are` macro now does not trigger a linter fault. #### Bugfixes diff --git a/resource/eastwood/config/clojure.clj b/resource/eastwood/config/clojure.clj index c7ef0b00..c67dc3f9 100644 --- a/resource/eastwood/config/clojure.clj +++ b/resource/eastwood/config/clojure.clj @@ -68,3 +68,11 @@ :function-symbol 'clojure.core/eduction :arglists-for-linting '([& xform]) :reason "eduction takes a sequence of transducer with a collection as the last item"}) + +(disable-warning + {:linter :suspicious-test + :if-inside-macroexpansion-of #{'clojure.test/are} + ;; only omit the warning if :suspicious-test failed due to a `true` value: + :qualifier true + :within-depth 9 + :reason "Support a specific pattern that tends to fail better."}) diff --git a/src/eastwood/linters/misc.clj b/src/eastwood/linters/misc.clj index b8ec228c..67ee375d 100644 --- a/src/eastwood/linters/misc.clj +++ b/src/eastwood/linters/misc.clj @@ -257,7 +257,7 @@ ;; (println (format " :op=%s" (:op lca-ast))) ;; (println (format " :form=%s" (:form lca-ast))) ;; ) - match (some #(util/meets-suppress-condition lca-ast encl-macros %) + match (some #(util/meets-suppress-condition lca-ast encl-macros :eastwood/unset %) suppress-conditions)] ;; (if (and match (:debug-suppression opt)) ;; ((util/make-msg-cb :debug opt) diff --git a/src/eastwood/linters/typos.clj b/src/eastwood/linters/typos.clj index 694767a0..fc87905d 100644 --- a/src/eastwood/linters/typos.clj +++ b/src/eastwood/linters/typos.clj @@ -244,49 +244,55 @@ ;;(println "Reading var-info.edn for :suspicious-test linter") (edn/read-string (slurp (io/resource "var-info.edn"))))) -(defn predicate-forms [subexpr-maps form-type] - (let [var-info-map @var-info-map-delayed] - (apply - concat - (for [{:keys [subexpr ast]} subexpr-maps - :let [f subexpr]] - (cond - (and (not (list? f)) - (constant-expr? f)) - [(let [meta-loc (-> f meta) - loc (or (pass/has-code-loc? meta-loc) - (pass/code-loc (pass/nearest-ast-with-loc ast)))] - {:loc loc :linter :suspicious-test, - :msg (format "Found constant form%s with class %s inside %s. Did you intend to compare its value to something else inside of an 'is' expresssion?" - (cond (-> meta-loc :line) "" - (string? f) (str " \"" f "\"") - :else (str " " f)) - (if f (.getName (class f)) "nil") form-type)})] - - (sequential? f) - (let [ff (first f) - cc-sym (and ff - (instance? clojure.lang.Named ff) - (symbol "clojure.core" (name ff))) - var-info (and cc-sym (var-info-map cc-sym)) +(defn predicate-forms [opt subexpr-maps form-type] + (let [var-info-map @var-info-map-delayed + result (for [{:keys [subexpr ast]} subexpr-maps + :let [f subexpr]] + (cond + (and (not (list? f)) + (constant-expr? f)) + (let [meta-loc (-> f meta) + loc (or (pass/has-code-loc? meta-loc) + (pass/code-loc (pass/nearest-ast-with-loc ast))) + warning {:loc loc + :linter :suspicious-test + :suspicious-test {:ast ast} + :qualifier f + :msg (format "Found constant form%s with class %s inside %s. Did you intend to compare its value to something else inside of an 'is' expresssion?" + (cond (-> meta-loc :line) "" + (string? f) (str " \"" f "\"") + :else (str " " f)) + (if f (.getName (class f)) "nil") form-type)}] + (when (util/allow-warning warning opt) + [warning])) + + (sequential? f) + (let [ff (first f) + cc-sym (and ff + (instance? clojure.lang.Named ff) + (symbol "clojure.core" (name ff))) + var-info (and cc-sym (var-info-map cc-sym)) ;; _ (println (format "dbx: predicate-forms ff=%s cc-sym=%s var-info=%s" ;; ff cc-sym var-info)) - loc (-> ff meta)] - (cond - (and var-info (get var-info :predicate)) - [{:loc loc - :linter :suspicious-test, - :msg (format "Found (%s ...) form inside %s. Did you forget to wrap it in 'is', e.g. (is (%s ...))?" - ff form-type ff)}] - - (and var-info (get var-info :pure-fn)) - [{:loc loc - :linter :suspicious-test, - :msg (format "Found (%s ...) form inside %s. This is a pure function with no side effects, and its return value is unused. Did you intend to compare its return value to something else inside of an 'is' expression?" - ff form-type)}] - - :else nil)) - :else nil))))) + loc (-> ff meta)] + (cond + (and var-info (get var-info :predicate)) + [{:loc loc + :linter :suspicious-test, + :msg (format "Found (%s ...) form inside %s. Did you forget to wrap it in 'is', e.g. (is (%s ...))?" + ff form-type ff)}] + + (and var-info (get var-info :pure-fn)) + [{:loc loc + :linter :suspicious-test, + :msg (format "Found (%s ...) form inside %s. This is a pure function with no side effects, and its return value is unused. Did you intend to compare its return value to something else inside of an 'is' expression?" + ff form-type)}] + + :else nil)) + :else nil))] + (->> result + (keep identity) + (apply concat)))) ;; suspicious-test used to do its job only examining source forms, but ;; now it goes through the forms on the :raw-forms lists of the AST @@ -346,8 +352,8 @@ pr-is-formasts pr-first-is-formasts] (concat (suspicious-is-forms pr-is-formasts) - (predicate-forms pr-deftest-subexprs 'deftest) - (predicate-forms pr-testing-subexprs 'testing)))) + (predicate-forms opt pr-deftest-subexprs 'deftest) + (predicate-forms opt pr-testing-subexprs 'testing)))) ;; Suspicious macro invocations. Any macros in clojure.core that can ;; have 'trivial' expansions are included here, if it can be diff --git a/src/eastwood/util.clj b/src/eastwood/util.clj index bc5944bc..89a64326 100644 --- a/src/eastwood/util.clj +++ b/src/eastwood/util.clj @@ -885,7 +885,7 @@ StringWriter." (reduce (fn [configs {:keys [linter] :as m}] (case linter (:constant-test :redefd-vars :unused-ret-vals - :wrong-tag + :wrong-tag :suspicious-test :unused-ret-vals-in-try) (update-in configs [linter] conj (dissoc m :linter)) @@ -920,17 +920,19 @@ StringWriter." (pst e nil)))) (process-configs @warning-enable-config-atom))) -(defn meets-suppress-condition [ast enclosing-macros condition] - (let [macro-set (:if-inside-macroexpansion-of condition) - depth (:within-depth condition) - enclosing-macros (if (number? depth) - (take depth enclosing-macros) - enclosing-macros)] - (some (fn [m] - (when (macro-set (:macro m)) - {:matching-condition condition - :matching-macro (:macro m)})) - enclosing-macros))) +(defn meets-suppress-condition [ast enclosing-macros qualifier condition] + (when (or (= qualifier :eastwood/unset) ;; `nil` can be a qualifier value, so :eastwood/unset conveys an absent value + (= qualifier (:qualifier condition))) + (let [macro-set (:if-inside-macroexpansion-of condition) + depth (:within-depth condition) + enclosing-macros (if (number? depth) + (take depth enclosing-macros) + enclosing-macros)] + (some (fn [m] + (when (macro-set (:macro m)) + {:matching-condition condition + :matching-macro (:macro m)})) + enclosing-macros)))) (defn allow-warning-based-on-enclosing-macros [w linter suppress-desc suppress-conditions opt] @@ -942,7 +944,10 @@ StringWriter." ;; no suppress-conditions to check, to save time. encl-macros (when (seq suppress-conditions) (enclosing-macros ast)) - match (some #(meets-suppress-condition ast encl-macros %) + match (some #(meets-suppress-condition ast + encl-macros + (:qualifier w :eastwood/unset) + %) suppress-conditions)] (when (and match (:debug-suppression opt)) ((make-msg-cb :debug opt) @@ -979,7 +984,7 @@ StringWriter." w linter (format " for invocation of macro '%s'" macro-symbol) suppress-conditions opt))) - (:unused-ret-vals :unused-ret-vals-in-try :constant-test :wrong-tag) + (:unused-ret-vals :unused-ret-vals-in-try :constant-test :wrong-tag :suspicious-test) (let [suppress-conditions (get-in opt [:warning-enable-config linter])] (allow-warning-based-on-enclosing-macros w linter "" suppress-conditions opt))))) diff --git a/test/eastwood/analyze_ns_test.clj b/test/eastwood/analyze_ns_test.clj index 539d66ae..1bc330e1 100644 --- a/test/eastwood/analyze_ns_test.clj +++ b/test/eastwood/analyze_ns_test.clj @@ -8,8 +8,9 @@ (are [f input expected] (let [result (f input)] (is (= result input) "Applying `f` should return an equivalent object (even after removing metadata)") - (= expected - (-> result second meta :const))) + (is (= expected + (-> result second meta :const))) + true) identity '(defn ^:const foo) true sut/cleanup '(defn ^:const foo) nil sut/cleanup '(defn foo) nil))) diff --git a/test/eastwood/lint_test.clj b/test/eastwood/lint_test.clj index f51333f2..c85b9710 100644 --- a/test/eastwood/lint_test.clj +++ b/test/eastwood/lint_test.clj @@ -84,11 +84,12 @@ (let [valid-namespaces (take 1 eastwood-src-namespaces) invalid-namespaces '[invalid.syntax]] (are [namespaces rethrow-exceptions? ok?] (testing [namespaces rethrow-exceptions?] - (try - (eastwood.lint/eastwood (assoc eastwood.lint/default-opts :namespaces namespaces :rethrow-exceptions? rethrow-exceptions?)) - ok? - (catch Exception _ - (not ok?)))) + (is (try + (eastwood.lint/eastwood (assoc eastwood.lint/default-opts :namespaces namespaces :rethrow-exceptions? rethrow-exceptions?)) + ok? + (catch Exception _ + (not ok?)))) + true) [] false true [] true true invalid-namespaces false true @@ -102,12 +103,14 @@ (eastwood.lint/eastwood (assoc eastwood.lint/default-opts :namespaces #{'testcases.large-defprotocol})))))) (deftest ignore-fault?-test - (are [input expected] (= expected - (sut/ignore-fault? input - {:warn-data {:namespace-sym 'some-ns - :line 1 - :column 2 - :linter :some-linter}})) + (are [input expected] (testing input + (is (= expected + (sut/ignore-fault? input + {:warn-data {:namespace-sym 'some-ns + :line 1 + :column 2 + :linter :some-linter}}))) + true) nil false {} false {:some-linter {'some-ns true}} true @@ -180,3 +183,23 @@ relative to a specific macroexpansion" {} {:some-warnings true} {:builtin-config-files ["disable_wrong_tag.clj"]} {:some-warnings false} {:builtin-config-files ["disable_wrong_tag_unrelated.clj"]} {:some-warnings true}))) + +(deftest are-true-test + (are [desc input expected] (testing input + (is (= expected + (-> eastwood.lint/default-opts + (assoc :namespaces input) + (eastwood.lint/eastwood))) + desc) + true) + "Supports the \"true at tail position\" pattern for `are`" + #{'testcases.are-true.green} {:some-warnings false} + + "does not ruin `(is true)` detection" + #{'testcases.are-true.red-one} {:some-warnings true} + + "does not ruin an edge case" + #{'testcases.are-true.red-two} {:some-warnings true} + + "only `true` is deemed an apt :qualifier value" + #{'testcases.are-true.red-three} {:some-warnings true})) diff --git a/test/eastwood/linter_executor_test.clj b/test/eastwood/linter_executor_test.clj index d173574d..1c6fbc79 100644 --- a/test/eastwood/linter_executor_test.clj +++ b/test/eastwood/linter_executor_test.clj @@ -6,15 +6,15 @@ (def proof (atom [])) (deftest works - (are [desc input pred] (testing [desc input] + (are [desc input pred] (testing input (reset! proof []) (with-out-str (eastwood.lint/eastwood {:namespaces ['eastwood.test.linter-executor.sample-ns] :builtin-config-files input})) - (is (pred @proof)) + (is (pred @proof) + desc) ;; Avoid duplicate failure reports, did the tests fail: - #_true ;; temporarily disabled because of a false positive - ) + true) "The custom `set-linter-executor!` successfully runs" ["linter_executor.clj"] seq