Clojure

Report errors on missing param list or return type of methods in gen-class and gen-interface

Details

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

Description

The following are invalid and should produce errors when invoked on gen-class or gen-interface:

(gen-interface :name clj1419.IFail :methods [[myMethod java.lang.String]])  ;; no params, throws error
(gen-interface :name clj1419.IFail :methods [[myMethod []]]) ;; no return type
(gen-interface :name clj1419.IFail :methods [[myMethod]])  ;; no params or return type

The first example throws an error. The second and third do not but will generate an invalid class, verify with:

(.getMethods clj1419.IFail)
ClassNotFoundException java.lang.  java.net.URLClassLoader$1.run (URLClassLoader.java:366)

Add checks to prevent these errors.

  1. 0001-CLJ-1419-default-to-void-return-type-in-gen-interfac.patch
    11/May/14 4:27 AM
    1 kB
    Nathan Zadoks
  2. 0001-CLJ-1419-map-nil-to-void-in-prim-class.patch
    11/May/14 4:27 AM
    0.7 kB
    Nathan Zadoks
  3. clj1419.clj
    11/May/14 3:45 AM
    0.1 kB
    Nathan Zadoks
  4. fail.log
    11/May/14 3:48 AM
    3 kB
    Nathan Zadoks

Activity

Hide
Nathan Zadoks added a comment -

I've implemented both fixes, and attached them as patches.

Show
Nathan Zadoks added a comment - I've implemented both fixes, and attached them as patches.
Hide
Nathan Zadoks added a comment -

I'd argue that the behaviour of asm-type is at fault here (it can output an invalid type name when passed a nil argument), so I prefer that fix over the purely symptomatic generate-interface fix.

Show
Nathan Zadoks added a comment - I'd argue that the behaviour of asm-type is at fault here (it can output an invalid type name when passed a nil argument), so I prefer that fix over the purely symptomatic generate-interface fix.
Hide
Andy Fingerhut added a comment -

Nathan, were you planning on submitting a signed Clojure Contributor's Agreement, or already have? Details here if you have not: http://clojure.org/contributing

Patches from non-contributors cannot be committed to Clojure.

Note: I cannot promise you that one of your patches will be accepted into Clojure if you sign a CA – only that it will not if you do not sign one.

Show
Andy Fingerhut added a comment - Nathan, were you planning on submitting a signed Clojure Contributor's Agreement, or already have? Details here if you have not: http://clojure.org/contributing Patches from non-contributors cannot be committed to Clojure. Note: I cannot promise you that one of your patches will be accepted into Clojure if you sign a CA – only that it will not if you do not sign one.
Hide
Alex Miller added a comment -

Please add an example of how this happens and the current error.

Show
Alex Miller added a comment - Please add an example of how this happens and the current error.
Hide
Nathan Zadoks added a comment -

Andy — Yep, I've read up on that. My CA will be underway to Rich soon. (filled in, signed, in an envelope, just need to await the arrival of those bloody international stamps…)

Alex Miller — Tahdah!

A demonstration of the issue, both attached and as a gist: https://gist.github.com/nathan7/3a7e3a09e458f1354cbb

Show
Nathan Zadoks added a comment - Andy — Yep, I've read up on that. My CA will be underway to Rich soon. (filled in, signed, in an envelope, just need to await the arrival of those bloody international stamps…) Alex Miller — Tahdah! A demonstration of the issue, both attached and as a gist: https://gist.github.com/nathan7/3a7e3a09e458f1354cbb
Hide
Nathan Zadoks added a comment -

and here's log of the compiler crash that results (also added to the gist now)

Show
Nathan Zadoks added a comment - and here's log of the compiler crash that results (also added to the gist now)
Hide
Nathan Zadoks added a comment -

