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

Split out immutable GadtConstraint #16602

Merged
merged 2 commits into from
Jan 26, 2023
Merged

Split out immutable GadtConstraint #16602

merged 2 commits into from
Jan 26, 2023

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 30, 2022

This change eagerly allocates a GadtConstraint whenever its state is changed, rather than simply changing the reference(s) in, what's now called, GadtConstraintHandling, such as constraint. Previous to a recent change, in order to restore the GADT constraints, a fresh copy was eagerly created in a number of cases, just so it can be used to restore, for the cases in which it must be restored. The recent change exposed the underlying pieces of the GADT constraint and allowed those to be restored as components. Now we still do that, but as a packaged up GadtConstraint that we created once, eagerly. That also makes sure that invariants in components are upheld, like the mappings.

@dwijnand dwijnand marked this pull request as ready for review December 30, 2022 22:33
@SethTisue

This comment was marked as resolved.

@dwijnand dwijnand assigned Linyxus and unassigned odersky Jan 23, 2023
@dwijnand dwijnand requested review from Linyxus and removed request for odersky January 23, 2023 16:11
Copy link
Contributor

@Linyxus Linyxus left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM! 👍

compiler/src/dotty/tools/dotc/core/Contexts.scala Outdated Show resolved Hide resolved
Comment on lines 139 to 143
sealed trait GadtConstraintHandling(private var myGadt: GadtConstraint) {
this: ConstraintHandling =>

def gadt: GadtConstraint = myGadt
private def gadt_=(g: GadtConstraint) = myGadt = g
Copy link
Contributor

@Linyxus Linyxus Jan 24, 2023

Choose a reason for hiding this comment

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

Maybe we should not make GadtConstraint a private variable in GadtConstraintHandling, but declare two methods for retrieving and updating GADT constraints in this trait, and require the class that extends this trait to provide this two methods, just like what we do in ConstraintHandling (here). Then GadtConstraintHandling will be a stateless mixin for GADT constraint manipulation, and Context.fresh would do the job of GadtConstraint.fresh as long as we put the GadtConstraint in the context.

@Linyxus Linyxus assigned dwijnand and unassigned Linyxus Jan 24, 2023
@dwijnand
Copy link
Member Author

Shall we have a GadtConstraint in the Context and extends GadtConstraintHandling wherever we would like to manipulate GADT constraints?

Maybe we should not make GadtConstraint a private variable in GadtConstraintHandling, but declare two methods for retrieving and updating GADT constraints in this trait, and require the class that extends this trait to provide this two methods, just like what we do in ConstraintHandling (here). Then GadtConstraintHandling will be a stateless mixin for GADT constraint manipulation, and Context.fresh would do the job of GadtConstraint.fresh as long as we put the GadtConstraint in the context.

So that doesn't work, because we want code that's running in an inner context to have changes apply to an outer context.
It's similar to TyperState. To that end I renamed it to GadtState, which is shorter and more palatable.

@dwijnand dwijnand assigned Linyxus and unassigned dwijnand Jan 24, 2023
@dwijnand dwijnand requested a review from Linyxus January 24, 2023 22:14
Copy link
Contributor

@Linyxus Linyxus left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. LGTM!

@Linyxus Linyxus merged commit 6ccdbd9 into scala:main Jan 26, 2023
@dwijnand dwijnand deleted the gadt-split branch January 26, 2023 08:49
@Kordyjan Kordyjan added this to the 3.3.0 milestone Aug 2, 2023
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.

5 participants