Bugzilla – Full Text Bug Listing
| Summary: | EpsilonTestSuite fails on test case "testRemove" from CollectionsTests.eol | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] Epsilon | Reporter: | Antonio Garcia-Dominguez <agarcdomi> | ||||||
| Component: | Core | Assignee: | Louis Rose <louis> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | dkolovos, louis, maarten.bezemer | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | interim | ||||||||
| Attachments: |
|
||||||||
Created attachment 190676 [details]
Patch which implements a 2-phase lookup process
This patch implements phase 1 of the JLS method resolution process, but we'd still be missing phase 3 (varargs). I really think we should stick to Class#getMethod.
I'm not sure if this is helpful, but on my Windows installation this test passes! Maybe due to a different version of Java? I'm running: Java(TM) SE Runtime Environment (build 1.6.0_15-b03) Java HotSpot(TM) Client VM (build 14.1-b02, mixed mode, sharing) I'm running OpenJDK 6 on Linux. And you're right: I ran the tests today on Windows 7 and this one passed :-/. One of our users has been bitten by this problem, according to this thread: http://www.eclipse.org/forums/index.php/mv/msg/351807/872040/#msg_872040 Dimitris, Louis: could you have another look at the patch? Antonio, Thanks for continuing to drive this forward I agree with your diagnosis. I've investigated whether we can use Class#getMethod, but sadly this doesn't help. According to [1,2], Class#getMethod doesn't correctly manage auto-boxing and it seems we'd have to continue to maintain our own logic for this. Consequently I believe your patch on comment 1 is the solution we should use. I've rebased this patch on HEAD, added some unit tests, and refactored out some duplication that has been in ReflectionUtil for a little while. [1] - http://royontechnology.blogspot.co.uk/2010/10/issue-of-autoboxing-and-reflection.html [2] - http://stackoverflow.com/questions/1894740/any-solution-for-class-getmethod-reflection-and-autoboxing Created attachment 215718 [details]
Rebased Antonio's patch on top of HEAD
Latest version of Antonio's solution attached.
@Dimitris, @Antonio: could you take a look, and give a +1 if we can push this forward?
(In reply to comment #6) > Created attachment 215718 [details] > Rebased Antonio's patch on top of HEAD > > Latest version of Antonio's solution attached. > > @Dimitris, @Antonio: could you take a look, and give a +1 if we can push this > forward? +1 The patch looks good to me. Did that fix testRemove at last? (In reply to comment #7) > (In reply to comment #6) > > @Dimitris, @Antonio: could you take a look, and give a +1 if we can push this > > forward? > > +1 > > The patch looks good to me. Did that fix testRemove at last? It did on Mac OS X. When I get home, I'll check on Windows (which I think implements Class#getMethod differently, judging by comment 2). (In reply to comment #8) > It did on Mac OS X. When I get home, I'll check on Windows (which I think > implements Class#getMethod differently, judging by comment 2). Confirming that the attached patch works on Windows (XP, Java 7 update 4). (In reply to comment #9) > (In reply to comment #8) > > It did on Mac OS X. When I get home, I'll check on Windows (which I think > > implements Class#getMethod differently, judging by comment 2). > > Confirming that the attached patch works on Windows (XP, Java 7 update 4). The attached patch also works on Linux: java.runtime.name=OpenJDK Runtime Environment java.runtime.version=1.6.0_24-b24 java.vm.info=mixed mode, sharing java.vm.name=OpenJDK Client VM java.vm.specification.name=Java Virtual Machine Specification java.vm.specification.vendor=Sun Microsystems Inc. java.vm.specification.version=1.0 java.vm.vendor=Sun Microsystems Inc. java.vm.version=20.0-b12 Louis, shall I go ahead and apply the patch, or would you like to show it to Dimitris first? (In reply to comment #11) > Louis, shall I go ahead and apply the patch, or would you like to show it to > Dimitris first? Let me just check my rebasing with Dimitris first. It'd be good to make sure I've not accidentally squashed one of Dimitris's recent changes! I've committed the rebased patch to SVN. This has been fixed in the latest interim version. Fixed in 1.0 |
Build Identifier: 20100917-0705 I've tried running the EpsilonTestSuite and the testRemove test case in CollectionsTests.eol in the o.e.e.eol.engine.test.acceptance project fails: @test operation testRemove() { var s = Sequence{1,2,3}; s.remove(1); assertEquals(s, Sequence{2,3}); } Apparently, l.remove(1) is being interpreted as "remove the second element" rather than "remove the element equal to 1", when l is an ArrayList. That is correct in Java (as 1 is used straight away as an int), but Epsilon converts the 1 to an Integer, so I would expect the test to pass. This would be consistent with the Epsilon book, which defines remove(item) but not remove(int), using removeAt(int) instead to avoid this ambiguity. I have looked a bit into it and I think I have found the cause of the problem: the method resolution process in ReflectionUtil#getMethodFor is slightly off. The correct process, according to the Java Language Specification, sections 15.12.2.2 to 15.12.2.4, is to do it in three stages: 1. Try to find it without autoboxing/unboxing. 2. Try to find it with autoboxing/unboxing. 3. Try to find it with varargs. However, ReflectionUtil#getMethodFor seems to do phase 2 straight away, as it used ReflectionUtil#isInstance, which seems to do the autoboxing/unboxing bits: /** * Checks if the instance is an instance of clazz * Necessary because in Java, int.class != Integer.class etc * @param clazz * @param instance * @return */ public static boolean isInstance(Class clazz, Object instance){ if (instance == null) return true; else if (clazz == int.class) return Integer.class.isInstance(instance); else if (clazz == float.class) return Float.class.isInstance(instance); else if (clazz == double.class) return Double.class.isInstance(instance); else if (clazz == boolean.class) return Boolean.class.isInstance(instance); else if (clazz == long.class) return Long.class.isInstance(instance); else return clazz.isInstance(instance); } I think method resolution is too tricky for us to be reimplementing it: perhaps we should just use Class#getMethod and cache the results to save time. Reproducible: Always Steps to Reproduce: 1. Run the EpsilonTestSuite in the org.eclipse.epsilon.test project.