[CLJ-1257] Suppress warnings when clojure.core symbols are explicitly replaces with "use" or "refer" Created: 06/Sep/13 Updated: 09/Sep/13
|Affects Version/s:||Release 1.5|
Problem: Libraries that provide DSLs (such as core.matrix) often replace or extend functions in core (such as "+", "==", "zero?"), since it is desirable to use the best / most idiomatic names.
Currently importing such libraries with "use" causes unwanted warnings like "WARNING: + already refers to: #'clojure.core/+ in namespace: test.blank, being replaced by: #'clojure.core.matrix/+".
Avoiding these warnings requires extra user effort and boilerplate code, which is frustrating for users since they have already explicitly asked for the full library to be imported into the current namespace (i.e. via "use" or ":refer :all").
Proposed solution is to introduce a new var warn-on-replace similar to warn-on-reflection which allows this warning to be controlled.
|Comment by Mike Anderson [ 06/Sep/13 10:40 PM ]|
Basic patch and test attached.
|Comment by Andy Fingerhut [ 07/Sep/13 3:22 PM ]|
I have no idea whether this idea will be vetted or not, but if it is, I have some comments on the proposed patch.
The new symbol warn-on-replace should have doc and metadata on it. See add-doc-and-meta for warn-on-reflection in core.clj for an example to copy and edit.
You check WARN_ON_REFLECTION before issuing the warning in Namespace.java, instead of WARN_ON_REPLACE.
Possible typo in the test description in ns_libs.clj: Maybe "symbol in clojure.core" instead of "symbol is clojure.core"?
If someone wants warnings from :use statements in ns forms, it seems the only way to do that with patch clj-1257.diff would be to do (set! warn-on-replace true) in a file before the ns form. That would not work well with the current version of tools.namespace, which assumes that if there is an ns form, it is the first form in the file. One can argue that tools.namespace should not make such an assumption, but it does right now.
Perhaps there should be a command line option clojure.compile.warn-on-replace like there is for clojure.compile.warn-on-reflection (search for warn-on-replace in Compile.java)?
|Comment by Mike Anderson [ 07/Sep/13 11:09 PM ]|
Thanks Andy for the feedback! I'll post an updated patch shortly.
It occurs to me that we should probably implement a more general approach to warnings in Clojure. Adding new vars and command line options for each warning doesn't seem like a good long-term strategy. I think that's beyond the scope of this patch though.
|Comment by Andy Fingerhut [ 08/Sep/13 12:49 AM ]|
Actually, there is something called compiler-options (search for the variations compiler-options, COMPILER_OPTIONS, and compiler_options for all related occurrences) that is a map where each key/value pair is a different option. That might be preferable for warn-on-replace, if it is in fact desired.
|Comment by Mike Anderson [ 08/Sep/13 1:47 AM ]|
Updated patch attached.
Compiler-options looks like it may indeed be a better place for this, if that is the preferred strategy for controlling warnings. But I'll wait for more feedback / confirmation on the approach before making that change.
|Comment by Alex Miller [ 09/Sep/13 1:43 PM ]|
Is (:refer-clojure :exclude [+ = zero?]) a valid workaround? Or are you really concerned with the consumers of the library?
|Comment by Mike Anderson [ 09/Sep/13 7:18 PM ]|
I'm mainly concerned with consumers of the library.
So while (:refer-clojure :exclude [+ = zero?]) is possible as a temporary workaround, it's very inconvenient for users. We should really fix this in Clojure itself. Users have enough trouble with ns forms already without adding to their woes
As an alternative solution: I personally wouldn't mind it if the library author could add some metadata to symbols (e.g. "^:replace-symbol") to signal that a library function is intended to replace something in core. That's a slightly different approach (and I think a bit trickier to implement) but it should also work.