Clojure

Suppress warnings when clojure.core symbols are explicitly replaced with "use" or "refer"

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

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.

  1. clj-1257.diff
    06/Sep/13 10:38 PM
    4 kB
    Mike Anderson
  2. clj-1257-2.diff
    08/Sep/13 1:46 AM
    8 kB
    Mike Anderson

Activity

Hide
Mike Anderson added a comment -

Basic patch and test attached.

Show
Mike Anderson added a comment - Basic patch and test attached.
Hide
Andy Fingerhut added a comment -

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)?

Show
Andy Fingerhut added a comment - 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)?
Hide
Mike Anderson added a comment -

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.

Show
Mike Anderson added a comment - 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.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Mike Anderson added a comment -

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.

Show
Mike Anderson added a comment - 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.
Hide
Alex Miller added a comment -

Is (:refer-clojure :exclude [+ = zero?]) a valid workaround? Or are you really concerned with the consumers of the library?

Show
Alex Miller added a comment - Is (:refer-clojure :exclude [+ = zero?]) a valid workaround? Or are you really concerned with the consumers of the library?
Hide
Mike Anderson added a comment -

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.

Show
Mike Anderson added a comment - 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.
Hide
Mike Anderson added a comment -

Example issue reported by a user because of this:

https://github.com/mikera/vectorz-clj/issues/23

Show
Mike Anderson added a comment - Example issue reported by a user because of this: https://github.com/mikera/vectorz-clj/issues/23

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated: