Details
Bug
Resolved
Major
Description
I suspect the method JavaParameter#getType()to give incorrect results for vararg-parameters, resulting in JavaClass#getMethodBySignature(String, Type[]) failing to correctly lookup existing methods.
getType() seems to just return the base type of a varargs-parameter. IMHO opinion the correct behaviour would be to return the corresponding array-type with dimension 1.
Example:
class Test {
method(String... param) {} // getType() returns Type 'String' instead of 'String[]'
method(String param) {} // getType() returns Type 'String' as well, so there's no way to distuingish the two methods
Result:
getMethodBySignature('method', new Type[] {new Type('String', 1)}) -> returns null
getMethodBySignature('method', new Type[] {new Type('String')}) -> returns first method
-> no way to retrieve second method at all?!
See attached test-case for a complete illustration of the problem, including a working JUnit-Test
Best regards,
Sam Bernet
Activity
Hi Robert,
I agree with your thought about the API. It seems OK to me to have an additional Constructor for Type:
public Type(String fullName, boolean varArgs) { this(fullName, varArgs ? 1 : 0); }
As shown above, internally we should just use a Type with a dimension of one, as the triple dot is in fact nothing but syntactic sugar for an Array. The Java VM itself doesn't know anything about varargs-Parameters, thus
void method(String[])
is fully equivalent to
void method(String...)
I suggest the following:
- adding above new Constructor to Type
- ensuring the given Type has dimension 1 in case the varargs-Flag is set in Constructor JavaParameter(Type, String, boolean, throwing an IllegalArgumentException if not - thus returning the correct type in JavaParameter#getType()
- adding overloaded method ModelBuilder#createType(TypeDef typeDef, boolean varargs), delegating to ModelBuilder#createType(TypeDef typeDef, int dimensions), with dimensions = varargs ? 1 : 0
- changing ModelBuilder#addParameter(FieldDef) to call the new createType method if fieldDef.isVarArgs is set
Forgot anything?
Regards,
Sam Bernet
Sam Bernet
added a comment - Hi Robert,
I agree with your thought about the API. It seems OK to me to have an additional Constructor for Type :
Type.java
public Type( String fullName, boolean varArgs) {
this (fullName, varArgs ? 1 : 0);
}
As shown above, internally we should just use a Type with a dimension of one, as the triple dot is in fact nothing but syntactic sugar for an Array. The Java VM itself doesn't know anything about varargs-Parameters, thus
void method(String[])
is fully equivalent to
void method(String...)
I suggest the following:
adding above new Constructor to Type
ensuring the given Type has dimension 1 in case the varargs-Flag is set in Constructor JavaParameter(Type, String, boolean , throwing an IllegalArgumentException if not - thus returning the correct type in JavaParameter#getType()
adding overloaded method ModelBuilder#createType(TypeDef typeDef, boolean varargs) , delegating to ModelBuilder#createType(TypeDef typeDef, int dimensions) , with dimensions = varargs ? 1 : 0
changing ModelBuilder#addParameter(FieldDef) to call the new createType method if fieldDef.isVarArgs is set
Forgot anything?
Regards,
Sam Bernet Funny, this morning I had a look to see if I missed some bugs and I noticed I still had to solve this one 
I've tried to work things out, but in the end I didn't like to solve this by changing Type to support varargs. It's just not the right place, it's JavaParameter vs. JavaClass/Type.
So I came up with another idea. Vararg says actually something about the method, why not solve it there? This way we're not hacking around, code adjustments are pretty straightforward.
Only one thing: getMethodBySignature(String name, Type[] types, boolean varArg) isn't possible, the signature is already in use where the boolean stands for 'useSuperclass'.
Another methodname is required. getVarargMethodBySignature is an option, but maybe it's better to try to keep this methods close to eachother by name like 'getMethod......', so it's easier for developers to recognize both methods.
Anybody any thoughts?
Robert Scholte
added a comment - Funny, this morning I had a look to see if I missed some bugs and I noticed I still had to solve this one
I've tried to work things out, but in the end I didn't like to solve this by changing Type to support varargs. It's just not the right place, it's JavaParameter vs. JavaClass/Type.
So I came up with another idea. Vararg says actually something about the method, why not solve it there? This way we're not hacking around, code adjustments are pretty straightforward.
Only one thing: getMethodBySignature(String name, Type[] types, boolean varArg) isn't possible, the signature is already in use where the boolean stands for 'useSuperclass'.
Another methodname is required. getVarargMethodBySignature is an option, but maybe it's better to try to keep this methods close to eachother by name like 'getMethod......', so it's easier for developers to recognize both methods.
Anybody any thoughts? Fixed in rev.705
I've added some overloaded methods.
Maybe with QDox-2.0 we have to change the order of parameters, but right now I've added it to the end to stay BC.
Snapshot deployed
Robert Scholte
added a comment - Fixed in rev.705
I've added some overloaded methods.
Maybe with QDox-2.0 we have to change the order of parameters, but right now I've added it to the end to stay BC.
Snapshot deployed Robert Scholte
made changes -
| Field | Original Value | New Value |
|---|---|---|
| Status | Open [ 1 ] | Resolved [ 5 ] |
| Resolution | Fixed [ 1 ] | |
| Assignee | Robert Scholte [ rfscholte ] | |
| Fix Version/s | 1.11 [ 16104 ] |
I still run into problem with vararg methods. Seems to me you forgot one line while adding the fix:
Regards,
Stefan Ackermann
Stefan Ackermann
added a comment - I still run into problem with vararg methods. Seems to me you forgot one line while adding the fix:
http://svn.qdox.codehaus.org/browse/qdox/trunk/qdox/src/java/com/thoughtworks/qdox/model/JavaClass.java?r=712#l441
Regards,
Stefan Ackermann Robert Scholte
made changes -
| Status | Resolved [ 5 ] | Reopened [ 4 ] |
| Resolution | Fixed [ 1 ] |
Confirm and reopened.
I wrote only unittests for the JavaMethod-part. I´ve written the failing JavaClass test, but it´s a bit more complicated. I´ve seen infinitive loops, so I need to restructure some parts.
Robert Scholte
added a comment - Confirm and reopened.
I wrote only unittests for the JavaMethod-part. I´ve written the failing JavaClass test, but it´s a bit more complicated. I´ve seen infinitive loops, so I need to restructure some parts. Fixed in rev. 725
Robert Scholte
added a comment - Fixed in rev. 725 Robert Scholte
made changes -
| Fix Version/s | 1.11 [ 16104 ] | |
| Resolution | Fixed [ 1 ] | |
| Fix Version/s | 1.12 [ 16282 ] | |
| Status | Reopened [ 4 ] | Resolved [ 5 ] |
I think we can keep the model cleaner if we avoid a translation from the triple dots to a dimension. Right now I'm thinking of a new constructor, where you can set a varargs boolean. This way the signature is much easier to recognize.