Bug 417923

Comment 1 Markus Keller 2013-10-15 15:58:09 EDT

We should not have any whitespace between an ArrayType's element type and its dimensions. String[][][][] is the common formatting.

Spaces can be used when there are annotations on dimensions, but if in doubt, better leave them out (especially if spaces could otherwise appear in non-annotated array types).

(from bug 413569 comment #29)
> a) ASTRewriteAnalyzer#visit(ArrayType): The rewriteNodeList(..., " ") call
> looks fishy. In the normal case (no annotations on dimensions), I wouldn't
> expect a space between consecutive [], e.g.: int[][]. No DOM test failed
> when I changed the last argument to "".
> ASTRewriteFlattener#visit(ArrayType) looks similar: I'd expect the separator
> to be "", not " ".
> 
> Please write two ASTRewrite tests that don't use any annotations:
> - create a new ArrayType with n > 1 dimensions
> - rewrite an existing ArrayType and insert a dimension

Comment 2 Srikanth Sankaran 2013-11-01 02:37:30 EDT

In conversations with Markus, this was called out as something the UI
team would like to see cleaned up fast.

Comment 3 Manoj N Palat 2013-11-06 01:33:48 EST

Created attachment 237222 [details]
Proposed Patch

Comment 4 Jay Arthanareeswaran 2013-11-06 10:11:40 EST

I see couple of issues with the fix (ref: the following code from ASTRewritingStatementsTest) :

int [] i []@Annot1 @Annot2 [] @Annot1 @Annot3 [] = new int @Annot1 @Annot2  [2] @Annot2 @Annot3 [size()] @Annot2 @Annot1 [][]@Annot3 @Annot2 @Annot1 [];\n");

1. Shouldn't we have a space between [] and the following annotation if any? In fact, in the tests in TypeAnnotationsConverterTest, we have examples of both kinds. We need to make them appear consistent.


2. "@Annot2  [2]" - There are two spaces between the annotation and [2]. Agree this was an existing code and not an extra dimension, but shouldn't we extend this fix to array initializers too?

Comment 5 Markus Keller 2013-11-06 14:36:49 EST

(In reply to Jayaprakash Arthanareeswaran from comment #4)

> 1. Shouldn't we have a space between [] and the following annotation if any?

I've read the 308 documents more closely, and I think the convention is to
(a) generally keep the terse notation without any spaces (e.g. int[][]), and
(b) if there are type annotations on a dimension, insert a space on both sides of the annotations list (e.g. int[] @A []).

I've asked on the spec list for confirmation.


> 2. "@Annot2  [2]" - There are two spaces between the annotation and [2].
> Agree this was an existing code and not an extra dimension, but shouldn't we
> extend this fix to array initializers too?

Yes, it should work the same for array initializers. In ASTRewritingStatementsTest.java on line 6042, I would expect "@Annot2 [2]" instead of "@Annot2  [2]", since newArrayType is initialized with ASTNode.copySubtree(..), which doesn't preserve formatting.

But on line 6123, it should stay "@Annot2  [2]", because that part of the array type is not modified. The
    rewrite.set(creation, ArrayCreation.TYPE_PROPERTY, arrayType, null);
at the end of the first block is a no-op, since arrayType is still the original node.


In general, ASTRewrite should not touch unaffected code. When type annotations are added/removed on a dimension, it should insert/remove adjacent spaces when inserting/removing the first/last annotation.

Comment 6 Markus Keller 2013-11-07 05:46:43 EST

> I've asked on the spec list for confirmation.

Has been confirmed, so we'll also use spaces iff a dimension is annotated.

Comment 7 Manoj N Palat 2013-11-10 16:56:47 EST

Created attachment 237347 [details]
Work in Progress patch

Comment 8 Manoj N Palat 2013-11-19 23:35:51 EST

Created attachment 237568 [details]
Proposed Patch

 - Fixed the space issue
 - Revamped the relevant code.
 - Added a lot more testcases as well.

Comment 9 Markus Keller 2013-11-21 08:11:32 EST

I quickly skimmed over the new tests and had two observations:

1. You always seem to use ast.newMarkerAnnotation(). Would be good to have a NormalAnnotation or a SingleMemberAnnotation once in a while, just to ensure it also works fine if an annotation has actual members with possibly nasty values like int[].class.

2. I didn't spot a test case that creates a new ArrayType, adds a new annotation to a dimension and then serializes the new array type node with an ASTRewrite.
I.e. create this variable declaration from scratch:

String[] @Anno [] var = new String[1] @Anno [2];

I suspect that the change in ASTRewriteFlattener could make this result in missing space, e.g. "int[]@Anno[]" instead of the correct "int[] @Anno []". Disclaimer: This is just a guess I didn't actually test. I could very well be totally wrong. But a having a test case with this scenario would still be good.

Comment 10 Jay Arthanareeswaran 2013-11-21 10:22:34 EST

I went through the patch and test cases and it appears to me that we are not consistent when it comes to removing the leading and trailing white spaces of an annotation. For e.g, consider the following two cases:


int[] @A   @B [][] - removing @B produces

            int[] @A [][]

int[] @A   @B @C [][] - removing @B produces

            int[] @A   @C [][]

And I am not sure if we should keep those spaces between @A and @C in the second case. I think, whenever we get an opportunity, i.e. we remove any of the annotations, we should try to keep exactly one space on either side of an annotation. That is, if that's not too much of work.

Comment 11 Manoj N Palat 2013-11-21 10:29:34 EST

(In reply to Markus Keller from comment #9)

Thanks Markus for the comments
> 
> 1.  Would be good to have a
> NormalAnnotation or a SingleMemberAnnotation once in a while, just to ensure
> it also works fine if an annotation has actual members with possibly nasty
> values like int[].class.
Will update the patch with such a test case.
> 
> 2. 
> I suspect that the change in ASTRewriteFlattener could make this result in
> missing space, e.g. "int[]@Anno[]" instead of the correct "int[] @Anno []".
This issue does happen but is agnostic to the change in ASTRFlattener, and happens due to formatting. Bug 422259 has been raised to track this.

Comment 12 Manoj N Palat 2013-11-21 10:37:41 EST

(In reply to Jayaprakash Arthanareeswaran from comment #10)
> 
> And I am not sure if we should keep those spaces between @A and @C in the
> second case. I think, whenever we get an opportunity, i.e. we remove any of
> the annotations, we should try to keep exactly one space on either side of
> an annotation. That is, if that's not too much of work.

Jay, Thanks for the comments. In fact, the special cases that we take care are the pre & post deletions of spaces if we are removing the "last" annotation (and similarly the first annotation insertion) as Markus pointed out in comment 5 last para. In case of other annotation removals we pass it through the standard rewriteNodeList() since it may involve too many special cases which require special treatment.

Comment 13 Jay Arthanareeswaran 2013-11-21 10:44:35 EST

(In reply to Manoj Palat from comment #12)
> Jay, Thanks for the comments. In fact, the special cases that we take care
> are the pre & post deletions of spaces if we are removing the "last"
> annotation (and similarly the first annotation insertion) as Markus pointed
> out in comment 5 last para. In case of other annotation removals we pass it
> through the standard rewriteNodeList() since it may involve too many special
> cases which require special treatment.

If we are sure that rewriteNodeList() is what produces the output mentioned in comment #10, then I am fine with what we have. Please release after incorporating the changes suggested so far.

Comment 14 Markus Keller 2013-11-21 10:45:56 EST

(In reply to Jayaprakash Arthanareeswaran from comment #10)
These examples look fine to me, but changing the implementation to "one space on each side of a touched node" would also be OK.

If the given formatting doesn't match what the formatter would produce, then we're operating by the "garbage in -> garbage out" principle. I.e. keep changes minimal, and choose the solution that is easiest to implement/understand in code. Avoid adding special cases just to "improve" behavior with strange input.

Comment 15 Manoj N Palat 2013-11-21 23:55:34 EST

(In reply to Markus Keller from comment #14)
> (In reply to Jayaprakash Arthanareeswaran from comment #10)
..
> I.e. keep changes minimal, and choose the solution that is easiest to
> implement/understand in code. Avoid adding special cases just to "improve"
> behavior with strange input.

Thanks Markus & Jay. Changes committed via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=cd7ab76e97bbde2dfa388ac94141efed3a1c5385

Will resolve the bug post bug 422259 closure and test.

Comment 16 Manoj N Palat 2013-11-24 04:03:40 EST

(In reply to Manoj Palat from comment #15)

> Will resolve the bug post bug 422259 closure and test.

Fixed