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: | Jackpot API Review | ||
---|---|---|---|
Product: | contrib | Reporter: | _ tball <tball> |
Component: | Jackpot | Assignee: | apireviews <apireviews> |
Status: | RESOLVED WONTFIX | ||
Severity: | blocker | CC: | hlovatt, jglick, jlahoda, jtulach, pflaska |
Priority: | P2 | Keywords: | API_REVIEW |
Version: | 5.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | TASK | Exception Reporter: |
Description
_ tball
2007-01-25 21:39:29 UTC
Jesse, Pavel, Jan, me: Tom needs some reviewers and we are the obvious candidates. Unless someone else wants to do it. So start reviewing and sending your comments to nbdev, or IZ or adding them directly to the opinion document I've just created at http://openide.netbeans.org/tutorial/reviews/opinions_93224.html Please send your comments soon, so we can have confcall review on Tue,Feb6,2007 +-1day. Hi, I would like to ask if the javadoc here: http://jackpot.netbeans.org/docs/org-netbeans-jackpot/overview-summary.html (linked from http://jackpot.netbeans.org/docs/index.html ) is not obsolete? It lists org.netbeans.api.java.source.TreeMaker as an interface - the same class is a final class in the current trunk and lists org.netbeans.api.java.source.query.ElementUtilities, whose methods have been AFAIK mostly transferred to org.netbeans.api.java.source.ElementUtilities and this class has been removed. Thank you very much. The API was built from the jackpot engine, since the java/source module hasn't started publishing its javadoc yet (at least not on http://www.netbeans.org/download/dev/javadoc/). As soon as it does, I'll reference it instead of duplicating classes essential to jackpot. [JG01] java/source Javadoc is now published in the usual place. (Unfortunately it does not link to Javadoc for com.sun.source.tree.**, which makes it difficult to understand a lot of it.) [JG02] I am curious why the dep on the classfile module is needed. I was under the impression that javac already has good support for parsing bytecode and creating javax.lang.model.** information from it. Is this too inefficient for Jackpot's needs, or is there some additional information you need from class files which javac discards, or for some other reason? [JG03] jackpot/external/modules/org-netbeans-jackpot.jar seems to include a few classes in the package org.netbeans.api.java.source. But this package is already used by the java/source module in the current dev builds. Why the overlap? Is this JAR file just obsolete? Are sources for it available somewhere? [JG04] http://jackpot.netbeans.org/docs/org-netbeans-jackpot/architecture-summary.html talks a lot about exposing the javac APIs and implementation. But aren't these already exposed by the Retouche infrastructure in the standard 6.0 build? Perhaps this section needs to be checked over. [JG05] Preferences - do you use NbPreferences yet? [JG06] A lot of publicly accessible members are missing any Javadoc. No real review is possible until everything is documented. [JG07] org.netbeans.api.java.source.query.UseFinder constants could perhaps be an enumeration. [JG08] SearchResult should not expose protected fields, especially if they are nonfinal, and especially especially if some may be mutable collections. Similarly for many other classes. No (nonconstant) fields in the API, please. [JG09] QueryResultTableModel should not extend a Swing class. It is OK for a QueryResult object to have a method such as public TableModel asTableModel(); for convenience, but please do not dump this into an inheritance hierarchy. Better yet, remove any Swing references from the result interfaces, and add a separate helper class which provides TableModel's (or whatever it is you need) when given a result object. [JG10] {Query,Search,Transform}Result should probably be interfaces (or pure abstract classes) with private implementations. There are a lot of overridable methods in these classes and I think subclassing from outside the module would be quite dangerous. [JG11] UseFinder.find links to org.netbeans.jackpot.model.ASTModel#getRoot which is not public. Is there some other means of obtaining a "root node"? [JG12] It's not clear to me why UseFinder publicly implements NodeScanner. This looks to me like an implementation detail that could be left to an inner class. Similarly for TreeMatcher, etc. Limit inheritance and API-visible methods to those which are actually used by client code. [JG13] If the only use case for subclassing CallGraph is to override acceptCall, then this method should be protected, and all other methods should be final. The default implementation in the base class should also be documented. [JG14] In ElementReferenceList<E>, it's not clear to me what the type parameter refers to. [JG15] The public signature of Query is quite large (and mostly undocumented). It looks like a lot of it belongs to some utility class, e.g. sideEffectFree(...). Probably this stuff should be merged into org.netbeans.api.java.source.ElementUtilities or something. [JG16] Could Change just be expressed as a java.util.Map (or pair of them), so that containsKey, get, etc. would work in the expected way? [JG17] UndoList.getOld is Object -> Object. Why? [JG18] ChangeList should not publicly extend LinkedList. [JG19] ChangeSet refers to org.netbeans.modules.java.source.engine.RootTree, which is not public. [JG20] The public signature of Transformer is a mess. Hard to tell what it all does. I think I'll stop here. If this is only intended as a friend API to a couple of modules you will maintain (or oversee the maintenance of), such as the Jackpot rule module and some part of the NB Java editor, then fine. If it is intended to be publicized for third parties (e.g. creators of custom queries and transformers), it needs a lot of cleanup. [JG02] I am curious why the dep on the classfile module is needed. It's for dynamically loading a classfile handed to it by another module. A URL specifies the file, but its actual name must be passed to ClassLoader.defineClass(). Yes, I can extract the name from the classfile's raw bytes, but that's what the classfile module already does. [JG03] jackpot/external/modules/org-netbeans-jackpot.jar seems to include a few classes in the package org.netbeans.api.java.source. But this package is already used by the java/source module in the current dev builds. Why the overlap? Is this JAR file just obsolete? Are sources for it available somewhere? Yes, jackpot/engine will be obsolete for 6.0, and will be deleted as soon as the merge with Retouche is complete. The overlap is there so that query writers can use this API without being blown out of the water by 6.0. [JG09] QueryResultTableModel should not extend a Swing class. What's wrong with using Swing classes? (Rumor has it that the IDE does). More specifically, there are no GUI references in either the TableModel interface or the AbstractTableModel base class, only references to TableModelEvent, TableModelListener, and EventListenerList. A five class closure is a small price to pay for a fully tested, complex event-based report model. [JG11] UseFinder.find links to org.netbeans.jackpot.model.ASTModel#getRoot which is not public. Is there some other means of obtaining a "root node"? The Retouche team wants me to get rid of all "root node" concepts. I'll fix this reference. [JG12] It's not clear to me why UseFinder publicly implements NodeScanner. This looks to me like an implementation detail that could be left to an inner class. Similarly for TreeMatcher, etc. Limit inheritance and API-visible methods to those which are actually used by client NodeScanner is a base class that maintains the current element during traversal. The current element is the closest "owner" of a node; a statement would be owned by its ExecutionElement, for example, while an instance variable by its ClassElement. [JG14] In ElementReferenceList<E>, it's not clear to me what the type parameter refers to. It's the Element type (I'll update the doc). If you are searching for method references, for example, its type is ExecutableElement; searching for variable references requires a VariableElement type. [JG15] The public signature of Query is quite large (and mostly undocumented). It looks like a lot of it belongs to some utility class, e.g. sideEffectFree(...). Probably this stuff should be merged into org.netbeans.api.java.source.ElementUtilities or something. Yes, merging it into ElementUtilities would make sense, but the Retouche team wants to keep their API as small as possible. All of the utility methods were developed to spare clients from having to re-invent them, but apparently that isn't the way we do things any more. [JG16] Could Change just be expressed as a java.util.Map (or pair of them), so that containsKey, get, etc. would work in the expected way? [JG18] ChangeList should not publicly extend LinkedList. [JG19] ChangeSet refers to org.netbeans.modules.java.source.engine.RootTree, which is not public. I have to discard Jackpot's nested change support, which the Retouche team refused to accept. With it, related changes could be grouped together, so as to more safely accept one set of changes while not accepting another. Using their system, a developer either accepts all changes, no changes, or takes responsibility for breaking code by accepting some changes. [JG17] UndoList.getOld is Object -> Object. Why? It returns the original version of an object; in the case of AST nodes, it makes diff'ing more accurate. Re. JG09 - referring to the Swing classes is OK but I think it should be in a clearly separated API method referring to the appropriate interfaces. Not by means of subclassing AbstractTableModel, an implementation detail. Similarly re. JG12 - I didn't ask why you *use* NodeScanner. I asked why you *extend* it and thus force this implementation detail into the public API. JG14 - so it should be <E extends Element>, right? JG15 - if a lot of people need exactly the same functionality then obviously that needs to be in an API and too bad if the API gets bigger. Adding static methods to a utility class does not increase the fundamental complexity much, anyway. JG17 - I was asking why the type is Object and not something more specific. Similarly in UndoEntry. Just looks suspicious - is a generic type parameter missing, for example? Re. JG09 - referring to the Swing classes is OK but I think it should be in a clearly separated API method referring to the appropriate interfaces. Not by means of subclassing AbstractTableModel, an implementation detail. Fine. Similarly re. JG12 - I didn't ask why you *use* NodeScanner. I asked why you *extend* it and thus force this implementation detail into the public API. You extend it for the same reason you extend TreeScanner: you want to inherit a specific traversal and set of reference data for your visitor class. It's not an implementation detail; it adds the notion of an "owning symbol" to your tree scan. Look at <a href="http://jackpot.netbeans.org/docs/src/BadFinallyBlocks.java">BadFinallyBlocks.java</a> for an example of client use. In that code you'll see references to a "currentSym" variable, which is updated by NodeScanner during the traversal. Perhaps the way to get rid of this is to change Query<R,P> to Query<Void,Element> so currentSym becomes a parameter instead of an instance variable. You didn't liked the idea of giving query writers control over the TreeVisitor methods' parameter and return values, so using it to pass the currentElement addresses both issues. JG14 - so it should be <E extends Element>, right? Yes, I'll fix it. JG17 - I was asking why the type is Object and not something more specific. Similarly in UndoEntry. Just looks suspicious - is a generic type parameter missing, for example? The parameter and return type are always identical, but the type may vary depending upon the undo list's contents. How does one write a type parameter to enforce the link between the parameter and return value without restricting the possible types? I don't think "Object<?> getOld(Object o)" adds any value. If anyone knows of a better way to declare this constraint, I'll be happy to put it in. Re. JG17: <T> T getOld(T x); right? Then you can write e.g. String x = undoList.getOld("foo"); without a cast. Of course, if everything in a given UndoList uses the same type, then perhaps it should be a parameter of UndoList itself. I don't really understand the usage of this class yet, so I'm just guessing in general terms. [JG12 cont'd] The BadFinallyBlocks example makes sense. I would rather see currentSym accessed as a protected method rather than a field, but the fact that Query extends NodeScanner in this case makes sense - you are expected to subclass Query and then use stuff from NodeScanner available through inheritance. The API could also be modified to use a flatter inheritance hierarchy, so that Query would extend Object and expose only public final methods for those running the query, protected final methods for the use of subclasses, and protected abstract or concrete methods where necessary for subclasses to define behavior. I am still not convinced by the original case I pointed to, however; I was not asking about Query. UseFinder and TreeMatcher both look like they should be final, extend only Object, expose only the required constructor and public methods to clients, and keep all details of NodeScanner as a private implementation detail. That is, if I am calling result = new UseFinder(env).find(method, project); then I do not need to know or care that UseFinder uses the visitor pattern to implement this functionality. Y01 The jackpot API shall be in its own module (let's call it "transquery"). The "transquery" document shall have dependency on java/source module. Preferably only on public API, possibly on non-public parts as well!? Y02 The "transquery" module shall expose an API that is not in Official namespace (as it is not stable http://openide.netbeans.org/tutorial/api-design.html#category-official). Let's call the package "org.netbeans.modules.transquery.api". As such Jesse's concern "If this is only intended as a friend API to a couple of modules you will maintain" - it going to be (partially) addressed. Y01 The jackpot API shall be in its own module (let's call it "transquery"). The "transquery" document shall have dependency on java/source module. Preferably only on public API, possibly on non-public parts as well!? Let's not call it that, as "transquery" sounds like the name for a proposition someone would make to a transvestite. That isn't jackpot's focus. :-) Since Jackpot is a Java-only technology and since it's most interesting focus is on source code transformations, I suggest "org.netbeans.api.java.transform". It takes "source" out of the name so that people don't confuse it with java/source, but still describes its core function. Y02 The "transquery" module shall expose an API that is not in Official namespace (as it is not stable http://openide.netbeans.org/tutorial/api-design.html#category-official). Let's call the package "org.netbeans.modules.transquery.api". As such Jesse's concern "If this is only intended as a friend API to a couple of modules you will maintain" - it going to be (partially) addressed. I talked with Honza Chalupa today about the API status. I incorrectly assumed that for the review you needed to know its current status, not what its status will be at release time. For the NetBeans 6.0 release I am very confident that the status will be "stable". The API has had more use than the java/source API, which has to be stable by then. After this review I don't see the need for any major changes to it, and certainly no changes that would affect backwards-compatibility. In particular, I strongly disagree that this API be just a hidden friend API, and suspect you misunderstood Jesse. The whole purpose of this exercise is to publish an API so that developers can independently create and distribute new queries and transformers. The rule language was never designed to handle all transformations, and we have always told developers that when they have an idea which the language can't handle they should write it in Java. If this were meant to be a private API, I never would have invested the time into transformer deployment or general documentation. Should I change resubmit this issue after making the suggested improvements with a "stable" status? Re. Y02 and more generally stability level: if there is to be a documented and official SPI in 6.0 for people to write queries and transformers, I think it needs to be substantially simplified and cleaned up. The place to start is to list all the expected use cases and then give annotated code samples of usages. (http://jackpot.netbeans.org/docs/api_examples.html looks like a good starting point but it is not clear those examples are up to date.) Nothing should be in the public SPI unless a real use case directly justifies it. On the other hand, there seems to be a lot of stuff which would be used only in IDE infrastructure modules. (EnableTest? UndoEntry? QueryResultTableModel?) This does not need to reviewed heavily and can be part of a separate friend API. Generally speaking, without use cases or complete Javadoc comments, it is impossible to review an API for official status. Reviewers need to understand the behavior and purpose of every class and method. For example, what does public <T extends Tree> List<T> Transformer.chopUnreachable(List<T> stats) do and why is it needed? I have no idea, so I cannot offer an opinion on whether this method is needed in the initial API release, much less whether it is likely to need to be deprecated or deleted in the future. What makes this an SPI? Queries aren't services, they are actions similar to the refactoring commands. All of the examples you gave are or were useful for query writers: . EnableTest: used when Jackpot could be run on arbitrary selections to allow the query to define when it should be enabled in a menu (usually based on the type of selection). The UI team nixed that usage and decided Jackpot should only be run against projects, so I just removed it. . UndoEntry: a transformer is a form of batch editor, and any modifications it makes outside of AST-rewriting should have undo entries. For example, one can imagine a transformer that finds all implementations of a service interface and lists them in that service's META-INF/service file. When the developer clicks "undo", shouldn't that list be restored to its original state (even though it's not a Java source)? All the query needs to do is define an UndoEntry when making the edit, and undo can happen after the query is gone. . QueryResultTableModel: a means to define a query report. We had a problem where each different query demanded unique fields on SearchResult, so the solution was not to keep adding fields for those special cases, but to instead give the query the ability to define its own report. Swing's TableModel was extended because it allows for the definition (and discovery) of an arbitrary two-dimensional table with column headers. SearchResult was then changed to implement this TableModel with the default report format. The Jackpot UI doesn't need to know any specifics, since it just sticks the model in a JTable and displays it. I think of Jackpot's SPI as an interface for plugging in query generators, such as the rule language parse or Lee Chuk Mun's module to write queries in JavaScript. The java/source team wants the rules language support moved to a separate module, and so I was starting to design such an SPI so java/source doesn't need to depend on specific query generators. ("SPI" just in the sense that you are primarily defining an interface for additional commands to be inserted into the system, rather than an interface for running existing commands on certain inputs. The terminology isn't important.) If all the classes in the proposed packages are in fact intended for use by query or transformer implementations, then fine. But they need to be properly documented and demonstrated so that we can do a meaningful review. Regarding an SPI for query generators - I would agree that .rules file support belongs in its own module, though this could probably use a friend SPI, assuming there are expected to be few implementations. A completely new version of the Jackpot API is now available at: http://jackpot.netbeans.org/docs/org-netbeans-modules-jackpot/overview-summary.html There are now only two packages: the API one supports query writers, while the SPI package supports those few people adding support for jackpot queries to their scripting modules (the rules module uses it, and Lee Chuk Munn expressed interest in updating his javascript modules to use it). All overlapping API with Java Source and javac's API has been eliminated. What is left is the minimum needed to write queries which can be run using jackpot's UI. Y11 If this API can achieve the same things as the previous version, then congrats Tom, you are heading in the right direction. This one is really more understandable. Y12 Please aliminate use of Node.Cookie from your API. Rename QueryCookie to for example to QueryProvider Y13 Usecases - fine you have some, however why not tell us how people really write scripts, where them register them, etc. Y14 You say you export just Java API - do not you read layers? Do not you do instanceof checks? Do not you read getProperty for some reason? Y15 I'd like to understand who creates what in the API package. For example I'd like to know why should I implement QueryContext? That is all for now from me, keep walking the right direction! > Y12 Please aliminate use of Node.Cookie from your API. Rename QueryCookie to > for example to QueryProvider Why? I realize the Cookie interface was abused many times when a service provider interface should have been used, but in this case I think it is justified because the provider is determined by the data object itself. Is there a better way to specify a service provider which is tied to a specific data-type? I thought that is what Cookies were originally intended to do. > Y13 Usecases - fine you have some, however why not tell us how people really > write scripts, where them register them, etc. Okay, but I don't think this documentation belongs in the Usecases section. Perhaps we need separate sections for recommended ways to the API is used (which is not the same as a use case, which shows why an API is necessary). Service registration is so common now that it deserves its own section, too. > Y14 You say you export just Java API - do not you read layers? Do not you do > instanceof checks? Do not you read getProperty for some reason? Yes, that section needs to be rewritten since it described the old engine. I will certainly describe the layers the Jackpot module expects, as well as any properties which are defined or referenced by queries. What internal instanceof checks or properties referenced should not be documented, however, as they are private implementation details -- documenting them unnecessarily locks the implementation to a specific design. > Y15 I'd like to understand who creates what in the API package. For example > I'd like to know why should I implement QueryContext? You don't implement it -- like AppletContext and ServletContext, it is provided by the container (the query executor) -- that's why I purposely called it a QueryContext. The reason it is in the API section is because these are the services provided by the container to the query, so the query needs a way to find them. I can add to the QueryContext documentation, but this should be a well-known concept to anyone who has previously written any sort of contained executable. Re. Y12: use interface QueryProvider { ... } and then get it from some lookup - e.g. Utilities.actionsGlobalContext().lookup(QueryProvider.class). They way you should also eliminate dependency on openide/nodes in the API completely. Re. Y13 - "which shows why an API" - so show it! Your usecases show nothing. Tell people how to write a Query, how to write a Transformer - make it entry point to the rest of the documentation. Re. "we need separate sections" - you can put it where ever you want, but people will not find it probably - arch-usecases sections gets included in the index page, and most of the (new) modules use it for this purpose. Re. Y15 - make it non subclassable class then - that is standard NetBeans coding pattern - so please make it consistent. If you disagree let's keep the resolution for the conf-call meeting. This review is no longer a productive use of anyone's time. |