Kibit 0.0.8 fails with Stack Overflow error when reading #{} set literals #78

Closed
jasonab opened this Issue Mar 11, 2013 · 20 comments

Projects

None yet
@jasonab
jasonab commented Mar 11, 2013

Exception in thread "main" java.lang.StackOverflowError
at clojure.core.logic$tree_term_QMARK_.invoke(logic.clj:972)
at clojure.core.logic$walk_STAR_$fn__1462.invoke(logic.clj:419)
at clojure.core.logic$eval1645$fn__1646.invoke(logic.clj:1068)
at clojure.core.logic$eval375$fn__376$G__366__383.invoke(logic.clj:85)
at clojure.core.logic$walk_STAR_.invoke(logic.clj:416)
at clojure.core.logic$walk_STAR_$fn__1462.invoke(logic.clj:420)
at clojure.core.logic$eval1645$fn__1646.invoke(logic.clj:1068)
at clojure.core.logic$eval375$fn__376$G__366__383.invoke(logic.clj:85)
at clojure.core.logic$walk_STAR_.invoke(logic.clj:416)
at clojure.core.logic$walk_STAR_$fn__1462.invoke(logic.clj:420)
at clojure.core.logic$eval1645$fn__1646.invoke(logic.clj:1068)
at clojure.core.logic$eval375$fn__376$G__366__383.invoke(logic.clj:85)

@jonase
Owner
jonase commented Mar 11, 2013

Did you run kibit on a codebase that is publicly available?

@jasonab
jasonab commented Mar 11, 2013

Sorry, no, it ran on my company's codebase. I can completely believe that there's something weird inside it, but unfortunately I don't have any more information to go on.

Maybe an option to report what file kibit is processing?

@nbeloglazov

Try run it on incanter's core.clj: https://github.com/liebke/incanter/blob/master/modules/incanter-core/src/incanter/core.clj
It fails with stackoverflow on my computer.

@nbeloglazov

It fails on following function:

