Community
Participate
Working Groups
20028303 Similar to VariableDeclarationFragment, SingleVariableDeclaration can also have dimensions appended to the name (it is possible to write public void foo(int[] i[])) How is this expressed in a SingleVariableDeclaration?
I think we missed that case. Jim - any comment?
In this case, int[] i[], what do you expect for the type: - an array of int with dimension 2 - an array of int with dimension 1? The extra dimensions would be 1.
SingleVariableDeclaration is different from VariableDeclarationFragment in that the former always contains both the type and the identifier. "int[] i[]" is represented exactly the same as "int[][] i". The other similar case is: public abstract int foo()[]; which is represented exactly the same way as: public abstract int[] foo(); So there should be no problem here.
Ok, but how do we know that there are brackets behing the local name?
This information is not represented in the AST. So it has the same status as whitespace and comments, which aren't in the AST either. Recommended usage is to always put "[]" after the type rather than the variable. So it is auguably not a great loss that the distinction between "int a[]" and "int[] a" is lost in the conversion to an AST.
The current implementation of SingleVariableDeclaration with having the extra dimensions folded in the variable type has the problem that the resulting ArrayType has invalid positions. There are use cases of the AST where simplifications fictions like this gives the AST user lots of work: - Flattening an AST: Having an AST to create the source string. Not possible to express how many brackets are behind the name (I know and totaly agree that this is bad style, but I don't think its up to the AST to teach about this) - Rewriting an AST: The rewriter needs to add and remove dimensions. To find out where the brackets are located now involves counting the brackets (with the scanner) to find out how many brackets are after the name. As VariableDeclarationFragment already introduced the concept of 'extra dimensions' I vote to add this to SingleVariableDeclaration and also to the MethodDeclaration. Or the doc defines that types in SingleVariableDeclaration and MethodDeclaration can contain components that have no source location set. (Note that this would introduce a general special handling for ArrayType locations)
(1) For the SingleVariableDeclaration "int a[]", the type child should have source range that extends through the "]". In this case, the source range of the type overlaps the source range of its sibling, the name. There is currently a bug here (MethodDeclaration has a similar problem). Clients should be able to see that there's something funny going on because of the existence of siblings with overlapping source ranges. (It is always the case that the source range of the child is contained within the source range of the parent.) (2) Concur that we could add 'extra dimensions' to SingleVariableDeclaration and MethodDeclaration. The number would be 0 by default, but could legitimately be any number as high as the number of dimensions in the array type. The extra dimension would not really do anything, other than record that some number of the dimensions were/should be placed outside the type. Done this way, this would not be a breaking API change, and should be easy to implement at the same time the above bug is fixed.
It is fine for me. Jim - Could you please add these new APIs and I will implement them in the same time I am fixing (1).
Added two API methods to MethodDeclaration and to SingleVariableDeclaration: public int getExtraArrayDimensions(); public void setExtraArrayDimensions(int dimensions); Unlike VariableDeclarationFragment, these fields are *not* considered by the standard implementation of ASTMatcher; that is, the ASTs for "int foo()[];" and "int[] foo();" would match.
Fixed and released in 2.1 stream.
One last question regardin overlapping source range (it is not clear to me how the proposed solution will deal with this). The rules for source ranges are as follows: - sibling ranges do not overlap - the parent range covers all sibling ranges. For an example like "int[] i[]" the node representing the type int[] spawns "int []" not "int[] i[]". Is this correct ?
What do I get now for int foo[]; type: PrimitiveType.INT name: foo extra dimension 1 is that correct? (that's what I would expect)
I would rather say: type: ArrayType dimension: 1 elementType: PrimitiveType.INT name: foo extra dimension 1 And the type binding for the type is an array type binding (int[]). For the fragments, it makes sense to have a type for the declaration which would be a primitive type int and a binding for the fragment which is an array type. In the case of a single variable declaration they are the same thing, so the type of the variable declaration fragment has to be an array type.
re: Dirk's comment No. Sibling source ranges overlap in the case of SingleVariableDeclaration and MethodDeclaration. But it is still true that the parent source range covers all sibling ranges.
Jim this violates lots of our assumptions we have made in AST rewriting and in the text change infrastructure. What is the reason that you want to support overlapping siblings ? And this seems to be different from what we do for variable declaration fragments. Why don't we handle this the same way ?
I took Martin's comment to mean that it was inconvenient because the source range excluded the "[]" tokens that we legitimately part of the type. But if non-overlapping sibiligs is important, we should back out of this part of the fix. We should clarify the API to make it clear that the extra array dimensions are not included in the source range. The type child of the SingleVariableDeclaration contains all the array dimensions, including the ones after the variable. This is the way it is in 2.0, and I would be reluctant to change this because it would be a breaking API change. The proposed methods provide the new functionality in a non- breaking way.
Could we clarify what we want to do? So far we have: 1) The type positions doesn't contain the brackets 2) The extra dimensions field reflects the number of brackets defined after the parameter of the method declaration or after the name of a single variable declaration. 3) The type of both nodes are still array types if the extra dimensions is different from 0. For now we have: 1) No 2) Yes 3) Yes Are these three statements fine for everybody? If yes, I will modify the code to get 1) right.
think the type not contain dimensions from the name. This is consistent with variable declaration fragments. Please consider this as a fix, don't be compatible on unspec'd, unclear code. (Its also a really rare case)
Do not compare with the variable declarations fragments. It is not the same thing. The fragments have their own bindings. They don't have a type. The type is set on the variable declaration statement and it is common to all fragments. This is why the type of the statement doesn't contain the extra dimensions. In the case of a single variable declaration, the type is on the single variable declaration. Therefore it has to be an array type if there is an extra dimension specified after the variable name. I think we might want to have a phone conversation instead of writing notes in this PR. It can be quite long to convince everybody.
Before we dicuss the topic on the phone here are my minimal "requirements" - siblings should not overlap. - parents must include all siblings. - we should be homogeneous whenever possible. Otherwise the AST isn't really a tree <g>. Here is my idea how to solve the problem. Consider the following example int[3] variable[]; will produce the following nodes: <SingleVariableDeclaration> <ArrayType> int[] resolveBinding() returns a binding for int[] </ArrayType/> <SimpleName> variable <SimpleName> extraDimensions() returns 1 resolveBinding().getType() returns a binding for int[][] </SingleVariableDeclaration> So the nodes and their bindings map to what their source ranges reprensent in code.
And how do you handle this: String[] foo(int i)[] { return null; } My point is that we introduce too much complexity in the tree. I don't like the idea that getType().resolveBinding() and resolveBinding().getType() don't return the same binding. It should be the same! I disagree that all the siblings should not overlap. As long as a parent contains all its children, it should be enough. Jim - any thoughts?
I've posted this to jdt-core-dev mailing list: The JDT DOM/AST API is does not make it clear how method declarations like int foo()[]; and parameter declarations like int a[] are handled. The issue concerns the extra "[]"s: - for the return type of a MethodDeclaration (the ones after the parameter list) - for the type of a SingleVariableDeclaration (the ones after the variable) (Note that there is no confusion with field declarations and regular local variable declarations, which are represented partly with VariableDeclaratrionFragment nodes and do not have this confusion). The 2.0 API does not clearly state what happens to the extra array dimensions. In the 2.0 implementation, the extra array dimensions are built in to the type but excluded from the type's source range. This has led to much confusion (see http://bugs.eclipse.org/bugs/show_bug.cgi?id=23284) Not addressing these, albeit obscure, cases does not seem wise because they will be a source of obscure bugs in the future. So for 2.1 we propose fixing the MethodDeclaration and SingleVariableDeclaration API as follows: - Add new API methods (get/setExtraArrayDimensions) for record the number of extra array dimensions. - State that extra array dimensions are not included structurally in the type. - State that extra array dimensions are not included in the source range of the type. Implementing these changes would break existing clients that are depending explicitly on certain readings of the current API contract, or ones implicitly relying on the currently implemented behavior. Before proceeding, we want to hear from others on the potential impact of this change.
Corrected API spec for MethodDeclaration and SingleVariableDeclaration as per above proposal. Also added words to AST.parseCompilationUnit to the effect that source ranges are properly nested within they paraents and siblings never overlap. This item is a minor breaking API change, and should be documented in the 2.1 release notes.
The javadocs of SingleVariableDeclaration.setExtraDimension(int) and MethodDeclaration.setExtraDimension(int) are wrong. They are just copy of the associated methods getExtraDimension().
Thanks Luc. I've corrected the mistakes.
Now both SingleVariableDeclaration and VariableDeclarationFragement have methods set/getExtraDimensions. Since they are subclasses of VariableDeclaration wouldn't it make sense to pull the methods up ?
Good suggestion. I've pulled set/getExtraDimensions up to VariableDeclaration. I've annotated the javadoc specs for both methods on VariableDeclarationFragement with @since 2.0 to make it clear that they are part of the 2.0 API.
Verified.
Regression tests added (test0393, test0394, test0395, test0396, test0397).
