Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ReferenceBuilder.getTypeReference can handle complex type-params in generics #6176

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rishivijayv
Copy link
Contributor

Fix #6068

The code is similar to and inspired from the local fix that was submitted in the Github issue/bug-report itself by @josple.

I have also added a total of 5 test-cases: 1 of which (testGetTypeReferenceComplexGenericsCase) failed with the original code, but passes with the new version of the code.

Included in the test cases are also 3 "sanity check" tests (the last 3) for some of the other things that getTypeReference should be doing based on my understanding, though they might not be strictly related to this change.

Apart from the null return check (when the argument is not a type) and the array case, I think there should be sanity checks for the other cases of input for getReferenceType here (more on why these 2 are absent in the Notes below). If they're not needed in this PR, let me know and I will remove :)

Some Notes

  • The main issue here, as indicated in the GitHub issue, was that we were splitting generics type-parameters using , with the following code: final String[] split = m.group(2).split(",");. We also seem to be doing that exact same thing in the getTypeParameterReference method (here). I didn't look in to this and not entirely sure if this is also an issue (wanted to keep the scope of this fix limited), but just wanted to call out in case anyone else might know more of it and if that is something which should be fixed too.
  • I'm not entirely sure why getTypeReference is public, since its only usages seem to be in ReferenceBuilder itself (as seen below, the extra 5 were the tests that I've added). I made this fix (and added the tests) assuming we want to keep getTypeReference public, but if we think this should be a private method, then another way to fix this could be to make the new getStringTypeParameters function I've added a static function in a utility class (and convert these tests to tests of that utility function). Let me know if that's a better approach!
Screenshot 2025-02-09 at 10 41 35 PM
  • While writing some sanity-tests for ReferenceBuilder.getTypeReference, I noticed that for the case getTypeReference("notType<Integer>"), the function actually returned a CtTypeReference instead of null. I thought null was supposed to be returned based on the documentation since notType won't be a name of an actual type (since it does not start with an upper-case letter)? Or does that only apply to the case when the type is not a generic type? In any case, there is no test which checks for the null return here.
  • There is also no "sanity test" for the array case, because I was having some issue with NPE in the setPackageOrDeclaringType method. I tried a string with an array of a custom generic type first, then a HashMap array, then a simple String array, but in all cases I got an error similar to this. Eventually decided against digging deep in to this since that could have added more setup and bloated this PR even more.
Screenshot 2025-02-09 at 10 09 58 PM

@algomaster99 @monperrus @I-Al-Istannen would appreciate a peek whenever you have the time! 😄 🙏🏽

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.

[Bug]: ReferenceBuilder.getTypeReference() cannot handle complex type parameters
1 participant