Community
Participate
Working Groups
We should review cases where we have changed JFace return types to return generic array type. More detailed discussion can be found here: http://dev.eclipse.org/mhonarc/lists/cross-project-issues-dev/msg09647.html
Just capturing this offline summary from Hendrik: Problems occur when the concrete type is not known. For example with a class like this GenericContentProvider<E, I extends List<E>> implements IStructuredContentProvider<E, I>. The returning of E[] is not possible, because it is not possible to create a new array of the E[] type. The reason for this is, that an array knows it's type at runtime, so you also have to know the type at runtime to create an array. The type parameter E is replaced at compile time, so it is not known at runtime. So if we want to change the signature back to Object[] getElements(I inputElement) this is no longer an issue, but then we lose the stronger types. So a lot of E typed parameters will also change back to Object types and maybe makes our generification less useful for the client. Currently I've just on solution for this issue: move from arrays to collections!
Note that in terms of eliminating casts, using type parameters for the method arguments of the methods does tend to help eliminate those, but using them for the return types of methods might well force new casts that didn't existing before (while perhaps helping eliminate them in the caller to the method), but generally for these content provider APIs, the callers are generic and do no casting neither to pass arguments in nor to process the data being returned. Collection-based APIs would be much nicer to write and to use. Introducing a set of parallel collection-based APIs would certainly avoid the array problem too. If that approach is taken though, will the old ones be deprecated? Would we ensure that a single class could implement both APIs, i.e., would we avoid name conflicts. It all becomes less attractive for migration if clients are forced to provide two different implementation classes in order to migrate as well as provide backward compatibility...
On the general issue of adding JFace generics (or even changing JFace API to rely on collections instead of arrays), why not starting this effort in a 4.0 branch of JFace ? As JFace bundles doesn't seem to be a singleton, it would keep backward compatibility with JFace 3.x, and would allow the benefit on productivity and code quality provided by more moderns APIs for adopters of JFace 4.0.
Suppose one project used the new APIs and another project used the old ones and a third depends on both those projects with the hope they'll continue to work well together. Will they work well together?
(In reply to Ed Merks from comment #4) > Suppose one project used the new APIs and another project used the old ones > and a third depends on both those projects with the hope they'll continue to > work well together. Will they work well together? Frankly I'm not sure. I'll try to find time to test this by the end of the day. My guess is that if none of the lower-level bundles (A & B) re-export jface APIs and don't have references to JFace types in the signature of visible classes, there is not reason for a conflict to happen, and bundle C can choose to use the version of JFace it prefers. But in any case, I'm sure that if jface packages are re-exported or JFace is used in the API, it will deal to OSGi errors or ClassCastException. I guess some EMF use-case, since EMF has many extension points, would highly risk to fall in the conflicting case. Max (Andersen) suggested in a Twitter discussion creation of a org.eclipse.jfacex project (forked from JFace and then adapted to use collections, generics and everything that represent some risk of backward uncompatibility). Would that make sense?
I think jfacex is the better way and e.g. break compat completely (e.g. return IStructuredSelection) but a jfacex should not only cover the problems of generics but also e.g. how we handle virtual tables/trees which is fairly broken in 3.x
This starts to look like a project proposal ;)
The scope of the GSOC project from Hendrik is to try to add generics to JFace without breaking compatibility.
I personally also like the idea of an jfacex project. But like Lars said, the scope of my GSoC project is to generifie JFace without braking the compatibility. Within the last two month I saw how hard it is to generifie an old API like JFace. Generics are not the easiest language element of java and keeping full compatibility to the old API, will reduce the usage of the generics. I would love to help to start a jfacex project, but my time is limited and the GSoC ends this month.
If someone wants to fork and work on a jfacex project they are welcome to, but I think in the end it will not be worth the migration pain. If it is not compatible it won't be widely adopted and coexistence would be impossible. Within the realm of binary compatible options, there are really two choices here: 1) Use Object[] in all cases where arrays are used, and just try to take advantage of generic types in other methods where that is available. 2) use E[], and live with the fact that generic concrete classes extending interfaces like IStructuredContentProvider may not be possible. Unlike collections, the majority of concrete content providers do know their element type so on balance this looks more useful. Regardless of those two, there are also several examples in JFace already where there are parallel APIs for both collections and arrays. I think this can be widened in a judicious way to continue moving towards more type safety.
Hello, Technically there is the not yet mentioned possibility to extract the "need of creation of the problem arrays of unknown type" from base super implementation to separate interface and let the final concrete implementation to pass an instance via constructor. Hard for me to explain better in words, but the code below is pretty straightforward: public static interface Something<T> { public void somethingUseful(T t); public T[] evilMethod(); } public static abstract class ArrayFactory<T> { public abstract T[] newArray(int size); public T[] toArray(Collection<? extends T> list) { return (T[]) list.toArray(newArray(list.size())); } } public static abstract class AbstractSomething<T> implements Something<T> { protected final ArrayFactory<T> myArrayFactory; public AbstractSomething(ArrayFactory<T> arrayFactory) { myArrayFactory = arrayFactory; } protected abstract List<T> getList(); @Override public T[] evilMethod() { return myArrayFactory.toArray(getList()); } @Override public void somethingUseful(T t) { // do something sueful } } public static class MyItem { } public static class SomethingImpl extends AbstractSomething<MyItem> { public SomethingImpl() { super(new ArrayFactory<MyItem>() { @Override public MyItem[] newArray(int size) { return new MyItem[size]; } }); } @Override protected List<MyItem> getList() { return Collections.emptyList(); } } Right now the constructor SomethingImpl() does not look great, with Java 8 and closures it will look much better. Have I missed some principal problems with this approach?
Please ignore the "static's" -- it was an inner classes for me to use single editor page.
One could just pass in a java.lang.Class for the component type and use Array.newInstance. Note that you could have made the same argument for Java's collections because clearly Collection.toArray would have been much nicer to return E[]. But then you'd have to do new ArrayList<Integer>(Integer.class), so it's not nicely compatible with the pre-generics constructors. I imagine the platform wants to keep all the existing implementations of content providers nicely compatible in much the same was the goal when Java collections where generified. But note that the argument is stronger that just this convenience. For example, consider the following method: List<String> foo() { List<Object> strings = new ArrayList<Object>(); strings.add("a"); strings.add(1); strings.add(1.0); for (int i = 0, size = strings.size(); i < size; i++) { if (!(strings.get(i) instanceof String)) { strings.remove(i); } } return (List<String>)(List<?>)strings; } At the end, we know the list contains only strings so we know that the unchecked cast on the return is safe because we've checked it ourselves. Such logic wouldn't be possible if toArray returned E[] because anyone calling toArray on the result would find it returns Object[], not String[]. The same type of issue applies for content providers. I imagine any argument that says "We should generify content providers like Java collections were generified, because they're like collections" and then also argues "We should generify content providers differently from how Java collections where generified" is probably a suspect argument.
My opinion, the E[] shouldn't be removed. As in the current generic implementation this would remove too much type information. The result of this change would be the lose of the E type for example in the LabelProvider. This would end in client code like this: ListViewer<Person, List<Person>> listViewer = new ListViewer<>(parent); listViewer.setContentProvider(new IStructuredContentProvider<Person, List<Person>>() { ... }); listViewer.setLabelProvider(new LabelProvider<Person>(){ @Override public String getText(Object element) { return ((Person)element).getFirstName()+" "+((Person)element).getName(); } }); So there are still casting, the castings we wanted to avoid. So if we want to keep E[] implementers of ContentProviders, who don't know the concrete type have different opinions. 1. Still use rawTypes (Okay ugly warnings, but every thing should stay the same) 2. Implement use <Object,Object> Viewer and ContentProvider(No use of the generic feature,but the erasures will lead to the old signatures) 3. Provide the type information via a "Class<E> componentType" parameter to use Array.newInstance(componentType). (So implementers have to change there API to get the type information, but it would be the cleanest way to do) As example the generic ArrayContentProvider( https://git.eclipse.org/r/#/c/16409/1/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/ArrayContentProvider.java ) (4.) It seems to be able to get some type information at runtime via reflection API, but I'm not sure if this is really use full and should ever be used. http://stackoverflow.com/questions/9548779/java-generics-accessing-generic-type-at-runtime From my point of view the three options are the same options you have, when you switch to java 1.5 and have arrays combined with collections in your code. So I think this is similar to the introducing of generics itself. We want to be compatible to old code, but also we want to allow the clients to produce typesafe code. So maybe this is only possible with inconvenient changes at the client code. But maybe there are other ways to introduce generics to the JFace Viewers, than the current one.
In your example » public E[] getElements(Object inputElement) { 115 53 » » » return (Object[]) inputElement; » » if (componentType != null) { 116 54 » » } » » » if (inputElement instanceof Object[]) { 117 55 if (inputElement instanceof Collection) { » » » » return (E[]) inputElement; 118 56 » » » return ((Collection) inputElement).toArray(); » » » } the cast is unchecked and wrong. You need to test that the inputElement's actual component type is a subtype of the componentType class. If it's not, should you create the right type of array and copy the contents to that? This is also wrong: » » if (inputElement instanceof Object[]) { 128 » » » return (E[]) inputElement; 129 » » } It's an unchecked cast and you've checked nothing. Hopefully people will read the Java doc and change their code to know that this will just not work properly. It's clear that defining an I helps make writing the method simpler, but defining E makes writing the method harder...
(In reply to Ed Merks from comment #15) Retro-fitting generics IMHO sometimes leads to code in which you can construct a CCE. The generified code is hopefully safer that before (without generics) but not perfect as we have to honor API stability. What should not happen is a CCE in code which supplies the correct E and I as input to the viewer as uses correct type parameters. As far as I see, this can not happen with the code of Hendrik (he adjusted his code from the first issue you reported). If you see such an example could you please provide it, so that we can try to solve the underlying problem?
It's not just a matter of using correct type substitutions for E and I but also providing new value parameters for new constructors and new methods. That's quite different from how the Java runtime itself was generified. There are no corresponding examples of such new methods taking class arguments being added. It's hard to comment on any changes without a link to see them. Looking at the previous changes I see this method doesn't look thread safe: » public static <E> ArrayContentProvider<E> getInstance(Class<E> componentType) { 96 97 » » @SuppressWarnings("unchecked") 98 » » ArrayContentProvider<E> arrayContentProvider = (ArrayContentProvider<E>) instanceMap.get(compone ntType); 99 » » if (arrayContentProvider == null) { 100 » » » synchronized (ArrayContentProvider.class) { 101 » » » » arrayContentProvider = new ArrayContentProvider<E>( 102 » » » » » » componentType); 103 » » » » instanceMap.put(componentType, arrayContentProvider); 104 » » » } 105 » » } 106 107 » » return arrayContentProvider; 108 » } It's not safe to read the map while something else might be adding to it, even if you're careful that two adds can't happen at once. Consider again how different this change is from Collections.emptyList where you really don't end up with many different emptyList instances; just casted versions of a single one. Doesn't all this given you even a little bit of a nagging feeling that there's something a little fishy about the approach being taken here?
I also can't help but think that if you're going to do something this disruptive, you'd be better to focus on producing better APIs, i.e., ones that aren't array based in the first place. After all, isn't it the case that most client code is using collections to build the results and end up having to produce arrays at the end, just to satisfy JFace's inconvenient return type? Making it even more difficult to conform to the return type by making that type stricter really doesn't corroborate the argument that we're achieving the goal of making it easier to implement such providers. And of course there's no point in trying to make the providers easier to use, because they're generally not used directly by clients as API but rather used internally in generic container classes (viewers), and those containers benefit not at all from the stricter return types. In fact, they won't generally be able to check conformance (unless you plan to change all the viewers to require Class arguments) so the non-conformance to E[] will likely go unnoticed (the generic container will effectively use Object[]), again making you wonder (I would hope), what's the point in forcing clients to return E[]?
(In reply to Ed Merks from comment #18) > [..] Making it even more difficult to conform to the return type by > making that type stricter really doesn't corroborate the argument that we're > achieving the goal of making it easier to implement such providers. [..] > what's the point in forcing clients to return E[]? I totally second that. As long as implementations of these APIs don't become significantly simpler and safer in the type's sense, there is no point in using a return type E[]. In fact, implementations could have uses a stricter return type already in the past, e.g. String[] is a valid specialization of Object[]. Did they choose to do so? Is there any added value in changes at exactly that place? (In reply to Ed Merks from comment #17) > [..] > Consider again how different this change is from Collections.emptyList where > you really don't end up with many different emptyList instances; just casted > versions of a single one. Doesn't all this given you even a little bit of a > nagging feeling that there's something a little fishy about the approach > being taken here? Yes, this is a very valid point. I wonder, what's so fundamentally different from this problem compared to other generification scenarios, e.g. in the java.util.Collection, that it deserves such a special solution?
> > what's the point in forcing clients to return E[]? The benefit is for example in the LabelProvider which can receive E as input and client don't have to cast. You find some examples of potential client code listed by Hendrik here: http://blog.vogella.com/2013/08/12/generics-for-the-combo-list-and-tableviewer/ From the point of view of customers implementing viewers I think the current solution simplifies the customer code as he can remove several casts in his coding. It also avoids simple errors by the customer as it gives him some structure rather than only Object. >>After all, isn't it the case that most client code is using collections to >>build the results and end up having to produce arrays at the end, just to >>satisfy JFace's inconvenient return type? I completely agree but the boundary condition requested for this GSoC project was that we do everything API compliant. getElements[] is unfortunately API. > Doesn't all this given you even a little bit of a > nagging feeling that there's something a little fishy about the approach > being taken here? I think it would be great to have a better solution which is still API complaint. In case you have such a solution in mind, it would be great if you can share it. The current solution is available as johna/402445 branch. In general everybody is more than welcome to suggest improvements or replacements to our existing solution proposal. To repeat our boundary condition: to my knowledge platform will not allow any breaking API changes.
Looking just at a class like ArrayContentProvider it is clear there is no value being added in the generification. You have to look at the context of the whole change though. After this bug was entered Hendrik created a branch where he explored the consequence of rolling back array return types. It has rippling consequences to many other classes where the type information is quite useful. The majority of JFace client code consists of custom content providers, label providers, filters, sorters etc, where the concrete type *is* known. The data passes in a chain from content provider, through filters and sorters, and then to label providers. To be useful, the generification has to be intact throughout the chain to allow the type knowledge to flow through from the provider to the rest of the classes. There are well over 100 content providers in the workbench alone, and so far ArrayContentProvider is a rare exception where concrete type information is not known. For the large majority of content providers the type information is known and is being lost today by casting through Object[] and cast back to concrete classes over and over again in client code. From the examples I have seen ported so far it genuinely does look cleaner. Maybe in the modelling world there are more examples of generic containers so it would be interesting to validate whether there are other problematic cases. As Lars said all the changes so far have been released in branch "johna/402445" so it is fairly easy to check it out. Note the work is still not complete and there are a few helper classes, notably the selection classes, that have not been done yet.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