Whoops, both of my patches were rather broken due to a misunderstanding on my side.
I forgot entirely that asm-type takes a symbol, not a string.
Modifying asm-type was definitely a bad idea, that check just looks whether it should defer to prim->class.
Adding nil to prim->class would work (and I've attached my patch for that too), but it's starting to look rather inelegant compared to just patching gen-interface.
(on a side note: I'm having a lot of fun exploring the Clojure codebase! thanks for that, humans!)

Show
Nathan Zadoks added a comment - Whoops, both of my patches were rather broken due to a misunderstanding on my side. I forgot entirely that asm-type takes a symbol, not a string. Modifying asm-type was definitely a bad idea, that check just looks whether it should defer to prim->class. Adding nil to prim->class would work (and I've attached my patch for that too), but it's starting to look rather inelegant compared to just patching gen-interface. (on a side note: I'm having a lot of fun exploring the Clojure codebase! thanks for that, humans!)
Hide
Alex Miller added a comment -

My reading of the docstring of gen-interface is that method declarations must specify a parameter list and a valid return type. I would expect all of these to be invalid:

(gen-interface :name clj1419.IFail :methods [[fail nil]])
(gen-interface :name clj1419.IFail :methods [[fail [] nil]])
(gen-interface :name clj1419.IFail :methods [[fail []]])

"nil" is not a valid type - you can use "void" for this purpose and this works fine:

(gen-interface :name clj1419.IFail :methods [[fail [] void]])

If this ticket is (as the title states) a request to allow omitting the return type or using "nil" as a return type, then I think the answer is no. If the ticket is a request to improve the error reporting of the failure cases above, then I think we can consider that but it will be very low priority.

Show
Alex Miller added a comment - My reading of the docstring of gen-interface is that method declarations must specify a parameter list and a valid return type. I would expect all of these to be invalid:
(gen-interface :name clj1419.IFail :methods [[fail nil]])
(gen-interface :name clj1419.IFail :methods [[fail [] nil]])
(gen-interface :name clj1419.IFail :methods [[fail []]])
"nil" is not a valid type - you can use "void" for this purpose and this works fine:
(gen-interface :name clj1419.IFail :methods [[fail [] void]])
If this ticket is (as the title states) a request to allow omitting the return type or using "nil" as a return type, then I think the answer is no. If the ticket is a request to improve the error reporting of the failure cases above, then I think we can consider that but it will be very low priority.
Hide
Nathan Zadoks added a comment -

The code seems to suggest otherwise though, seeing the explicit extra branch for pclasses being nil.
As much as I like PL trivia, I haven't run into `void` in Clojure anywhere else yet, and I'm surprised to see it here.
Maintaining the principle of least surprise seems preferable to pedantry about whether nil is a type: (= "nil" (str (type (.methodReturningVoid obj)))

Show
Nathan Zadoks added a comment - The code seems to suggest otherwise though, seeing the explicit extra branch for pclasses being nil. As much as I like PL trivia, I haven't run into `void` in Clojure anywhere else yet, and I'm surprised to see it here. Maintaining the principle of least surprise seems preferable to pedantry about whether nil is a type: (= "nil" (str (type (.methodReturningVoid obj)))
Hide
Alex Miller added a comment -

The two places to look for words to rely on are docstrings and the http://clojure.org/documentation pages. Implementation details are just that.

"nil" is not a type. "void" is a documented type identifier indicating the absence of a return value - http://clojure.org/java_interop#Java%20Interop-Aliases

Show
Alex Miller added a comment - The two places to look for words to rely on are docstrings and the http://clojure.org/documentation pages. Implementation details are just that. "nil" is not a type. "void" is a documented type identifier indicating the absence of a return value - http://clojure.org/java_interop#Java%20Interop-Aliases
Hide
Nathan Zadoks added a comment -

Okay. Better error-checking in asm-type then?

Show
Nathan Zadoks added a comment - Okay. Better error-checking in asm-type then?
Hide
Alex Miller added a comment -

I have updated the title and description based on my understanding of what this ticket should be, which is enhanced error-checking on the method specs for gen-class and gen-interface. I'm not sure if that's in asm-type or somewhere earlier.

Show
Alex Miller added a comment - I have updated the title and description based on my understanding of what this ticket should be, which is enhanced error-checking on the method specs for gen-class and gen-interface. I'm not sure if that's in asm-type or somewhere earlier.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated: