WIP feat: javadoc {@link} can be made fully-qualified

[surli]

Requirements implemented:

• R1: add support for getting the original comments done in feature: CtComment#getRawContent() #2746
• R2: the original comment can be analyzed from the model and from the serialized model, even if the Java files don't exist anymore on disk) abandoned
• R3: the @link fragments can be analyzed done in feat: add support for javadoc inline tags #2802
• R4: javadoc {@link} can be made fully-qualified here

fix #2170

[monperrus]

first version with parsing code from JavaParser.

[pvojtechovsky]

@monperrus, It is great that you are working on model of javadoc comment content!

[monperrus]

Work:

• add support for parsing Javadoc comments, with a mix of Thomas and JavaParser code
  • Thomas code to clean comments (remove the prefix " * "), this maximizes backward compatibility (esp in WildComments, which is indeed a wild class)
  • Javaparser code to parse tags and descriptions, this is mature code which has already been debugged.
• add method "getRawComment()" for clients who want to do their own parsing (this was impossible before)
• simplication: removed brittle and hard to maintain post-processing of comments (many bugs were due to things inconsistently handled between pre-parsing or post-parsing). We now have the sniper mode for those who want to keep the comments "as is".
• unification: CtElementImpl#getDocComment() and CommentHelper#printComment() is now based on the same code.
• in fqn mode, rewrite @link, fix bug: in FQN mode, javadoc references are printed as the original ones #2170

[monperrus]

rebased. CI OK.

[pvojtechovsky]

I see the problem with line separators. The new javadoc parsing code often uses System.lineSeparator(), but it is usually not fitting to the EOL of processes java source file.

Note that Spoon CtComment is always using \n as line separator for the value returned by CtComment.getContent(). See javadoc of CtComment.LINE_SEPARATOR.

	/**
	 * This line separator is used in comments returned by {@link #getContent()}.
	 * It is OS independent.
	 * It has no influence to pretty printed comments, which uses by default OS dependent line separator
	 */
	String LINE_SEPARATOR = "\n";

It is much simpler to analyze comments when you can be sure that EOL is always \n, independent on the EOLs used in source file. Note that java sources are sometime messy concerning different kinds of EOLs ... which can differ even in files of the same project.

So the idea is: When Spoon parses comments it detects all possible kinds of EOL and replaces them by \n.
When Spoon prints comments back to file, then
A) it uses EOL defined by Environment
B) it uses EOL which was used in origin source file (SniperPrinter)

So I guess that code of this PR will work well on Linux and will always fail on other OSes.

[monperrus]

So I guess that code of this PR will work well on Linux and will always fail on other OSes.

Normally no. I took care of not writing anything OS-dependent.

When Spoon prints comments back to file, then
A) it uses EOL defined by Environment
B) it uses EOL which was used in origin source file (SniperPrinter)

This PR implements a third option, in normal model (no sniper), it uses the original line breaks from the original source file.

I now look at your detailed comments.

[monperrus]

Addressed all your comments.

[pvojtechovsky]

How could you see this one hour ago? Mistery!

No mistery ... I am fighting with github comment approval process. I made this comment weeks ago. But I just found that I have to approve them before anybody else can see them...

[monperrus]

Working on this, based on @pvojtechovsky comments in order to make progress towards merge

• removing the call to getConcent in #equals for sake of performance.
• adding setRawContent

[pvojtechovsky]

Spoon model is "semantic java model" not source code java model. Therefore I am not happy with storing rawComment in model which may cause performance problems.
Cannot we store cleaned comment and to implement getRawComment by getting of String from origin sources using Comment position start/end?

[monperrus]

which may cause performance problems.

I disagree here, we have no measurement yet to claim anything or take a decision. Are you talking about memory performance or runtime performance?

[pvojtechovsky]

I talk about runtime performance. But I do not have any numbers, so I agree to continue like you suggest. We can always change it. It is a minor internal thing.

[monperrus]

I agree to continue like you suggest.

cool :)

We can always change it.

yes, software is soft!

[pvojtechovsky]

Printing of raw comment has these bad side effects:

