This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | Prevent the interfaces in org.netbeans.api.debugger.jpda package from being implementable. | ||
---|---|---|---|
Product: | debugger | Reporter: | Martin Entlicher <mentlicher> |
Component: | Java | Assignee: | Martin Entlicher <mentlicher> |
Status: | CLOSED FIXED | ||
Severity: | blocker | CC: | arseniy |
Priority: | P2 | Keywords: | API, API_REVIEW |
Version: | 5.x | ||
Hardware: | PC | ||
OS: | All | ||
Issue Type: | TASK | Exception Reporter: | |
Bug Depends on: | |||
Bug Blocks: | 59072 | ||
Attachments: |
The ugly javadoc that is created when a package private interface is added as a basis of JPDAThread.
The requested API change. The new JPDA Javadoc The complete diff of this API change including all affected modules. |
Description
Martin Entlicher
2005-05-20 16:21:44 UTC
I suggest to document that the interfaces must not be implemented in client code. If it's necessary to enforce this, we can create a new private interface, that would be a basis of all existing interfaces. This is my prefered solution, since it will not affect users of the API. Other solution can be to change the interfaces to abstract classes, which will have an abstract package private method, or final classes, or create static delegate methods (like JPDA.isSuspended(JPDAThread)). Well, to create a private basis of the interfaces is not nice at all. It adds an ugly and confusing clutter to the javadoc. IMHO it would be best if the description in the javadoc would be sufficient. It's also problematic to change the interfaces into abstract classes, since the interfaces form a hierarchy (e.g. Variable - ObjectVariable - LocalVariable), which is not possible using abstract classes. We would have to rewrite the API, but this would be incompatible also from the API point of view. Created attachment 22231 [details]
The ugly javadoc that is created when a package private interface is added as a basis of JPDAThread.
It is forbidden for a publically visible class or interface to reference a private class or interface in any part of its signature. I think a note in the Javadoc that the interface may only be implemented by certain parts of the code is sufficient. We already use this style in other APIs. Cool, thanks Jesse. Therefore I propose to add this into the javadoc of every interface in org.netbeans.api.debugger.jpda package: "This interface may evolve in the future. Do not try to implement it. Objects implementing this interface are produced by debugger core module." Please adjust the text as appropriate. Any comments from others? JG: "We already use this style in other APIs" Well Jesse, I cannot realize where we use final interfaces. I know that org.nb.jmi does that, but that is ok as we selected org.nb.jmi namespace to indicate that the evolution of the api will be different. Maybe we did this once in window system API, but otherwise I do not know about any other stable API that would use this poor style. Imho creating new interface JPDAThread2 is the most standard solution. That one has imho chance to be accepted in API_REVIEW_FAST. All others (including comment in javadoc) are in fact incompatible and need to go thru standard review. The goal is to make sure that all users of the api agree with it being changed incompatibly and record that decision. Side note: changing interfaces to abstract classes is incompatible change for callers. Methods are called as #invokeinterface vs. #invokevirtual, so such change is going to break binary compatibility. Re. interfaces with restricted implementors: e.g. ProjectState. Anyway it is true that if you did not *initially* specify on these interfaces who was permitted to implement them, you cannot add that restriction now (without breaking compatibility, requiring a new major release version as a signal). I agree that making a new interface and deprecating the old one is the only compatible approach in this case. 1) JPDA Debugger APIs are separated to API / SPI, so it is clear what is designed to be implemented by client, even without specific notification in each class. 2) There is no reason to extend interfaces from this API. You can not use them in any reasonable way. Thats why I am suggesting to: - add some notification in JavaDoc. - add this new method to existing interface - if you think that its "incompatible" change we can update version number, notify on nbdev etc, do full review. But I do not see any reason for polluting our apis with some xxx2 things. One more question. Do we have some business reason why our API related processes are more strict than processes used by JDK team? JPDA debugger is based on JDI interface. It contains many interfaces. These interfaces are incompatibly changed in "each" new version of JDK. JDI has several implementations, and many clients. We should have some strong reason to be more strict with API changes than JDK team... The only bussiness reason is that NetBeans debugger interfaces are clasified as stable while JDI is not. Comparing these two cases is like comparing apples and oranges. If you want to compare NetBeans debugger interface evolution with some other one, select something that is stable. For example java.util.*. As far as I know our DevRev procedures, java.util.* has a small chance to be approved. For example: 1) this api is not divided to API <> SPI, this is BIG problem! 2) Classes like Arrays, Collections, Date, shoud be final - TCR! 3) They are adding new methods to (API - SPI) classes It can be incompatible change according our rules: http://openide.netbeans.org/versioning-policy.html#2 Anyway, I would suggest to do this incompatible change (+update version number), and add some notice to JavaDocs. It allows to do more changes of this API in the future. Solution with Threads2 interface does not fix problem with future imporvements. This solution is fully compatible with our processes too. Off-topic: I would indeed complain about the design of many Java platform API classes if they were submitted as new proposals; there are many mistakes in java.** and javax.** classes that it's too late to fix now. But this has nothing to do with what we are reviewing. Personally I am +0 on permitting the new method to be added, provided that 1. There is no plausible reason for anyone else to impl this interface (e.g. for purposes of proxying or composition), nor are there any known instances of third parties implementing these interfaces. 2. The design of the API and its existing documentation do not imply that the interface could or should be implemented. 3. The Javadoc (for each such interface individually) be edited to expressly forbid impl of the interface by anyone except the the debugger module. 4. Perhaps also mark an incompatible rev on the major version. But let's see what other reviewers besides myself and Yarda think about it. I agree with Jesse in that if there are no known clients of that interface then it's not necessary to create extra xxx2 interface. I have no strong opinion whether major module version should be increased or not. Awyway this particular case reminds me that all the NB reviewers should first sit and agree on (and document) the proper solutions for the particular types of API changes (like e.g. this one) and then finally say a consistent answer to the review submitter. I agree with Jesse's summary as for what should be done from the technical point of view (with issue 59072). Adding new method to the interfaces is IMO reasonable under the mentioned conditions. From the process point of view, Yarda is right that according to our rules this can be considered incompatible change in official API, thus controversial and need to go through standard review. We should do the review probably also because we should discuss this case in general, whether to make it a precedent - a possible way of extending API that will not be considered incompatible in future and will be able to go through fast review (without heated discussions). If we agree on that. The #1 thing we need is consistency across all NetBeans stable (http://openide.netbeans.org/tutorial/api-design.html#category-stable) and official APIs. The evolution of platform, project, java, debugger, and all other APIs declared as stable or official has to be the same. That is what our users (module writers) expect. Btw. We do not want them to differentiate individual apis (for example platform and debugger ones). We want them to use "NetBeans 4.x APIs" (like http://www.netbeans.org/download/4_1/javadoc/). They all shall follow the same rules of evolution. The intent is to have all interfaces in org.netbeans.api.debugger.jpda package categorized as "not-to-implement" by client code. There's one exception - JPDABreakpointListener. This is expected to be implemented by clients I guess. So we'll create a package org.netbeans.api.debugger.jpda.event and move it there (likely together with JPDABreakpointEvent). I'll prepare the full change by tomorrow and attach it here... Created attachment 23523 [details]
The requested API change.
Created attachment 23524 [details]
The new JPDA Javadoc
Can the "do not subclass" warning be added to javadoc of each class and interface in the org.netbeans.api.debugger.jpda package? y02 Now, when the binary compatiblity is broken, would it not be better to fix some of the design and turn the interfaces into classes? For example SmartSteppingFilter has no reason to be interface, imho. y03 do not forget to increase major version of the module version. y04 any problem with making jpdathreadgroup, jpdathread, callstackframe classes? Any paying need to reuse multiple inheritance? y02) Well, I've tried to convert the SmartSteppingFilter into a final class, but I've run into a bunch of problems. The implementation of the SmartSteppingFilter is available via lookup. We can not get it into the lookup if it would not have a public constructor. But we do not want to provide the ability to construct the instances of this class... Another problem is, that SmartSteppingFilterImpl have one extra public method, which is not declared in SmartSteppingFilter and is used in JPDA module. We would have to have SmartSteppingFilterImpl, which does not implement SmartSteppingFilter (can not if it's final), but delegates to it somehow... The usages would have to be verified... there is more problems with that then advantages IMHO. So I suggest to leave SmartSteppingFilter as it is. y03) it's done in the manifest.mf: org.netbeans.api.debugger.jpda/2 y04) I would rather leave them as they are... I do not want to do many changes in that area... Created attachment 23751 [details]
The complete diff of this API change including all affected modules.
Thanks for the review, I'm going to commit that change later today. Committed into trunk: /cvs/tomcatint/tomcat5/nbproject/project.xml,v <-- project.xml new revision: 1.12; previous revision: 1.11 /cvs/j2ee/debug/nbproject/project.xml,v <-- project.xml new revision: 1.5; previous revision: 1.4 /cvs/j2ee/earproject/nbproject/project.xml,v <-- project.xml new revision: 1.11; previous revision: 1.10 /cvs/j2ee/ejbjarproject/nbproject/project.xml,v <-- project.xml new revision: 1.19; previous revision: 1.18 /cvs/j2eeserver/nbproject/project.xml,v <-- project.xml new revision: 1.10; previous revision: 1.9 /cvs/debuggerjpda/ant/nbproject/project.xml,v <-- project.xml new revision: 1.19; previous revision: 1.18 /cvs/debuggerjpda/api/apichanges.xml,v <-- apichanges.xml new revision: 1.10; previous revision: 1.9 /cvs/debuggerjpda/api/manifest.mf,v <-- manifest.mf new revision: 1.11; previous revision: 1.10 /cvs/debuggerjpda/api/nbproject/project.xml,v <-- project.xml new revision: 1.8; previous revision: 1.7 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/CallStackFrame.java,v <-- CallStackFrame.java new revision: 1.6; previous revision: 1.5 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/Field.java,v <-- Field.java new revision: 1.5; previous revision: 1.4 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDABreakpoint.java,v <-- JPDABreakpoint.java new revision: 1.9; previous revision: 1.8 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDABreakpointEvent.java,v <-- JPDABreakpointEvent.java new revision: delete; previous revision: 1.4 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDABreakpointListener.java,v <-- JPDABreakpointListener.java new revision: delete; previous revision: 1.2 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDADebugger.java,v <-- JPDADebugger.java new revision: 1.18; previous revision: 1.17 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDAThread.java,v <-- JPDAThread.java new revision: 1.6; previous revision: 1.5 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDAThreadGroup.java,v <-- JPDAThreadGroup.java new revision: 1.3; previous revision: 1.2 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/JPDAWatch.java,v <-- JPDAWatch.java new revision: 1.7; previous revision: 1.6 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/LocalVariable.java,v <-- LocalVariable.java new revision: 1.5; previous revision: 1.4 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/ObjectVariable.java,v <-- ObjectVariable.java new revision: 1.7; previous revision: 1.6 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/SmartSteppingFilter.java,v <-- SmartSteppingFilter.java new revision: 1.4; previous revision: 1.3 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/Super.java,v <-- Super.java new revision: 1.3; previous revision: 1.2 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/This.java,v <-- This.java new revision: 1.3; previous revision: 1.2 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/Variable.java,v <-- Variable.java new revision: 1.3; previous revision: 1.2 /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/package.html,v <-- package.html new revision: 1.3; previous revision: 1.2 RCS file: /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/event/JPDABreakpointEvent.java,v done /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/event/JPDABreakpointEvent.java,v <-- JPDABreakpointEvent.java initial revision: 1.1 RCS file: /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/event/JPDABreakpointListener.java,v /cvs/debuggerjpda/api/src/org/netbeans/api/debugger/jpda/event/JPDABreakpointListener.java,v <-- JPDABreakpointListener.java initial revision: 1.1 /cvs/debuggerjpda/nbproject/project.xml,v <-- project.xml new revision: 1.13; previous revision: 1.12 /cvs/debuggerjpda/src/org/netbeans/modules/debugger/jpda/JPDADebuggerImpl.java,v <-- JPDADebuggerImpl.java new revision: 1.79; previous revision: 1.78 /cvs/debuggerjpda/src/org/netbeans/modules/debugger/jpda/breakpoints/BreakpointImpl.java,v <-- BreakpointImpl.java new revision: 1.21; previous revision: 1.20 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/BreakpointResumeTest.java,v <-- BreakpointResumeTest.java new revision: 1.5; previous revision: 1.4 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/ClassBreakpointTest.java,v <-- ClassBreakpointTest.java new revision: 1.5; previous revision: 1.4 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/ExceptionBreakpointTest.java,v <-- ExceptionBreakpointTest.java new revision: 1.5; previous revision: 1.4 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/FieldBreakpointTest.java,v <-- FieldBreakpointTest.java new revision: 1.9; previous revision: 1.8 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/JspLineBreakpointTest.java,v <-- JspLineBreakpointTest.java new revision: 1.3; previous revision: 1.2 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/LineBreakpointTest.java,v <-- LineBreakpointTest.java new revision: 1.9; previous revision: 1.8 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/MethodBreakpointTest.java,v <-- MethodBreakpointTest.java new revision: 1.8; previous revision: 1.7 /cvs/debuggerjpda/test/unit/src/org/netbeans/api/debugger/jpda/ThreadBreakpointTest.java,v <-- ThreadBreakpointTest.java new revision: 1.6; previous revision: 1.5 /cvs/debuggerjpda/ui/nbproject/project.xml,v <-- project.xml new revision: 1.10; previous revision: 1.9 /cvs/debuggerjpda/ui/src/org/netbeans/modules/debugger/jpda/ui/BreakpointOutput.java,v <-- BreakpointOutput.java new revision: 1.15; previous revision: 1.14 /cvs/debuggerjpda/ui/src/org/netbeans/modules/debugger/jpda/ui/actions/RunIntoMethodActionProvider.java,v <-- RunIntoMethodActionProvider.java new revision: 1.11; previous revision: 1.10 /cvs/web/jspdebug/nbproject/project.xml,v <-- project.xml new revision: 1.12; previous revision: 1.11 /cvs/serverplugins/sun/appsrv81/manifest.mf,v <-- manifest.mf new revision: 1.6; previous revision: 1.5 /cvs/serverplugins/sun/appsrv81/nbproject/project.xml,v <-- project.xml new revision: 1.5; previous revision: 1.4 /cvs/web/project/nbproject/project.xml,v <-- project.xml new revision: 1.32; previous revision: 1.31 /cvs/ide/golden/cluster-deps.txt,v <-- cluster-deps.txt new revision: 1.27; previous revision: 1.26 /cvs/ide/golden/deps.txt,v <-- deps.txt new revision: 1.137; previous revision: 1.136 /cvs/ide/golden/modules.txt,v <-- modules.txt new revision: 1.35; previous revision: 1.34 /cvs/ide/golden/public-packages.txt,v <-- public-packages.txt new revision: 1.29; previous revision: 1.28 Verified ... and Closing all issues resolved into NetBeans 6.7 and earlier. |