Community
Participate
Working Groups
Build R2.1 Historically, IWorkingCopy gathered all working copy concerns, and compilation units implement this interface, though only the factory method makes sense for them; thus their implementation of the working copy features do nothing relevant to clients. Working copies also implements the spec'ed factory method, but it doesn't work for these. The proposal is to have IWorkingCopy extend ICompilationUnit with only relevant services be exposed to raw compilation unit clients. Along the same line, the factory method would only be defined on compilation unit, and answer an IWorkingCopy (instead of a useless IJavaElement as today).
Jim - do you agree with this API breakage ?
Requested feedback on jdt newsgroup: Historically, the JavaModel defined ICompilationUnit and IWorkingCopy; but we believe they are incorrectly related to each other (ICompilationUnit extends IWorkingCopy). We would like to take advantage of the 3.0 release so as to reorganize our API and make it more coherent. Compilation units denote file units, and working copies are providing an in-memory abstraction of a unit being edited. A working copy can be created from a compilation unit; its contents can be changed independently from the underlying unit, as long as the working copy contents isn't committed. A working copy thus adds functionalities to a compilation unit, and working copy services are defined by IWorkingCopy. However, ICompilationUnit is currently extending the IWorkingCopy interface, though it should be the other way around. We believe that client code will read better, and will provide some insights on how to migrate existing code. If you disagree with this evolution, please let us know, and annotate bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=36987 accordingly.
I suggest IWorkingCopy and ICompilationUnit should be handled independently and not extend each other, in neither direction. Having inspected the code it looks like both interfaces are representing independent concepts. Only four methods defined for IWorkingCopy are used in the CompilationUnit context: getOriginal(type) getOriginalElement() isBasedOn(resource) isWorkingCopy() I suggest to create a new interface IWorkingCopyCandidate (or so) that contains just these methods. ICompilationUnit would "extend" this interface. The original IWorkingCopy is only implemented by the "real WorkingCopy" classes. Here another (not yet fully elaborated) idea: I think ideally ICompilationUnit should not know about the notion of "Working Copies" at all. Concrete implementations (e.g. those that are WorkingCopies) may know more about it. It might be possible to move into this direction (if desired) by having a more detailed look at the current usages of the methods mentioned above and doing some refactorings.
Actually, 3 of these methods, i.e. getOriginal(type), getOriginalElement() and isBasedOn(resource), are specific to a working copy. They don't make sense for a compilation unit. The 4th method, i.e. isWorkingCopy() would need to move to ICompilationUnit if reorganizing the hierarchy. Other methods like getAllTypes() make sense for both a working copy and a compilation unit. So it is natural that IWorkingCopy extends ICompilationUnit.
I agree with Jerome, working copies are pretty much in-memory units which remember which file units they correspond to.
I agree with Jerome that getOriginal(type), getOriginalElement() and isBasedOn(resource) don't make sense for a compilation unit. I would also say that isWorkingCopy() does not make sense for a compilation unit. Conceptually it predicts something on the inheritance tree below ICompilationUnit (namely that there will be a "sub interface" IWorkingCopy). One may argue on whether this is good style or not, but I would prefer that a base class/interface should not make assumptions on its derived classes/interface. Also a method like "isWorkingCopy()" leads to conditional code that may be avoided by using other implementation techniques (e.g. using overwriting methods etc.). So I suggest to also get rid of the isWorkingCopy() method. --- On the "naturalness" of "IWorkingCopy extends ICompilationUnit": Just because methods make sense both for working copies and compilation units would not lead directly to this extension. Obviously it was not obviously that "IWorkingCopy extends ICompilationUnit" since it is currently the other way round ("for historical reasons"). Looking at this situation as an "outsider" I would suggest to step back for a second and rethink if "extending" is really the right mean in this situation. What do we really gain in extending either IWorkingCopy or ICompilationUnit? Wouldn't it be sufficient just to have both interface side-by-side? Also: when I look at "ClassFileWorkingCopy" the code somehow "smells". Changing the "extends" order would not help here. Again: maybe "extends" is not the right thing. What would you think of the following hierachies? public interface IWorkingCopy { // as today public interface ICompilationUnit extends IJavaElement, ISourceReference, IParent, IOpenable, ISourceManipulation, ICodeAssist { // i.e. as today, without IWorkingCopy public class CompilationUnit ... implements ICompilationUnit ... { // as today public class WorkingCopy extends CompilationUnit implements IWorkingCopy { // i.e. here we use the "parallel" IWorkingCopy interface public class ClassFileWorkingCopy implements IOpenable, IJavaElement { // i.e. just the interface currently needed are declared. // Note that this will simplify the class significantly. // // (BTW: Having analysed the current usage of the class, // may "ClassFileWorkingCopy" is not a good name. It does not need // anything from the IWorkingCopy interface).
I agree that isWorkingCopy() is relevant only if the 2 interfaces are related. However I still don't see how you could call getAllTypes() on a working copy (or any of the methods defined on ICompilationUnit, ISourceReference, etc.) if it doesn't extend ICompilationUnit. You would need to know about the implementation and cast the working copy to ICompilationUnit, ISourceReference, etc.
Some of our APIs are accepting working copies in argument, which will take precedence over underlying compilation units. Search may thus answer matches in either units or working copies. Clients may need to distinguish in between these two entities, and thus the predicate is interesting to have (instead of using instanceof). Again, since working copies can be answered along with units, some common supertype must be available for easing client code. Thus, ICompilationUnit seems the most generic type to use (in the past it was IWorkingCopy).
I agree with the comments stating that they should not extend each other in either direction. Think Liskov Substitution Principle here: "Subtypes must be substitutable for their supertypes". Substitutable means in terms of pre and postconditions for *EVERY* public interface method: "A routine redeclaration [in a derivative] may only replace the original precondition by one equal or weaker, and the original postcondition by one equal or stronger." (Meyer, but copied from Robert Martin's great new book on Agile Software Development)
Jerome: >> However I still don't see how you could call getAllTypes() on a working copy... Not extending the interfaces ICompilationUnit and IWorkingCopy in any direction does not mean that it is not allowed to derive a concrete class WorkingCopy from the concrete CompilationUnit like I suggested above: public class WorkingCopy extends CompilationUnit implements IWorkingCopy {... Through this inheritance on the classes you can call getAllTypes() etc. on the WorkingCopy class without doing any casting or so. ---- Maybe one reason for the different opinions on this bug report comes from the understanding what WorkingCopies are for. When reading the IWorkingCopy documentation I hear that it is the "Common protocol for Java elements that support working copies." When I read your comments I mainly see that you are talking about CompilationUnits and not Java elements in general. So do you think that CompilationUnit will be the only Java element supporting IWorkingCopy, even in the future? So are we in reality talking about an "ICompilationUnitWorkingCopy" interface? The general idea described for "IWorkingCopy" seems to cover more. E.g. other elements than CompilationUnit may support "WorkingCopy". This might be an option in the future e.g. when we leave the old "file centered compilation unit" paradigm and allow working copies on a finer granulatity (e.g. when the "source elements" are stored in an object oriented database or so). Of cause one may decide that the original concept of IWorkingCopy was too general and a more specific one is a better choice. In that case I suggest to get rid of the (general) IWorkingCopy interface completely. Maybe this general IWorkingCopy interface will reappear in the future when there is a better understanding how it might be used without thinking *only* of CompilationUnits (and files). Assuming that the IWorkingCopy interface is removed there is still the fact that we have WorkingCopy instances and CompilationUnit instances. Related to this, Phillippe stated: "Clients may need to distinguish in between these two entities, and thus the predicate [ub: isWorkingCopy()] is interesting to have (instead of using instanceof)". I think it would be a good design goal to come up with a solution that does not require the clients to make this distinction. I think this should be possible (i.e. to get rid of a (public) isWorkingCopy()). Just to give you an example: ------ verify() in CommitWorkingCopyOperation is using isWorkingCopy() Notice that CommitWorkingCopyOperation is only created for a WorkingCopy instance in WorkingCopy.commit(...). So I suggest to change the constructor for CommitWorkingCopyOperation to CommitWorkingCopyOperation(WorkingCopy element, boolean force). Because we now really know that the element is a WorkingCopy: a) the cast in executeOperation() to (WorkingCopy) is safe now (even when analysing the code only statically). b) we don't need to check for "isWorkingCopy" in verify() ----- Of cause this "hiding the WorkingCopy concept to the client" idea requires some more refactoring than just switching the extends direction as originally suggested in the proposal. But this change should simplify the model for the clients of the JavaModel since there is one differentiation less they must take care of.
Udo, you're right: when we talk about working copies, we think of working copies on compilation units. The original intent of IWorkingCopy was to be more general, but in almost 3 years we didn't find a use case for a more general working copy. So yes, we would need to rewrite the spec to make it clear that a working copy is a compilation unit that exists only in memory. As for closing the gap between an IWorkingCopy and an ICompilationUnit, a discussion is taking place in bug 36888. Frank, this is good theory, but in pratice this would mean that clients would need to add casts everywhere if they want to use the ICompilationUnit methods. We're trying to simplify the life of the JDT/Core clients in 3.0.
Given we closed the gap in between units and working copies. Compilation units can simply be used as working copies (no need to have a separate entity). A compilation unit is intrisically a potential working copy, and can become one upon request (keeping the same handle). Thus creating new working copies should in a consistent manner answer back new instances of compilation unit since units already support all old working copy behaviors. Proposal is to simply get rid of type IWorkingCopy. (Asked PMC approval)
Obsolete bug. Need to keep IWorkingCopy so as not to not break existing clients.
