Clojure

make use of deprecated namespaces/vars easier to spot

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Prescreened

Description

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?
  1. 706-deprecated-ns-var-warnings-tested.diff
    07/Mar/16 9:18 PM
    10 kB
    Cezary Kosko
  2. 706-deprecated-ns-var-warnings-tested-2.diff
    11/Mar/16 5:33 AM
    20 kB
    Cezary Kosko
  3. 706-deprecated-ns-var-warnings-tested-3.diff
    11/Mar/16 12:37 PM
    19 kB
    Cezary Kosko
  4. 706-deprecated-var-warning.diff
    26/Oct/12 1:37 PM
    6 kB
    Luke VanderHart
  5. 706-deprecated-var-warning-patch-v2.txt
    13/Feb/13 12:38 AM
    6 kB
    Andy Fingerhut
  6. 706-fix-deprecation-warnings-agents.diff
    26/Oct/12 1:37 PM
    1 kB
    Luke VanderHart
  7. 706-fix-deprecation-warnings-on-replicate.diff
    26/Oct/12 1:37 PM
    3 kB
    Luke VanderHart
  8. 706-fix-deprecation-warning-test-junit.diff
    26/Oct/12 1:37 PM
    3 kB
    Luke VanderHart
  9. 706-warning-on-deprecated-ns.diff
    29/Oct/12 1:02 AM
    0.9 kB
    Ghadi Shayban

Activity

Hide
Alex Miller added a comment -

Marking pre-screened for Rich to look at.

Show
Alex Miller added a comment - Marking pre-screened for Rich to look at.
Hide
Alex Miller added a comment -

One more (hopefully final) round and then I think we're good:

  • The docstring for warn-on-deprecated should be updated now that we've expanded scope to cover ns too.
  • In the deprecated ns warning message, can we make it: "Deprecation warning: loading deprecated namespace `foo` from namespace `bar`."
  • In the macro and var warnings can we change "is tagged as deprecated" to just "is deprecated"?
  • Clean up the hanging parens in test/clojure/test_clojure/compilation/deprecated.clj

Thanks for the work on this!!

Show
Alex Miller added a comment - One more (hopefully final) round and then I think we're good:
  • The docstring for warn-on-deprecated should be updated now that we've expanded scope to cover ns too.
  • In the deprecated ns warning message, can we make it: "Deprecation warning: loading deprecated namespace `foo` from namespace `bar`."
  • In the macro and var warnings can we change "is tagged as deprecated" to just "is deprecated"?
  • Clean up the hanging parens in test/clojure/test_clojure/compilation/deprecated.clj
Thanks for the work on this!!
Hide
Cezary Kosko added a comment -

Uploaded a new diff addressing the comments & added warning on macroexpansion.

As far as the namespace deprecation warning goes, though, the code's only printing the current namespace, did not know whether there was a decent way to get a file/line combo.

Show
Cezary Kosko added a comment - Uploaded a new diff addressing the comments & added warning on macroexpansion. As far as the namespace deprecation warning goes, though, the code's only printing the current namespace, did not know whether there was a decent way to get a file/line combo.
Hide
Alex Miller added a comment - - edited
  • The if in the first change in core.clj should be a when instead.
  • Can namespace deprecation warning include more about where this is occurring?
  • I'm having a hard time reproducing the deprecated ns warning in a manual test (see below). There seems to be something weird about the binding+printf as the conditions appear to be satisfied. I'm thinking it has something to do with flushing *err*? Seems like (println "Warning: loading deprecated ns" lib) would be better there.
(set! *warn-on-deprecated* true)
(ns foo {:deprecated true})
(ns bar (:require foo))
  • src/jvm/clojure/lang/Compile.java needs added support for clojure.compile.warn-on-deprecated RT flag
  • I think we should turn on warn-on-deprecated in the Clojure build itself (in build.xml)
  • If you do that, the following deprecation warnings exist in the Clojure build itself and we should fix those:

[java] Deprecation warning, clojure/core_proxy.clj:112:75 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/genclass.clj:149:41 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/genclass.clj:235:65 : var #'clojure.core/replicate is tagged as deprecated
[java] Deprecation warning, clojure/test/junit.clj:118:22 : var #'clojure.test/file-position is tagged as deprecated

  • Mark clojure.parallel as deprecated in the ns meta
