Clojure

Classes generated by deftype and defrecord don't play nice with .getPackage

Details

  • Type: Enhancement Enhancement
  • Status: Reopened Reopened
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6, Release 1.7, Release 1.8
  • Fix Version/s: Release 1.11
  • Component/s: None
  • Patch:
    Code and Test
  • Approval:
    Incomplete

Description

Classes generated loaded by DynamicClassLoader return nil for .getPackage. Tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.

(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

Proposed: During DynamicClassLoader.defineClass(), invoke definePackage() on the class being defined (similar to what URLClassLoader does).

Patch: clj-1550-v4.patch

Screened by: Alex Miller

  1. 0001-CLJ-1550-define-package-for-class-in-DynamicClassLoa.patch
    21/Jul/15 12:17 PM
    2 kB
    Nicola Mometto
  2. CLJ-1550-v2.patch
    22/Jul/15 12:32 PM
    2 kB
    Michael Blume
  3. clj-1550-v3.patch
    14/Nov/16 12:10 PM
    2 kB
    Alex Miller
  4. clj-1550-v4.patch
    29/Jun/18 11:00 AM
    2 kB
    Alex Miller

Activity

Hide
Alex Miller added a comment -

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Show
Alex Miller added a comment - According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.
Hide
Bozhidar Batsov added a comment -

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Show
Bozhidar Batsov added a comment - A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.
Hide
Kevin Downey added a comment -

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Show
Kevin Downey added a comment - if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue
Hide
Kevin Downey added a comment -

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Show
Kevin Downey added a comment - actually, it must just be that dynamicclassloader doesn't define a package for classes it loads
Hide
Alex Miller added a comment -

Yep, I believe that's correct.

Show
Alex Miller added a comment - Yep, I believe that's correct.
Hide
Stuart Halloway added a comment -

There is no problem statement here. What is package information needed for?

Show
Stuart Halloway added a comment - There is no problem statement here. What is package information needed for?
Hide
Bozhidar Batsov added a comment -

I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.
This might not be problem when running your apps, but it's definitely a problem when inspecting their state...

Show
Bozhidar Batsov added a comment - I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints. This might not be problem when running your apps, but it's definitely a problem when inspecting their state...
Hide
Michael Blume added a comment -

s/Packate/Package

Show
Michael Blume added a comment - s/Packate/Package
Hide
Alex Miller added a comment -

Refreshed patch to apply to current master, attribution retained, no semantic changes. Marked prescreened.

Show
Alex Miller added a comment - Refreshed patch to apply to current master, attribution retained, no semantic changes. Marked prescreened.
Hide
Ghadi Shayban added a comment -

We should consider interactions with the JDK9 module system here

Show
Ghadi Shayban added a comment - We should consider interactions with the JDK9 module system here
Hide
Alex Miller added a comment -

off the top of my head, I don't see how this would do anything negative wrt the JDK9 module system

Show
Alex Miller added a comment - off the top of my head, I don't see how this would do anything negative wrt the JDK9 module system
Hide
Alex Miller added a comment -

v4 patch rebases to master and retains attribution, no semantic changes.

Show
Alex Miller added a comment - v4 patch rebases to master and retains attribution, no semantic changes.
Hide
Bozhidar Batsov added a comment -

Alex, is this going to be included in Clojure 1.10?

Show
Bozhidar Batsov added a comment - Alex, is this going to be included in Clojure 1.10?
Hide
Alex Miller added a comment -

That's the current path...

Show
Alex Miller added a comment - That's the current path...
Hide
Ghadi Shayban added a comment -

This is a bad patch that slipped through. The motivation linked above shows an nREPL excerpt that attempts to symbolize the package, basically:

(symbol (.getName (.getPackage TheClass)))
. The original code can derive the package name in the same way the JVM grabs it, from the binary name. (Take the prefix up until the last period.)

The fix also introduces a deprecated call, and more work during classloading (which is critical path).

As an aside, JDK9+ automatically defines a package upon class definition, derived from the binary name:

## JDK 8
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:8-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
:nada

## JDK 9
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:9-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
#object[java.lang.Package 0x10993713 "package user"]

## JDK 11
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:11-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
#object[java.lang.Package 0x6f3c660a "package user"]
➜  clojure git:(f5cfd24d) ✗

I think this should get reverted, and let user space calculate the package from the binary name.

Show
Ghadi Shayban added a comment - This is a bad patch that slipped through. The motivation linked above shows an nREPL excerpt that attempts to symbolize the package, basically:
(symbol (.getName (.getPackage TheClass)))
. The original code can derive the package name in the same way the JVM grabs it, from the binary name. (Take the prefix up until the last period.) The fix also introduces a deprecated call, and more work during classloading (which is critical path). As an aside, JDK9+ automatically defines a package upon class definition, derived from the binary name:
## JDK 8
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:8-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
:nada

## JDK 9
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:9-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
#object[java.lang.Package 0x10993713 "package user"]

## JDK 11
➜  clojure git:(f5cfd24d) ✗ docker run --rm -v $HOME/.m2:/m2 openjdk:11-slim java -jar /m2/repository/org/clojure/clojure/1.8.0/clojure-1.8.0.jar -e '(deftype T [])' -e '(or (.getPackage T) :nada)'
user.T
#object[java.lang.Package 0x6f3c660a "package user"]
➜  clojure git:(f5cfd24d) ✗
I think this should get reverted, and let user space calculate the package from the binary name.
Hide
Bozhidar Batsov added a comment -

> The original code can derive the package name in the same way the JVM grabs it, from the binary name. (Take the prefix up until the last period.)

I don't completely understand the workaround you're proposing. Can you elaborate on this?

> I think this should get reverted, and let user space calculate the package from the binary name.

If that's working on newer JDKs with no changes needed in the user space and in Clojure, that's fine by me. By now I had lost faith that was going to be fixed in Clojure anyways.

Show
Bozhidar Batsov added a comment - > The original code can derive the package name in the same way the JVM grabs it, from the binary name. (Take the prefix up until the last period.) I don't completely understand the workaround you're proposing. Can you elaborate on this? > I think this should get reverted, and let user space calculate the package from the binary name. If that's working on newer JDKs with no changes needed in the user space and in Clojure, that's fine by me. By now I had lost faith that was going to be fixed in Clojure anyways.
Hide
Ghadi Shayban added a comment -

https://github.com/clojure-emacs/orchard/blob/db6d8a7be853b52c9986c5af8d009639644bb390/src/orchard/java.clj#L157-L159

Took me a while to track it down, but instead of asking for the package name then grabbing the symbol, do this:

(defn package
  [^Class kls]
  (let [kls (.getName kls)
	idx (clojure.string/last-index-of kls \.)]
    (when (pos? idx)
      (subs kls 0 idx))))
Show
Ghadi Shayban added a comment - https://github.com/clojure-emacs/orchard/blob/db6d8a7be853b52c9986c5af8d009639644bb390/src/orchard/java.clj#L157-L159 Took me a while to track it down, but instead of asking for the package name then grabbing the symbol, do this:
(defn package
  [^Class kls]
  (let [kls (.getName kls)
	idx (clojure.string/last-index-of kls \.)]
    (when (pos? idx)
      (subs kls 0 idx))))
Hide
Alex Miller added a comment -

Patch reverted

Show
Alex Miller added a comment - Patch reverted

People

Vote (15)
Watch (3)

Dates

  • Created:
    Updated: