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.

Sign up for GitHub

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

Jump to bottom

Fix Java record problems (#19578) and (#19386) #19583

Merged
merged 6 commits into from
Feb 9, 2024
Merged

Fix Java record problems (#19578) and (#19386) #19583

merged 6 commits into from
Feb 9, 2024

Conversation

yilinwei
Copy link
Contributor

@yilinwei yilinwei commented Jan 31, 2024 •
edited by bishabosha
Loading

This is a dependency of #19577 and should be reviewed and merged first.

fixes #18639
fixes #19386
fixes #19578

@nicolasstucki
Copy link
Contributor

It seems that #19578 is still failing.

@nicolasstucki nicolasstucki self-assigned this Feb 1, 2024
@bishabosha
Copy link
Member

should link this to #18639

@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 1, 2024 •
edited
Loading

@nicolasstucki, @bishabosha

I started taking a look at this. I don't think the issue is in the parser - the type parameter looks good within the JavaParser - pretty sure the issue is in the Desugar.classDef. I can take this over since it pretty much overlaps with #19577 if you haven't made progress with the issue.

@bishabosha
Copy link
Member

@yilinwei I haven't started anything yet so please feel free

@yilinwei
Fix records with type parameters (scala#19578).
f7eb589 
This fixes a whole host of subtle issues.

 - The type parameter was not stamped correctly on the constructor
   causing the original error
 - The parsed record was not stamped with `JavaDefined`, which meant
   the duplicate constructors in the case of overrides were not
   removed.
@yilinwei
Add arity 0 regression test for records.
781c25f
@yilinwei yilinwei changed the title Add regression test for generic records (#19578) Fix (#19578) Feb 1, 2024
@yilinwei yilinwei changed the title Fix (#19578) Fix generic records (#19578) Feb 1, 2024
@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 1, 2024 •
edited
Loading

Turned out you were right, problem was in the parser @bishabosha, and that the parsed tree didn't trigger the correct branches within the Namer causing it to duplicate the type parameters on the primary constructor.

This should be all good to go, if it doesn't cause any other regressions.

@yilinwei
Fixes record accessors incorrect signature (scala#19386).
47511ae 
We change the proxy method to take in a single argument. This also
exposed a second issue where the overriden methods were not
invalidated correctly in the `Namer`.
@yilinwei yilinwei changed the title Fix generic records (#19578) Fix Java record problems (#19578) and (#19386) Feb 1, 2024
@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 8, 2024

@nicolasstucki @bishabosha is there anything I can do to help move this along?

@bishabosha
Copy link
Member

I've just been busy, sorry, free tomorrow

bishabosha
bishabosha approved these changes Feb 9, 2024
View reviewed changes
Copy link
Member

@bishabosha bishabosha left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, and I tried with some other variants, thanks for seeing this through!

@bishabosha bishabosha dismissed noti0na1’s stale review February 9, 2024 14:40

comments were addressed

@bishabosha bishabosha merged commit 2c0b021 into scala:main Feb 9, 2024
19 checks passed
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
@som-snytt som-snytt mentioned this pull request Mar 14, 2024
Merged
@He-Pin
Copy link

He-Pin commented Mar 16, 2024

@Kordyjan Will this be backported to 3.3.x?

@SethTisue SethTisue added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Mar 16, 2024
@ek-gh
Copy link

ek-gh commented Apr 4, 2024

I too am anxiously waiting for this to be back-ported to 3.3.x as well.

@WojciechMazur WojciechMazur mentioned this pull request Jun 28, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 1, 2024
@WojciechMazur
Backport "Fix Java record problems (#19578) and (#19386)" to LTS (#20890
95b8e13 
)

Backports #19583 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noti0na1 noti0na1 noti0na1 left review comments

@bishabosha bishabosha bishabosha approved these changes

@nicolasstucki nicolasstucki Awaiting requested review from nicolasstucki

Assignees

@nicolasstucki nicolasstucki

Labels
backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it.
Projects
None yet
Milestone
3.4.1
8 participants
@yilinwei @nicolasstucki @bishabosha @He-Pin @ek-gh @noti0na1 @SethTisue @Kordyjan