halgari / odin

An embedded extensible logic DSL for Clojure.
183 stars 10 forks source link

nil breaking unification #12

Open karlmikko opened 7 years ago

karlmikko commented 7 years ago

Thanks for an awesome lib! I have found it extremely useful!

I don't know if this is by design or not. I have noticed when using standard clojure functions that returning nil for a non result o/projection seems to break unification.

Running on [com.tbaldridge/odin "0.2.0"]

(def data ["One" "Two" "Three"])

(set (o/for-query 
          (o/and 
            (d/query data ?path _ ?val) 
            (o/project (re-find #"o" ?val) ?matched)) 
          ?val))
;;=> IllegalArgumentException No method in multimethod '-unify' for dispatch value: [com.tbaldridge.odin.unification.LVar nil]  clojure.lang.MultiFn.getFn (MultiFn.java:156)

Current workaround is to add an o/when and or check to ensure the projection is a value.

(set (o/for-query 
           (o/and 
              (d/query data ?path _ ?val) 
              (o/project (or (re-find #"o" ?val) false) ?matched)  
              (o/when (string? ?matched))) 
           ?val))
;;=> #{"Two"}

This workaround is kind of clunky considering it is very common for clojure standard lib functions to return nil.

karlmikko commented 7 years ago

Actually just worked out that unification breaks if there is nil in any of the result values - not just projection.

(set (o/for-query (d/query [1 2 3] ?path _ ?val) ?val))
;;=> #{1 3 2}

(set (o/for-query (d/query [1 2 3 nil] ?path _ ?val) ?val))
;;=> IllegalArgumentException No method in multimethod '-unify' for dispatch value: [com.tbaldridge.odin.unification.LVar nil] 

However in this approach I can't use or to return some lVar for the unification.

halgari commented 7 years ago

Thanks for the report, the use of nils to signal a failing unification is pretty pervasive in Odin. But there's no reason it has to be that way, I'll look at getting a fix in.

karlmikko commented 7 years ago

Thanks :) I feel that the nil value should flow through to the result and allow o/when to filter it out if needed.

karlmikko commented 7 years ago

false in the data also comes through not quite as expected.

(set (o/for-query (d/query [1 2 3 false] ?path _ ?val) ?val))
;;=>#{1 3 LVar@991888756 2}
karlmikko commented 7 years ago

I just figured out how to get the false value

(let [data [1 2 3 false]] 
     (set (o/for-query 
               (o/and 
                 (d/query data ?path _ _) 
                 (o/project (get-in data ?path) ?val)) 
               ?val)))
;;=> #{[1 2 3 false]}

However this workaround still doesn't work for (def data [1 2 3 nil]), producing the same IllegalArgumentException.

halgari commented 7 years ago

Nil = non unification is somewhat baked into Odin at the moment, I'm thinking about what it would take to remove it. False = non unification is a bug however, that I think can be fixed fairly quickly

On Mon, Jan 16, 2017 at 7:01 PM, Karl Mikkelsen notifications@github.com wrote:

I just figured out how to get the false value

(let [data [1 2 3 false]] (set (o/for-query (o/and (d/query data ?path ) (o/project (get-in data ?path) ?val)) ?val)));;=> #{[1 2 3 false]}

However this workaround still doesn't work for (def data [1 2 3 nil]), producing the same IllegalArgumentException.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/halgari/odin/issues/12#issuecomment-273002803, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn9HTS8SShGQj7-fIRMXSN59VAw6Lbzks5rTCEGgaJpZM4LkL6U .

-- “One of the main causes of the fall of the Roman Empire was that–lacking zero–they had no way to indicate successful termination of their C programs.” (Robert Firth)

karlmikko commented 7 years ago

Added an issue #13 for the false = non unification.