Clojure

Use qualified class names for return type hints of standard Clojure functions

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: None
  • Component/s: None
  • Patch:
    Code

Description

The attached patch converts all function return type hints to spell out the class name fully qualified. There are two reasons for doing this:

1. Simple names in return type hints cause the issue described in http://dev.clojure.org/jira/browse/CLJ-1232. That's usually not a problem with return type hints referring to java.lang-classes because those are always imported. However, using `ns-unmap` you can remove them. For example, after `(ns-unmap ns 'String)` in my namespace, `(.length (format "foo = %s") 1)` throws an IllegalArgumentException: Unable to resolve classname: String. By using fully-qualified class names, that problem goes away.

2. tools.analyzer (used by the Clojure lint tool Eastwood) crashes when encountering such a simple-named return type hint. So currently, I cannot lint parts of my project because there's code that calls `clojure.core/format`.

Activity

Hide
Alex Miller added a comment -

1. that seems like a pretty weird thing to do
2. sounds like an issue with tools.analyzer, not with Clojure?

Show
Alex Miller added a comment - 1. that seems like a pretty weird thing to do 2. sounds like an issue with tools.analyzer, not with Clojure?
Hide
Nicola Mometto added a comment - - edited

Just to clarify, tools.analyzer(.jvm) can analyze just fine forms in the form (defn x ^Class []) as long as Class is resolvable, whereas it will throw an exception if that function is then used in a namespace where that class is no longer resolvable, which is similar to what Clojure already does, except tools.analyzer.jvm will throw an exception even if the type hint is not used.

Since version 0.5.1 there's an handler that can be provided to change that behaviour, see https://github.com/clojure/tools.analyzer.jvm/blob/master/src/main/clojure/clojure/tools/analyzer/passes/jvm/validate.clj#L232

Show
Nicola Mometto added a comment - - edited Just to clarify, tools.analyzer(.jvm) can analyze just fine forms in the form (defn x ^Class []) as long as Class is resolvable, whereas it will throw an exception if that function is then used in a namespace where that class is no longer resolvable, which is similar to what Clojure already does, except tools.analyzer.jvm will throw an exception even if the type hint is not used. Since version 0.5.1 there's an handler that can be provided to change that behaviour, see https://github.com/clojure/tools.analyzer.jvm/blob/master/src/main/clojure/clojure/tools/analyzer/passes/jvm/validate.clj#L232
Hide
Nicola Mometto added a comment -

Now a comment regarding this ticket: the patch in this ticket is just a work-around for the issue exposed in http://dev.clojure.org/jira/browse/CLJ-1232, IMHO the correct move would be to actually recognize that issue as a bug rather than as an accepted "limitation" as Rich's comment seems to suggest so that a fix might be commited.

Show
Nicola Mometto added a comment - Now a comment regarding this ticket: the patch in this ticket is just a work-around for the issue exposed in http://dev.clojure.org/jira/browse/CLJ-1232, IMHO the correct move would be to actually recognize that issue as a bug rather than as an accepted "limitation" as Rich's comment seems to suggest so that a fix might be commited.
Hide
Tassilo Horn added a comment -

@Alex: 1. is not as weird as it sounds at first. For example, consider you have macros that generate complete APIs for something into some new namespace. Then it can make sense to use a real vanilla namespace, i.e., without referring clojure.core and importing java.lang. With 2. I side with Nicola and consider CLJ-1232 a bug.

@Nicola: Today I've used Eastwood (0.1.4) to lint my project. It crashed when it encountered this definition:

(defmacro error
  "Throws an exception with the given message and cause."
  ([msg]
     `(error ~msg nil))
  ([msg cause]
     `(throw (java.lang.Exception. ~msg ~cause))))

