make use of deprecated namespaces/vars easier to spot

Question

From the mailing list http://groups.google.com/group/clojure/msg/c41d909bd58e4534. It is easy to use deprecated namespaces or vars without knowing you are doing so. The documentation warnings are small, and there is no compiler warning.

*Proposed:*

* Add new {{\*warn-on-deprecated*}} dynamic var, defaulted to false
* Warn to stderr when {{{:deprecated true}}} namespace is loaded.
* Warn to stderr when {{{:deprecated true}}} var is analyzed.
* Warn to stderr when {{{:deprecated true}}} macro is expanded.
* New system property clojure.compiler.warn-on-deprecated
* Compile Clojure itself with clojure.compiler.warn-on-deprecated
* Fix deprecation warnings inside Clojure (replicate, clear-agent-errors)
* Mark clojure.parallel as deprecated with :deprecation tag

Examples:


(set! *warn-on-deprecated* true)

;; use of a deprecated var (on compile)
(defn ^:deprecated f [x] x)
(f 5)
;;=> Deprecation warning, NO_SOURCE_PATH:7:1 : var #'user/f is deprecated

;; use of a deprecated macro (on macro expansion)
(defmacro ^:deprecated m [x] x)
(m 5)
;;=> Deprecation warning, NO_SOURCE_PATH:7:1 : macro #'user/m is deprecated

;; use of a deprecated namespace (on load)
(ns foo {:deprecated "1.1"})
(ns bar (:require foo))
;;=> Deprecation warning: loading deprecated namespace `foo` from namespace `bar`


*Patch:* 706-deprecated-ns-var-warnings-tested-3.diff

*Questions:* Should default for deprecation warnings be true instead? People upgrading are likely to see new warnings which might be surprising.

* Should default be to warn or not warn on deprecated?

Answer 1

Comment made by: richhickey

I don't mind warning to stderr

Answer 2

Comment made by: lvanderhart

706-deprecated-var-warning.diff adds warnings when using deprecated vars. The other three patches clean up deprecation warnings.

Answer 3

Comment made by: jafingerhut

Great stuff. I looked through the first patch, and didn't see anything in there that lets someone disable deprecation warnings from the command line, the way that warn-on-reflection can today be set to true with a command line option.

Is that something important to have for deprecation warnings?

Answer 4

Comment made by: jafingerhut

I was hoping it would be quick and easy to add source file, line, and column info to the deprecation warning messages. It isn't as easy as adding them to the format() call, because the method analyzeSymbol doesn't receive these values as args. Is this deprecation check being done in a place where it is not easy to relate it to the source file, line, and column? Could it be done in a place where that info is easily available?

Answer 5

Comment made by: gshayban

Another patch - this time to warn on loading deprecated namespaces, instead of vars. This patch requires the first one.

Re: line/column, I'll figure out how to thread the compile context through if it's desired.

Re: Compile flag. I have a patch for this also, but I'm still verifying how to invoke. How is warn-on-reflection set by command line?

Answer 6

Comment made by: jafingerhut

Re: Compile flag. Don't hold off on implementing a flag if you want to, but it might be worth hearing from others whether such a command line option is even desired. I was asking in hopes of eliciting such a response.

For the way that it is handled in the Clojure compiler, search for REFLECTION_WARNING_PROP and related code in Compile.java. If you invoke the Clojure compiler directly via a Java command line, use -Dclojure.compile.warn-on-reflection=true (default is false). See the recent email thread sent to Clojure Dev Google group if you want to know how to do it via ant or Maven. Link: https://mail.google.com/mail/?shva=1#label/clojure-dev/13aa0e34530196c3

There is also a separate command-line flag called compiler-options (see Compile.java) that is implemented as a map inside the compiler. It was added more recently than warn-on-reflection, and might be the preferred method to add more such options, to avoid having to continue to add more arguments to the pushThreadBindings calls done in several places.

Answer 7

Comment made by: gshayban

Thanks, Andy.

Alternately for the last ns patch, it is equivalent to call (print-method msg err), rather than binding out to err, may be more readable. I'll be glad to send that in if it's preferable.

Answer 8

Comment made by: jafingerhut

706-deprecated-var-warning-patch-v2.txt dated Feb 12 2013 is identical to 706-deprecated-var-warning.diff dated Oct 26 2012, except it applies cleanly to latest master.

Answer 9

Comment made by: jafingerhut

For the information of anyone examining this ticket wishing for this feature, the Eastwood lint tool reports calls to deprecated Clojure functions, and also to deprecated Java methods. https://github.com/jonase/eastwood

Answer 10

Comment made by: alexmiller

I'm interested in considering this for Clojure 1.9 but I need some help getting it ready. Some comments I have on the current state: - Ticket needs to have more details about the current approach

• I prefer } over because it echoes the keyword you use to mark deprecated vars
• The warning message does not tell you a location, which is grr - should be similar to the reflection messages
• Needs tests - see test/clojure/test_clojure/compilation.clj and test/clojure/test_helper.clj (should-not-reflect) for examples
• clojure itself has some instances of deprecated usage - it would be nice to clean those up in the patch too. That may need to be in a separate patch, depends if they are easy to fix or not. If there are cases in test/ that are actually good to leave, can set * } to false in that namespace.
• Current default is true - should probably be false instead to match the reflection warning default.

Answer 11

Comment made by: vijaykiran

(link: ~alexmiller) I can give this a shot.

Answer 12

Comment made by: alexmiller

Hey Vijay, Andrew Rosa assigned it to himself so please coordinate with him as he was starting to work on it.

Answer 13

Comment made by: bozhidar

Just one small remark - isn't it more common to have deprecation warnings enabled by default? One could argue they are way more important than reflection warnings, as your code might get broken in the future because you didn't notice you were using deprecated stuff.

Answer 14

Comment made by: alexmiller

(link: ~bozhidar) I'm on the fence. My main hesitation in making it the default is that people will suddenly have a bunch of new warnings (which could be either good or bad I suppose). Depends how strongly we want people to care about deprecations I guess.

Answer 15

Comment made by: pbwolf

A deprecation warning that is off by default does not address the first and primary problem given in this ticket: "It is easy to use deprecated namespaces without knowing you are doing so."

It's unlike the reflection warning. You may focus on speed at any time, at your leisure. But the eventual removal of at-risk features will be a sudden, unpleasant surprise; a warning would be helpful.

But - Suppose I wrote 300 lines of Clojure and use a million lines that come from jars. Will any deprecation problems in my own code be buried in a tsunami of warnings about those jars? Worse, the tsunami will likely linger for weeks or months, until the libraries' respective authors catch up. Inasmuch as the jars are covered (much more expediently) by 'lein ancient' and similar, I would prefer to be able to limit deprecation warnings to just my stuff, perhaps by namespace prefix if from-a-jar-or-not is inconvenient from the compiler's point of view.