warning the user at compile time about the unused bindings

[wandersoncferreira]

A first solution to #444 , however there are some questions:

i) How would we handle switching it on/off? dynamic var? include a logging library and provide control through log levels?
ii) What will be the default behavior? I think it should be off. People may notice a difference in response time due to additional code that she does not sign up for.
iii) It need an explicit test or does the fact that it is running for all the tests in the suite already good enough?

I was curious to see how the clara-rules project itself would perform on this new flag. I uploaded it to a webpaste service but I can paste it here if someone want it. It has 1k lines in total.

[WilliamParker]

Regarding those questions:

i) I don't think we want to force a particular log library on users; there was some discussion of this on #425 (comment) in the context of issue 275. If there are more things to log I like the idea of an overall "development mode" where these warnings are turned on and the user supplies a logging implementation, possibly with Clara including implementation(s) for likely choices. This could be as simple as a function that takes a level keyword and a string to log.
ii) I don't think we want to run this check on each invocation of session construction. Note that this is currently cached so users may expect it to be highly performant after the first call. Only doing it by default on cache misses might be OK in terms of perf, but it could be annoying if the user knows and accepts the unused bindings e.g. if they are the consequence of some DSL built by the user and removing them would add complication to it. I know of a number of cases where Clara has been a compilation target rather than rules being directly written for it.
iii) I think we'd want an explicit test that it does indeed detect the unused bindings; it running against the test suite just demonstrates that it doesn't break existing functionality.

[WilliamParker]

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.

[mrrodriguez]

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...

[wandersoncferreira]

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.

[WilliamParker]

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.

[mrrodriguez]

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.

[mrrodriguez]

i) I don't think we want to force a particular log library on users; there was some discussion of this on #425 (comment) in the context of issue 275. If there are more things to log I like the idea of an overall "development mode" where these warnings are turned on and the user supplies a logging implementation, possibly with Clara including implementation(s) for likely choices. This could be as simple as a function that takes a level keyword and a string to log.

Agreed that we should not attempt to supply any sort of default logging infrastructure. If we want to make the logging of warnings/etc pluggable, we likely can just provide some functions via an API option and perhaps default those to stdout.

[EthanEChristian]

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