Skip to content

Conversation

hfreeb
Copy link
Contributor

@hfreeb hfreeb commented May 15, 2020

Currently, varargs in the symbol solver seem to only really be supported when the number of parameters still equals the number of arguments. This PR fixes that.

One caveat is that I don't believe a vararg of arrays will work properly since if an array is the only argument for the variadic parameter, then it is compared with the type of the variadic parameter itself, and not its component type (I found this assumption in other places too).

@hfreeb hfreeb changed the title Add support to the symbol solver for varargs with zero or more than one argument [WIP] Add support to the symbol solver for varargs with zero or more than one argument May 15, 2020
@hfreeb hfreeb changed the title [WIP] Add support to the symbol solver for varargs with zero or more than one argument Add support to the symbol solver for varargs with zero or more than one argument May 15, 2020
@hfreeb hfreeb changed the title Add support to the symbol solver for varargs with zero or more than one argument Add symbol solver support for variadic parameters given zero or more than one argument May 15, 2020
@MysterAitch MysterAitch added this to the next release 3.15.23 milestone May 17, 2020
@MysterAitch
Copy link
Member

When I look at PRs I try to run the tests on master before then also apply the fixes.

It seems that the tests all pass even without making the changes to MethodCallExprContext.java -- I've just pushed a commit with many of the changes commented out.

@hfreeb - it's an area I don't fully understand what's needed (or not) -- could you take another look and advise please? 😄

@hfreeb
Copy link
Contributor Author

hfreeb commented May 17, 2020

I've removed the comments and added a new test case which does require the changes to MethodResolutionLogic.
I'm sure that the test case could be generalised, but I'm not sure what determines which of the two MethodResolutionLogic#isApplicable methods is called.

Also, I fixed the caveat I mentioned in the PR text, so I believe this will now work as expected for a vararg of arrays.

…blic static final variables to Position.java from Node.java (deprecated them in Node.java with a message), deprecated the static method for pos in favour of calling the constructor directly,
@MysterAitch
Copy link
Member

MysterAitch commented May 21, 2020

I got a bit carried away in trying to understand what was going on, and added in a bunch of comments / renamed variables to be more explicit etc., so hopefully it's also now a bit more self-documenting.

Really grateful to @hfreeb for getting this started! This is your PR though -- please do take a look and make any changes you think are appropriate!

@hfreeb
Copy link
Contributor Author

hfreeb commented May 21, 2020

I got a bit carried away in trying to understand what was going on, and added in a bunch of comments / renamed variables to be more explicit etc., so hopefully it's also now a bit more self-documenting.

Really grateful to @hfreeb for getting this started! This is your PR though -- please do take a look and make any changes you think are appropriate!

The changes look good to me, the comments should definitely help the next person trying to figure all this out!

@MysterAitch
Copy link
Member

Cool :)

I'll merge it if you're happy to give the okay to do so!

@hfreeb
Copy link
Contributor Author

hfreeb commented May 21, 2020

Cool :)

I'll merge it if you're happy to give the okay to do so!

Yep, sounds good to me!

@MysterAitch MysterAitch merged commit f05105d into javaparser:master May 21, 2020
@hfreeb hfreeb deleted the hfreeb/varargs branch May 21, 2020 21:29
hfreeb added a commit to hfreeb/javaparser that referenced this pull request May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants