Created attachment 102159 [details]
Example use of the meta-annotation.

Please review, thanks.

[JG01] I am perplexed by why you would have an annotation processor to generate another annotation processor, when a la https://sezpoz.dev.java.net/svn/sezpoz/trunk/sezpoz/src/main/java/net/java/sezpoz/impl/Indexer6.java you could just have a plain annotation processor which looks for any meta-annotated annotations, and then in turn looks for any elements annotated with them, and creates registrations.

For that matter, I see no clear need for a separate annotation for each of these interfaces, since they all seem to be registered in essentially the same way. The duplication seems messy and does not make things any simpler for the module developer. Would rather suggest defining the location where interfaces are registered, using a CLASS-retention (?) meta-annotation:

@MimeLocation(subfolderName="CompletionProviders")
public interface CompletionProvider {...}

and then a single annotation to register particular implementations:

@MimeRegistration(service=CompletionProvider.class, mimeType="text/x-maven-pom+xml")
public class POMCompletions implements CompletionProvider {...}

which would verify that its service class was marked with @MimeLocation.


[JG02] Since mimeType attributes are String, not String[], there should be a way to list multiple registrations of a single implementation under different MIME types (possibly each with a different position).

Y01 Less classes loaded eagerly is better. The patch #102158 generates subclasses of Class2LayerFolder that will all be loaded during first query. Please eliminate that. My preferred approach would be to create a generic subclass with one factory method (argument Map) and register it in layer's Services folder. Similar style as used in @AntBasedProjectRegistration

Re. JG01: We should think about the end user experience and performance. Details of internal implementation or complexities in less used usecases are not that important, imho.

As far as I can see the performance characteristics of both proposals are the same. They both generate the same content to layer (both seem to be able to implement Y01 too). 

What about the end users? They will in majority use the @CompletionProvider.Registration or @MimeRegistration as suggested by Jesse. We shall optimize for them. The main question then is: "What is more natural f rom their point of view?" Usage complexity seems to be the same. However for many APIs we tend to prefer the Something.Registration style as it explicitly connects the API with its registration. Thus I slightly prefer that.

Y02 The name "MimeLookupRegistrationMetaAnnotation annotation" seems to repeat known facts (like "CORBA architecture" or "polní feltflaška"). Why not just MimeMetaRegistration or even MimeProviderRegistration?

Y03 There shall be error when one tries to apply @CompletionProvider.Registration to classes not implementing CompletionProvider. Is there one?

Y04 Provide code completion of known mime-types in current platform/ide setup. Here I am thinking about hardcoding some example and using something like  915fc0098ed5, possibly reusing the same PathCompletions class.

Continuing JG01: not a blocking issue, but - at least based on what I understand of the use cases - I still think that a generic @MimeRegistration is better in this case. Consider @ServiceProvider: if there is only one basic way in which various interfaces can be registered, then only one annotation is needed for this technique. We do not have @IOProvider.Registration, @InstalledFileLocator.Registration, @FileEncodingQueryImplementation.Registration, etc., because their registration all follows the same syntax and semantics; it is enough to note that "an implementation may be {@linkplain ServiceProvider added to global lookup}". Similarly, a generic @ProjectServiceProvider suffices for all interfaces dropped into project lookup in the usual way. Custom registration annotations are useful in cases where implementations are registered in idiosyncratic ways, and the processor may need to offer specialized warnings, errors, or completions.

(On a more pragmatic level, I am not sure that having an annotation processor generate classes in public packages will work smoothly. They will appear in Javadoc if javadoc is run after javac and includes the generated source dir, which may not yet be true in Ant- and/or Maven-based module projects. Popup Javadoc in the editor via attached sources may work if the Java editor implements this. Would need to be tested before committing to this approach.)

JG01: having one AP handling "*" annotations would mean that this AP would need to be used for every class in every module (transitivelly) depending on editor.mimelookup, which might slow down indexing. Also, the meta-annotation would not be resolved without a client's compile-time dependency on editor.mimelookup. Note that the APs were not supposed to be generated into the API/SPI folders - the "targetPackage" attribute was supposed to contain a target folder for the AP. The Registration annotations were supposed to be created manually. Anyway, I like the @MimeLocation and @MimeRegistration better, thanks. As a side effect, this allows removal of Class2LayerFolder (I originally wanted to make this independent of this effort, but Y01 is apparently requesting these to be done in sync). I have a set of working patches, which I will attach soon.

JG02: done using @MimeRegistrations.

Re Y04: Is there an API to get the mime-types (in the target platform, otherwise the CC would not be very useful)?

For Y04, there is no real API. There is a hack in ActionProcessor.getCompletions which relies on help from apisupport (PathCompletions).

Created attachment 102193 [details]
Introducing @Mime{Location|Registration|Registrations} annotations

Working patch - introducing @Mime{Location|Registration|Registrations} annotations are proposed by Jesse.

Created attachment 102194 [details]
All services that use Class2LayerFolder need to use @MimeLocation.

Working patch - adding @MimeLocation to all services that have Class2LayerFolder.

Created attachment 102195 [details]
Example use of the @MimeRegistration annotation

Created attachment 102196 [details]
Using @MimeLocation instead of Class2LayerFolder

Y01: Working patch - changing the infrastructure to use declarative data from @MimeLocation instead of Class2LayerFolder, deleting the useless Class2LayerFolder implementations.

Patch summary is now misleading, BTW.


[JG03] getElementsAnnotatedWith(Class) and getAnnotation(Class) is easier than what you are doing, I think. You just need to use MirroredTypeException when looking up the service field. See ServiceProviderProcessor.


[JG04] Avoid @SupportedAnnotationTypes; override the corresponding method to use class constants, for better integration with Find Usages etc. (See other processors for examples.)


[JG05] Class.forName on apiClass is wrong; will usually fail. Need to rather reflectively check that the annotated type is assignable to the service type.


[JG06] What is the actual semantics of supersedes()? Javadoc does not say, and processor impl is suspicious - will only work if entries end in ".instance" or similar.


[JG07] I believe @MimeLocation can use RetentionPolicy.CLASS - try it.


[JG08] @MimeLocation on MarkProviderCreator looks wrong.


[JG09] I might suggest making @MimeLocation mandatory on any service used from @MimeRegistration, but making subfolderName optional (i.e. default ""). This would add a level of error checking - you could not accidentally register a service which was not designed to be looked up in a MIME folder. On the other hand, this prevents use of @MR on "legacy" APIs that have not been updated to use @ML, so I'm not sure if it's a good thing or not.

(For @ServiceProvider it seemed impractical to require a @Service marker since there were so many services, distributed in so many modules, and we even look up some interfaces in the JRE which could not possibly be marked.)


[JG10] Unused import in DummySetting.java.

Overall it looks very nice. I especially like the amount of removed code due to usage of @MimeLocation. Using Retention.RUNTIME (I don't believe JG07 is true) makes this really smooth. Simpler than I originally envisioned.

If you are all fine with @MimeRegistration, I am fine too.

JG03: I don't like this mixing of "design" time (i.e. the annotation and the current source code) and "runtime" (in compilation though) where the processor is running.

JG04: This causes massive slowdowns during indexing/CC invocations, etc. The classloader that loaded the processor is cached in post-6.9, so it is not as bad as in 6.9, but as soon as the CL is evicted, it all needs to be loaded again just to find out that the AP should not be run. So I consider this to be an anti-pattern.

JG05: Ok. LayerBuilder.instanceFile should be fixed to take TypeMirror/DeclaredType/TypeElement so that the error check is done on one place.

JG07: it won't work. SwitchLookup reads that annotation at runtime (see the last patch).

JG09: there are way too many services that did not have Class2LayerFolder for this to be practical, IMO.

JG03 - as you like. Just a way to make the code simpler and more readable.


JG04 - not sure I follow. You need to load & instantiate every available processor anyway, since 269 specifies no way of preindexing processors by @SAT. Why would resolving three additional small interfaces matter compared to that amount of bytecode? (Note that if JG03 is accepted, they would be loaded anyway.)


JG05 - probably yes, but subject of a separate API review.


JG07 - right, did not notice that.


JG09 - does not matter whether they used Class2LayerFolder or not; just depends on whether you expect there to be any MIME-registered services in common use outside the apparent nb.org codebase. But not important.

JG04: e.g. snapshot from bug #190337: ~24s are spent in JavaCustomIndexer (i.e. Java indexing), out of which 7.9s is spent in o.n.d.r.DebuggerProcessor.getSupportedAnnotationTypes, which is loading classes. Note that the processor may not be even called after all - if the given file(s) are not annotated with a supported annotation, the process method of the processor will not be called (unless it contributed in a different round, etc.). So loading the annotation classes may be wasted time (the annotation classes are not necessarily loaded just because the processor was loaded, esp. if verifier is off). (This snapshot is probably from 6.9 - the loading should be faster in post-6.9 builds, but loading the classes uselessly may still affect code completion and indexing.)

Also, if @SAT is used, the IDE might try to cache the values and prevent loading of the processors - this cannot (reasonably) be done with custom getSupportedAnnotationTypes (I am not saying that we are planning to create such a cache). I do not think this is generally prohibited.

(In reply to comment #16)
> 7.9s is spent in
> o.n.d.r.DebuggerProcessor.getSupportedAnnotationTypes, which is loading
> classes

8 seconds to load 5 interface classes?? Something seems pathological here. Anyway off topic I guess.

> if @SAT is used, the IDE might try to cache the values and prevent
> loading of the processors

Possible, so long as it first loads the processor class and verifies that it does not also override the method from AbstractProcessor.

Created attachment 102227 [details]
Updated patch for registration annotation for mime-lookup

Updated the commit message. Deleted the superseded attribute ([JG07]) - the format would need to be the exact registration file from the layer, which is cumbersome. Also not sure it hidding from mime-lookup really works.

Created attachment 102259 [details]
Current version of the patches.

Spec. versions of all modules that use @MimeLocation updated. I plan to commit this set of patches tomorrow unless someone objects or some other problem is found.

[JG11] @MimeRegistration omits @Retention; I guess it should be SOURCE. @MimeRegistrations also omits @Target.


[JG12] @MimeLocation Javadoc should likely link to @MimeRegistration.

Integrated into 'main-golden', will be available in build *201010070000* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/aa4bd3810b3c
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #190606: Introducing @Mime{Location|Registration|Registrations} to simplify registration of services to the mime-lookup.

Y04 implemented in 65d14921df6a

According to comment #19 and comment #21 this should already be FIXED, right?

No response? Looks FIXED unless I am missing something.