Clojure

Fix an edge case in the Reflector's search for a public method declaration

Details

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

Description

// all: package demo;

public interface IBar {
  void stuff();
}

class Bar {
  public void stuff() { System.out.println("stuff"); }
}

class SubBar extends Bar implements IBar {
  // implements IBar, via non-public superclass Bar
}

public class Factory {
  public static IBar get() { return new SubBar(); }
}

Can't invoke from Clojure:

(import '[demo Factory IBar])
(def inst (Factory/get))  ;; instance of SubBar, inferred as IBar
(.stuff inst)
;; IllegalArgumentException Can't call public method of non-public class: public void demo.Bar.stuff()  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:88)

Cause: The Reflector is not taking into account that a non-public class can implement an interface, and have a non-public parent class contain an implementation of a method on that interface.

Approach: The solution I took is to pass in the target object's class instead of the declaring class of the method object.

Patch: clj-1609-3.patch + clj-1609-test.patch

Screened by: Fogus

  1. clj-1609-2.patch
    18/Aug/15 4:58 PM
    1 kB
    Alex Miller
  2. clj-1609-3.patch
    24/Aug/15 12:11 PM
    1 kB
    Alex Miller
  3. clj-1609-test.patch
    18/Aug/15 4:58 PM
    2 kB
    Alex Miller
  4. reflector_method_bug.patch
    05/Dec/14 12:39 AM
    1 kB
    Jeremy Heiler

Activity

Hide
Alex Miller added a comment -

Probably a dupe of CLJ-259. I'll probably dupe that one to this though.

Show
Alex Miller added a comment - Probably a dupe of CLJ-259. I'll probably dupe that one to this though.
Hide
Jeremy Heiler added a comment -

Thanks. Sorry for not finding that myself. CLJ-259 refers to CLJ-126, which I think is covered with this patch.

Show
Jeremy Heiler added a comment - Thanks. Sorry for not finding that myself. CLJ-259 refers to CLJ-126, which I think is covered with this patch.
Hide
Jeremy Heiler added a comment - - edited

I was looking into whether or not the target could be null, and it can be when invoking a static method. However, I don't think that code path would make it to the target.getClass() because non-public static methods aren't returned by getMethods().

Show
Jeremy Heiler added a comment - - edited I was looking into whether or not the target could be null, and it can be when invoking a static method. However, I don't think that code path would make it to the target.getClass() because non-public static methods aren't returned by getMethods().
Hide
Stuart Halloway added a comment -

Jeremy,

The compile-tests phase of the build provides a point for compiling Java classes that that tests can then consume. Does that provide the hook you would need to add a test?

Show
Stuart Halloway added a comment - Jeremy, The compile-tests phase of the build provides a point for compiling Java classes that that tests can then consume. Does that provide the hook you would need to add a test?
Hide
Alex Miller added a comment -

afaict, this ticket is incomplete pending a test from Stu's last comment so moving there

Show
Alex Miller added a comment - afaict, this ticket is incomplete pending a test from Stu's last comment so moving there
Hide
Alex Miller added a comment -

clj-1609-2.patch is identical to previous patch, just adds jira id to beginning of commit message

clj-1609-test.patch adds a reflection test that fails without the patch and passes with it.

Show
Alex Miller added a comment - clj-1609-2.patch is identical to previous patch, just adds jira id to beginning of commit message clj-1609-test.patch adds a reflection test that fails without the patch and passes with it.
Hide
Alex Miller added a comment -

should be ready to screen now

Show
Alex Miller added a comment - should be ready to screen now
Hide
Fogus added a comment -

The patch and test are straight-forward and clean.

Show
Fogus added a comment - The patch and test are straight-forward and clean.
Hide
Rich Hickey added a comment -

Can I get some more context in this diff please?

Show
Rich Hickey added a comment - Can I get some more context in this diff please?
Hide
Alex Miller added a comment -

Added clj-1609-3.patch which is identical to prior but has 15 lines of context instead.

Show
Alex Miller added a comment - Added clj-1609-3.patch which is identical to prior but has 15 lines of context instead.

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: