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

Use simple name constructor in inner classes #2574

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rawi01
Copy link
Collaborator

@Rawi01 Rawi01 commented Sep 2, 2020

This PR fixes #2571.

Using @With in inner classes generates a constructor using the name of the outer and the inner class. ecj does not like that if there also is a generic. To solve this lombok now generates constructor class using raw types: new Outer<T>.Inner<U>() -> new Outer.Inner().

@Gaibhne
Copy link

Gaibhne commented Sep 7, 2020

I manually built the jar to test it, and both the test case as well as the project the issue originally occurred in compile fine with my Eclipse now. Thank you!

@rspilker
Copy link
Collaborator

rspilker commented Sep 7, 2020

I'm not sure it solves the right problem. In general, having a non-static inner type where the outer type contains generics is asking for compiler problems.

Maybe @rzwitserloot can share his experiences with this problem?

@Rawi01
Copy link
Collaborator Author

Rawi01 commented Sep 7, 2020

Is there a case that is not covered by test cases?

Information about inner classes + inner constructors + generics are quite rare and it seems like that even compilers disagree about how that should be handled 🤷‍♂️

@Gaibhne
Copy link

Gaibhne commented Sep 8, 2020

For what it's worth: the bug this is addressing is a regression - the code that triggers the compiler error without this PR used to compile just fine a while ago.

@rzwitserloot
Copy link
Collaborator

The problem is, I recently changed lombok to go from generating new Inner() to new Outer.Inner() to work around another bug. So, apparently, we're stuck between a rock and a hard place. Will investigate

@rzwitserloot
Copy link
Collaborator

I added all this to fix issue #2268; this revert would mean 2268 is then broken again. We can debate two things:

  • Is it even possible to fix both this issue as well as 2268? If not, is ecj just broken here and should we be filing bugs over there instead?
  • Failing that, is undoing the fix for 2268 and thereby fixing 2571 the lesser of the two evils?

But.. not for the next release, it'll have to wait.

new Outer<String>.Inner<Integer>() -> new new Outer.Inner()
@Rawi01
Copy link
Collaborator Author

Rawi01 commented Nov 16, 2020

@rzwitserloot Reworked the solution, it now uses raw types for the constructor call in eclipse/ecj. I also reverted the javac changes because I think that we should not worsen the delombok output to fix this weird ecj behaviour.

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] @With generates non-compiling code under Eclipse
4 participants