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

Indexer vs DotName.equals() for a nested class with simple name that ends with $ #97

Closed
mkouba opened this issue Dec 2, 2020 · 11 comments · Fixed by #130
Closed

Indexer vs DotName.equals() for a nested class with simple name that ends with $ #97

mkouba opened this issue Dec 2, 2020 · 11 comments · Fixed by #130

Comments

@mkouba
Copy link
Contributor

mkouba commented Dec 2, 2020

If there is a nested class with simple name Test$, e.g. org.jboss.jandex.test.IndexerTestCase$Test$ (note that although it's not recommended it's legal to use $ in a class name) then Indexer decodes the DotName as:

componentized	true	
innerClass	true	
local	"" (id=108)	
prefix	DotName  (id=111)	
   componentized	true	
   innerClass	true	
   local	"Test" (id=117)
   prefix	DotName  (id=86)	
      componentized	true	
      innerClass	false	
      local	"test" (id=130)	
      prefix	DotName  (id=131)	
         ...

The toString() of the DotName is org.jboss.jandex.test.IndexerTestCase$Test$ however the equals() method does not match the following:

DotName org = DotName.createComponentized(null, "org");
DotName jboss = DotName.createComponentized(org, "jboss");
DotName jandex = DotName.createComponentized(jboss, "jandex");
DotName test = DotName.createComponentized(jandex, "test");
DotName indexerTestCase = DotName.createComponentized(test, "IndexerTestCase");
DotName testName = DotName.createComponentized(indexerTestCase, Test$.class.getSimpleName(), true);

whose toString() is also org.jboss.jandex.test.IndexerTestCase$Test$.

Maybe I'm missing something obvious.

@mkouba
Copy link
Contributor Author

mkouba commented Dec 2, 2020

CC @Sanne @n1hility

mkouba added a commit to mkouba/jandex that referenced this issue Dec 2, 2020
@mkouba
Copy link
Contributor Author

mkouba commented Dec 2, 2020

A test case is available in my branch: https://github.com/mkouba/jandex/tree/issue-97

@n1hility
Copy link
Contributor

n1hility commented Dec 4, 2020

thanks will check it out

ungerts added a commit to ungerts/jandex that referenced this issue Dec 22, 2020
@ungerts
Copy link

ungerts commented Dec 22, 2020

At least the test execution is now successful.

@ungerts
Copy link

ungerts commented Dec 24, 2020

I think a general solution should take into account the InnerClasses Attribute of the class file and iterate through the outer classes which are already parsed.

@msogrin
Copy link

msogrin commented Jun 8, 2021

Any chance of a fix?
This bug breaks Quarkus compatibility with Scala (at least with Scala 2.13) which has several classes ending with $ in its standard library.

MikeEdgar added a commit to MikeEdgar/jandex that referenced this issue Aug 9, 2021
MikeEdgar pushed a commit to MikeEdgar/jandex that referenced this issue Aug 9, 2021
MikeEdgar added a commit to MikeEdgar/jandex that referenced this issue Aug 9, 2021
@Ladicek
Copy link
Contributor

Ladicek commented Aug 12, 2021

I'm looking at the idea of using the InnerClasses attribute and while I agree it would be the best way to do it, I also believe doing that would require a major overhaul of the indexing algorithm. The ClassFile attributes are stored at the very end of the class file, and we [currently] [have to] parse names way before reaching that point. So it's currently not practical, unfortunately.

@mkouba
Copy link
Contributor Author

mkouba commented Aug 26, 2021

Do we really want to close this issue? IIUIC that PR does not solve the problem, or?

@Ladicek
Copy link
Contributor

Ladicek commented Aug 26, 2021

It does solve the problem -- for a class name that ends with a single $ :-)

@mkouba
Copy link
Contributor Author

mkouba commented Aug 26, 2021

It does solve the problem -- for a class name that ends with a single $ :-)

Ok, how about multiple $? :D

No, I'm fine if you say it does solve the problem. Let's keep the issue closed.

@Ladicek
Copy link
Contributor

Ladicek commented Aug 26, 2021

It's been a few days, so I don't have it fresh in memory, but if I recall correctly, I couldn't find a proper solution that wouldn't use the InnerClasses attribute, and that would require a huge refactoring (or parsing the class file twice).

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 a pull request may close this issue.

5 participants