Add unused binding warning to the compiler

[EthanEChristian]

Per community feedback from the slack channel, It would be helpful if during compilation Clara would warn when a rule bound a variable but didn't use it. This sort of warning would be helpful to track down minor spelling mistakes or logical mistakes that would cause rules to misbehave.

add production seems like a likely place that we could add logic to determine unused bindings.

[wandersoncferreira]

The idea of this issue is to be able to diferentiate between the two cases below?

(rl/defrule rule-testing
    [?cold <- ColdAndWindy (= ?w windspeed) (< temperature 10)
                           (= ?w 20)]
    =>
    (println "All bindings used"))

  (rl/defrule rule-testing-warning
    [?cold <- ColdAndWindy (= ?w windspeed) (< temperature 10)]
    =>
    (println "Warning, ?w not used"))

In the rule-testing-warning I should print something warning the user that ?w is not being used, is that right?

[EthanEChristian]

Correct. ?cold is technically bound and unused as well

[wandersoncferreira]

Do you have any ideas for the implementation? I did something potentially ugly depending on how to approach the problem.

I thought of not interfering with anything that is already happening inside add-production so, I created a function to receive the production and parse the :lhs and :rhs as strings and use a simple regex to find for every binding.

(defn warning-unused-bindings [production]
    (let [re-bindings #"\?[\w-]+"
          constraints (map str (mapcat :constraints (:lhs production)))
          constraints-bindings (filter some? (map (partial re-find re-bindings) constraints))
          fact-bindings (map name (map :fact-binding (:lhs production)))
          rhs-bindings (re-seq re-bindings (str (:rhs production)))]
      (doseq [[binding freq] (->> fact-bindings
                                  (concat constraints-bindings rhs-bindings)
                                  frequencies)]
        (when (= 1 freq)
          (println (format "WARNING: %s NOT BEING USED" binding))))))

If something appears only once in the whole definition, it gets a warning message. There are space for improvements in this solution for better performance, but I prefer to get in the right track before it.

[mrrodriguez]

https://github.com/cerner/clara-rules/issues/444#issuecomment-573356528 is an interesting approach. I was certainly thinking more of something that'd happen within the compiler while dealing with bindings instead. Perhaps an external pass like this for this sort of "linting" may be somewhat reasonable. I don't immediately see how it'd be overlooking something obvious if we are strict on the rule that ? prefixed symbols are always referring to rule bindings - never something sneaky like:

(defrule r
=>
(let [?x :ha-ha]
  :done))

but in that case, the warning is probably acceptable since you aren't supposed to do that.

[wandersoncferreira]

As this functionality is not a core activity of the compiler, I would prefer to keep it separate. Also providing some form of switch on/off to the warning itself. I don't know if for performace reasons someone may choose to disable the warning in production env.

But even if you opt to touch within the compiler, it would be inspecting the bindings as strings or something else?

[mrrodriguez]

But even if you opt to touch within the compiler, it would be inspecting the bindings as strings or something else?

Now that I think about it again, in the RHS it'd be a compilation error to use an undefined symbol. So finding only 1 in the RHS wouldn't matter - would either be a compile error, or something like the let binding example above.

So really you find all in the LHS and then probably only search the RHS for those known bindings for frequency checking. Either way, the impl is still reasonable for a linting sort of operation.

[wandersoncferreira]

Cool! I will improve it and submit a PR while we progress with the discussions.

[mrrodriguez]

@wandersoncferreira I think that sounds reasonable to me.