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

Fixup symbol ownership in eta expansion #6186

Merged
merged 2 commits into from Aug 7, 2018

Conversation

retronym
Copy link
Member

The fix for scala/bug#6274 did this selectively when function symbols
were introduced, but we really ought to insert the eta$n temp vals into
the owner chain of all lifted expressions.

@scala-jenkins scala-jenkins added this to the 2.13.0-M3 milestone Nov 21, 2017
@retronym
Copy link
Member Author

retronym commented Nov 21, 2017

The risk of this change here is that we run into the same problems that led to the reversion (#2601) of 83c9c76.

@retronym retronym added the WIP label Nov 21, 2017
tree.changeOwner(typer.context.owner -> valSym)
}
valSym.setInfo(rhs.tpe)
ValDef(valSym, rhs)
Copy link
Member

Choose a reason for hiding this comment

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

looks good; def etaExpand could now take a owner: Symbol parameter instead of the full typer: Typer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed a commit with that refactoring.

@adriaanm adriaanm modified the milestones: 2.13.0-M3, 2.13.0-M4 Dec 13, 2017
@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018
@retronym retronym self-assigned this Jun 6, 2018
tree.changeOwner(typer.context.owner -> valSym)
}
valSym.setInfo(rhs.tpe)
ValDef(valSym, rhs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed a commit with that refactoring.

retronym and others added 2 commits August 3, 2018 19:05
The fix for scala/bug#6274 did this selectively when function symbols
were introduced, but we really ought to insert the eta$n temp vals into
the owner chain of all lifted expressions.
Based on review by lrytz
@SethTisue
Copy link
Member

assuming not critical for M5. retarget if you disagree

@adriaanm adriaanm removed the WIP label Aug 7, 2018
@adriaanm adriaanm modified the milestones: 2.13.0-RC1, 2.13.0-M5 Aug 7, 2018
@adriaanm
Copy link
Contributor

adriaanm commented Aug 7, 2018

I think this one is ready to go, actually. If the resetAttrs interaction proves problematic, we can refine.

@adriaanm adriaanm merged commit 526d8ed into scala:2.13.x Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants