<< Back to previous view

[CLJ-850] Hinting the arg vector of a primitive-taking fn with a non-primitive type results in AbstractMethodError when invoked Created: 09/Oct/11  Updated: 14/Aug/13  Resolved: 14/Aug/13

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.3
Fix Version/s: Release 1.6

Type: Defect Priority: Major
Reporter: Alexander Taggart Assignee: Stuart Halloway
Resolution: Completed Votes: 2
Labels: None

Attachments: File CLJ-850-conform-to-invokePrim.diff     Text File CLJ-850.patch     Text File CLJ-850-test.patch     Text File clj-850-type-hinted-fn-abstractmethoderror-patch4.txt    
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].



 Comments   
Comment by Ben Smith-Mannschott [ 15/Oct/11 11:54 AM ]

CLJ-850-test.patch added.

Comment by Ben Smith-Mannschott [ 16/Oct/11 7:32 AM ]

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.

Comment by Ben Smith-Mannschott [ 16/Oct/11 10:17 AM ]

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...

Comment by Ben Smith-Mannschott [ 18/Oct/11 12:29 AM ]

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
Comment by Ben Smith-Mannschott [ 18/Oct/11 1:40 AM ]

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.

Comment by Ben Smith-Mannschott [ 18/Oct/11 2:54 PM ]

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.

Comment by Ben Smith-Mannschott [ 19/Oct/11 5:06 AM ]

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.

Comment by Andy Fingerhut [ 28/Feb/12 12:49 AM ]

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.

Comment by Andy Fingerhut [ 23/Mar/12 7:50 PM ]

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.

Comment by Rich Hickey [ 13/Apr/12 9:35 AM ]

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.

Comment by Andy Fingerhut [ 01/Nov/12 7:22 PM ]

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.

Comment by Mike Anderson [ 09/Dec/12 3:42 PM ]

+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?

Comment by Rich Hickey [ 19/Dec/12 8:03 AM ]

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

Comment by Ghadi Shayban [ 19/Dec/12 3:41 PM ]

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

Comment by Andy Fingerhut [ 21/Dec/12 10:53 PM ]

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.

Generated at Sat Nov 29 00:17:22 CST 2014 using JIRA 4.4#649-r158309.