Welcome! Please see the About page for a little more info on how this works.

Ask a Question

Refactor method resolution code out of Compiler and into Reflector

0 votes
in Clojure by

Issues:

  1. Code for obtaining method/constructor instances is duplicated across the Compiler
  2. 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.

15 Answers

0 votes
by

Comment made by: stuart.sierra

Patch does not apply as of commit f5bcf64.

0 votes
by

Comment made by: ataggart

Yeah, year-old patches tend to do that.

0 votes
by

Comment made by: jafingerhut

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.

0 votes
by

Comment made by: bronsa

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.

0 votes
by

Comment made by: michaelblume

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

`

0 votes
by

Comment made by: michaelblume

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.

0 votes
by

Comment made by: michaelblume

Aha, tried just moving getMatchingParams using IntelliJ refactor and comparing, looks like the assign "tied = false" somehow got dropped. That fixes that test error and leaves a new one. Bisecting...

0 votes
by

Comment made by: michaelblume

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.

0 votes
by

Comment made by: michaelblume

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.

0 votes
by

Comment made by: michaelblume

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.

0 votes
by

Comment made by: bronsa

Michael, what about doing

if (Reflector.getMethods(target.getJavaClass(), args.count(), methodName, false) == null) RT.errPrintWriter().format("Reflection warning, %s:%d:%d - call to method %s on %s can't be resolved (no such method).\n", SOURCE_PATH.deref(), line, column, methodName, target.getJavaClass().getName()); else RT.errPrintWriter().format("Reflection warning, %s:%d:%d - call to method %s on %s can't be resolved (argument types: %s).\n",SOURCE_PATH.deref(), line, column, methodName, target.getJavaClass().getName(), getTypeStringForArgs(args));

inside the block starting with
`
if(method == null && RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))`

in Compiler.java?

0 votes
by

Comment made by: michaelblume

That makes sense, it runs into trouble later when Reflector.getMethods gets replaced with the private getMethodsForName but maybe we can hack around that...

0 votes
by

Comment made by: michaelblume

No, still doesn't print out the right warnings

`

 [java] Testing clojure.test-clojure.rt
 [java]
 [java] FAIL in (error-messages) (rt.clj:43)
 [java] reflection cannot resolve instance method because it is missing
 [java] expected: (clojure.core/re-matches #"Reflection warning, .*:\d+:\d+ - call to method zap on java\.lang\.String can't be resolved \(no such method\)\.\r?\n" (clojure.test-helper/with-err-string-writer (clojure.test-helper/eval-in-temp-ns (defn foo [x] (.zap x 1)))))
 [java]   actual: (not (clojure.core/re-matches #"Reflection warning, .*:\d+:\d+ - call to method zap on java\.lang\.String can't be resolved \(no such method\)\.\r?\n" "Reflection warning, /Users/michael.blume/workspace/clojure/src/script/run_test.clj:45:28 - call to method zap on java.lang.String can't be resolved (argument types: long).\n"))
 [java]
 [java] FAIL in (error-messages) (rt.clj:43)
 [java] reflection cannot resolve instance method because it is missing
 [java] expected: (clojure.core/re-matches #"Reflection warning, .*:\d+:\d+ - call to method zap on java\.lang\.String can't be resolved \(no such method\)\.\r?\n" (clojure.test-helper/with-err-print-writer (clojure.test-helper/eval-in-temp-ns (defn foo [x] (.zap x 1)))))
 [java]   actual: (not (clojure.core/re-matches #"Reflection warning, .*:\d+:\d+ - call to method zap on java\.lang\.String can't be resolved \(no such method\)\.\r?\n" "Reflection warning, /Users/michael.blume/workspace/clojure/src/script/run_test.clj:45:28 - call to method zap on java.lang.String can't be resolved (argument types: long).\n"))

`

0 votes
by

Comment made by: michaelblume

Patch series in v5 is also at https://github.com/MichaelBlume/clojure/commits/compile-refactor
Patch series with Nicolo's suggestion is at https://github.com/MichaelBlume/clojure/commits/compile-refactor-nicolo

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJ-792 (reported by ataggart)
Maintained by Cognitect on behalf of the Clojure community
...