Details

Description

Issues:

Code for obtaining method/constructor instances is duplicated across the Compiler

Code for resolving a preferred overloaded method lives in the Compiler

By consolidating the duplicated code, moving the reflection-related parts into Reflector, and providing a straightforward API, it should be easier to read and understand the method resolution process. Further, improvements to (e.g., CLJ-445) the mechanism for reflecting on class members can largely be isolated from the Compiler. And the few points of coordination (e.g., Compiler emitting same arg and return types as Reflector does when invoking) can be clearly identified and documented.

Andy Fingerhut
added a comment - 20/Feb/12 1:11 PM I don't know if this is helpful or not, but this updated version of Alexander's patch applies cleanly to latest Clojure head as of Feb 20, 2012. It compiles, but does not pass ant test.

Nicola Mometto
added a comment - 30/Jan/15 7:29 AM I would be very interested in having this patch considered again and if there's interest I could help with porting the patch to the current master.
I believe anybody who spent some time between Compiler.java and Reflector.java would agree that this refactoring/cleanup has been long needed.

I've attempted to forward-port the patch again. The Java code compiles, and then is able to compile the Clojure source, but then fails in compiling the clojure tests with

test-example:
[java] Exception in thread "main" java.lang.IllegalArgumentException: Found multiple uncheckedByteCast methods in clojure.lang.RT for argtypes: byte, compiling:(clojure/test_clojure/numbers.clj:127:3)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6568)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6556)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:981)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6556)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6007)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.access$200(Compiler.java:38)
[java] at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5957)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2182)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5124)
[java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3753)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6559)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3565)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6563)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2153)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5691)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5691)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5124)
[java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3753)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6559)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$MapExpr.parse(Compiler.java:2882)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6360)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:559)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler.eval(Compiler.java:6624)
[java] at clojure.lang.Compiler.load(Compiler.java:7047)
[java] at clojure.lang.RT.loadResourceScript(RT.java:370)
[java] at clojure.lang.RT.loadResourceScript(RT.java:361)
[java] at clojure.lang.RT.load(RT.java:440)
[java] at clojure.lang.RT.load(RT.java:411)
[java] at clojure.core$load$fn__5424.invoke(core.clj:5848)
[java] at clojure.core$load.doInvoke(core.clj:5847)
[java] at clojure.lang.RestFn.invoke(RestFn.java:408)
[java] at clojure.core$load_one.invoke(core.clj:5653)
[java] at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
[java] at clojure.core$load_lib.doInvoke(core.clj:5692)
[java] at clojure.lang.RestFn.applyTo(RestFn.java:142)
[java] at clojure.core$apply.invoke(core.clj:628)
[java] at clojure.core$load_libs.doInvoke(core.clj:5731)
[java] at clojure.lang.RestFn.applyTo(RestFn.java:137)
[java] at clojure.core$apply.invoke(core.clj:628)
[java] at clojure.core$require.doInvoke(core.clj:5814)
[java] at clojure.lang.RestFn.invoke(RestFn.java:408)
[java] at user$eval72.invoke(run_test.clj:6)
[java] at clojure.lang.Compiler.eval(Compiler.java:6620)
[java] at clojure.lang.Compiler.load(Compiler.java:7047)
[java] at clojure.lang.Compiler.loadFile(Compiler.java:7003)
[java] at clojure.main$load_script.invoke(main.clj:274)
[java] at clojure.main$script_opt.invoke(main.clj:336)
[java] at clojure.main$main.doInvoke(main.clj:420)
[java] at clojure.lang.RestFn.invoke(RestFn.java:408)
[java] at clojure.lang.Var.invoke(Var.java:379)
[java] at clojure.lang.AFn.applyToHelper(AFn.java:154)
[java] at clojure.lang.Var.applyTo(Var.java:700)
[java] at clojure.main.main(main.java:37)
[java] Caused by: java.lang.IllegalArgumentException: Found multiple uncheckedByteCast methods in clojure.lang.RT for argtypes: byte
[java] at clojure.lang.Reflector.getMatchingParams(Reflector.java:385)
[java] at clojure.lang.Reflector.getMatchingMember(Reflector.java:419)
[java] at clojure.lang.Reflector.getMatchingMethod(Reflector.java:485)
[java] at clojure.lang.Reflector.getMatchingStaticMethod(Reflector.java:499)
[java] at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1578)
[java] at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:983)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] ... 110 more

Michael Blume
added a comment - 30/Jan/15 12:22 PM I've attempted to forward-port the patch again. The Java code compiles, and then is able to compile the Clojure source, but then fails in compiling the clojure tests with

test-example:
[java] Exception in thread "main" java.lang.IllegalArgumentException: Found multiple uncheckedByteCast methods in clojure.lang.RT for argtypes: byte, compiling:(clojure/test_clojure/numbers.clj:127:3)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6568)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6556)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:981)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6556)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6007)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.access$200(Compiler.java:38)
[java] at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5957)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2182)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5124)
[java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3753)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6559)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3565)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6563)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2153)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5691)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5691)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5693)
[java] at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5124)
[java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3753)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6559)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6549)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$MapExpr.parse(Compiler.java:2882)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6360)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:559)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6352)
[java] at clojure.lang.Compiler.analyze(Compiler.java:6313)
[java] at clojure.lang.Compiler.eval(Compiler.java:6624)
[java] at clojure.lang.Compiler.load(Compiler.java:7047)
[java] at clojure.lang.RT.loadResourceScript(RT.java:370)
[java] at clojure.lang.RT.loadResourceScript(RT.java:361)
[java] at clojure.lang.RT.load(RT.java:440)
[java] at clojure.lang.RT.load(RT.java:411)
[java] at clojure.core$load$fn__5424.invoke(core.clj:5848)
[java] at clojure.core$load.doInvoke(core.clj:5847)
[java] at clojure.lang.RestFn.invoke(RestFn.java:408)
[java] at clojure.core$load_one.invoke(core.clj:5653)
[java] at clojure.core$load_lib$fn__5373.invoke(core.clj:5693)
[java] at clojure.core$load_lib.doInvoke(core.clj:5692)
[java] at clojure.lang.RestFn.applyTo(RestFn.java:142)
[java] at clojure.core$apply.invoke(core.clj:628)
[java] at clojure.core$load_libs.doInvoke(core.clj:5731)
[java] at clojure.lang.RestFn.applyTo(RestFn.java:137)
[java] at clojure.core$apply.invoke(core.clj:628)
[java] at clojure.core$require.doInvoke(core.clj:5814)
[java] at clojure.lang.RestFn.invoke(RestFn.java:408)
[java] at user$eval72.invoke(run_test.clj:6)
[java] at clojure.lang.Compiler.eval(Compiler.java:6620)
[java] at clojure.lang.Compiler.load(Compiler.java:7047)
[java] at clojure.lang.Compiler.loadFile(Compiler.java:7003)
[java] at clojure.main$load_script.invoke(main.clj:274)
[java] at clojure.main$script_opt.invoke(main.clj:336)
[java] at clojure.main$main.doInvoke(main.clj:420)
[java] at clojure.lang.RestFn.invoke(RestFn.java:408)
[java] at clojure.lang.Var.invoke(Var.java:379)
[java] at clojure.lang.AFn.applyToHelper(AFn.java:154)
[java] at clojure.lang.Var.applyTo(Var.java:700)
[java] at clojure.main.main(main.java:37)
[java] Caused by: java.lang.IllegalArgumentException: Found multiple uncheckedByteCast methods in clojure.lang.RT for argtypes: byte
[java] at clojure.lang.Reflector.getMatchingParams(Reflector.java:385)
[java] at clojure.lang.Reflector.getMatchingMember(Reflector.java:419)
[java] at clojure.lang.Reflector.getMatchingMethod(Reflector.java:485)
[java] at clojure.lang.Reflector.getMatchingStaticMethod(Reflector.java:499)
[java] at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1578)
[java] at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:983)
[java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:6561)
[java] ... 110 more

Michael Blume
added a comment - 30/Jan/15 12:55 PM clj-793-v4 has the useful property of compiling on every commit, for easier bisection, though the tests still don't run
The tests seem to start failing at "move getMatchingParams method from Compiler to Reflector" which looks like an entirely innocuous change to me, so I'm a bit confused.

Michael Blume
added a comment - 30/Jan/15 1:12 PM By the way I'm very grateful to Alexander Taggart for authoring this patch as a series of small commits so that it could be relatively easily forward-ported/bisected.

In "Moved instance-method-finding code from Compiler to Reflector" the code that's being moved out of Compiler has two different calls to RT.errPrintWriter which use lots of information that's only available in the Compiler (column, row, etc). When this code got moved to the Reflector, I tried to be clever in my forward-port and just leave the error-printing in one spot in the Compiler, but that doesn't work because the two calls are printing different messages. I'm not actually sure what a good way to move this to Reflector would be.

Michael Blume
added a comment - 30/Jan/15 1:29 PM ok, this has the tied=false fix, but there's a new problem
In "Moved instance-method-finding code from Compiler to Reflector" the code that's being moved out of Compiler has two different calls to RT.errPrintWriter which use lots of information that's only available in the Compiler (column, row, etc). When this code got moved to the Reflector, I tried to be clever in my forward-port and just leave the error-printing in one spot in the Compiler, but that doesn't work because the two calls are printing different messages. I'm not actually sure what a good way to move this to Reflector would be.
Relevant change is also at https://github.com/MichaelBlume/clojure/commit/56995f3376795a3cfcfc6339e7d8ad24d8459d31
look for the calls to RT.errPrintWriter and you'll see what changed.

To summarize perhaps a bit better, there's some code that this patch moves from Compiler into its own method in Reflector that was relatively easy to move back when this patch was first authored, but now has some extra reflection-warning-printing that wants to know row/column stuff that's available in its original location in Compiler but not in the extracted function in Reflector. We could pass row/column into that method, but then we'd have to eventually pass them through a few other Reflector methods and that sounds ugly. Alternately we could pass some indication out of that reflector method of how exactly it failed to find a method so that the Compiler method could make its own decisions about how to print the failure, but that would involve either inventing exceptions or some sort of Success/Failure ADT.

Michael Blume
added a comment - 30/Jan/15 1:44 PM To summarize perhaps a bit better, there's some code that this patch moves from Compiler into its own method in Reflector that was relatively easy to move back when this patch was first authored, but now has some extra reflection-warning-printing that wants to know row/column stuff that's available in its original location in Compiler but not in the extracted function in Reflector. We could pass row/column into that method, but then we'd have to eventually pass them through a few other Reflector methods and that sounds ugly. Alternately we could pass some indication out of that reflector method of how exactly it failed to find a method so that the Compiler method could make its own decisions about how to print the failure, but that would involve either inventing exceptions or some sort of Success/Failure ADT.

Michael Blume
added a comment - 30/Jan/15 2:08 PM That makes sense, it runs into trouble later when Reflector.getMethods gets replaced with the private getMethodsForName but maybe we can hack around that...