Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Instantiate more type variables to hard unions #15632
Instantiate more type variables to hard unions #15632
Changes from all commits
c1f35aa
f177266
d15558d
d7521bf
dfcfb6b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original idea was to change https://github.com/lampepfl/dotty/blob/6540ad9154797164561281080b6a4e24a191691d/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala#L266 which currently uses
|
(and thus creates soft unions) to instead check a flag to decide whether to use a soft or hard union. This would avoid having to convert unions after creating them, and it means we can reset the flag after adding the constraint.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I see with this is that the lower bound might be a union with type variables in it. In that case the type variables would end up in the ordering constraint and there would be no union to harden until the time we instantiate the type variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so we couldn't reset the flag but could check
hardVars
inConstraintHandling
when we create the unions I think, that would cover unions created later from propagating constraints based on ordering.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
hardVars
belongs inTyperState
. If it's inConstraintHandling
we would lose it when we commit a constraint.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I meant we could leave it in TyperState but check it in ConstraintHandling#addOneBound instead of checking it in TypeVar#instantiate, but I don't know if we're actually supposed to access the currrent TyperState from ConstraintHandling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
addOneBound
does not necessarily create a union. The union might be created by fullBounds when we instantiate the variable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as far as I can tell we don't use
fullBounds
to instantiate a type variable (except for GADTs but we know that's problematic: #14288)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use
fullLowerBound
inapproximation
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's right, so we'd have to check hardVars both in addOneBound and fullLowerBound, I'm neutral on whether this is better than the current scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to tie it to typevariable instantiation, since we do
fullLowerBound
also elsewhere. We could possibly add it toapproximate
, but maybe that mixes up things too much.