Clojure

Functions with non-qualified return type hints force import of hinted classes when called from other namespace

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5, Release 1.6, Release 1.7
  • Fix Version/s: Release 1.8
  • Component/s: None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

You can add a type hint to function arglists to indicate the return type of a function like so.

user> (import '(java.util List))
java.util.List
user> (defn linkedlist ^List [] (java.util.LinkedList.))
#'user/linkedlist
user> (.size (linkedlist))
0

The problem is that now when I call `linkedlist` exactly as above from another namespace, I'll get an exception because java.util.List is not imported in there.

user> (in-ns 'user2)
#<Namespace user2>
user2> (refer 'user)
nil
user2> (.size (linkedlist))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: List, compiling:(NO_SOURCE_PATH:1:1)
user2> (import '(java.util List)) ;; Too bad, need to import List here, too.
java.util.List
user2> (.size (linkedlist))
0

There are two workarounds: You can import the hinted type also in the calling namespace, or you always use fully qualified class names for return type hints. Clearly, the latter is preferable.

Approach: Resolve the tags in the defn macro.

Patch: 0001-CLJ-1232-auto-qualify-arglists-class-names.patch

Screened by: Fogus

Alternate approach: Make the compiler resolve the return tags when necessary (tag is not a string, primitive tag (^long) or array tag (^longs)) and update the Var's :arglist appropriately. Patch: 0001-auto-qualify-arglists-class-names-v4.patch. Note that this patch had problems with Hystrix, which was using non-conformant arglists - Hystrix has since been patched.

Activity

Hide
Alex Miller added a comment -

Moving back to screened as fogus screened the preferred (by Rich) version.

Show
Alex Miller added a comment - Moving back to screened as fogus screened the preferred (by Rich) version.
Hide
Fogus added a comment -

It would be better if the description said the word "preferred," but based on my reading resolving the tags in defn is the winner (0001-CLJ-1232-auto-qualify-arglists-class-names.patch). Thankfully that's the one that I screened as I agree that non-trivial changes to the compiler are to be avoided. That said, of course that patch does modify the compiler but since it's a change that pulls out some code into a useful method I let it through. To my mind that was a trivial (and justifiable change). Of course, if we want to avoid any compiler changes then this patch would have to be reworked.

Show
Fogus added a comment - It would be better if the description said the word "preferred," but based on my reading resolving the tags in defn is the winner (0001-CLJ-1232-auto-qualify-arglists-class-names.patch). Thankfully that's the one that I screened as I agree that non-trivial changes to the compiler are to be avoided. That said, of course that patch does modify the compiler but since it's a change that pulls out some code into a useful method I let it through. To my mind that was a trivial (and justifiable change). Of course, if we want to avoid any compiler changes then this patch would have to be reworked.
Hide
Nicola Mometto added a comment -

Alex Miller Reading Fogus' last comment, it seems to me that he actually screened the current patch (the one who resolves tags in defn rather than in the compiler) "Applied the 0001-CLJ-1232-auto-qualify-arglists-class-names.patch to master [..]"

Show
Nicola Mometto added a comment - Alex Miller Reading Fogus' last comment, it seems to me that he actually screened the current patch (the one who resolves tags in defn rather than in the compiler) "Applied the 0001-CLJ-1232-auto-qualify-arglists-class-names.patch to master [..]"
Hide
Alex Miller added a comment -

Nicola and Fogus, would appreciate an eye as to whether I just made the proper alterations in the description.

Show
Alex Miller added a comment - Nicola and Fogus, would appreciate an eye as to whether I just made the proper alterations in the description.
Hide
Alex Miller added a comment -

Made preferred approach clear in the description and moved back to vetted. I believe the other ticket was screened by Fogus so this needs to be re-screened with the preferred approach ticket.

Show
Alex Miller added a comment - Made preferred approach clear in the description and moved back to vetted. I believe the other ticket was screened by Fogus so this needs to be re-screened with the preferred approach ticket.
Hide
Rich Hickey added a comment -

I'd rather not change the compiler and it seems hystrix was broken. Also please make it clear what the single strategy and patch is at the top of the ticket.

Show
Rich Hickey added a comment - I'd rather not change the compiler and it seems hystrix was broken. Also please make it clear what the single strategy and patch is at the top of the ticket.
Hide
Fogus added a comment -

Applied the 0001-CLJ-1232-auto-qualify-arglists-class-names.patch to master (1.8) and ran through the example scenario to check the veracity of the change. Additionally, I ran a modified snippet using def and verified that it too worked. Finally, after checking the code it seems reasonable in implementation and scope.

Show
Fogus added a comment - Applied the 0001-CLJ-1232-auto-qualify-arglists-class-names.patch to master (1.8) and ran through the example scenario to check the veracity of the change. Additionally, I ran a modified snippet using def and verified that it too worked. Finally, after checking the code it seems reasonable in implementation and scope.
Hide
Matt Jacobs added a comment - - edited

I've released hystrix-clj 1.4.14, with a fix for https://github.com/Netflix/Hystrix/issues/831: https://github.com/Netflix/Hystrix/releases/tag/v1.4.14. If anything else is needed on the Hystrix side, please open another issue.

Show
Matt Jacobs added a comment - - edited I've released hystrix-clj 1.4.14, with a fix for https://github.com/Netflix/Hystrix/issues/831: https://github.com/Netflix/Hystrix/releases/tag/v1.4.14. If anything else is needed on the Hystrix side, please open another issue.
Hide
Nicola Mometto added a comment -

patch updated

Show
Nicola Mometto added a comment - patch updated
Hide
Stuart Halloway added a comment -

Can somebody with context update this patch to apply to latest master?

Show
Stuart Halloway added a comment - Can somebody with context update this patch to apply to latest master?
Hide
Stuart Halloway added a comment -

Opened https://github.com/Netflix/Hystrix/issues/831 to see what is up with hystrix.

Show
Stuart Halloway added a comment - Opened https://github.com/Netflix/Hystrix/issues/831 to see what is up with hystrix.
Hide
Alex Miller added a comment -

ah, yes. sorry.

Show
Alex Miller added a comment - ah, yes. sorry.
Hide
Nicola Mometto added a comment -

test.generative uses non-standard :tag, not :arglists

Show
Nicola Mometto added a comment - test.generative uses non-standard :tag, not :arglists
Hide
Alex Miller added a comment -

test.generative uses non-standard arglists too. I haven't looked at the patch, but if it's sensitive to that, it's probably not good enough.

Show
Alex Miller added a comment - test.generative uses non-standard arglists too. I haven't looked at the patch, but if it's sensitive to that, it's probably not good enough.
Hide
Michael Blume added a comment -

Possibly just ask Hystrix not to do that?

Show
Michael Blume added a comment - Possibly just ask Hystrix not to do that?
Hide
Nicola Mometto added a comment -

During analysis the compiler understands only arglists in the form of (quote ([..]*)) (see https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L557-L558), hystrix emits arglists in the form of (list (quote [..])*).

Not sure what to do about this.

Show
Nicola Mometto added a comment - During analysis the compiler understands only arglists in the form of (quote ([..]*)) (see https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L557-L558), hystrix emits arglists in the form of (list (quote [..])*). Not sure what to do about this.
Hide
Michael Blume added a comment -

https://github.com/MichaelBlume/hystrix-demo

passes lein check with 1.7 beta3, fails with v3 patch applied

Show
Michael Blume added a comment - https://github.com/MichaelBlume/hystrix-demo passes lein check with 1.7 beta3, fails with v3 patch applied
Hide
Michael Blume added a comment -

I'm seeing an odd failure with this patch and hystrix defcommands, will post a small reproduction shortly

Show
Michael Blume added a comment - I'm seeing an odd failure with this patch and hystrix defcommands, will post a small reproduction shortly
Hide
Nicola Mometto added a comment -

Patch 0001-auto-qualify-arglists-class-names-v2.patch avoids doing unnecessary lookups (dotted names, special tags (primitive tags, array tags)) and adds a testcase

Show
Nicola Mometto added a comment - Patch 0001-auto-qualify-arglists-class-names-v2.patch avoids doing unnecessary lookups (dotted names, special tags (primitive tags, array tags)) and adds a testcase
Hide
Alex Miller added a comment -

Nicola, in this:

if (tag != null &&
                        !(tag instanceof String) &&
                        primClass((Symbol)tag) == null &&
                        !tagClass((Symbol) tag).getName().startsWith("["))
                        {
                            argvec = (IPersistentVector)((IObj)argvec).withMeta(RT.map(RT.TAG_KEY, Symbol.intern(tagClass((Symbol)tag).getName())));
                        }

doesn't tagClass already handle most of these cases properly already? Can this be simplified? Is there an optimization case in avoiding lookup for a dotted name?

Show
Alex Miller added a comment - Nicola, in this:
if (tag != null &&
                        !(tag instanceof String) &&
                        primClass((Symbol)tag) == null &&
                        !tagClass((Symbol) tag).getName().startsWith("["))
                        {
                            argvec = (IPersistentVector)((IObj)argvec).withMeta(RT.map(RT.TAG_KEY, Symbol.intern(tagClass((Symbol)tag).getName())));
                        }
doesn't tagClass already handle most of these cases properly already? Can this be simplified? Is there an optimization case in avoiding lookup for a dotted name?
Hide
Rich Hickey added a comment -

Please work on the simplest patch that resolves the names

Show
Rich Hickey added a comment - Please work on the simplest patch that resolves the names
Hide
Tassilo Horn added a comment -

I wasn't aware of the fact that you can put it on the var's name. That's not documented at http://clojure.org/java_interop#Java Interop-Type Hints. But IMHO the documented version with putting the tag on the argument vector is more general since it supports different return type hints for the different arity version. In any case, if both forms are permitted then they should be equivalent in the case the function has only one arity.

Show
Tassilo Horn added a comment - I wasn't aware of the fact that you can put it on the var's name. That's not documented at http://clojure.org/java_interop#Java Interop-Type Hints. But IMHO the documented version with putting the tag on the argument vector is more general since it supports different return type hints for the different arity version. In any case, if both forms are permitted then they should be equivalent in the case the function has only one arity.
Hide
Andy Fingerhut added a comment -

Tassilo (or anyone), is there a reason to prefer putting the tag on the argument vector in your example? It seems that putting it on the Var name instead avoids this issue:

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (import '(java.util List))
java.util.List
user=> (defn ^List linkedlist [] (java.util.LinkedList.))
#'user/linkedlist
user=> (.size (linkedlist))
0
user=> (ns user2)
nil
user2=> (refer 'user)
nil
user2=> (.size (linkedlist))
0

I suppose that only allows a single type tag, rather than an independent one for each arity.

Show
Andy Fingerhut added a comment - Tassilo (or anyone), is there a reason to prefer putting the tag on the argument vector in your example? It seems that putting it on the Var name instead avoids this issue:
user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (import '(java.util List))
java.util.List
user=> (defn ^List linkedlist [] (java.util.LinkedList.))
#'user/linkedlist
user=> (.size (linkedlist))
0
user=> (ns user2)
nil
user2=> (refer 'user)
nil
user2=> (.size (linkedlist))
0
I suppose that only allows a single type tag, rather than an independent one for each arity.
Hide
Andy Fingerhut added a comment -

Created ticket CLJ-1543 for the issue raised in my comment earlier on 30 Sep 2014.

Show
Andy Fingerhut added a comment - Created ticket CLJ-1543 for the issue raised in my comment earlier on 30 Sep 2014.
Hide
Alex Miller added a comment -

Andy, can you file that as a separate ticket?

Show
Alex Miller added a comment - Andy, can you file that as a separate ticket?
Hide
Andy Fingerhut added a comment - - edited

Not sure whether the root cause of this behavior is the same as the example in the description or not, but seems a little weird that even for fully qualified Java class names hinting the arg vector, it makes a difference whether it is done with defn or def:

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3
Show
Andy Fingerhut added a comment - - edited Not sure whether the root cause of this behavior is the same as the example in the description or not, but seems a little weird that even for fully qualified Java class names hinting the arg vector, it makes a difference whether it is done with defn or def:
Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3
Hide
Tassilo Horn added a comment - - edited

For what it's worth, I'd prefer the first patch because the second doesn't help in situations where the caller lives in a namespace where the called function's return type hinted class is `ns-unmap`-ed. And there a good reasons for doing that. For example, Process is a java.lang class and Process is a pretty generic name. So in some namespace, I want to define my own Process deftype or defrecord. Without unmapping 'Process first to get rid of the java.lang.Process auto-import, I'd get an exception:

user> (deftype Process [])
IllegalStateException Process already refers to: class java.lang.Process in namespace: user  clojure.lang.Namespace.referenceClass (Namespace.java:140)

Now when I call some function from some library that has a `^Process` return type hint (meaning java.lang.Process there), I get the same exception as in my original report.

I can even get into troubles when only using standard Clojure functions because those have `^String` and `^Class` type hints. IMO, Class is also a pretty generic name I should be able to name my custom deftype/defrecord. And I might also want to have a custom String type/record in my astrophysics system.

Show
Tassilo Horn added a comment - - edited For what it's worth, I'd prefer the first patch because the second doesn't help in situations where the caller lives in a namespace where the called function's return type hinted class is `ns-unmap`-ed. And there a good reasons for doing that. For example, Process is a java.lang class and Process is a pretty generic name. So in some namespace, I want to define my own Process deftype or defrecord. Without unmapping 'Process first to get rid of the java.lang.Process auto-import, I'd get an exception:
user> (deftype Process [])
IllegalStateException Process already refers to: class java.lang.Process in namespace: user  clojure.lang.Namespace.referenceClass (Namespace.java:140)
Now when I call some function from some library that has a `^Process` return type hint (meaning java.lang.Process there), I get the same exception as in my original report. I can even get into troubles when only using standard Clojure functions because those have `^String` and `^Class` type hints. IMO, Class is also a pretty generic name I should be able to name my custom deftype/defrecord. And I might also want to have a custom String type/record in my astrophysics system.
Hide
Nicola Mometto added a comment -

Attached two patches implementing two different solutions:

  • 0001-auto-qualify-arglists-class-names.patch makes the compiler automatically qualify all the tags in the :arglists
  • 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch makes the compiler throw an exception for all public defs whose return tag is a symbol representing a non-qualified class that is not in the auto-import list (approach proposed in IRC by Alex Miller)
Show
Nicola Mometto added a comment - Attached two patches implementing two different solutions:
  • 0001-auto-qualify-arglists-class-names.patch makes the compiler automatically qualify all the tags in the :arglists
  • 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch makes the compiler throw an exception for all public defs whose return tag is a symbol representing a non-qualified class that is not in the auto-import list (approach proposed in IRC by Alex Miller)
Hide
Max Penet added a comment -

Same here, I was bit by this in the past. The current behavior is clearly counterintuitive.

Show
Max Penet added a comment - Same here, I was bit by this in the past. The current behavior is clearly counterintuitive.
Hide
Alexander Kiel added a comment -

I'm with Nicola here. I also think that defn should resolve the type hint according the imports of the namespace defn is used in.

Show
Alexander Kiel added a comment - I'm with Nicola here. I also think that defn should resolve the type hint according the imports of the namespace defn is used in.
Hide
Nicola Mometto added a comment - - edited

I don't understand why we should enforce this complexity to the user.
Why can't we just make the Compiler (or even defn itself) update all the arglists tags with properly resolved ones? (that's what I'm doing in tools.analyzer.jvm)

Show
Nicola Mometto added a comment - - edited I don't understand why we should enforce this complexity to the user. Why can't we just make the Compiler (or even defn itself) update all the arglists tags with properly resolved ones? (that's what I'm doing in tools.analyzer.jvm)
Hide
Andy Fingerhut added a comment -

I would suggest something like the following for a documentation change, after this part of the text on the page Alex links in the previous comment:

For function return values, the type hint can be placed before the arguments vector:

(defn hinted
(^String [])
(^Integer [a])
(^java.util.List [a & args]))

-> #user/hinted

If the return value type hint is for a class that is outside of java.lang, which is the part auto-imported by Clojure, then it must be a fully qualified class name, e.g. java.util.List, not List.

Show
Andy Fingerhut added a comment - I would suggest something like the following for a documentation change, after this part of the text on the page Alex links in the previous comment: For function return values, the type hint can be placed before the arguments vector: (defn hinted (^String []) (^Integer [a]) (^java.util.List [a & args])) -> #user/hinted If the return value type hint is for a class that is outside of java.lang, which is the part auto-imported by Clojure, then it must be a fully qualified class name, e.g. java.util.List, not List.
Hide
Alex Miller added a comment -

Based on Rich's comment, this ticket should probably morph into an enhancement request on documentation, probably on http://clojure.org/java_interop#Java Interop-Type Hints.

Show
Alex Miller added a comment - Based on Rich's comment, this ticket should probably morph into an enhancement request on documentation, probably on http://clojure.org/java_interop#Java Interop-Type Hints.
Hide
Rich Hickey added a comment -

Type hints on function params are only consumed by the function definition, i.e. in the same module as the import/alias. Type hints on returns are just metadata, they don't get 'compiled' and if the metadata is not useful to consumers in other namespaces, it's not a useful hint. So, if it's not a type in the auto-imported set (java.lang), it should be fully qualified.

Show
Rich Hickey added a comment - Type hints on function params are only consumed by the function definition, i.e. in the same module as the import/alias. Type hints on returns are just metadata, they don't get 'compiled' and if the metadata is not useful to consumers in other namespaces, it's not a useful hint. So, if it's not a type in the auto-imported set (java.lang), it should be fully qualified.
Hide
Tassilo Horn added a comment -

Hi Andy. Tassilo here, not Nicola. But yes, the example should work as-is. When I'm allowed to use type hints with simple imported class names for arguments, then doing so for return values should work, too.

Show
Tassilo Horn added a comment - Hi Andy. Tassilo here, not Nicola. But yes, the example should work as-is. When I'm allowed to use type hints with simple imported class names for arguments, then doing so for return values should work, too.
Hide
Andy Fingerhut added a comment -

To make sure I understand, Nicola, in this ticket you are asking that the Clojure compiler change behavior so that the sample code works correctly with no exceptions, the same way as it would work correctly without exceptions if one of the workarounds were used?

Show
Andy Fingerhut added a comment - To make sure I understand, Nicola, in this ticket you are asking that the Clojure compiler change behavior so that the sample code works correctly with no exceptions, the same way as it would work correctly without exceptions if one of the workarounds were used?

People

Vote (8)
Watch (7)

Dates

  • Created:
    Updated: