Clojure

Hinting the arg vector of a primitive-taking fn with a non-primitive type results in AbstractMethodError when invoked

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.3
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Hinting the arg vector of a primitive-taking fn with a non-primitive type results in AbstractMethodError when invoked.

user=> (defn f1 ^String [^String s] s)
#'user/f1
user=> (f1 "foo")
"foo"
user=> (defn f2 ^long [^String s ^long i] i)
#'user/f2
user=> (f2 "foo" 1)
1
user=> (defn f3 ^String [^String s ^long i] s)                                       
#'user/f3
user=> (f3 "foo" 1)
AbstractMethodError user$f3.invokePrim(Ljava/lang/Object;J)Ljava/lang/Object;  user/eval8 (NO_SOURCE_FILE:6)

Solution: Add check to emit invokePrim with return type of double or long if appropriate, otherwise do prior behavior of emitting return type of Object.

Patch: The {{CLJ-850-conform-to-invokePrim.diff}} patch is constructed per Rich's feedback, and appears good to me [Stu].

  1. CLJ-850.patch
    18/Oct/11 2:54 PM
    2 kB
    Ben Smith-Mannschott
  2. CLJ-850-conform-to-invokePrim.diff
    19/Dec/12 3:41 PM
    2 kB
    Ghadi Shayban
  3. CLJ-850-test.patch
    15/Oct/11 11:54 AM
    2 kB
    Ben Smith-Mannschott
  4. clj-850-type-hinted-fn-abstractmethoderror-patch4.txt
    01/Nov/12 7:22 PM
    4 kB
    Andy Fingerhut

Activity

Alexander Taggart made changes -
Field Original Value New Value
Summary Hinting the arg vector of a primitive-taking fn with a non-primitive type results in AbstractMethodError Hinting the arg vector of a primitive-taking fn with a non-primitive type results in AbstractMethodError when invoked
Hide
Ben Smith-Mannschott added a comment -

CLJ-850-test.patch added.

Show
Ben Smith-Mannschott added a comment - CLJ-850-test.patch added.
Ben Smith-Mannschott made changes -
Attachment CLJ-850-test.patch [ 10400 ]
Hide
Ben Smith-Mannschott added a comment - - edited

When the compiler tries to generates the call to the correct overload of invokePrim, it's failing to take the return type into account. I should be calling invokePrim(Ljava/lang/Object;J)J;.

XXX this is where I got myself confused. The invokePrim overload it's trying to invoke is the correct one. But, that apparently is no the one that's being generated. Sorry for the noise.

Show
Ben Smith-Mannschott added a comment - - edited When the compiler tries to generates the call to the correct overload of invokePrim, it's failing to take the return type into account. I should be calling invokePrim(Ljava/lang/Object;J)J;. XXX this is where I got myself confused. The invokePrim overload it's trying to invoke is the correct one. But, that apparently is no the one that's being generated. Sorry for the noise.
Hide
Ben Smith-Mannschott added a comment - - edited

Here's what I think I'm seeing:

HostExpr.Parse.parse() loses track of the return type, in the final else branch where method calls are handled. This is because tagOf(form), where form is something like: (. foo invokePrim 1) returns nil. (The form itself doesn't have a :tag, but I believe foo does, though that's the name of the appropriate invokePrim interface (i.e. IFn$OLL).

new InstanceMethodExpr(...) then gets constructed with tag==null, at which point we've already lost sine InstanceMethodExpr can't correctly consider overloading on the result type if it doesn't know what it is.

It's not yet clear to me how I can get InstanceMethodExpr to consider the return type, if it knew it...

Show
Ben Smith-Mannschott added a comment - - edited Here's what I think I'm seeing: HostExpr.Parse.parse() loses track of the return type, in the final else branch where method calls are handled. This is because tagOf(form), where form is something like: (. foo invokePrim 1) returns nil. (The form itself doesn't have a :tag, but I believe foo does, though that's the name of the appropriate invokePrim interface (i.e. IFn$OLL). new InstanceMethodExpr(...) then gets constructed with tag==null, at which point we've already lost sine InstanceMethodExpr can't correctly consider overloading on the result type if it doesn't know what it is. It's not yet clear to me how I can get InstanceMethodExpr to consider the return type, if it knew it...
Hide
Ben Smith-Mannschott added a comment -

There are two things going on here. I'm not sure which is the error.

It looks like the return type of the generated invokePrim method is too specific. It's generated as returning String, though the IFn$LO interface specifies returning Object.

The caller attempts to call invokePrim returning Object, which is what the interface IFn$LO specifies, but this doesn't work because methodSL doesn't actually implement that method. Instead it implements an overload returning String.

  1. methodSL.invokePrim is declared as (long)->String
  2. methodSL.invoke does invokeinterface with the correct return type WRT methodSL, but the wrong return type WRT the IFn$LO interface.
  3. callSL.invoke does invokeinterface with the wrong return type WRT methodSL, but the correct return type WRT IFn$LO. (This is the failure we observe in the clj-850 unit test.)
(defn methodSL  ^String [^long i] (str i))
<<1>> public final java.lang.String invokePrim(long);  <<1>>
      Code:
       0:   getstatic   #25; 
            //Field const__0:Lclojure/lang/Var;
       3:   invokevirtual   #34; 
            //Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6:   checkcast   #36; 
            //class clojure/lang/IFn
       9:   lload_1
       10:  invokestatic    #42; 
            //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       13:  invokeinterface #46,  2; 
            //InterfaceMethod clojure/lang/IFn.invoke:(Ljava/lang/Object;)Ljava/lang/Object;
       18:  checkcast   #48; 
            //class java/lang/String
       21:  areturn
      public java.lang.Object invoke(java.lang.Object);
      Code:
       0:   aload_0
       1:   aload_1
       2:   checkcast   #54; 
            //class java/lang/Number
       5:   invokestatic    #58; 
            //Method clojure/lang/RT.longCast:(Ljava/lang/Object;)J
<<2>>  8:   invokeinterface #60,  3; 
            //InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/String;
       13:  areturn
(defn callSL ^String [] (methodSL 42))
    public java.lang.Object invoke();
      Code:
       0:   getstatic   #25; 
            //Field const__0:Lclojure/lang/Var;
       3:   invokevirtual   #43; 
            //Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6:   checkcast   #45; 
            //class clojure/lang/IFn$LO
       9:   ldc2_w  #26; 
            //long 42l
<<3>>  12:  invokeinterface #49,  3; 
            //InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
       17:  areturn
Show
Ben Smith-Mannschott added a comment - There are two things going on here. I'm not sure which is the error. It looks like the return type of the generated invokePrim method is too specific. It's generated as returning String, though the IFn$LO interface specifies returning Object. The caller attempts to call invokePrim returning Object, which is what the interface IFn$LO specifies, but this doesn't work because methodSL doesn't actually implement that method. Instead it implements an overload returning String.
  1. methodSL.invokePrim is declared as (long)->String
  2. methodSL.invoke does invokeinterface with the correct return type WRT methodSL, but the wrong return type WRT the IFn$LO interface.
  3. callSL.invoke does invokeinterface with the wrong return type WRT methodSL, but the correct return type WRT IFn$LO. (This is the failure we observe in the clj-850 unit test.)
(defn methodSL  ^String [^long i] (str i))
<<1>> public final java.lang.String invokePrim(long);  <<1>>
      Code:
       0:   getstatic   #25; 
            //Field const__0:Lclojure/lang/Var;
       3:   invokevirtual   #34; 
            //Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6:   checkcast   #36; 
            //class clojure/lang/IFn
       9:   lload_1
       10:  invokestatic    #42; 
            //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
       13:  invokeinterface #46,  2; 
            //InterfaceMethod clojure/lang/IFn.invoke:(Ljava/lang/Object;)Ljava/lang/Object;
       18:  checkcast   #48; 
            //class java/lang/String
       21:  areturn
      public java.lang.Object invoke(java.lang.Object);
      Code:
       0:   aload_0
       1:   aload_1
       2:   checkcast   #54; 
            //class java/lang/Number
       5:   invokestatic    #58; 
            //Method clojure/lang/RT.longCast:(Ljava/lang/Object;)J
<<2>>  8:   invokeinterface #60,  3; 
            //InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/String;
       13:  areturn
(defn callSL ^String [] (methodSL 42))
    public java.lang.Object invoke();
      Code:
       0:   getstatic   #25; 
            //Field const__0:Lclojure/lang/Var;
       3:   invokevirtual   #43; 
            //Method clojure/lang/Var.getRawRoot:()Ljava/lang/Object;
       6:   checkcast   #45; 
            //class clojure/lang/IFn$LO
       9:   ldc2_w  #26; 
            //long 42l
<<3>>  12:  invokeinterface #49,  3; 
            //InterfaceMethod clojure/lang/IFn$LO.invokePrim:(J)Ljava/lang/Object;
       17:  areturn
Hide
Ben Smith-Mannschott added a comment -

Given P is some primitive type, O is type Object, and R some subclass of Object:

When Clojure generates a R invokePrim(P x), it also generates a Object invoke(Object x), which delegates to R invokePrim(P x).

R invokePrim(P x) overloads, but does not override the method of the corresponding Fn$PO interface.

If Clojure were to generate an additional O invokePrim(P x) which delegates to R invokePrim(P x), it would satisfy the requirements of the Fn$PO interface, and should fix this issue.

Show
Ben Smith-Mannschott added a comment - Given P is some primitive type, O is type Object, and R some subclass of Object: When Clojure generates a R invokePrim(P x), it also generates a Object invoke(Object x), which delegates to R invokePrim(P x). R invokePrim(P x) overloads, but does not override the method of the corresponding Fn$PO interface. If Clojure were to generate an additional O invokePrim(P x) which delegates to R invokePrim(P x), it would satisfy the requirements of the Fn$PO interface, and should fix this issue.
Hide
Ben Smith-Mannschott added a comment -

CLJ-850.patch fixes the issue.

I consider this patch to be pretty hackish and hope that there's a cleaner way of addressing CLJ-850. This is the first time I've tried to understand (much less change) the Clojure compiler, so don't expect genius.

Show
Ben Smith-Mannschott added a comment - CLJ-850.patch fixes the issue. I consider this patch to be pretty hackish and hope that there's a cleaner way of addressing CLJ-850. This is the first time I've tried to understand (much less change) the Clojure compiler, so don't expect genius.
Ben Smith-Mannschott made changes -
Attachment CLJ-850.patch [ 10411 ]
Hide
Ben Smith-Mannschott added a comment -

The patch lies slightly:

Clojure needs to generate an additional O invokePrim(P x) method to
satisfy the interface. This also delegates to R invokePrim(P x).

It turns out that what I'm actually doing is generating a R invokePrim(P x) which is a copy of O invokePrim(P x), instead of delgating to O invokePrim(P x). This works, but the resulting class file would be smaller if the patch actually did what it says it does.

Show
Ben Smith-Mannschott added a comment - The patch lies slightly:
Clojure needs to generate an additional O invokePrim(P x) method to satisfy the interface. This also delegates to R invokePrim(P x).
It turns out that what I'm actually doing is generating a R invokePrim(P x) which is a copy of O invokePrim(P x), instead of delgating to O invokePrim(P x). This works, but the resulting class file would be smaller if the patch actually did what it says it does.
Ben Smith-Mannschott made changes -
Patch Code and Test [ 10002 ]
Hide
Andy Fingerhut added a comment -

clj-850-type-hinted-fn-abstractmethoderror-patch2.txt is identical to Ben's two patches combined into one, with the small modification that the new tests are added to metadata.clj instead of creating a new test file. The patch applies cleanly to latest master as of Feb 27, 2012. One of the new tests does fail without the change to the compiler, and succeeds with it. I can't vouch for the correctness of the change myself, not knowing enough about the compiler internals to judge.

Show
Andy Fingerhut added a comment - clj-850-type-hinted-fn-abstractmethoderror-patch2.txt is identical to Ben's two patches combined into one, with the small modification that the new tests are added to metadata.clj instead of creating a new test file. The patch applies cleanly to latest master as of Feb 27, 2012. One of the new tests does fail without the change to the compiler, and succeeds with it. I can't vouch for the correctness of the change myself, not knowing enough about the compiler internals to judge.
Andy Fingerhut made changes -
Attachment clj-850-type-hinted-fn-abstractmethoderror-patch2.txt [ 10970 ]
Hide
Andy Fingerhut added a comment -

Same comments as made on Feb 27, 2012, except the patch clj-850-type-hinted-fn-abstractmethoderror-patch3.txt applies cleanly to latest master as of Mar 23, 2012. Updated because previous patch (now removed) no longer applied cleanly. git patches often fail to apply if context lines near changes are modified.

Show
Andy Fingerhut added a comment - Same comments as made on Feb 27, 2012, except the patch clj-850-type-hinted-fn-abstractmethoderror-patch3.txt applies cleanly to latest master as of Mar 23, 2012. Updated because previous patch (now removed) no longer applied cleanly. git patches often fail to apply if context lines near changes are modified.
Andy Fingerhut made changes -
Attachment clj-850-type-hinted-fn-abstractmethoderror-patch3.txt [ 11004 ]
Andy Fingerhut made changes -
Attachment clj-850-type-hinted-fn-abstractmethoderror-patch2.txt [ 10970 ]
Hide
Rich Hickey added a comment -

We don't support sigs taking prims and returning anything other than prim or Object. Overloading on return value only is a bad idea (and forbidden in Java). The return type of the generated method should be Object, and the String return hint should be used only as a hint.

Show
Rich Hickey added a comment - We don't support sigs taking prims and returning anything other than prim or Object. Overloading on return value only is a bad idea (and forbidden in Java). The return type of the generated method should be Object, and the String return hint should be used only as a hint.
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Rich Hickey made changes -
Approval Incomplete [ 10006 ]
Hide
Andy Fingerhut added a comment -

clj-850-type-hinted-fn-abstractmethoderror-patch4.txt dated Nov 1 2012 is same as Ben Smith-Mannschott's CLJ-850.patch and CLJ-850-test.patch, except it has been combined into one patch and does not create a new test source file.

Show
Andy Fingerhut added a comment - clj-850-type-hinted-fn-abstractmethoderror-patch4.txt dated Nov 1 2012 is same as Ben Smith-Mannschott's CLJ-850.patch and CLJ-850-test.patch, except it has been combined into one patch and does not create a new test source file.
Andy Fingerhut made changes -
Andy Fingerhut made changes -
Attachment clj-850-type-hinted-fn-abstractmethoderror-patch3.txt [ 11004 ]
Hide
Mike Anderson added a comment - - edited

+10 for solving this issue: it keeps biting me in 1.4 and wouuld love to see in 1.5

I'm not familiar with the Clojure compiler internals, but looking at the approach, shouldn't we produce a primitive method with a different name (since Java doesn't support overloading on return types as Rich correctly points out). Also I think there should be 4 methods:

R invokePrimExact(P x) - the actual method, used when compiler can infer
R invokePrimExact(O x) - delegates, used when compiler can't infer type of x
Object invokePrim(P x) - primitive method, conforms to IFn$PO interface, delegates
Object invoke(Object x) - general method, delegates

I think this solves all the important cases?

Show
Mike Anderson added a comment - - edited +10 for solving this issue: it keeps biting me in 1.4 and wouuld love to see in 1.5 I'm not familiar with the Clojure compiler internals, but looking at the approach, shouldn't we produce a primitive method with a different name (since Java doesn't support overloading on return types as Rich correctly points out). Also I think there should be 4 methods: R invokePrimExact(P x) - the actual method, used when compiler can infer R invokePrimExact(O x) - delegates, used when compiler can't infer type of x Object invokePrim(P x) - primitive method, conforms to IFn$PO interface, delegates Object invoke(Object x) - general method, delegates I think this solves all the important cases?
Hide
Rich Hickey added a comment -

Still no patch incorporating my feedback, afaict. Pushing to next release.

Show
Rich Hickey added a comment - Still no patch incorporating my feedback, afaict. Pushing to next release.
Rich Hickey made changes -
Fix Version/s Release 1.5 [ 10150 ]
Hide
Ghadi Shayban added a comment - - edited

Does this new patch address the issue and concerns? (This incorporates Ben's tests from the previous patch, wasn't sure how to attribute him on that hunk) CLJ-850-conform-to-invokePrim.diff

Show
Ghadi Shayban added a comment - - edited Does this new patch address the issue and concerns? (This incorporates Ben's tests from the previous patch, wasn't sure how to attribute him on that hunk) CLJ-850-conform-to-invokePrim.diff
Ghadi Shayban made changes -
Attachment CLJ-850-conform-to-invokePrim.diff [ 11769 ]
Hide
Andy Fingerhut added a comment -

Presumptuously changing state from Incomplete back to Vetted after Ghadi Shayban added the patch CLJ-850-conform-to-invokePrim.diff dated Dec 19 2012 after the status was changed to Incomplete.

Show
Andy Fingerhut added a comment - Presumptuously changing state from Incomplete back to Vetted after Ghadi Shayban added the patch CLJ-850-conform-to-invokePrim.diff dated Dec 19 2012 after the status was changed to Incomplete.
Andy Fingerhut made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Rich Hickey made changes -
Fix Version/s Release 1.6 [ 10157 ]
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description See the following examples:

{noformat}
user=> (defn f1 ^String [^String s] s)
#'user/f1
user=> (f1 "foo")
"foo"
user=> (defn f2 ^long [^String s ^long i] i)
#'user/f2
user=> (f2 "foo" 1)
1
user=> (defn f3 ^String [^String s ^long i] s)
#'user/f3
user=> (f3 "foo" 1)
AbstractMethodError user$f3.invokePrim(Ljava/lang/Object;J)Ljava/lang/Object; user/eval8 (NO_SOURCE_FILE:6)
{noformat}
Summary: The CLJ-850-conform-to-invokePrim.diff patch is constructed per Rich's feedback, and appears good to me [Stu].

See the following examples:

{noformat}
user=> (defn f1 ^String [^String s] s)
#'user/f1
user=> (f1 "foo")
"foo"
user=> (defn f2 ^long [^String s ^long i] i)
#'user/f2
user=> (f2 "foo" 1)
1
user=> (defn f3 ^String [^String s ^long i] s)
#'user/f3
user=> (f3 "foo" 1)
AbstractMethodError user$f3.invokePrim(Ljava/lang/Object;J)Ljava/lang/Object; user/eval8 (NO_SOURCE_FILE:6)
{noformat}
Alex Miller made changes -
Description Summary: The CLJ-850-conform-to-invokePrim.diff patch is constructed per Rich's feedback, and appears good to me [Stu].

See the following examples:

{noformat}
user=> (defn f1 ^String [^String s] s)
#'user/f1
user=> (f1 "foo")
"foo"
user=> (defn f2 ^long [^String s ^long i] i)
#'user/f2
user=> (f2 "foo" 1)
1
user=> (defn f3 ^String [^String s ^long i] s)
#'user/f3
user=> (f3 "foo" 1)
AbstractMethodError user$f3.invokePrim(Ljava/lang/Object;J)Ljava/lang/Object; user/eval8 (NO_SOURCE_FILE:6)
{noformat}
Hinting the arg vector of a primitive-taking fn with a non-primitive type results in AbstractMethodError when invoked.

{noformat}
user=> (defn f1 ^String [^String s] s)
#'user/f1
user=> (f1 "foo")
"foo"
user=> (defn f2 ^long [^String s ^long i] i)
#'user/f2
user=> (f2 "foo" 1)
1
user=> (defn f3 ^String [^String s ^long i] s)
#'user/f3
user=> (f3 "foo" 1)
AbstractMethodError user$f3.invokePrim(Ljava/lang/Object;J)Ljava/lang/Object; user/eval8 (NO_SOURCE_FILE:6)
{noformat}

*Solution:* Add check to emit invokePrim with return type of double or long if appropriate, otherwise do prior behavior of emitting return type of Object.

*Patch:* The {{CLJ-850-conform-to-invokePrim.diff}} patch is constructed per Rich's feedback, and appears good to me [Stu].
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: