Eliminate class hierarchy in GadtConstraint #16194
Conversation
|
This messes with your big PR, @Linyxus, so I'm leaving you in charge of deciding when we can merge it 😄 |
There was a problem hiding this comment.
LGTM in general! There is one thing that concerns me: before this PR, calling methods like addToConstraint and addBound on EmptyGadtConstraint will issue an error. Now we would update the state of GadtConstraint.empty in such cases. I am not sure whether we would really do something ridiculous regard the empty constraint, but if that really happens then in this PR we would end up modifying the state of a constraint which we assume is always empty. This may give us errors and problems that are difficult to trace.
|
Maybe @abgruszecki would also want to have a look at this PR. :) Regarding the conflicts with #14754, it would not be a problem: I can always redo these changes on that branch since they do not change the behavior of the constraint solver in general (except for the empty constraint as I mentioned before, but should not be a problem). |
dwijnand
force-pushed
the
GadtConstraint/eliminate-hierarchy
branch
from
2177a2a
to
e6a9ffd
Compare
| @@ -10,77 +10,25 @@ import util.{SimpleIdentitySet, SimpleIdentityMap} | |||
| import collection.mutable | |||
| import printing._ | |||
|
|
|||
| import scala.annotation.internal.sharable | |||
| object GadtConstraint: | |||
| def apply(): GadtConstraint = empty | |||
There was a problem hiding this comment.
Should we have a "canonical" empty GadtConstraint, to avoid allocating an extra object every time we create a new constraint?
I don't think that should cause any issues. We may even want to "dynamically" reconstruct bounds on abstract types which are not yet registered in the |
I was concerned about this because I was thinking that there may be some cases where we really do not want any GADT reasoning (and has some assumption that the constraint is empty). But it seems that we does not have such cases in the compiler. If so, Dale's original implementation in this PR (where there is a sharable empty GADT constraint object) should be what we want. Sorry for the noise in my first review! |
We don't because any code like that could throw an exception today, i.e. without this PR. I don't think we should have a single shared, mutable empty Gadt Constraint. Because tomorrow someone could forget to call .fresh on it and we're leaking lambdas and bounds that belong to some other tree, other compilation unit, or even a previous compiler run.. |
|
Agreed, that makes sense. Then I guess this PR should be merged? |
|
Yeah, but let's do it after today's meeting. |
No description provided.