Preserve tvar bounds that represent kinds #6156
Conversation
Just an optimization (which might not actually be an optimization in practice if we're not careful and end up with non-local returns). |
| @@ -929,7 +929,10 @@ object Contexts { | |||
| // - we don't want TyperState instantiating these TypeVars | |||
| // - we don't want TypeComparer constraining these TypeVars | |||
| val poly = PolyType(DepParamName.fresh(sym.name.toTypeName) :: Nil)( | |||
| pt => TypeBounds.empty :: Nil, | |||
| pt => (sym.info match { | |||
| case tb @ TypeBounds(lo, hi) if (lo eq defn.NothingType) && hi.isLambdaSub => tb | |||
There was a problem hiding this comment.
The lower bound being Nothing or not shouldn't change anything here.
There was a problem hiding this comment.
My rationale was to restrict the change to just those bounds which only convey kind information. Happy to loosen that restriction if you think it's the right thing to do.
There was a problem hiding this comment.
If you don't want to loose the kind information when the lower bound is Nothing, then I don't think you want to loose it just because someone put a more precise lower bound. Try adding some extra testcases to clarify this.
There was a problem hiding this comment.
I agree that it should not matter whether lower bound is Nothing. Btw, to test for nothingness, use lo.isRef(NothingClass). eq on types is unpredictable, since types might be re-generated in interactive scenarios between runs.
I am also mystified by the treatment of the upper bound. As things stand, we make the upper bound Any for all first kinded types (including ones with a bound), but we copy the upper bound from sym if it is higher-kinded. That looks unsystematic to me. Does the precise bound matter at all? This is a question for @AleksanderBG .
There was a problem hiding this comment.
I've removed the restriction on the lower bound and added a couple more tests as suggested by @smarter. The last of these (with List as a lower bound) failed with the restriction in place.
|
@AleksanderBG should also review this. Also, I noted that GADTMaps now constitute a large chunk of code in an already large file |
The kinds of type variables being solved are represented by by their upper bounds. If we loose track of these then kinds will be incorrect on the RHS of inline matches and match types. Without the additional change in TypeComparer this breaks tests/pos/i5574.scala. The added fallback to fourthTry brings the higher kinded case into line with the path that's followed in the simply kinded case and restores the correct behaviour. Unfortunately I'm not able to justify it other than that it emerged from extensive trial and error and comparison with the simply kinded case. I'd also love to understand the use return in TypeComparer. Is it an optimization or does it have some semantic significance? Fixes scala#6014.
| pt => (sym.info match { | ||
| case tb @ TypeBounds(_, hi) if hi.isLambdaSub => tb | ||
| case _ => TypeBounds.empty | ||
| }) :: Nil, |
There was a problem hiding this comment.
This code looks a bit suspicious (we should already record the info as bounds in newPatternBoundSymbol), so I checked why does it make a difference. It seems that when adding bounds to Constraint, it checks whether they fit in the TypeParamRefs original bounds. This turns out to actually fail when dealing with HK types.
This'll do for now, but I'd like to touch up this code in the future - in general when inserting a symbol into GADTMap, we probably should just use the initial bounds as the fake TypeParamRefs bounds.
There was a problem hiding this comment.
OK, then can you open an issue and add a TODO in the code that points to the issue so we can keep track of this ?
|
Test renamed as requested. |
|
Also need to rename in the blacklist ... |
@AleksanderBG happy for this to be merged? |
|
@milessabin There were some post-#6165 conflicts, I'll merge once the tests pass (or you can do that if you notice it first 😄). Note to self: remember that on Github, "resolve conflicts" apparently means "merge master into the branch, unnecessarily convoluting history". Best not to touch that thing in the future. |
The kinds of type variables being solved are represented by their upper bounds. If we lose track of these then kinds will be incorrect on the RHS of inline matches and match types.
Without the additional change in
TypeComparerthis breakstests/pos/i5574.scala. The added fallback tofourthTrybrings the higher kinded case into line with the path that's followed in the simply kindedcase and restores the correct behaviour. Unfortunately I'm not able to justify it other than that it emerged from extensive trial and error and comparison with the simply kinded case.
I'd also love to understand the use of
returninTypeComparer. Is it an optimization or does it have some semantic significance?Fixes #6014.