Show
Alex Miller added a comment - - edited
  • The if in the first change in core.clj should be a when instead.
  • Can namespace deprecation warning include more about where this is occurring?
  • I'm having a hard time reproducing the deprecated ns warning in a manual test (see below). There seems to be something weird about the binding+printf as the conditions appear to be satisfied. I'm thinking it has something to do with flushing *err*? Seems like (println "Warning: loading deprecated ns" lib) would be better there.
(set! *warn-on-deprecated* true)
(ns foo {:deprecated true})
(ns bar (:require foo))
  • src/jvm/clojure/lang/Compile.java needs added support for clojure.compile.warn-on-deprecated RT flag
  • I think we should turn on warn-on-deprecated in the Clojure build itself (in build.xml)
  • If you do that, the following deprecation warnings exist in the Clojure build itself and we should fix those:
[java] Deprecation warning, clojure/core_proxy.clj:112:75 : var #'clojure.core/replicate is tagged as deprecated [java] Deprecation warning, clojure/genclass.clj:149:41 : var #'clojure.core/replicate is tagged as deprecated [java] Deprecation warning, clojure/genclass.clj:235:65 : var #'clojure.core/replicate is tagged as deprecated [java] Deprecation warning, clojure/test/junit.clj:118:22 : var #'clojure.test/file-position is tagged as deprecated
  • Mark clojure.parallel as deprecated in the ns meta
Hide
Andy Fingerhut added a comment -

Cezary, I have bumped up your permissions on JIRA so you should be able to edit tickets now. Please reload the page and try again.

Show
Andy Fingerhut added a comment - Cezary, I have bumped up your permissions on JIRA so you should be able to edit tickets now. Please reload the page and try again.
Hide
Cezary Kosko added a comment - - edited

Uploaded a patch that coalesces var/ns-related patches by Luke & adds tests.
The patch does not, however, warn the user about deprecated macros, I assume I should adjust it, then?

Also, I'm not able adjust the description, so how do I take care of Alex's list's first bullet?

Show
Cezary Kosko added a comment - - edited Uploaded a patch that coalesces var/ns-related patches by Luke & adds tests. The patch does not, however, warn the user about deprecated macros, I assume I should adjust it, then? Also, I'm not able adjust the description, so how do I take care of Alex's list's first bullet?
Hide
Alex Miller added a comment -

There is a middle ground here to turn it off by default in the compiler, but to turn it on by default in the tools (like lein). But there's a reasonable chance that whatever I prefer, Rich will have a preference that overrules it when it gets to him.

I think creating more complexity around namespace prefixes is unlikely to help this ticket move forward.

Show
Alex Miller added a comment - There is a middle ground here to turn it off by default in the compiler, but to turn it on by default in the tools (like lein). But there's a reasonable chance that whatever I prefer, Rich will have a preference that overrules it when it gets to him. I think creating more complexity around namespace prefixes is unlikely to help this ticket move forward.
Hide
Phill Wolf added a comment -

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.

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

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

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

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.

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

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

Show
Alex Miller added a comment - Hey Vijay, Andrew Rosa assigned it to himself so please coordinate with him as he was starting to work on it.
Hide
Vijay Kiran added a comment -

Alex Miller I can give this a shot.

Show
Vijay Kiran added a comment - Alex Miller I can give this a shot.
Hide
Alex Miller added a comment - - edited

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 *warn-on-deprecated* over *warn-on-deprecation* 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 *warn-on-deprecated* to false in that namespace.
  • Current default is true - should probably be false instead to match the reflection warning default.
Show
Alex Miller added a comment - - edited 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 *warn-on-deprecated* over *warn-on-deprecation* 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 *warn-on-deprecated* to false in that namespace.
  • Current default is true - should probably be false instead to match the reflection warning default.
Hide
Andy Fingerhut added a comment -

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

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

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.

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

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.

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

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.

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

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?

Show
Ghadi Shayban added a comment - 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?
Hide
Andy Fingerhut added a comment -

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?

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

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?

Show
Andy Fingerhut added a comment - 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?
Hide
Luke VanderHart added a comment -

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

Show
Luke VanderHart added a comment - 706-deprecated-var-warning.diff adds warnings when using deprecated vars. The other three patches clean up deprecation warnings.
Hide
Rich Hickey added a comment -

I don't mind warning to stderr

Show
Rich Hickey added a comment - I don't mind warning to stderr

People

Vote (19)
Watch (8)

Dates

  • Created:
    Updated: