Clojure

Class name clash between top-level functions and defn'ed ones

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Vetted

Description

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Demonstration script: demo1.clj

Patch: 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.

See also: This patch also fixes the issue reported in CLJ-1227.

Activity

Hide
Ambrose Bonnaire-Sergeant added a comment -

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Show
Ambrose Bonnaire-Sergeant added a comment - This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error. This doesn't really matter, but I thought it may or may not be useful information to someone.
Hide
Nicola Mometto added a comment -

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Show
Nicola Mometto added a comment - Attached a fix. This also fixes AOT compiling of code like:
(def x (fn foo []))
(fn foo [])
Hide
Nicola Mometto added a comment -

Cleaned up patch

Show
Nicola Mometto added a comment - Cleaned up patch
Hide
Alex Miller added a comment -

It looks like the patch changes indentation of some of the code - can you fix that?

Show
Alex Miller added a comment - It looks like the patch changes indentation of some of the code - can you fix that?
Hide
Nicola Mometto added a comment -

Updated patch without whitespace changes

Show
Nicola Mometto added a comment - Updated patch without whitespace changes
Hide
Alex Miller added a comment -

Thanks, that's helpful.

Show
Alex Miller added a comment - Thanks, that's helpful.
Hide
Alex Miller added a comment -

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Show
Alex Miller added a comment - There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it. Some questions we would like to answer: 1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into. 2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests. 3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.
Hide
Nicola Mometto added a comment -

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

Show
Nicola Mometto added a comment - Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28 My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case. Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue. Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware. I can add some tests to test that
(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

People

Vote (6)
Watch (9)

Dates

  • Created:
    Updated: