<< Back to previous view

[CLJ-792] Refactor method resolution code out of Compiler and into Reflector Created: 11/May/11  Updated: 30/Jan/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clj-792-reorg-reflector-patch2.txt     Text File clj-792-v3.patch     Text File clj-793-v4.patch     Text File clj-793-v5.patch     Text File reorg-reflector.patch    

 Description   

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.



 Comments   
Comment by Stuart Sierra [ 17/Feb/12 2:28 PM ]

Patch does not apply as of commit f5bcf64.

Comment by Alexander Taggart [ 17/Feb/12 3:14 PM ]

Yeah, year-old patches tend to do that.

Comment by Andy Fingerhut [ 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.

Comment by Nicola Mometto [ 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.

Comment by Michael Blume [ 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
Comment by Michael Blume [ 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.

Comment by Michael Blume [ 30/Jan/15 1:08 PM ]

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

Comment by Michael Blume [ 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.

Comment by Michael Blume [ 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.

Comment by Michael Blume [ 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.

Comment by Nicola Mometto [ 30/Jan/15 1:45 PM ]

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?

Comment by Michael Blume [ 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...

Comment by Michael Blume [ 30/Jan/15 2:27 PM ]

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"))
Comment by Michael Blume [ 30/Jan/15 2:29 PM ]

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

Generated at Sat Apr 18 02:18:02 CDT 2015 using JIRA 4.4#649-r158309.