Fix #5202: Refine argForParam #5214
Conversation
|
I have a tendency to merge this now. It's probably too hard to review in short time. |
| case arg => reapply(arg) | ||
| } | ||
| case TypeBounds(lo, hi) => | ||
| range(lo, hi) |
There was a problem hiding this comment.
Why not do range(atVariance(-variance)(reapply(lo)), reapply(hi)) here, like above ?
| @@ -4479,12 +4490,15 @@ object Types { | |||
| else pre match { | |||
| case Range(preLo, preHi) => | |||
| val forwarded = | |||
| if (tp.symbol.is(ClassTypeParam)) expandParam(tp, preHi) | |||
| if (tp.symbol.is(ClassTypeParam)) expandParam(tp, preHi, tp.symbol.paramVariance) | |||
| else tryWiden(tp, preHi) | |||
| forwarded.orElse( | |||
| range(super.derivedSelect(tp, preLo), super.derivedSelect(tp, preHi))) | |||
There was a problem hiding this comment.
Those derivedSelect calls could return TypeBounds too, but this isn't checked like what is done just below
There was a problem hiding this comment.
I would think that's not possible, since TypeBounds don't appear in ranges, sopreLo and preHi cannot be TypeBounds.
There was a problem hiding this comment.
preLo and preHi cannot be TypeBounds
Right, but super.derivedSelect(tp, preLo) may call tp.argForParam(preLo) which may return a TypeBounds if tp is an invariant type parameter, or am I missing something ?
There was a problem hiding this comment.
Yes, you are right.
|
|
||
| f.apply(5) // error - found: Int expected: Int & String | ||
| f("c") // error - found: String expected: Int & String | ||
| } |
There was a problem hiding this comment.
More examples would be nice, e.g. I had one at #5202 (comment), which could be adapted to try every combination of (covariant, invariant, contravariant) x (and, or)
| */ | ||
| def argForParam(pre: Type)(implicit ctx: Context): Type = { | ||
| def argForParam(pre: Type, variance: Int)(implicit ctx: Context): Type = { |
There was a problem hiding this comment.
Do we actually need to pass the variance here? Shouldn't it always be the same as symbol.paramVariance ?
There was a problem hiding this comment.
It seems like we don't.
argForParam was overly naive for the cases where the prefix was an & or | type.
|
@smarter Your remarks should be all addressed now. |
argForParam was overly naive for the cases where the prefix was an & or | type.
We can't simply propagate the &/| then, this only works for co-variant parameters. What we need
to do instead is considerably more involved.