Bug 237051 - [formatter] Formatter insert blank lines after javadoc if javadoc contains Commons Attributes @@ annotations
Summary: [formatter] Formatter insert blank lines after javadoc if javadoc contains Co...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4.1   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-13 07:09 EDT by Carl-Eric Menzel CLA
Modified: 2008-08-28 12:16 EDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (40.00 KB, patch)
2008-06-13 12:44 EDT, Frederic Fusier CLA
no flags Details | Diff
New proposed patch (15.88 KB, patch)
2008-06-19 09:31 EDT, Frederic Fusier CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carl-Eric Menzel CLA 2008-06-13 07:09:39 EDT
Build ID: I20080530-1730

Steps To Reproduce:
1. Create a method with Javadoc and include a Commons Attributes annotation (starting with @@) at the end, i.e., not followed by a regular Javadoc tag.
2. Let the formatter format the source file.
3. A blank line is inserted between the javadoc comment and the method signature. If you repeat step 2, a second line will be inserted, but after that it stops. A third line does not appear.

More information:
I tried this with several different formatter profiles, including the built-in Java and Eclipse profiles.

This:
    /**
     * foo
     * 
     * @@Foo("foo")
     */
    Object doSomething(Object object) throws Exception;

gets turned into this:
    /**
     * foo
     * 
     * @@Foo("foo")
     */
     
    Object doSomething(Object object) throws Exception;

Note the spurious blank line. A second formatter call gives me:
    /**
     * foo
     * 
     * @@Foo("foo")
     */
     

    Object doSomething(Object object) throws Exception;

A second blank line. Here it stops, there will be no third line. Strangely, if I add a regular Javadoc tag after the @@ part, like this:
   /**
     * foo
     * 
     * @@Foo("foo")
     * @deprecated
     */
    Object doSomething(Object object) throws Exception;

Then it works. No line is added. This used to work in Europa and has occurred for me now at least in the Ganymede build i mentioned.
Comment 1 Carl-Eric Menzel CLA 2008-06-13 07:12:31 EDT
This is currently a problem for me as a member of a large team in a project using Commons Attributes rather heavily. I can't use the code formatter before checking in, because I break formatting, causing much large diffs than necessary. Other team members likely won't upgrade to Ganymede because of this. I really like the rest of Ganymede though, so I would love to get this fixed before the release :-) Is there anything else I can do to help?
Comment 2 Frederic Fusier CLA 2008-06-13 07:18:10 EDT
I'll investigate
Comment 3 Frederic Fusier CLA 2008-06-13 08:01:26 EDT
(In reply to comment #1)
> This is currently a problem for me as a member of a large team in a project
> using Commons Attributes rather heavily. I can't use the code formatter before
> checking in, because I break formatting, causing much large diffs than
> necessary. Other team members likely won't upgrade to Ganymede because of this.
> I really like the rest of Ganymede though, so I would love to get this fixed
> before the release :-) Is there anything else I can do to help?
> 
Unfortunately, I'm afraid this was definitely too late for 3.4.0 :-(

This is a regression introduced  in 3.4M7 when activating the new comment formatter. It was worst than that in that milestone but was partially fixed by patch of bug 233224...
Comment 4 Philipe Mulet CLA 2008-06-13 09:02:23 EDT
We should fix it for 3.4.1, and possibly issue a separate patch for early adopters.
Comment 5 Carl-Eric Menzel CLA 2008-06-13 09:31:53 EDT
(In reply to comment #4)
> We should fix it for 3.4.1, and possibly issue a separate patch for early
> adopters.

I heartily agree with that, of course :-) How would I apply such a patch - would that simply be a jar to drop somewhere, or will we take a snapshot and compile it ourselves? Either is fine, I'm just curious.

Thanks for the quick replies.
Comment 6 Frederic Fusier CLA 2008-06-13 12:44:45 EDT
Created attachment 104889 [details]
Proposed patch

The patch consider such tags as invalid which makes them considered as simple text...

Note that the observed behavior was due that the entire annotations was wrongly consumed as a tag name which screwed up the parser after the end of the javadoc comment. This explains why put another valid tag after is a valid workaround. It would have been possible to put any text after the tag. E.g.
public interface Test {
/**
 * foo
 * 
 * @@Foo("foo") text
 */
Object doSomething(Object object) throws Exception;
}
is also formatted correctly...

So, the proposed patch is only a fix for the formatter which can be released rapidly after 3.4.0 delivery and minimize the impact of the changes. However, it does not fix the problem for other comment parser. I'll open another bug to fix the issue for all parsers in 3.5 when this one will be accepted and released...
Comment 7 Frederic Fusier CLA 2008-06-19 03:31:56 EDT
(In reply to comment #6)
> So, the proposed patch is only a fix for the formatter which can be released
> rapidly after 3.4.0 delivery and minimize the impact of the changes. However,
> it does not fix the problem for other comment parser. I'll open another bug to
> fix the issue for all parsers in 3.5 when this one will be accepted and
> released...
> 
Opened bug 237742 to fix this issue in a proper way... Note that the proposed patch for this bug has some side effect on current tests workspaces as following tags are no longer considered as valid:
@@return
@{link
@<!--

The output is changed as instead of starting these tags at the beginning of the line the formatter considers them as normal text and append them to the previous text of the comment...

This would have no impact on the given example, but the slightly changed one:
    /**
     * foo
     * @@Foo("foo")
     */
    Object doSomething(Object object) throws Exception;

will be formatted as:
	/**
	 * foo @@Foo("foo")
	 */
	Object doSomething(Object object) throws Exception;

Would it be acceptable waiting for a more elaborate fix which will be implemented in 3.5 for bug 237742?
Comment 8 Carl-Eric Menzel CLA 2008-06-19 05:15:04 EDT
(In reply to comment #7)
> will be formatted as:
>         /**
>          * foo @@Foo("foo")
>          */
>         Object doSomething(Object object) throws Exception;

I think this is not beautiful, but certainly acceptable.

> Would it be acceptable waiting for a more elaborate fix which will be
> implemented in 3.5 for bug 237742?

I'm not quite following. Do you mean not fixing this right now with the imperfect (but IMO bearable) patch and instead waiting for the next release cycle? That would be a year from now, wouldn't it?

Frankly, I'd very much like to see this fixed as soon as possible, for the reasons given in an earlier comment. The only thing where I'm not sure is whether this here will prevent any custom-tag tools from working, if the tag is not at the beginning of the line. If that is not a concern, I am all for doing both: Apply the workaround now and make a real fix later.
Comment 9 Frederic Fusier CLA 2008-06-19 06:29:00 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > will be formatted as:
> >         /**
> >          * foo @@Foo("foo")
> >          */
> >         Object doSomething(Object object) throws Exception;
> 
> I think this is not beautiful, but certainly acceptable.
> 
> > Would it be acceptable waiting for a more elaborate fix which will be
> > implemented in 3.5 for bug 237742?
> 
> I'm not quite following. Do you mean not fixing this right now with the
> imperfect (but IMO bearable) patch and instead waiting for the next release
> cycle? That would be a year from now, wouldn't it?
> 
> Frankly, I'd very much like to see this fixed as soon as possible, for the
> reasons given in an earlier comment. The only thing where I'm not sure is
> whether this here will prevent any custom-tag tools from working, if the tag is
> not at the beginning of the line. If that is not a concern, I am all for doing
> both: Apply the workaround now and make a real fix later.
> 
Sorry for the confusion... I meant:
1) apply the proposed patch which will fix the problem of the unnecessary inserted line *and* let the tag at the beginning of the line in the example you provided in comment 0.
2) fix the side effect described in comment 6 only in version 3.5 when fixing
bug 237742

However, I'm working on a better patch which would fix both cases! I should attach it as soon as all tests are OK... :-)
Comment 10 Frederic Fusier CLA 2008-06-19 09:31:13 EDT
Created attachment 105409 [details]
New proposed patch

This one is definitely better... No impact on existing comment formatter behavior except 1 file in Ganymede RC4 workspace which does no longer has different output while formatting it twice :-)

As I said previously, this patch fixes both comment 0 test case and comment 6, producing the same output. Which means that '@@Foo' is still considered as a javadoc tag and then still starts a the beginning of the line as expected in this case...
Comment 11 Carl-Eric Menzel CLA 2008-06-19 10:03:17 EDT
(In reply to comment #10)
> This one is definitely better... No impact on existing comment formatter
> behavior except 1 file in Ganymede RC4 workspace which does no longer has
> different output while formatting it twice :-)
> 
> As I said previously, this patch fixes both comment 0 test case and comment 6,
> producing the same output. Which means that '@@Foo' is still considered as a
> javadoc tag and then still starts a the beginning of the line as expected in
> this case...

That sounds excellent :-) Thanks!
Comment 12 Frederic Fusier CLA 2008-06-20 11:58:19 EDT
Released for 3.5M1 in HEAD stream.
Comment 13 Jerome Lanneluc CLA 2008-06-25 02:04:02 EDT
+1 for backport to 3.4.1
Comment 14 Frederic Fusier CLA 2008-06-25 05:07:46 EDT
Released for 3.4.1
Comment 15 Olivier Thomann CLA 2008-08-06 12:24:51 EDT
Verified for 3.5M1 using I20080805-1307
Comment 16 Olivier Thomann CLA 2008-08-06 13:26:36 EDT
Reopen to close as RESOLVED/FIXED. Will be closed as VERIFIED during 3.4.1
verification pass.
Comment 17 Olivier Thomann CLA 2008-08-06 13:26:47 EDT
Fixed.
Comment 18 Olivier Thomann CLA 2008-08-28 12:16:15 EDT
Verified for 3.4.1 using M20080827-2000