• the indentation of the comments will not fit with indentation of context
• the EOL in comment body may be different to other EOLs in the file
• the setComment("first line\nsecond line"), will be printed wrong now

Looking at all necessary changes I would say that storing of raw content is not good.

add method "getRawComment()" for clients who want to do their own parsing (this was impossible before)

@monperrus Is it the only reason why we store raw content and print wrong aligned comments with inconsistent EOLs now? It is quite high price for that :-(

[pvojtechovsky]

See alternative implementation of getRawContent in #2746.
WDYT?

[monperrus]

* the indentation of the comments will not fit with indentation of context

Yes, I agree with you, we want to keep this behavior.

* the EOL in comment body may be different to other EOLs in the file

Correct, this is not good.

* the setComment("first line\nsecond line"), will be printed wrong now

What do you mean?

Looking at all necessary changes I would say that storing of raw content is not good.

I disagree. The problem is not about storing it, it's about using it in the way I've explored yesterday , which is not satisfactory, I agree.

See alternative implementation of |getRawContent| in #2746 <#2746>.

It does not comply with requirement R2 in #2172 (comment). But the more I think about it, the more I prefer this alternative implementation you propose. It would still enable client code to do what the they want in most cases. WDYT?

[pvojtechovsky]

R2: the comments (incl. the original one), can be analyzed from the model and the serialized model, even if the Java files don't exist anymore on disk)

I did not noticed that requirement. What is the use case behind that: "the Java files don't exist anymore on disk"? By the way Sniper printer will not work in such case too.

the setComment("first line\nsecond line"), will be printed wrong now

What do you mean?

if printing would use original tabs and EOLs then it would print

/* first line
second line */
   someElementWithIndent;

instead of current

   /*
    * first line
    * second line
    */
   someElementWithIndent;

But the more I think about it, the more I prefer this alternative implementation you propose. It
would still enable client code to do what the they want in most cases.

I liked the origin cleaned/semantic comments. They are more fitting to concept of semantic Java AST, then rawComments. So I would prefer that simple alternative solution too.

[monperrus]

I liked the origin cleaned/semantic comments

Now that we're introducing an object tree for comment comments, we have to basically give away the String field, and only maintain an object field. (note that we already had a proto-tree with the tags before ... and actually some hidden synchronization problems already) The pretty-printed string version of the comment has to be computed on the fly (and possibly cached ... to the price of maintaining a cache infrastructure). WDYT?

[pvojtechovsky]

I do not know all side effects yet, but I agree that we should get rid of String representation of comment and switch to the object tree for comment content... but that is longer way ... because we need comment AST nodes with same capabilities like CtElement node has. I need mainly access to SourcePosition to be able build source fragments and handle sniper pretty printing correctly. Also the change listeners are needed ... but all that can be done later. For now I like current PR, but without that rawContent stuff. Just parsing of comments as internal invisible stuff.

[pvojtechovsky]

Please maximize compatibility - most of old tests should pass without all these changes.

[monperrus]

OK, we're along the same line. I agree, we can add support for position and change listeners later.

[monperrus]

And you know me, I take backward compatibility super seriously.

[monperrus]

the diff is clean and CI is OK ... but the performance cost is huge: (11 minutes versus 7 minutes per job) https://travis-ci.org/INRIA/spoon/builds/462260778

What are the solutions?

[pvojtechovsky]

If I understood it well, then updatedInlineLinksInJavadoc method needs that time.

There is also another reason why we should do it different - new concept of handling of imports, which I discussed with Simon (#2683). One of the ideas of this concept is that model is normally not modified to show/hide fully qualified names. Only just before printing, there are started some model procesors/validators, which changes model to apply imports or FQN mode.

So may be updatedInlineLinksInJavadoc should be called from such model validator before printing only. While actually it is called all the time even during building of the model.

So I suggest to wait with this PR after #2683 is merged.

[monperrus]

Yes the super slow call is this.getFactory().getModel().filterChildren(new NamedElementFilter<>(CtType.class, stype)).first(CtType.class)

OK for me, let's wait. I WIP this one again.

push-forced with accepted #2746, #2802

[monperrus]

#3160 is a better alternative, way faster (no call to Type().get()).