(defmacro errorf
  "Throws an exception with the given `msg` and `objs` passed to `format`.
  `msg` is a format string."
  [msg & objs]
  `(error (format ~msg ~@objs)))  ;; This is line 112 where the crash occurs

The message was:

Exception thrown during phase :analyze+eval of linting namespace funnyqt.tg-test
A function, macro, protocol method, var, etc. named clojure.core/format has been used here:
{:file "funnyqt/utils.clj",
 :end-column 19,
 :column 12,
 :line 112,
 :end-line 112}
Wherever it is defined, or where it is called, it has a type of String
This appears to be a Java class name with no package path.
Library tools.analyzer, on which Eastwood relies, cannot analyze such files.
If this definition is easy for you to change, we recommend you prepend it with
a full package path name, e.g. java.net.URI
Otherwise import the class by adding a line like this to your ns statement:
    (:import (java.net URI))

An exception was thrown while analyzing namespace funnyqt.tg-test 
Lint results may be incomplete.  If there are compilation errors in
your code, try fixing those.  If not, check above for info on the
exception.

So it seems it crashes because `format` has a `^String` return type hint. The namespace containing the `errorf` macro above has no modified ns-imports, i.e., all java.lang classes are imported there, too.

Show
Tassilo Horn added a comment - @Alex: 1. is not as weird as it sounds at first. For example, consider you have macros that generate complete APIs for something into some new namespace. Then it can make sense to use a real vanilla namespace, i.e., without referring clojure.core and importing java.lang. With 2. I side with Nicola and consider CLJ-1232 a bug. @Nicola: Today I've used Eastwood (0.1.4) to lint my project. It crashed when it encountered this definition:
(defmacro error
  "Throws an exception with the given message and cause."
  ([msg]
     `(error ~msg nil))
  ([msg cause]
     `(throw (java.lang.Exception. ~msg ~cause))))

(defmacro errorf
  "Throws an exception with the given `msg` and `objs` passed to `format`.
  `msg` is a format string."
  [msg & objs]
  `(error (format ~msg ~@objs)))  ;; This is line 112 where the crash occurs
The message was:
Exception thrown during phase :analyze+eval of linting namespace funnyqt.tg-test
A function, macro, protocol method, var, etc. named clojure.core/format has been used here:
{:file "funnyqt/utils.clj",
 :end-column 19,
 :column 12,
 :line 112,
 :end-line 112}
Wherever it is defined, or where it is called, it has a type of String
This appears to be a Java class name with no package path.
Library tools.analyzer, on which Eastwood relies, cannot analyze such files.
If this definition is easy for you to change, we recommend you prepend it with
a full package path name, e.g. java.net.URI
Otherwise import the class by adding a line like this to your ns statement:
    (:import (java.net URI))

An exception was thrown while analyzing namespace funnyqt.tg-test 
Lint results may be incomplete.  If there are compilation errors in
your code, try fixing those.  If not, check above for info on the
exception.
So it seems it crashes because `format` has a `^String` return type hint. The namespace containing the `errorf` macro above has no modified ns-imports, i.e., all java.lang classes are imported there, too.
Hide
Nicola Mometto added a comment -

Tassilo, since `errorf` is a macro, that error is probably caused at the expansion point of that macro in a namespace that unmaps 'String.
If that's not the case, please open a ticket in the eastwood repo

Show
Nicola Mometto added a comment - Tassilo, since `errorf` is a macro, that error is probably caused at the expansion point of that macro in a namespace that unmaps 'String. If that's not the case, please open a ticket in the eastwood repo
Hide
Tassilo Horn added a comment -

Nicola, you are correct. As I've explained above to Alex, I generate APIs in fresh namespaces that don't refer clojure.core and also ns-unmap all java.lang classes, and the generated code also contains `errorf`-forms.

Well, since `ns-unmap` is there, I think it's legit to use it. So that makes CLJ-1232 even more important. But until that gets fixed which requires a common agreement that it is indeed a bug, I'd be very happy if this patch could be accepted. I mean, when it cannot do any harm and doesn't obscure anything but helps at least one person, then why not do it?

Show
Tassilo Horn added a comment - Nicola, you are correct. As I've explained above to Alex, I generate APIs in fresh namespaces that don't refer clojure.core and also ns-unmap all java.lang classes, and the generated code also contains `errorf`-forms. Well, since `ns-unmap` is there, I think it's legit to use it. So that makes CLJ-1232 even more important. But until that gets fixed which requires a common agreement that it is indeed a bug, I'd be very happy if this patch could be accepted. I mean, when it cannot do any harm and doesn't obscure anything but helps at least one person, then why not do it?

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: