<< Back to previous view

[CLJ-1227] Definline functions do not work as higher-order functions when AOT compiled Created: 27/Jun/13  Updated: 22/Jan/14  Resolved: 21/Jan/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.5
Fix Version/s: None

Type: Defect Priority: Blocker
Reporter: Colin Fleming Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: aot
Environment:

OSX 10.8, Linux


Attachments: Text File 0001-CLJ-1227-Fix-definline-in-AOT-scenarios.patch     Zip Archive aot-test.zip    
Patch: Code and Test

 Description   

See discussion on Clojure group: https://groups.google.com/d/topic/clojure/v0ipoiP8X1o/discussion

Functions defined with definline appear to misbehave when AOT compiled and used with higher-order functions - it seems like the macro function is stored instead of the expansion. I've attached a small test project here reproducing the issue. It can be run with:

lein compile
lein uberjar
java -jar target/aot-test-0.1.0-SNAPSHOT-standalone.jar

The relevant part of the test namespace is:

(definline java-list? [element]
  `(instance? List ~element))

(defn -main [& args]
  (println (java-list? 2))
  (println ((var-get #'java-list?) 2))
  (println (filter java-list? [1 2 3]))
  (println (map java-list? [1 2 3])))

The output produced is:

false
(clojure.core/instance? java.util.List 2)
(1 2 3)
((clojure.core/instance? java.util.List 1) (clojure.core/instance? java.util.List 2) (clojure.core/instance? java.util.List 3))


 Comments   
Comment by Tassilo Horn [ 14/Jan/14 9:15 AM ]

I've just fallen into the very same trap. This is definitively a blocker as it means that AOT-compiled code will probably not work correctly if there's a definline in any dependency (transitively).

Comment by Tassilo Horn [ 14/Jan/14 9:57 AM ]

BTW, the problem also applies to the definlines defined in clojure.core such as ints, longs, doubles, etc.

;; This NS is AOT-compiled
(ns aot-test.core
  (:gen-class))

(defn -main [& args]
  (let [ary (make-array Integer/TYPE 5)]
    (println (apply ints [ary]))))

The output is:

% lein uberjar && java -jar target/aot-test-0.1.0-SNAPSHOT-standalone.jar
Compiling aot-test.core
Compiling aot-test.core
Created /home/horn/tmp/aot-test/target/aot-test-0.1.0-SNAPSHOT.jar
Created /home/horn/tmp/aot-test/target/aot-test-0.1.0-SNAPSHOT-standalone.jar
(. clojure.lang.Numbers clojure.core/ints #<int[] [I@39b65439>)
Comment by Tassilo Horn [ 15/Jan/14 3:48 AM ]

I debugged a bit further. What made me wonder is why standard clojure functions with :inline metadata work correctly in AOT-scenarios whereas definlines which expand to normal functions with :inline metadata do not.

The bug can be fixed by adding the :inline metadata immediately in the defn form in the expansion instead of providing it after the defn form using alter-meta!. Here's a demo example:

(ns aot-test.core
  (:gen-class))

;; (definline n? [x]
;;   `(clojure.lang.Util/identical ~x nil))
;;
;; It expands into the following which doesn't work in AOT-scenarios, e.g.,
;; (apply n? [nil]) returns (clojure.lang.Util/identical nil nil) rather than
;; true.
(do
  (clojure.core/defn n? [x] (clojure.lang.Util/identical x nil))
  (clojure.core/alter-meta!
    #'n?
    clojure.core/assoc
    :inline
    (clojure.core/fn n? [x]
      (clojure.core/seq
        (clojure.core/concat
          (clojure.core/list 'clojure.lang.Util/identical)
          (clojure.core/list x)
          (clojure.core/list 'nil)))))
  #'n?)

;; If the expansion would look like this, i.e., the metadata is added directly
;; instead of with alter-meta!, then it works fine in AOT-scenarios.
(do
  (clojure.core/defn nl?
    {:inline (clojure.core/fn fn? [x]
                                 (clojure.core/seq
                                  (clojure.core/concat
                                   (clojure.core/list 'clojure.lang.Util/identical)
                                   (clojure.core/list x)
                                   (clojure.core/list 'nil))))}
    [x] (clojure.lang.Util/identical x nil)))

(defn -main [& args]
  (println "n?")
  (println (meta #'n?))       ;=> {:inline #<core$n_QMARK_ aot_test.core$n_QMARK_@78ffb648>, :ns #<Namespace aot-test.core>, :name n?, :arglists ([x]), :column 3, :line 8, :file aot_test/core.clj}
  (println (apply n? [nil]))  ;=> (clojure.lang.Util/identical nil nil)  BROKEN!!!
  (println (apply n? [1]))    ;=> (clojure.lang.Util/identical 1 nil)    BROKEN!!!
  (println (n? nil))          ;=> true
  (println (n? 1))            ;=> false

  (println "nl?")
  (println (meta #'nl?))      ;=> {:ns #<Namespace aot-test.core>, :name nl?, :file aot_test/core.clj, :column 3, :line 22, :arglists ([x]), :inline #<core$fn_QMARK_ aot_test.core$fn_QMARK_@3b7541a>}
  (println (apply nl? [nil])) ;=> true
  (println (apply nl? [1]))   ;=> false
  (println (nl? nil))         ;=> true
  (println (nl? 1)))          ;=> false
Comment by Tassilo Horn [ 15/Jan/14 4:56 AM ]

Here's a patch fixing the issue by making definline expand only to a defn form where the :inline metadata is added immediately instead of providing it afterwards with alter-meta!.

I also added a deftest in test_clojure/macros.clj which checks for the correctness of the expansions.

Although this patch fixes the problem, it should be somehow/somewhere documented why the former approach didn't work in AOT-scenarios.

Comment by Alex Miller [ 21/Jan/14 9:36 AM ]

definline should be considered to be like defmacro in that it is not a function and cannot be used as a HOF. Additionally, definline is considered to be an experimental feature and Rich would like to discourage its use as the hope is to remove it in the future. The desired replacement is something like common lisp compiler macros that could allow the compiler to detect special situations and optimize the result but leave behind a function invocation for the case where no special behavior is available.

I am declining the ticket based on the info above and per Rich's request.

Comment by Colin Fleming [ 21/Jan/14 2:20 PM ]

This is a little disappointing since there's really no alternative right now, and I'm assuming that this sort of advanced optimisation is a ways off. If this is the plan, I'd definitely recommend marking definline as deprecated and making clear in the docstring that it shouldn't be relied on to return a function. Currently it states: "like defmacro, except defines a named function...", and based on this ticket that is clearly people's expectation.

Comment by Alex Miller [ 21/Jan/14 4:20 PM ]

Sorry about that. I will now split some definitional hairs by saying that definline is marked as "experimental" to indicate it may or may not even become a feature whereas "deprecation" indicates that it was a feature which you should no longer use. I think in this case they serve the same functional intent which is to warn you away from relying on it as part of the language.

Comment by Colin Fleming [ 21/Jan/14 5:28 PM ]

Fair enough, although it's been experimental for five major releases now. If we're convinced it's a failed experiment (and if it can't be relied on to create a function, it pretty much is since you might as well use a macro) I'd argue that deprecation is justified and sends a stronger signal that it doesn't actually work as intended. But either way, I'm certainly convinced now

Comment by Tassilo Horn [ 22/Jan/14 2:58 AM ]

Alex, definline's intention is to be considered like defmacro in case of (my-definline x) where it saves one function call, but to be callable as a normal function in higher-order scenarios like (map my-definline [x y z]). Therefore, it expands to a normal defn form with :inline metadata which is a macro that's used by the compiler.

Wether it should be removed in the future is a separate issue. It is there right now, and people use it. The consequence of this bug is that you cannot trust AOT-compiled code, because there might be some dependency that uses definline. Additionally, all clojure.core definlines (booleans, bytes, chars, shorts, floats, ints, doubles, longs) are broken when applied as HOF, because clojure itself is always distributed in AOT-compiled form.

Really, it would be grossly negligent to release Clojure 1.6 knowing of this bug.

I've written a more detailed mail to the clojure-dev list:

https://groups.google.com/d/msg/clojure-dev/UeLNJzp7UiI/UCyVvYLfHWMJ

Comment by Tassilo Horn [ 22/Jan/14 3:35 PM ]

The root cause of this issue is CLJ-1330 as investigated by Nicola Mometto in the thread cited above: http://dev.clojure.org/jira/browse/CLJ-1330
So fixing the latter will fix this one, too.

Generated at Tue Sep 02 19:12:04 CDT 2014 using JIRA 4.4#649-r158309.