Community
Participate
Working Groups
Build ID: I20080502-0100 (Eclipse 3.4M7) Consider following rather common scenario: /** @noimplement Do not implement directly. Extend abstract base Foo instead. */ public interface IFoo { //... } public abstract class Foo implements IFoo { //... } With following client code: interface IFooBar extends IFoo { public void bar(); } class MyFooBar extends Foo implements IFooBar { public void bar(); } In this case, the client code legally implements IFoo, because it does so by extending abstract base Foo. This scenario should not be flagged as an error: Just extending an interface does not introduce any binary compatibility issues. But currently the API Tooling flags IFooBar extends IFoo as an error.
We elected to close this loophole - i.e. if an interface is marked as @noimplement then clients should not be able to "sneak" an implementation in by extending it. Since, in this case they are actually implementing the base interface as well. Same goes if you are subclassing an abstract class that implementents an interface - the client is implementing the interface. See bug 227368 for the trail on this.
(In reply to comment #1) I understand the intent of forbidding to "sneak in" implementations. But looking at the whole picture, I find it problematic for two reasons: (1) The error message is not consistent with the tag. Since the message is flagged at the place where the interface is extended, and not where anything is implemented. You might argue that extending an interface leads to implementing it somewhere eventually. But still, looking at the error message alone, it's unclear how to fix the issue since there is nothing being implemented at the place where the error occurs. Changing the @noimplement boilerplate text to "...not intended to be extended or implemented by clients" would slightly improve the situation, but not really fix it. A better fix would be to flag the error at the place where the (child) interface is actually implemented, rather than where the interface is extended. (2) It is a very common pattern that an interface is declared for clients to use, and along with the interface there is a provider API that declares an abstract base class to extend for the providers. The abstract base class is provided in order to ensure that the API interface can be evolved by adding more methods in the future, without breaking any clients. So that's the intent of @noimplement in our case -- being able to evolve the interface in the future. Now in our case, the interface is used legally because at the place where it (or its child interface) is actually implemented, ALL MEMBERS OF THE INTERFACE are actually inherited from the abstract base class. I think that a proper algorithm could look approximately look like this: WHERE class X implements interface Y * FOREACH parent interface YP of Y that has @noimplement * IF any base class XP of X already implements YP * DO NOT FLAG an error ... because the error would already have been flagged at the parent XP, and might have deliberately been ignored at that place because XP is a part of the framework that also declares YP. How does that sound?
(In reply to comment #2) > Changing the @noimplement boilerplate text to "...not intended to be > extended or implemented by clients" would slightly improve the situation, > but not really fix it. A better fix would be to flag the error at the > place where the (child) interface is actually implemented, rather than > where the interface is extended. Updating the boilerplate comment would be good. I still think that flagging interface extensions as problems is correct in this case (since the intent is that you can't implement/extend the interface). > (2) It is a very common pattern that an interface is declared for clients to > use, and along with the interface there is a provider API that declares > an abstract base class to extend for the providers. > The abstract base class is provided in order to ensure that the API > interface can be evolved by adding more methods in the future, without > breaking any clients. So that's the intent of @noimplement in our case -- > being able to evolve the interface in the future. > Now in our case, the interface is used legally because at the place where > it (or its child interface) is actually implemented, ALL MEMBERS OF THE > INTERFACE are actually inherited from the abstract base class. > I think that a proper algorithm could look approximately look like this: > WHERE class X implements interface Y > * FOREACH parent interface YP of Y that has @noimplement > * IF any base class XP of X already implements YP > * DO NOT FLAG an error > ... because the error would already have been flagged at the parent XP, and > might have deliberately been ignored at that place because XP is a part of the > framework that also declares YP. > How does that sound? I'm not sure I want to make this too complicated... but note: If interface IA is @noimplement, and class X implements IA (in same bunlde as IA). Then subclasses of X are not flagged as problematic... unless they explicitly claim to implement the interface IA (i.e. they don't use the implements keyword with IA.. they just extedn X). I think this should be sufficient/simple enough? But as well in this case, marking an interface as @noimplement feels wrong as the client is providing an implementation (even if it is via an abstract super class). Generally, this pattern was followed in the past to allow an implementation to evolve (add more methods in a non-breaking way). Looking back, where this was done there probably should only have been an abstract class and *no* inteface. But, based on evolution we ended up with both the interface and abstract class. I don't think it's a good pattern to use though... moving foward I think API providers should only provide an abstract class in cases like these.
(In reply to comment #3) > interface extensions as problems is correct in this case (since the intent is > that you can't implement/extend the interface). In my case, the intent is that the interface can be extended or implemented, but implementations must use the provided abstract base class. See below for more. > IA). Then subclasses of X are not flagged as problematic... unless they > explicitly claim to implement the interface IA (i.e. they don't use the > implements keyword with IA.. they just extedn X). > > I think this should be sufficient/simple enough? It is sufficient when the interface is only directly implemented / provided. But it's not sufficient when clients want to add additional capabilities to the interface. In my case, the framework provides an ISubSystem, along with an abstract SubSystem class for the provider. Clients can extend the interface to give "meaning" to the generic subsystem: IRemoteFileSubSystem, ITerminalSubSystem and the like. The implementation that they provide is only for the new "add-on" features that they provide, but not for the base functionality. Now one could argue that aggregation would be better than extension, and the additional capabilities could be provided by clients by means of IAdaptable or getAdditionalFeatures() or the like. But, historically, extension of the interface has been chosen and I don't think it's totally wrong doing so. > But as well in this case, marking an interface as @noimplement feels wrong as > the client is providing an implementation (even if it is via an abstract super > class). The client is not providing an implementation of the "blacklisted" interface. It's only providing an implementation of the add-on features which extend the blacklisted one. All blacklisted implementation comes from the authorized base class. > Generally, this pattern was followed in the past to allow an > implementation to evolve (add more methods in a non-breaking way). Looking > back, where this was done there probably should only have been an abstract > class and *no* inteface. I thought about this, but the drawback is that Java limits us to a single inheritance hierarchy in this case. I generally like splitting up interface and abstract base class, because this allows for using the interface as a "mix-in" with a totally different kind of implementation eventually. Think about mock objects, for instance. > though... moving foward I think API providers should only provide an abstract > class in cases like these. I don't think so, see above. Why do you think that my proposed algorithm is too complicated? What is says is basically a bit similar to what Java does with the "final" or "synchronized" keywords: it says that having the whole interface marked "noimplement" is equivalent to having each method marked "noimplement". Makes sense?
FYI, this bug is the number one reason for API Tooling false positive reports in our workspace. I have outlined the idea for an algorithm to fix this in comment #2. Could this be targeted for 3.5 ?
This is causing compiler warnings in DataBinding as well. There is no way we could unify the our API on a single base class, so we use interfaces with @noimplement to ensure we can evolve our API as needed. Users are expected to extend one of our base classes: org.eclipse.core.databinding plugin: /** @noimplement */ interface IObservable org.eclipse.jface.databinding plugin: /** @noimplement */ interface ISWTObservable extends IObservable { public Widget getWidget(); } /** @noimplement */ interface IViewerObservable extends IObservable { public Viewer getViewer(); } Originally we had IObservable marked with both @noextend and @noimplement, and this has been causing errors from API tools. We thought that by removing @noimplement, those errors would go away, and it was surprising when they didn't. My take on this is that if interfaces should really be locked down, then they should be tagged @noimplement *and* @noextend. With just @noimplement I would rather not see the API error when someone is just adding an extension to an interface. Martin's suggested algorithm in comment #2 seems a reasonable approach for preventing illegal implementation of @noimplement interfaces.
Created attachment 107348 [details] Sample set of projects demonstrating the desired API If you Import, General -> Existing Projects into Workspace the attached file, you'll see an error from API tools on IExtensionInterface for extending the @noimplement ICoreInterface. Also, I would expect to see an error on ExtensionImplementation for directly implementing ICoreInterface (via IExtensionInterface) rather than extending a legal implementation of the superinterface.
Just to clarify: I *don't* expect an error on IExtensionInterface, and *do* expect an error on ExtensionImplementation.
Ping.
(In reply to comment #9) > Ping. Yeah... we don't really have a good solution to this issue yet. If we re-define what @noimplement means, then all (99.9%) developers that have used the tag have to go back and add @noextend to their interfaces. It changes/breaks the existing meaning of the tag.
(In reply to comment #10) To be honest, I don't really see what's bad or problematic about extending an interface. The API Tooling tags are all about specifying API constraints, such that the API can be evolved in the future, or am I misunderstanding? Just extending an API interface (as long as it isn't implemented) doesn't hurt anybody except for the theoretical case that a new field or method should be added to the base, but that creates a name clash since an extender already has such a method. According to the Evolving Java-based APIs: "if the likelihood of this kind of name coincidence is sufficiently low, this kind of change is often treated as if it were non-breaking." http://wiki.eclipse.org/Evolving_Java-based_APIs#Example_4_-_Adding_an_API_method That being said, I do not believe that 99,9% of the users even care whether their interfaces can be extended or not -- as long as the tag is checked where it really matters, and that's where the interface is being implemented. Even the @noimplement boilerplate text doesn't mention the side-effect of doing "noextend" at the same time. I'd even daresay that the side-effect of also performing noextend checks is an unexpected behavior (==bug) that's confusing users (not only me, apparently). And with respect to the end of comment #3, I think it's not fair to restrict tooling for a case that's considered "obsolete" although often used. In my opinion, the separate interface + base class pattern is a direct result of Java's single inheritance paradigm, which requires to create interfaces in order to "mix in" additional functionality; having an interface, I can do much more with it than with the abstract class. Plus, that pattern creates a clear separation of client concerns versus provider concerns (think API versus SPI==service provider interface). I think that the tags should do what they say, and for those (likely few) who care about extending interfaces, you could add an @noextend tag - simple, explicit and clear. For all the others, it's likely sufficient to make the check at the time the interface is actually implemented, and to allow separate class/interface hierarchies as per my algorithm in comment #2.
(PS I hope you don't read this as nagging or ranting, I find this discussion very interesting and would like to come up with the best technical solution).
In DataBinding we took the @noextend and @noimplement tags at face value (according to what their names imply) and tagged our interfaces with both. So this change would affect us positively, not negatively. When we started getting API tooling errors, we realized that we're okay with folks extending our interfaces as long as they extend one of the provided base implementations. We expected the API tooling errors to disappear when we removed @noextend from the interfaces, and it was surprising when it did not. It would be interesting to take a poll of committers using API tooling to see how many of them used the tags as we did in DataBinding, versus how many use @noimplement to prevent extending interfaces, or expect it to behave this way. Is there a way we could run a script during a build to see which interfaces are tagged @noimplement, or does API tooling create some sort of report during the build?
What about sending an E-Mail to eclipse.org-committers with a poll? I created a little poll which you can test here: http://www.misterpoll.com/admin/polls/134573 Suggestions to change wording or alternatives are welcome. In the E-Mail, I'd propose to only ask committers who have been actively using API Tooling so far to vote.
Sorry, wrong URL, the right one is http://www.misterpoll.com/polls/355912
Do you guarentee that extenders of the base class/interface do not override/provide an implementation of the interface by making the implementation methods final? What are the semantics here? is a client allowed to override one of the interface methods from the @noimplement interface?
(In reply to comment #16) > Do you guarentee that extenders of the base class/interface do not > override/provide an implementation of the interface by making the > implementation methods final? What are the semantics here? is a client allowed > to override one of the interface methods from the @noimplement interface? Couldn't this case be handled in the base class using @nooverride?
(In reply to comment #17) > Couldn't this case be handled in the base class using @nooverride? Yes, but I am wondering if you did anything in the past to guarentee that clients don't provide an implementation.
(In reply to comment #13) We in Text and JDT/UI like to code according to specs (in this case the shipped help documents that describe the @no* tags), and not on the basis of what we guess or think a command should do. So a usage report in existing code is of little value to this discussion. Nevertheless, I think it would make sense to support the scenario from comment 0. Of course, allowing interfaces to extend an @noimplement interface can only be allowed if the algorithm from comment 2 is implemented. In comment 0, class Foo has to decide whether it allows clients to subclass, and if it does not have an @noextend, it also has to make sure that it makes methods from IFoo final or @nooverride if the subclasses should not change them. This is in fact independent from whether Foo implements IFoo or not, and therefore also independent from the question whether clients should be allowed to extend an @noimplement interface. So in fact, the only visible change would be that IFooBar is not flagged with a problem any more, but a client class DirectFooBar would be flagged: class DirectFooBar implements IFooBar { public void bar() { } } While this is a slightly breaking change to the published semantics of @noimplement, I don't think it would be a problem for any client, and since people have shown real-world scenarios where the change would be beneficial, I think it should be implemented.
BTW: This would also allow us to remove an API problem filter in the org.eclipse.ui.workbench.texteditor plug-in: ITextEditorActionConstants extends org.eclipse.ui.IWorkbenchActionConstants, and both are @noimplement.
(In reply to comment #18) > (In reply to comment #17) > > Couldn't this case be handled in the base class using @nooverride? > > Yes, but I am wondering if you did anything in the past to guarentee that > clients don't provide an implementation. There's nothing I can do to "guarantee" that clients won't implement our interfaces. However our javadocs have always clearly stated these restrictions: > * @noimplement This interface is not intended to be implemented by clients. > * Clients should instead subclass one of the classes in the > * framework that implement this interface. Note that direct > * implementers of this interface outside of the framework will be > * broken in future releases when methods are added to this > * interface.
(In reply to comment #16) > Do you guarentee that extenders of the base class/interface do not > override/provide an implementation of the interface by making the > implementation methods final? I don't think that this is relevant here: restricting "override" is a matter between the base class and its extender only, isn't it? The interface and/or base class would have Javadocs specifying the API Contracts that overriders have to abide by, regardless of whether methods are final or not. For me, then #1 reason of marking a class @noimplement is in order to be free to add fields or methods in future releases without breaking API, and in order to be able to make the API specification from a client (user's) point of view only. The provider's point of view can be specified in an extendable base class if I want the provider to be exchangeable.
(In reply to comment #22) > For me, then #1 reason of marking a class @noimplement is in order to be free to > add fields or methods in future releases without breaking API, and in order to > be able to make the API specification from a client (user's) point of view only. > The provider's point of view can be specified in an extendable base class if I > want the provider to be exchangeable. Same here.
The arguments for supporting @noextend and @noimplement on an interface are reasonable. We need to consider how we could ensure that all components using the @noimplement will add the @noextend tag where required (else they are vulnerable to API abuse). As well, we'd need to have special consideration in API tooling that would allow interface restrictions to be changed in this case - i.e. adding a restriction to an interface is OK in this situation since it was not previously supported. To ensure propert migration, a broadcast to a mailing list, architecture meeting, and an entry in the migration guide is likely the best we can do. Marking for 3.5 consideration.
I pledge to buy a beer at the next EclipseCon for whoever implements this. :)
Created attachment 117700 [details] patch This patch is a work in progress. It adds back the noextend tag to interfaces, adds regression tests for the new availability of the tag and implements the indirect implementation algorithm mentioned. With the patch problems are detected for: 1. an interface directly extending a noextend one 2. a class directly implementing a noimplement interface 3. a class implementing an extension interface iff the class has no parent class that implements the interface The indirect implement problem message thus far reads: "ExtensionImplementation illegally implements IBaseInterface via IExtensionInterface" A bit more testing would be good and I am creating a 'clean up' wizard that will aid in adding noextend tags with noimplement tags (where the user wants them).
opened bug 255104 about a method to clean up (pair tags).
applied patch to HEAD with minor changes to accommodate the pde plugins moving.
Olivier please verify
I don't see any value to the pattern described in comment #0. Since it would be illegal for implementations of IFooBar to implement any method inherited from IFoo, extending IFoo doesn't buy you anything. I.e., this is equivalent: /** @noimplement Do not implement directly. Extend abstract base Foo instead. */ public interface IFoo { //... } public abstract class Foo implements IFoo { //... } With following client code: interface IFooBar { public void bar(); } class MyFooBar extends Foo implements IFooBar { public void bar(); }
Martin, could you comment on comment#30. Would this pattern work? Why do you require to extend the interface? is it just history?
comment #4 says: > I generally like splitting up interface and > abstract base class, because this allows for using the interface as a "mix-in" > with a totally different kind of implementation eventually. Think about mock > objects, for instance. I think by the same argument, by extending the base interface rather than having a standalone interface with the additional methods, you're reducing possible opportunties for "mix-in" use of your new interface. I.e., say the new methods on IRemoteFileSubSystem turn out to be useful in different scenarios where the full ISubSystem API isn't applicable. By making IRemoteFileSubSystem extend ISubSystem, you've reduced the reusability of this new interface without getting any benefit (as far as I can see).
(In reply to comment #30) > I don't see any value to the pattern described in comment #0. An advantage of letting IFooBar extend IFoo is that clients can write APIs where they use IFooBar in a method signature and they can statically ensure that IFoo methods are available on all objects of type IFooBar. Otherwise, they would have to cast to IFoo to call those methods.
(In reply to comment #30) I was about to write quite the same as Markus, but with different words. Compared to John's proposal, clients see a difference when they start passing around IFooBar objects, and users of IFooBar frequently need to access IFoo functionality as well. The "mix-in" paradigm is nice, but at times you'll know that in order to do anything useful with your mix-in you'll need the base interface as well. In those cases, it makes sense to extend the interface. It's just what the "extend" keyword suggests: some provider engages on an API contract with clients that's an extension of an existing API contract. We could, theoretically, achieve that with plain abstract classes as well, but due to the Java single inheritance limitation that would limit the ways how it can be implemented. There can be multiple alternative implementations.
Ok, that sounds reasonable. My next question is whether this can be achieved without breaking the old semantics of @noimplement, which would require every API using API tools to be revisited to decide if the new tag is reasonable. For the Eclipse project at least, our API guidelines state that "no implement" implies "no extend", so almost every interface would need the new tag. An alternative is to have a new tag to explicitly allow extension where desired: @noimplement Clients cannot implement this interface @extend Clients may extend this interface That would allow the people who want this to get this different behaviour, without breaking the meaning of @noimplement for existing API.
My only observation from John's suggestion of having an @extend tag is that there is not way to say "implement this interface but don't extend it". In practice, I'm not sure that matters. This also changes the style of "API restrictions", as @extend specifies an exception to a restriction rather than a restriction. It requires less changes on the client end to exsiting specifications, but makes our tags inconsistent. Also, not sure how much that matters.
After some more discussions with John (A), we agree to support the @noextend restriction on interfaces. Developers will need to decide whether to add the tag to existing interfaces documented as @noimplement, based on the following observation. * If you intend to be able add constants to an interface in the future (public static final fields), then you need to have both @noimplement and @noextend tags. (@see http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_Interfaces - Adding a field to an interface is binary incompatible if clients can implement the interface or an extension of it). For example, IDebugUIConstants is tagged as @noimplement but has new constants added in 3.5 - API tooling notes this as an error since someone could potentially extend the interface. Adding the @noextend tag fixes the error. Ideally, API tooling could tell that there are no API implementations of IDebugUIContants that can be legally subclassed (thus adding a field should not be an error). This would be an expensive analysis, so I think we'll tag these as errors for now. In practice, interfaces tagged as @noimplement that do not contain constants (and will not have added constants in the future), can remain as @noimplement without adding @noextend, without consequence. But, we should encourage developers to add the @noextend tag where field addition is possible.
Verified.
