Clojure

1.7.0-alpha3 breakage due to symbol conflicts

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code
  • Approval:
    Ok

Description

I've been trying to build core.matrix with 1.7.0-alpha3 and I get a failures due to symbol conflicts with clojure.core (specifically the new update function).

java.lang.IllegalStateException: update already refers to: #'clojure.core.matrix.utils/update in namespace: clojure.core.matrix.impl.ndarray-magic
	at clojure.lang.Namespace.warnOrFailOnReplace(Namespace.java:88)
	at clojure.lang.Namespace.reference(Namespace.java:110)
	at clojure.lang.Namespace.refer(Namespace.java:168)
	at clojure.core$refer.doInvoke(core.clj:4071)
	at clojure.lang.RestFn.invoke(RestFn.java:439)
	at clojure.core.matrix.impl.ndarray_magic$eval9762$loading__5295__auto____9763.invoke(ndarray_magic.clj:1)
	at clojure.core.matrix.impl.ndarray_magic$eval9762.invoke(ndarray_magic.clj:1)

Simpler case to reproduce:

(ns foo)
(def inc dec) ;; gets a warning 
(ns bar (:require [foo :refer :all])) ;; gets another warning
(ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload)

Cause: In the case of a load, foo/inc is replacing clojure.core/inc and that causes the expected warning. In the case of a reload, clojure.core/inc is replacing foo/inc - this case is not currently handled and falls into the error case.

Approach: In the case of clojure.core/inc replacing foo/inc (should only happen during a reload), ignore and issue neither warning or error.

Patch: 0001-CLJ-1578-don-t-throw-when-a-core-Var-replaces-anothe.patch

Screened by: Alex Miller

Activity

Hide
Alex Miller added a comment -

The warnings I would expect / the failures I would not. Can you boil down the reproduction of the exception somehow?

Show
Alex Miller added a comment - The warnings I would expect / the failures I would not. Can you boil down the reproduction of the exception somehow?
Hide
Nicola Mometto added a comment -

I have seen similar failures when re-compiling a namespace that shadows a core Var:

  • ns foo is created
  • ns foo maps 'update to #'clojure.core/update
  • ns foo interns 'update, the compiler emits a warning
  • ns foo now maps 'update to #'foo/update
  • ns foo is reloaded
  • ns foo tries to map 'update to #'clojure.core/update but it's already mapped to #'foo/update

The logic in clojure.lang.Namespace/warnOnReplace makes it so that shadowing a clojure.core Var produces a warning while shadowing a Var from another namespace produces an error, this is what happening after reloading the namespace.

I haven't looked into the core.matrix code but I highly suspect that's what's going on there.

Show
Nicola Mometto added a comment - I have seen similar failures when re-compiling a namespace that shadows a core Var:
  • ns foo is created
  • ns foo maps 'update to #'clojure.core/update
  • ns foo interns 'update, the compiler emits a warning
  • ns foo now maps 'update to #'foo/update
  • ns foo is reloaded
  • ns foo tries to map 'update to #'clojure.core/update but it's already mapped to #'foo/update
The logic in clojure.lang.Namespace/warnOnReplace makes it so that shadowing a clojure.core Var produces a warning while shadowing a Var from another namespace produces an error, this is what happening after reloading the namespace. I haven't looked into the core.matrix code but I highly suspect that's what's going on there.
Hide
Alex Miller added a comment -

Definitely interested in a patch for this for the special case of clojure.core.

Show
Alex Miller added a comment - Definitely interested in a patch for this for the special case of clojure.core.
Hide
Nicola Mometto added a comment -

The attached patch fixes this issue by making warnOrFailOnReplace silently ignore when a clojure.core Var shadows another Var, which should only happen on namespace reload.

Show
Nicola Mometto added a comment - The attached patch fixes this issue by making warnOrFailOnReplace silently ignore when a clojure.core Var shadows another Var, which should only happen on namespace reload.
Hide
Mike Anderson added a comment -

