Clojure

CompilerException / IllegalStateException when overriding vars

Details

  • Type: Enhancement Enhancement
  • Status: Reopened Reopened
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: None
  • Component/s: None
  • Approval:
    Triaged

Description

=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:4:1)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6745)
	clojure.lang.Compiler.analyze (Compiler.java:6529)
	clojure.lang.Compiler.analyze (Compiler.java:6490)
	clojure.lang.Compiler.eval (Compiler.java:6801)
	clojure.lang.Compiler.eval (Compiler.java:6760)
	clojure.core/eval (core.clj:3079)
	clojure.main/repl/read-eval-print--7095/fn--7098 (main.clj:240)
	clojure.main/repl/read-eval-print--7095 (main.clj:240)
	clojure.main/repl/fn--7104 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)
	clojure.main/main (main.clj:422)
Caused by:
IllegalStateException a already refers to: #'foo/a in namespace: bar
	clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
	clojure.lang.Namespace.intern (Namespace.java:72)
	clojure.lang.Compiler$DefExpr$Parser.parse (Compiler.java:534)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6738)
	clojure.lang.Compiler.analyze (Compiler.java:6529)

I would expect (at worst) a similar warning to the initial namespace loading, rather than an exception here.

Activity

Hide
Alex Miller added a comment -

Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.

Show
Alex Miller added a comment - Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.
Hide
Andy Fingerhut added a comment -

I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:

(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))

Then start a REPL with 'lein repl', and I see this behavior:

user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil

Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports.

It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.

Show
Andy Fingerhut added a comment - I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:
(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))
Then start a REPL with 'lein repl', and I see this behavior:
user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports. It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.
Hide
Alex Miller added a comment -

Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.

Show
Alex Miller added a comment - Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.
Hide
Mike Anderson added a comment -

To reproduce:

