-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow inline matches to bind type variables #5657
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
Conversation
89a164b
to
8c32798
Compare
@@ -1004,7 +1005,7 @@ trait Implicits { self: Typer => | |||
val generated2 = | |||
if (cand.isExtension) Applications.ExtMethodApply(generated1).withType(generated1.tpe) | |||
else generated1 | |||
SearchSuccess(generated2, ref, cand.level)(ctx.typerState) | |||
SearchSuccess(generated2, ref, cand.level)(ctx.typerState, ctx.gadt) |
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.
Would it make sense to combine typerState and GADT map?
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'm keeping my eye on it. I tried to do that, but couldn't find a clean approach. The problem is we don't want to set fresh typer state when we set fresh GADT state, and vice versa - it's really easiest if the two are separate objects. If current approach turns out to be too clumsy, I'll find a way to merge the two.
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 main concern is about the logic around getBinds
and findBoundVars
. The rest is cosmetic.
ctx.gadt.addBound(sym, hi, isUpper = true) | ||
} | ||
|
||
def addTypeBindings(list: List[(TypeSymbol, Boolean)])(implicit ctx: Context): Unit = |
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.
Something looks suspicious here: list
is a list of type variable occurrences. A symbol may appear several times in that list. Hence a symbol can appear several times in fromBuf
with different copies.
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.
Maybe that's still OK since every symbol is referred to only once, at its binding location (not sure about that!). But then it would be good to make the accumulator findBoundVars
more specific and robust. Why not combine findBoundVars
and getBind
?
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've taken another look at my approach - it looks like currently it's really not possible to get multiple bindings in a list, but relying on that could be fragile and it's relatively obvious that we should do the same as IsFullyDefinedAccumulator
, so I've reworked the List
into a type TypeBindsMap = SimpleIdentityMap[TypeSymbol, java.lang.Boolean]
and added appropriate value-merging logic to findBoundVars
.
Unless I'm missing something, we cannot merge findBoundVars
and getBinds
, since the latter locates symbols bound by the current pattern (as opposed to enclosing patterns) and the former find the variance of the symbol by traversing the type. I've renamed findBoundVars
to extractBindVariance
and added a comment to make this clearer.
c1fe272
to
aa014d4
Compare
I've addresed the comments. The rebase was strictly to make the branch mergeable and didn't modify the commits that were there, modulo including 01078c4. |
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.
Otherwise LGTM
Use SmartGADTMap and Constraint#approximation to infer and extract type values. Widen pattern-bound types in ProtoTypes so implicit search can find relevant candidates. When dropping unused bindings in Inliner, insert type bindings with a single pre-pass to correctly handle updating singleton types. Previously Select trees retained old type values, which prevented pickler roundtripping.
Re-typing the pattern in GADT context is unnecessary - reducing the pattern will on its own perform the same subtyping checks. Previously if a pattern binding a HK type was inlined, a warning saying that its kind is different from the kind of its parameter was emitted _after_ preparing for reduction. This replaced the type of the Bind node with an error, preventing the bind from being located and narrowed when reducing the inline match.
62a8499
to
e772dbb
Compare
Fixes #5405, #5574.
Github diff is unusually horrible for
dropUnusedBindings
, so let me quote part of commit message: