Adding support for generating eclipse markers through m2e injuected buildContext

[svketen]

I wrote an annotation processor which does some checks and creates diagnostic errors messages. On cli builds those error messages are printed to the console and informs about errors within sources (e.g. wrong use of parameters, exceptions on processing).

In eclipse those messages are not shown directly. I added support for generating eclipse warning/error markers through the injected m2e buildContext on the annotated elements which contains the problems.

[timowest]

Thanks for the PR. Could you fix the indentation of your contributions (4 spaces)?

[svketen]

added new commit with corrected indent.

[timowest]

Consider removing the final usage inside this method's body.

[svketen]

finals are removed.

[Shredder121]

Diagnostics are not supposed to be null, so you can rely on that fact.
This null check is not necessary.

[Shredder121]

I also added a few comments, the only thing that I see (but I don't know if that is a matter of personal preference or not) is the switch.

Nevermind, I hadn't seen that the commit also reordered the cases for the switch block.

[svketen]

thanks for review, added commit with changes to consider your comments

[Shredder121]

Finally, the only thing I am still speculating is the following.

There is a fine distinction between a message that has a source (which you check) and if it has a valid position in said source (which you don't check).

I think checking for diagnostic.getPosition != Diagnostic.NOPOS instead serves as both a safety check to see if you can process it (if it returns a valid, positive integer, it means the getsource() method returns a nonnull value, and it has a position), as well as document what you are doing (so the explaining comment is not necessary anymore)

I call it speculation, since we don't yet have a test case that validates the expected behavior, so it only theoretically improves safety.

@timowest @sketelsen What do you think?

[timowest]

@Shredder121 Would the rendering of notifications without valid positions have negative side effects?

[Shredder121]

That's exactly what I am unsure about.
I don't quite know a way of testing it, since the BuildContext is usually provided by the Plexus container and is not injected when we instantiate it (its logger is null).
Moreover, I don't know what Eclipse thinks of -1 positions, maybe it still adds markers, since Diagnostic.NOPOS might be implemented?

It was just a hunch, @sketelsen can you build the apt-maven-plugin and see if you can run your processor with the results you expected?

[svketen]

@Shredder121 I prefer the null check instead of assume some indirect behavior (with a valid position there will be a source) because i run into the NPE by myself. From the viewpoint of the annotation processor developer the element is just an optional parameter when using the Messager.printMessage(...) method.

Eclipse renders a marker with a valid destination file and an invalid line/column (-1) as a marker on the first line. I tested this with luna 4.4.1 and mars 4.5M4. As long as the line number will not exceed the file length there will be a marker within the java editor view. After solving/rebuild all markers will be deleted, no matter of valid or invalid position. The only poor behavior followed from buildContext.addMessage(null, ...). The marker will be created on the maven project file and must be deleted manually after solving/rebuild.

[Shredder121]

I see, thanks for giving us some insight.

Then all my doubts are settled, I think it's good to merge.
Could you maybe post a screen shot, for future reference?

[Shredder121]

One more thing.
I just wrote a small test, and all errors from the internal bootstrapper of apt are ignored now, is it an idea to also process these?
These don't have a source, but are messages like:

For a private static class:
(Kind.ERROR) Could not instantiate an instance of processor 'com.mysema.maven.apt.ProcessorTest$CustomProcessor'
(Kind.WARNING) Annotation processing without compilation requested but no processors were found.

Making it public:
(Kind.WARNING) No SupportedSourceVersion annotation found on com.mysema.maven.apt.ProcessorTest$CustomProcessor, returning RELEASE_6.
(Kind.WARNING) Supported source version 'RELEASE_6' from annotation processor 'com.mysema.maven.apt.ProcessorTest$CustomProcessor' less than -source '1.7'
And so it can go on.

I think resolving this in a separate pull request is in order?

[timowest]

@Shredder121 Maybe these could be handled via a separate PR. Shall we merge this?

[Shredder121]

Yes, we can merge this.
A separate pull request is good, I'll at least push the test case.

[Shredder121]

Thank you @sketelsen for the addition!