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.