(ns op)
(defn * [a b] (clojure.core/* a b)) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives error!

I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed.

The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op

Show
Mike Anderson added a comment - To reproduce: (ns op) (defn * [a b] (clojure.core/* a b)) ;; gives warning (ns use-op (:require [op :refer :all])) ;; gives warning (ns use-op (:require [op :refer :all])) ;; gives error! I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed. The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op
Hide
Alex Miller added a comment -

Duped in CLJ-1578.

Show
Alex Miller added a comment - Duped in CLJ-1578.
Hide
Mike Anderson added a comment -

This is still affecting me, and causing breakage with the latest versions of core.matrix. I don't know if this is a regression or not, but it certainly happens in 1.7.0-RC1

Any chance we can get a fix for 1.7? It is really annoying to have code fail because of this and force of refactoring of user code (my use case is adding a new var to clojure.core.matrix namespace, compiler error in user code that previously defined a var with the same name).

Show
Mike Anderson added a comment - This is still affecting me, and causing breakage with the latest versions of core.matrix. I don't know if this is a regression or not, but it certainly happens in 1.7.0-RC1 Any chance we can get a fix for 1.7? It is really annoying to have code fail because of this and force of refactoring of user code (my use case is adding a new var to clojure.core.matrix namespace, compiler error in user code that previously defined a var with the same name).
Hide
Mike Anderson added a comment -

Closing because I think this is better handled in the related issue

Show
Mike Anderson added a comment - Closing because I think this is better handled in the related issue
Hide
Mike Anderson added a comment -

Reopening because CLJ-1578 apparently does not resolve this specific issue, it only covers vars in clojure.core.

I'd still like to see this fixed for all namespaces, not just clojure.core.

Show
Mike Anderson added a comment - Reopening because CLJ-1578 apparently does not resolve this specific issue, it only covers vars in clojure.core. I'd still like to see this fixed for all namespaces, not just clojure.core.
Hide
Mike Anderson added a comment -

Reproduction:

=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:1:1)
=> clojure-version
{:major 1, :minor 7, :incremental 0, :qualifier "RC1"}

Stack trace:

CompilerException java.lang.RuntimeException: Unable to resolve symbol: pst in this context, compiling:(NO_SOURCE_PATH:1:1)
clojure.lang.Compiler.analyze (Compiler.java:6543)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3737)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6735)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5861)
clojure.lang.Compiler$FnMethod.parse (Compiler.java:5296)
clojure.lang.Compiler$FnExpr.parse (Compiler.java:3925)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6731)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.eval (Compiler.java:6789)
Caused by:
RuntimeException Unable to resolve symbol: pst in this context
clojure.lang.Util.runtimeException (Util.java:221)
clojure.lang.Compiler.resolveIn (Compiler.java:7029)
clojure.lang.Compiler.resolve (Compiler.java:6973)
clojure.lang.Compiler.analyzeSymbol (Compiler.java:6934)
clojure.lang.Compiler.analyze (Compiler.java:6506)
clojure.lang.Compiler.analyze (Compiler.java:6485)

Show
Mike Anderson added a comment - Reproduction: => (ns foo) nil => (def a 1) #'foo/a => (ns bar (:require [foo :refer :all])) nil => (def a 2) CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:1:1) => clojure-version {:major 1, :minor 7, :incremental 0, :qualifier "RC1"} Stack trace: CompilerException java.lang.RuntimeException: Unable to resolve symbol: pst in this context, compiling:(NO_SOURCE_PATH:1:1) clojure.lang.Compiler.analyze (Compiler.java:6543) clojure.lang.Compiler.analyze (Compiler.java:6485) clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3737) clojure.lang.Compiler.analyzeSeq (Compiler.java:6735) clojure.lang.Compiler.analyze (Compiler.java:6524) clojure.lang.Compiler.analyze (Compiler.java:6485) clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5861) clojure.lang.Compiler$FnMethod.parse (Compiler.java:5296) clojure.lang.Compiler$FnExpr.parse (Compiler.java:3925) clojure.lang.Compiler.analyzeSeq (Compiler.java:6731) clojure.lang.Compiler.analyze (Compiler.java:6524) clojure.lang.Compiler.eval (Compiler.java:6789) Caused by: RuntimeException Unable to resolve symbol: pst in this context clojure.lang.Util.runtimeException (Util.java:221) clojure.lang.Compiler.resolveIn (Compiler.java:7029) clojure.lang.Compiler.resolve (Compiler.java:6973) clojure.lang.Compiler.analyzeSymbol (Compiler.java:6934) clojure.lang.Compiler.analyze (Compiler.java:6506) clojure.lang.Compiler.analyze (Compiler.java:6485)
Hide
Nicola Mometto added a comment -

As I already commented in CLJ-1578, I don't think this is a bug and I think this ticket should be declined.

Overriding non clojure.core vars has always (since 1.2 at least) caused an exception to be thrown.

Show
Nicola Mometto added a comment - As I already commented in CLJ-1578, I don't think this is a bug and I think this ticket should be declined. Overriding non clojure.core vars has always (since 1.2 at least) caused an exception to be thrown.
Hide
Nicola Mometto added a comment -

Mike, maybe it would make sense to bring this issue up in the clojure-dev ml to get some opinions?

Show
Nicola Mometto added a comment - Mike, maybe it would make sense to bring this issue up in the clojure-dev ml to get some opinions?
Hide
Mike Anderson added a comment -

Re-classify it as a feature request, if you prefer.

I still regard it as a defect because I expect :refer :all to work sanely.

Either way, this issue keeps breaking user code in my area (data science / exploratory statistics / data management). The ability to use / refer all is very useful for setting up a convenient namespace for exploratory work, so I don't accept that forcing users to explicitly require every single var used (as Nicola suggests in CLJ-1578) is a practical workaround.

I've also had it cause problems when working at the REPL and reloading namespaces.

If the Clojure core team really wants to keep this annoying behaviour, can we at least have some way to turn it off at the library level? Perhaps some namespace metadata that I can add to the clojure.core.matrix namespace to stop this from triggering?

Show
Mike Anderson added a comment - Re-classify it as a feature request, if you prefer. I still regard it as a defect because I expect :refer :all to work sanely. Either way, this issue keeps breaking user code in my area (data science / exploratory statistics / data management). The ability to use / refer all is very useful for setting up a convenient namespace for exploratory work, so I don't accept that forcing users to explicitly require every single var used (as Nicola suggests in CLJ-1578) is a practical workaround. I've also had it cause problems when working at the REPL and reloading namespaces. If the Clojure core team really wants to keep this annoying behaviour, can we at least have some way to turn it off at the library level? Perhaps some namespace metadata that I can add to the clojure.core.matrix namespace to stop this from triggering?
Hide
Nicola Mometto added a comment -

Mike, this is just my personal opinion, I'm not a part of the core team and I don't speak for them, this is why I suggested you wrote on the clojure-dev ml.

Also to clarify, this issue you're reporting cannot manifest itself while reloading namespaces as the exception is thrown as soon as the redefinition happens.

Show
Nicola Mometto added a comment - Mike, this is just my personal opinion, I'm not a part of the core team and I don't speak for them, this is why I suggested you wrote on the clojure-dev ml. Also to clarify, this issue you're reporting cannot manifest itself while reloading namespaces as the exception is thrown as soon as the redefinition happens.
Hide
Alex Miller added a comment -

You could use :exclude for this

(ns bar (:require [foo :refer :all :exclude (a)]))
Show
Alex Miller added a comment - You could use :exclude for this
(ns bar (:require [foo :refer :all :exclude (a)]))
Hide
Mike Anderson added a comment -

Hi Alex, that works as a fix when the problem occurs, but doesn't solve the problem of future user code breakage, unless the user accurately anticipates what symbols might get added to "bar" in the future. Which again seems like an unreasonable burden on the user.

What I'm arguing for, I guess, is a default presumption that if the user defines a var in their own namespace, they are happy to replace a similarly named var in any namespaces that they have previously use'd / refer-all'd.

If the user is genuinely concerned about overriding things by accident, I'd be happy with a warn-on-replace which does something analogous to warn-on-reflection. I proposed something similar in CLJ-1257 a while back, even wrote a patch that solves the whole problem in this way. Can we get that patch or something similar in 1.7?

Show
Mike Anderson added a comment - Hi Alex, that works as a fix when the problem occurs, but doesn't solve the problem of future user code breakage, unless the user accurately anticipates what symbols might get added to "bar" in the future. Which again seems like an unreasonable burden on the user. What I'm arguing for, I guess, is a default presumption that if the user defines a var in their own namespace, they are happy to replace a similarly named var in any namespaces that they have previously use'd / refer-all'd. If the user is genuinely concerned about overriding things by accident, I'd be happy with a warn-on-replace which does something analogous to warn-on-reflection. I proposed something similar in CLJ-1257 a while back, even wrote a patch that solves the whole problem in this way. Can we get that patch or something similar in 1.7?
Hide
Nicola Mometto added a comment -

CLJ-1746 is relevant and I like that proposal much better than CLJ-1257

Show
Nicola Mometto added a comment - CLJ-1746 is relevant and I like that proposal much better than CLJ-1257
Hide
Mike Anderson added a comment - - edited

Hi Nicola, CLJ-1746 looks like a reasonable suggestion but still doesn't address the problem here, for the same reason that :exclude doesn't (see my comment to Alex)

The fundamental issue is that the current behaviour causes user code to break when libraries are upgraded to add new vars and requires changes to user code to fix / work around it. I consider that broken behaviour, or at least very bad design.

This is especially since we are encouraged to choose good names: I quote from the library coding standards(http://dev.clojure.org/display/community/Library+Coding+Standards)

"Use good names, and don't be afraid to collide with names in other namespaces. That's what the flexible namespace support is there for."

It isn't very flexible when user code keep breaking.....

Show
Mike Anderson added a comment - - edited Hi Nicola, CLJ-1746 looks like a reasonable suggestion but still doesn't address the problem here, for the same reason that :exclude doesn't (see my comment to Alex) The fundamental issue is that the current behaviour causes user code to break when libraries are upgraded to add new vars and requires changes to user code to fix / work around it. I consider that broken behaviour, or at least very bad design. This is especially since we are encouraged to choose good names: I quote from the library coding standards(http://dev.clojure.org/display/community/Library+Coding+Standards) "Use good names, and don't be afraid to collide with names in other namespaces. That's what the flexible namespace support is there for." It isn't very flexible when user code keep breaking.....
Hide
Nicola Mometto added a comment -

From the same page, just a two lines below:
"Be explicit and minimalist about dependencies on other packages. (Prefer :require :refer in 1.4+ or :use :only in 1.0-1.3)."

If users carelessly import a whole package, I'd say that's their fault. Since clojure >1.3 the popular consensus has been that :use/:refer :all are not a good idea and people have been moving away from that, preferring :require :refer or :require :as instead.

The few that still use :use/:refer :all do it mostly for backward compatibility or in tests (I myself do it in tools.reader for the former reason).

I really don't think this is such a bad design choice as you seem to think, and I think that most people in the community would agree that the current behaviour and drive towards using :require :refer/require :as in lieu of :use/:refer :all is way more beneficial than harmful

Show
Nicola Mometto added a comment - From the same page, just a two lines below: "Be explicit and minimalist about dependencies on other packages. (Prefer :require :refer in 1.4+ or :use :only in 1.0-1.3)." If users carelessly import a whole package, I'd say that's their fault. Since clojure >1.3 the popular consensus has been that :use/:refer :all are not a good idea and people have been moving away from that, preferring :require :refer or :require :as instead. The few that still use :use/:refer :all do it mostly for backward compatibility or in tests (I myself do it in tools.reader for the former reason). I really don't think this is such a bad design choice as you seem to think, and I think that most people in the community would agree that the current behaviour and drive towards using :require :refer/require :as in lieu of :use/:refer :all is way more beneficial than harmful
Hide
Mike Anderson added a comment -

Hi Nicola, I have no issue with people using explicit :refer / :require, and I agree it is normal practice. It's good to be explicit and I generally do that myself (except in test / demo code).

However we aren't talking about that case : this issue is most relevant in those situations where users (for their own legitimate reasons) have decided to import a whole namespace. This is often convenient (e.g. REPL usage), sometimes it is valuable for specific purposes (e.g. testing).

I personally care about this mostly from the perspective of a library author: I want users to be able to use the library in whatever way is most convenient (which may include importing all vars), and I don't want user code to break randomly when I make a new release. Note that this also means I don't have control over user code so any solution that involves explicit requires, excludes, or some other manual workarounds is not a practical solution.

You can say "that's their fault" but this is currently supposed to be supported in Clojure so I think it ought to work sanely.

Show
Mike Anderson added a comment - Hi Nicola, I have no issue with people using explicit :refer / :require, and I agree it is normal practice. It's good to be explicit and I generally do that myself (except in test / demo code). However we aren't talking about that case : this issue is most relevant in those situations where users (for their own legitimate reasons) have decided to import a whole namespace. This is often convenient (e.g. REPL usage), sometimes it is valuable for specific purposes (e.g. testing). I personally care about this mostly from the perspective of a library author: I want users to be able to use the library in whatever way is most convenient (which may include importing all vars), and I don't want user code to break randomly when I make a new release. Note that this also means I don't have control over user code so any solution that involves explicit requires, excludes, or some other manual workarounds is not a practical solution. You can say "that's their fault" but this is currently supposed to be supported in Clojure so I think it ought to work sanely.
Hide
Alex Miller added a comment -

I think the library / evolution argument is a good one and I like that it reduces breakage due to evolution of 3rd party libraries. (I feel very strongly about this in clojure.core itself which is auto-referred, less strongly otherwise.)

On the flip side, if we remove this error, we should be explicit about what we are giving up to fully consider it. Presumably we are at least giving up a helpful error that occurs in the case of an accidental override. Are there other impacts?

I do not expect that we're going to change anything in 1.7.

Show
Alex Miller added a comment - I think the library / evolution argument is a good one and I like that it reduces breakage due to evolution of 3rd party libraries. (I feel very strongly about this in clojure.core itself which is auto-referred, less strongly otherwise.) On the flip side, if we remove this error, we should be explicit about what we are giving up to fully consider it. Presumably we are at least giving up a helpful error that occurs in the case of an accidental override. Are there other impacts? I do not expect that we're going to change anything in 1.7.
Hide
Stuart Halloway added a comment -

Recategorizing as a feature request, as the current behavior was the original intent.

Could a namespace-reflecting macro provide for this use case without requiring a change to core?

Show
Stuart Halloway added a comment - Recategorizing as a feature request, as the current behavior was the original intent. Could a namespace-reflecting macro provide for this use case without requiring a change to core?

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated: