Community
Participate
Working Groups
PROBLEM: public API change from GMF causes compatibility issues. public API: List getSemanticChildrenList() from CanonicalEditPolicy has recently been changed to List<EObject> getSemanticChildrenList() This is a public API change and causes compatibility issues....
Tao, is this the only issue with Java 5.0 upgrade to canonixcal? Or there are more problems?
I see only 2 options: 1) Revert changes to signatures only to specific methods: #getSemanticChildrenList() in this case. Continue with the correct 5.0 upgrade of canonical as suggested in bug 243634. In this case I think we need to create interface IViewDescriptor to be able to use wildcard correctly (can't just use <? extends ViewDescriptor>, because this would exclude List<ViewDescriptor>) 2) Revert all method signature changes that went in with the fix for 243634, and postpone this till e4. Anyone wants to express his opinion or preferable option?
If this issue was caused by Bug 243634 , we can back the change out, as it was a cleanup / cosmetic change. Why did API tooling not catch this?
I'm under the impression that API tooling ignores generics... If I change the return type of #getSemanticChildrenList() from List<EObject> to List<View> or even to List<EditPart> API tooling does not report that as an error. The plan for this revert the return type change for CanonicalEditPolicty#getSemanticChildrenList() and revert changes to method signatures in CreateViewRequest class
Created attachment 126189 [details] proposed patch Patch for CanonicalEditPolicy#getSemanticChildrenList() and CreateViewRequest class
(In reply to comment #4) > I'm under the impression that API tooling ignores generics... Ahh, I get it. Your patch is good.
Fix committed for 2.2 M6
Ok, List<? extends ViewDescriptor> does include List<ViewDescriptor> the problem is that clients use Iterator<ViewDescriptor>, but won't be able to do that once we switch to <? extends ViewDescriptor>.
(In reply to comment #8) > Ok, List<? extends ViewDescriptor> does include List<ViewDescriptor> the > problem is that clients use Iterator<ViewDescriptor>, but won't be able to do > that once we switch to <? extends ViewDescriptor>. > this compiles: public class Generics { static class ItemBaseClass { } static class ItemSubClass extends ItemBaseClass { } static class ContainerClass { List<? extends ItemBaseClass> foo() { List<ItemSubClass> list = new ArrayList<ItemSubClass>(); list.add(new ItemSubClass()); return list; } } public static void main(String[] args) { ContainerClass container = new ContainerClass(); List<? extends ItemBaseClass> list = container.foo(); Iterator<? extends ItemBaseClass> iterator = list.iterator(); ItemBaseClass item = iterator.next(); } }
It does, but this doesn't public static void main(String[] args) { ContainerClass container = new ContainerClass(); List<? extends ItemBaseClass> list = container.foo(); Iterator<ItemBaseClass> iterator = list.iterator(); ItemBaseClass item = iterator.next(); } }
(In reply to comment #10) > It does, but this doesn't > > public static void main(String[] args) { > ContainerClass container = new ContainerClass(); > List<? extends ItemBaseClass> list = container.foo(); > > Iterator<ItemBaseClass> iterator = list.iterator(); > > ItemBaseClass item = iterator.next(); > } > } > true... but it shouldn't. Before this change, clients would just unsafely cast to what they assumed would come out of the method, without any compiler checking for validity, optimizing for performance, or providing editing support in the IDE. I'd classify breaking that line of code as new constraints uncovering buggy code and missing documentation that should be fixed, delivering the value we're aiming to get out of taking the time to add those extra constraints.
Ok, I'll correct the CreateViewRequest and add generics back + Ed's proposed patch. Clients will have to fix a few compiler errors in this case. I'll commit that tomorrow unless someone protest.
Apparently the patch we committed for you guys breaks your build? You do not like the change we made to CreateViewRequest? You will need to tell us if the change is ok or not 199. ERROR in /build/buildroot/XXX/XXXCreationEditPolicy.java (at line 47) Iterator<ViewDescriptor> descriptors = cvr.getViewDescriptors() .iterator(); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Type mismatch: cannot convert from Iterator<capture#1-of ? extends CreateViewRequest.ViewDescriptor> to Iterator<CreateViewRequest.ViewDescriptor>
The defect was that you broke public API and asked for API to be restored. It was not restored; just broken in a different way. Please read comment #11: even that states that there will be API breakage. There is nothing for us to tell you - the public API is a contract and if it is changed in a non-compatible way it is broken.
It is dumb that we are discussing this in open source land, but James fixed this issue yesterday, it was not tagged. So we just need to rerun a build.
If we cannot add generics in a binary API compatible way, then we cannot add generics.
(In reply to comment #16) > If we cannot add generics in a binary API compatible way, then we cannot add > generics. They should be binary compatible due to "type erasure". Generics are applied at compile time to prevent bugs, but stripped from class files to maintain backwards compatibility. http://java.sun.com/docs/books/tutorial/java/generics/erasure.html The line: Iterator<ViewDescriptor> descriptors = cvr.getViewDescriptors().iterator(); ...is a coding error, the iterator method does not return an iterator of ViewDescriptor, it returns an iterator of some class that implements at least ViewDescriptor, which is denoted by <? extends ViewDescriptor>. Had generics been left out of that line, it would be a compiler warning. Since it has been applied, but applied incorrectly, once we actually implement generics below it, the error surfaces.
The previous signature of the method was: List<ViewDescriptor> getViewDescriptors() So the line we had was not a coding error. You changed that signature now to be: <? extends ViewDescriptor> getViewDescriptors() As for the binary compatibility, it is true that generics are stripped from class files to maintain backward compatibility but they do change method signature when the class file is regenerated. If you do not recompile the code that is invoking it, you will get "Method not found" exception at runtime. Hence, you did not maintain binary compatibility.
Ah, I think I see what happened here. In the original bug 243634 that added generics, I added the <ViewDescriptor> restriction. Ed Merks then validly commented that it should be <? extends ViewDescriptor> to maintain flexibility in the API. Not being a high priority to get generics in, I left the bug open to fix later, although I should have left a note to say not to commit it until I had applied Ed's change. It got committed in the meantime, resulting in this bug, which I advised Alex to fix by correcting the original mistake of leaving out "? extends" from the interface. BUT, I guess you guys had already compiled your code against the improper generics-enabled patch, which lost you the binary compatibility protections which I expected to be able to rely upon. So, technically I don't think we broke API accross a contract-protected release boundary... and even if we did, it doesn't make much sense to me to freeze in the bad API that only existed by accident for a few weeks.
(In reply to comment #19) > So, technically I don't think we broke API accross a contract-protected release > boundary... and even if we did, it doesn't make much sense to me to freeze in > the bad API that only existed by accident for a few weeks.
[target cleanup] 2.2 M6 was the original target milestone for this bug
[GMF Restructure] Bug 319140 : product GMF and component Runtime was the original product and component for this bug