The simplest way I can find to reproduce the general issue at the REPL is as follows:

(ns foo)
(def inc dec) ;; gets a warning
(ns bar (:require [foo :refer :all])) ;; gets another warning
(ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload)

Preventing the exception is the biggest priority, it would be really nice to be also suppress the warnings. There are often good reasons to re-use names in clojure.core so it shouldn't cause a non-suppressible warning.

Note that the Clojure library coding standards say "Use good names, and don't be afraid to collide with names in other namespaces" so it is very inconsistent to trigger warnings / exceptions when people do exactly this.

Show
Mike Anderson added a comment - The simplest way I can find to reproduce the general issue at the REPL is as follows: (ns foo) (def inc dec) ;; gets a warning (ns bar (:require [foo :refer :all])) ;; gets another warning (ns bar (:require [foo :refer :all])) ;; causes the exception (effectively a ns reload) Preventing the exception is the biggest priority, it would be really nice to be also suppress the warnings. There are often good reasons to re-use names in clojure.core so it shouldn't cause a non-suppressible warning. Note that the Clojure library coding standards say "Use good names, and don't be afraid to collide with names in other namespaces" so it is very inconsistent to trigger warnings / exceptions when people do exactly this.
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 -

FWIW, I think the current patch is simply too narrow in scope. We should apply the logic of not throwing an error to any var, not just core.

Show
Mike Anderson added a comment - FWIW, I think the current patch is simply too narrow in scope. We should apply the logic of not throwing an error to any var, not just core.
Hide
Nicola Mometto added a comment -

Can you add a new reproduction case? The one you previously posted (that is in the ticket description) now works.

Show
Nicola Mometto added a comment - Can you add a new reproduction case? The one you previously posted (that is in the ticket description) now works.
Hide
Mike Anderson added a comment -

The simplest way that I can reproduce it is as follows:

=> (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)

Think of "foo" as a library namespace like "clojure.core.matrix" and "bar" as user code. I would expect user code to be able to override the var that has been referred (possibly with a configurable warning). So the scenario I am trying to avoid is user code breaking whenever I add a new var like "a" to core.matrix.

Show
Mike Anderson added a comment - The simplest way that I can reproduce it is as follows: => (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) Think of "foo" as a library namespace like "clojure.core.matrix" and "bar" as user code. I would expect user code to be able to override the var that has been referred (possibly with a configurable warning). So the scenario I am trying to avoid is user code breaking whenever I add a new var like "a" to core.matrix.
Hide
Nicola Mometto added a comment -

I'm re-closing this ticket since the snipeet you just posted demonstrating has nothing to do with the bug reported in this ticket and is actually not a bug at all but a design decision of clojure that's alwasy been this way.

Only overriding clojure.core vars will give you a warning since they are referred by default, overriding var from other namespace will and has always caused an error since you control whether to refer them or not.

:refer :all is discouraged for this reason, among others.

Show
Nicola Mometto added a comment - I'm re-closing this ticket since the snipeet you just posted demonstrating has nothing to do with the bug reported in this ticket and is actually not a bug at all but a design decision of clojure that's alwasy been this way. Only overriding clojure.core vars will give you a warning since they are referred by default, overriding var from other namespace will and has always caused an error since you control whether to refer them or not. :refer :all is discouraged for this reason, among others.
Hide
Mike Anderson added a comment -

OK I understand, though I think it's a poor design decision if that is the case, and will probably continue to complain whenever it breaks user code.

Ultimately, I think it is very inconvenient to push users to explicitly declare every single used var, especially in exploratory / demo code. In fact I can't think of another language off the top of my head that enforces something like this as a policy.

Show
Mike Anderson added a comment - OK I understand, though I think it's a poor design decision if that is the case, and will probably continue to complain whenever it breaks user code. Ultimately, I think it is very inconvenient to push users to explicitly declare every single used var, especially in exploratory / demo code. In fact I can't think of another language off the top of my head that enforces something like this as a policy.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: