Skip to content

warning the user at compile time about the unused bindings #448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

warning the user at compile time about the unused bindings #448

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1826686
warning the user at compile time about the unused bindings
wandersoncferreira Jan 17, 2020
29ca628
unused bindings
wandersoncferreira Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/clojure/clara/rules/compiler.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
(:require [clara.rules.engine :as eng]
[clara.rules.listener :as listener]
[clara.rules.platform :as platform]
[clara.rules.logger :as logger]
[clara.rules.schema :as schema]
[clojure.core.reducers :as r]
[clojure.reflect :as reflect]
Expand Down Expand Up @@ -2060,6 +2061,8 @@
(transient #{}))
persistent!)]

(logger/warn-unused-bindings! productions)

(if-let [session (get @session-cache [productions options])]
session
(let [session (mk-session* productions options)]
Expand Down
25 changes: 25 additions & 0 deletions src/main/clojure/clara/rules/logger.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
(ns clara.rules.logger)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A binding in an accumulator constraint will cause accumulation to occur over each value of that binding, so even if it is a new binding that isn't used elsewhere it actually does impact rule semantics. So these two rules don't behave the same way:

(defrule get-current-temperature
  [?current-temp <- newest-temp :from [Temperature (= ?location location)]]
  =>
  (insert! (->CurrentTemperature ?current-temp))
  )

(defrule get-current-temperature
  [?current-temp <- newest-temp :from [Temperature]]
  =>
  (insert! (->CurrentTemperature ?current-temp))
  )

http://www.clara-rules.org/docs/accumulators/ discusses this, specifically the paragraph after the first example. So I think we need logic that excludes from this check bindings that only appear once, but where that appearance is in an accumulator.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I'm glad you pointed this out. I should have paid more attention. Can't this be the case for more than just an accumulator then? This may make the whole effort here need to shift to something within the compiler more than an external pass over the production structures as currently being done.

I thought this external "linting" approach was fine since we could just ignore nearly all structure and find variables. However, seeing this accumulator case reminds me there are more semantics to perhaps be aware of. The detection may be more "context dependent" than I thought...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting corner case, I will write some code to handle the accumulator approach. Maybe if something else appear that is more dependent of context, we can revisit this approach of external linting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with this approach, I'd not call the ns "logger".

Something like analyzer or analysis seems more typical. Logging is a separate concern.


(defn- warn-unused-binding
[{:keys [lhs rhs] :as production}]
(let [re-binding #"\?[\w-]+"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest using the is-variable? function in the interest of consistency; we could factor it out into say an "analysis-utils" namespace to avoid a cyclic dependency.

constraints-bindings (->> lhs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit/style: Could these be replaced by transducers?

(mapcat :constraints)
(map (comp (partial re-find re-binding) str))
(filter some?))
fact-bindings (->> lhs
(map :fact-binding)
(filter some?)
(map name))
rhs-bindings (re-seq re-binding (str rhs))]
(run! (fn [[?bind freq]]
(when (= freq 1)
(println (format "WARNING: binding %s defined at %s is not being used" ?bind (:name production)))))
(->> constraints-bindings
(concat fact-bindings rhs-bindings)
(filter some?)
frequencies))))

(defn warn-unused-bindings!
[productions]
(run! warn-unused-binding productions))