(defn- except-for-cols
"
"
  ([data except-cols]
     (let [colnames (:column-names data)
           _except-cols (if (coll? except-cols)
                          (map #(get-column-id data %) except-cols)
                          [(get-column-id data except-cols)])
           except-names  (if (some number? _except-cols)
                           (map #(nth colnames %) _except-cols)
                           _except-cols)]
       (for [name colnames :when (not (some #{name} except-names))]
         name))))

Actually on this part:

(not (some #{name} except-names))
@clyce
clyce commented Dec 28, 2013

on my project, stack over flows too

@aspasia
aspasia commented Feb 20, 2014

same here

@willgorman

I can trigger a stack overflow running kibit on a file containing the form (->> [[1 2]] (into #{})) It looks like it may have something to do with the combination of the ->> macro and the set reader macro. The form (->> [[1 2]] (into {})) does not cause a stack overflow. I'm using kibit 0.0.8, leiningen 2.3.4, clojure 1.5.1, and java 1.7.0_45.

@dustinconrad

This is happening for me. Codebase is publicly available.

@willgorman

I think the problem is that core.logic doesn't support sets (Also: LOGIC-154)

@dustinconrad

Will, I'm not sure that is it, as I replaced instances of #{...} with (hash-set ...) and now the stackoverflow isn't happening.

@danielcompton danielcompton added the bug label Nov 9, 2014
@danielcompton danielcompton modified the milestone: 0.1.0 release Nov 10, 2014
@danielcompton
Collaborator

This is still happening on core.logic 0.8.8. It looks possible (though maybe not advisable) to extend core.logic to be able to handle sets in kibit. - http://stackoverflow.com/questions/18017924/core-logic-stackoverflow-when-using-sets. Another way would be to modify core.logic.core/tree-term? from inside kibit, but that would be an even dirtier hack. Modifying tree-term? will let us read Clojure code, without throwing exceptions when we run across a set.

(defn tree-term? [x]
  (and (or (coll? x)
           (instance? ITreeTerm x))
       (not (instance? IPersistentSet x))))
@eigenhombre

This failed for us recently too and cost us an hour or so. Here's what it barfs on:

(defn killit [coll]
  (not (some #{"string1" "string2"} (map ffirst coll))))
@danielcompton danielcompton changed the title from Kibit 0.0.8 fails with Stack Overflow error to Kibit 0.0.8 fails with Stack Overflow error when reading #{} set literals Nov 25, 2014
@danielcompton danielcompton removed this from the 0.1.0 release milestone Nov 25, 2014
@evalapply
Contributor

Hey, same problem here (Kibit 0.0.8, Leiningen 2.5.1, Clojure both 1.7.0-alpha6 and 1.6.0). In my case it's this:
(defn f [x] (when (some #{x} [1]) (do (println "hello") 0)))
Kibit runs OK when I replace (some #{x} [1]) with true or if I remove the redundant do (either of these simplifications prevents StackOverflowError).

@gshakhn gshakhn added a commit to gshakhn/freefrog that referenced this issue May 4, 2015
@gshakhn gshakhn Fix kibit stack overflow by changing `into #{}` to `into hash-set`.
A bit ugly, but it's a workaround until jonase/kibit#78 gets fixed.

Re-enable kibit for Travis CI builds since it should work now.
6b865af
@schmir
schmir commented Jun 26, 2015

Can we catch the error and go on with checking expressions until a proper fix is in place?

@danielcompton danielcompton referenced this issue in jonase/lein-kibit Feb 8, 2016
Open

java.lang.StackOverflowError #8

@a613
a613 commented Oct 20, 2016

Still happening

@torbjornvatn

Any hope for this beeing fixed? It's really annoying to get that looooong stacktrace in the output

@arrdem
Collaborator
arrdem commented Oct 26, 2016

The "quick fix" which for the record I fully support would be to modify tree-term? as described by @danielcompton above. It's a dirty hack, but I think that influencing core.logic in this matter is either unlikely or at least likely to be slow. See how this issue has sat around for three years, and how the comments above about core.logic support for sets are in the same vintage.

Unless someone here wants to make the effort to enable core.logic to support sets properly.

@Rovanion
Rovanion commented Nov 9, 2016

Another example of crash causing code:

(-> 1 (into #{}))
@Rovanion Rovanion added a commit to WeatherMagic/weather-front that referenced this issue Nov 9, 2016
@Rovanion Rovanion Ugly hack to work around bug #78 in kibit. 98a581c
@arrdem
Collaborator
arrdem commented Nov 9, 2016

After talking with @danielcompton about this, the consensus seems to be that kibit will take the low road here. I'll drop a patch tonight which will run the kibit linter with a monkeypatch of the relevant part(s) of core.logic.

@arrdem
Collaborator
arrdem commented Nov 10, 2016 edited

Patch:

commit 07292e392c6b8adb44f4a37ba3dd952d78beffa3
Author: Reid 'arrdem' McKenzie <me@arrdem.com>
Date:   Thu Nov 10 00:10:56 2016 -0800

    WIP

diff --git a/src/kibit/driver.clj b/src/kibit/driver.clj
index caab14e..02bc091 100644
--- a/src/kibit/driver.clj
+++ b/src/kibit/driver.clj
@@ -3,9 +3,17 @@
             [kibit.rules :refer [all-rules]]
             [kibit.check :refer [check-file]]
             [kibit.reporters :refer :all]
-            [clojure.tools.cli :refer [cli]])
+            [clojure.tools.cli :refer [cli]]
+            [clojure.core.logic :refer [tree-term?]])
   (:import [java.io File]))

+(.setDynamic ^clojure.lang.Var #'tree-term?)
+
+(defn our-tree-term? [x]
+  (and (or (coll? x)
+           (instance? clojure.core.logic.protocols.ITreeTerm x))
+       (not (instance? clojure.lang.IPersistentSet x))))
+
 (def cli-specs [["-r" "--reporter"
                  "The reporter used when rendering suggestions"
                  :default "text"]])
@@ -33,16 +41,19 @@

 (defn run [source-paths rules & args]
   (let [[options file-args usage-text] (apply (partial cli args) cli-specs)
-        source-files (mapcat #(-> % io/file find-clojure-sources-in-dir)
-                             (if (empty? file-args) source-paths file-args))]
-    (mapcat (fn [file] (try (check-file file
-                                        :reporter (name-to-reporter (:reporter options)
-                                                                    cli-reporter)
-                                        :rules (or rules all-rules))
-                            (catch Exception e
-                              (binding [*out* *err*]
-                                (println "Check failed -- skipping rest of file")
-                                (println (.getMessage e))))))
+        source-files                   (mapcat #(-> % io/file find-clojure-sources-in-dir)
+                                               (if (empty? file-args) source-paths file-args))]
+    (mapcat (fn [file]
+              (try
+                (with-bindings {}
+                  (check-file file
+                              :reporter (name-to-reporter (:reporter options)
+                                                          cli-reporter)
+                              :rules (or rules all-rules)))
+                (catch Exception e
+                  (binding [*out* *err*]
+                    (println "Check failed -- skipping rest of file")
+                    (println (.getMessage e))))))
             source-files)))

 (defn external-run
diff --git a/test/kibit/test/collections.clj b/test/kibit/test/collections.clj
index e219f86..a81a1ea 100644
--- a/test/kibit/test/collections.clj
+++ b/test/kibit/test/collections.clj
@@ -4,24 +4,30 @@

 (deftest collections-are
   (are [expected-alt-form test-form]
-       (= expected-alt-form (:alt (kibit/check-expr test-form)))
-    '(seq a) '(not (empty? a))
+      (= expected-alt-form (:alt (kibit/check-expr test-form)))
+    '(seq a)          '(not (empty? a))
     '(when (seq a) b) '(when-not (empty? a) b)
     '(when (seq a) b) '(when (not (empty? a)) b)
-    '(vector a) '(conj [] a)
+
+    '(vector a)   '(conj [] a)
     '(vector a b) '(conj [] a b)
-    '(vec coll) '(into [] coll)
+    '(vec coll)   '(into [] coll)
+
     '(set coll) '(into #{} coll)
+
     '(update-in coll [k] f) '(assoc coll k (f (k coll)))
     '(update-in coll [k] f) '(assoc coll k (f (coll k)))
     '(update-in coll [k] f) '(assoc coll k (f (get coll k)))
+
     '(assoc-in coll [k0 k1] a) '(assoc coll k0 (assoc (k0 coll) k1 a))
     '(assoc-in coll [k0 k1] a) '(assoc coll k0 (assoc (coll k0) k1 a))
     '(assoc-in coll [k0 k1] a) '(assoc coll k0 (assoc (get coll k0) k1 a))
+    '(assoc-in coll [k1 k2] v) '(update-in coll [k1 k2] assoc v)
+
     '(update-in coll [k] f a b c) '(assoc coll k (f (k coll) a b c))
     '(update-in coll [k] f a b c) '(assoc coll k (f (coll k) a b c))
     '(update-in coll [k] f a b c) '(assoc coll k (f (get coll k) a b c))
-    '(assoc-in coll [k1 k2] v) '(update-in coll [k1 k2] assoc v)
+
     '(repeatedly 10 (constantly :foo)) '(take 10 (repeatedly (constantly :foo)))

     ;; some wrong simplifications happened in the past:
diff --git a/test/kibit/test/driver.clj b/test/kibit/test/driver.clj
index 2cd3534..ed93f4d 100644
--- a/test/kibit/test/driver.clj
+++ b/test/kibit/test/driver.clj
@@ -13,5 +13,9 @@
 (deftest find-clojure-sources-are
   (is (= [(io/file "test/resources/first.clj")
           (io/file "test/resources/second.cljx")
+          (io/file "test/resources/sets.clj")
           (io/file "test/resources/third.cljs")]
          (driver/find-clojure-sources-in-dir (io/file "test/resources")))))
+
+(deftest test-set-file
+  (is (driver/run ["test/resources/sets.clj"] nil)))
diff --git a/test/resources/sets.clj b/test/resources/sets.clj
new file mode 100644
index 0000000..f9025cd
--- /dev/null
+++ b/test/resources/sets.clj
@@ -0,0 +1,5 @@
+(ns resources.sets)
+
+(defn killit [coll]
+  (not (some #{"string1" "string2"}
+             (map ffirst coll))))

Test output:


lein test kibit.test.arithmetic

lein test kibit.test.check

lein test kibit.test.collections

lein test kibit.test.control-structures

lein test kibit.test.core

lein test kibit.test.driver

lein test :only kibit.test.driver/test-set-file

ERROR in (test-set-file) (logic.clj:359)
expected: (driver/run ["test/resources/sets.clj"] nil)
  actual: java.lang.StackOverflowError: null
 at clojure.core.logic.Substitutions.walk (logic.clj:359)
    clojure.core.logic$walk_STAR_$fn__1798.invoke (logic.clj:231)
    clojure.core.logic$eval1980$fn__1981.invoke (logic.clj:984)
    clojure.core.logic.protocols$eval456$fn__457$G__447__464.invoke (protocols.clj:55)
    clojure.core.logic$walk_STAR_.invokeStatic (logic.clj:229)
    clojure.core.logic$walk_STAR_.invoke (logic.clj:227)
    clojure.core.logic$walk_STAR_$fn__1798.invoke (logic.clj:233)
    clojure.core.logic$eval1980$fn__1981.invoke (logic.clj:984)
    [[ repeating frames elided ]]

lein test kibit.test.equality

lein test kibit.test.misc

lein test kibit.test.reporters

lein test resources.sets

Ran 15 tests containing 121 assertions.
0 failures, 1 errors.
@arrdem arrdem closed this in #172 Nov 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment