Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 369628

Summary: org.eclipse.jdt.debug does not compile when using OpenJDK
Product: [Eclipse Project] JDT Reporter: Krzysztof Daniel <krzysztof.daniel>
Component: DebugAssignee: Michael Rennie <Michael_Rennie>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: akurtakov, kkazmierczyk+eclipse, krzysztof.daniel, Lars.Vogel, markus.kell.r, Michael_Rennie, overholt, remy.suen, robert.munteanu, srikanth_sankaran
Version: 3.8   
Target Milestone: 3.8 M6   
Hardware: PC   
OS: Linux   
Whiteboard:

Description Krzysztof Daniel CLA 2012-01-25 02:54:24 EST
When I build Eclipse JDT using OpenJDK, I get a number of bugs 
    [javac] /org.eclipse.jdt.debug/jdi/org/eclipse/jdi/internal/request/EventRequestManagerImpl.java:432: error: inconvertible types
    [javac] 		return new ArrayList<AccessWatchpointRequest>((Collection<? extends AccessWatchpointRequest>) fRequests.get(ACCESS_WATCHPOINT_INDEX));
    [javac] 		                                                                                                           ^
    [javac]   required: Collection<? extends AccessWatchpointRequest>
    [javac]   found:    HashSet<EventRequest>

Initially I thought it is a bug in OpenJDK compiler, but after closer investigation it looks like JDT allows for potentially unsafe code.

To understand the problem, lets look at the offending line:

return new ArrayList<AccessWatchpointRequest>((Collection<? extends AccessWatchpointRequest>) fRequests.get(ACCESS_WATCHPOINT_INDEX));

and field declaration

private ArrayList<HashSet<EventRequest>> fRequests;

fRequests.get(ACCESS_WATCHPOINT_INDEX) returns a HashSet<EventRequest>, which may or may not be casted to (Collection<? extends AccessWatchpointRequest>), because EventRequest is a parent of AccessWatchpointRequest.

Compiler cannot know that this code will work.
Comment 1 Srikanth Sankaran CLA 2012-01-25 03:20:03 EST
See https://bugs.eclipse.org/bugs/show_bug.cgi?id=205195#c7

There are many instances where javac implementation is tighter
than the underspecified rules in this area.

We don't have plans to match (the undocumented) behavior here.
Comment 2 Lars Vogel CLA 2012-01-28 15:05:45 EST
@Daniel: From your blog entry I understood that you are patching this. Could you provide a patch for this specific situation so that Sankaran could check if it would be save to apply to JDT.

I think it would be nice to be able to compile Eclipse with OpenJDK, that is the de-facto standard in the Javac world.
Comment 3 Srikanth Sankaran CLA 2012-01-28 20:01:43 EST
(In reply to comment #2)
> @Daniel: From your blog entry I understood that you are patching this. Could
> you provide a patch for this specific situation so that Sankaran could check if
> it would be save to apply to JDT.

What is being patched ? Debug or the compiler ? The latter approach could be
problematic as some times when we attempted to match javac on specific cases
in the past, it backfired in some other cases and we ended up in a worse
situation (i.e we could not compile something javac would which is much worse
than our accepting something javac would not)
Comment 4 Lars Vogel CLA 2012-01-28 20:06:39 EST
@Sankaran I believe Daniel has a patch for the source code, e.g. EventRequestManagerImpl.java.

Maybe I'm wrong, lets see what Daniel says.
Comment 5 Krzysztof Daniel CLA 2012-01-30 02:46:36 EST
I have seen some inconsistency between ecj and javac earlier, usually ecj was correct. 

In Eclipse Fedora Build, I patch the failing source.
The patch is available here:
http://git.eclipse.org/c/linuxtools/org.eclipse.linuxtools.eclipse-build.git/tree/eclipse-build/patches/jdt-debug-compilation.patch.

Unfortunately the differences between javac and ecj are big enough to break ecj compiler when the patch is applied (unnecessary casts, unchecked exceptions etc).

Frankly speaking, I do not have an idea how to adjust source code to make it working for both ecj and javac and do not introduce too many changes...

--
Chris
Comment 6 Srikanth Sankaran CLA 2012-01-30 04:20:56 EST
Why don't give JDT/Debug team a chance to look at this first ?
Recently things got generified there.
Comment 7 Lars Vogel CLA 2012-01-30 04:25:20 EST
@Chris: Thanks. I'm surprised how small the problem is. Thats all what is preventing Eclipse from getting compiled by OpenJDK?
Comment 8 Krzysztof Daniel CLA 2012-01-30 04:50:56 EST
Yes, that's the whole patch - and I was able to build Eclipse with it :
http://koji.fedoraproject.org/koji/taskinfo?taskID=3739589
Comment 9 Michael Rennie CLA 2012-01-30 12:58:11 EST
(In reply to comment #8)
> Yes, that's the whole patch - and I was able to build Eclipse with it :
> http://koji.fedoraproject.org/koji/taskinfo?taskID=3739589

The patch introduces 32 compile errors in my workspace - 16 unnecessary cast errors and 16 type safety errors.

Description	Resource	Path	Location	Type
Type safety: Unchecked cast from Object to Collection<? extends AccessWatchpointRequest>	EventRequestManagerImpl.java	/org.eclipse.jdt.debug/jdi/org/eclipse/jdi/internal/request	line 432	Java Problem
Comment 10 Michael Rennie CLA 2012-01-30 13:37:37 EST
(In reply to comment #9) 
> The patch introduces 32 compile errors in my workspace - 16 unnecessary cast
> errors and 16 type safety errors.

Chatting a bit with Markus there does not appear to be a good type-safe way to fix this, other than to separate out the lists of EventRequests. I'll code something up today.
Comment 11 Michael Rennie CLA 2012-02-01 12:19:54 EST
Markus had a great idea of using a generic type to separate out the collections and avoid casting / type safety problems. So building on his example, I pushed the changes to http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=36ba3d537eb874455538949bb1fb509d4544188f

Krzysztof, please verify.
Comment 12 Krzysztof Daniel CLA 2012-02-09 02:40:28 EST
I was able to built org.eclipse.jdt.debug (3.7.100.v20120201-1716) with javac. It looks OK.
Comment 13 Lars Vogel CLA 2012-02-09 03:02:05 EST
Thanks Chris, Srikanth and Michael. Great to hear! Thanks for your efforts.