Fix #10123: Try to fully define qualifier type #10205
Conversation
| else if !isFullyDefined(qualType, ForceDegree.none) && isFullyDefined(qualType, ForceDegree.failBottom) then | ||
| // try again with more defined qualifier type | ||
| selectionType(tree, qual1) |
There was a problem hiding this comment.
isFullyDefined traverses the whole type to instantiate type variables, but to be able to type a selection we really only need to make sure the top-level part of the type is instantiated, I've been experimenting with this separately since it would become even more necessary if we allow lambda binders to not be fully instantiated, if you want I can try to extract that into its own PR.
There was a problem hiding this comment.
Yes, please go ahead with this!
There was a problem hiding this comment.
I added a new method couldInstantiateTypeVar to Inferencing that does this.
odersky
force-pushed
the
fix-#10123
branch
2 times, most recently
from
4fc7c5a to
636e50e
Compare
| /** If `tp` is type variable with a lower bound in the current constraint, | ||
| * instantiate it from below. | ||
| */ | ||
| def couldInstantiateTypeVar(tp: Type)(using Context): Boolean = tp.dealias match |
There was a problem hiding this comment.
I don't think that's quite right either since given ?F[?A] I need to instantiate at least ?F, and then if it's a type lambda I may or may not need to instantaite ?A too. Also since this is a selection prefix, we want to avoid Nothing if possible, in fact in all situation where we're trying to avoid Nothing, we only want to avoid it at the top-level. This is what I came up last time I was looking at this: https://github.com/dotty-staging/dotty/blob/63c7bad7f01c5fccf4620376aa7c8f70bc90b66d/compiler/src/dotty/tools/dotc/typer/Inferencing.scala#L49-L83
but I was a bit concerned that this duplicated code from IsFullyDefinedAccumulator.
There was a problem hiding this comment.
Right. In this particular case we need to instantiate the ?F. The ?A can come later when we recursively call typedSelect on the new call. I have taken your changes and adapted them to couldInstantiateTypeVar. You don't need to check for LazyRef in either code, since that is already eliminated by dealias. On the other hand, one should look inside AndTypes, OrTypes, and AnnotatedTypes.
|
Ready to merge? |
|
I'd like to spend some time reviewing it tomorrow. |
|
OK. I let you take it over then. |
|
I propose to merge this now. |
Try to fully define qualifier type before emitting an error for a typed Select. The one possible objection to doing this is that it might hide other errors where a type was not sufficiently well defined before we select on it. On the other hand: - My comment in issue scala#10123 shows that the problem is more widespread than just a single case. - We otherwise rely on interpolation to instantiate type variables in many cases. But interpolation is meant as an optimization only, to keep constraint sets smaller. So, we should be prepared that interpolation will not do its job. The fix in this commit is a way to do so.
scala#10082 now compiles as well. The original test was about a confusing error message.
| && tvar.hasLowerBound => | ||
| tvar.instantiate(fromBelow = true) |
There was a problem hiding this comment.
When trying to select from something that doesn't have a lower-bound, rather than giving up I think it makes sense to try the upper bound, since it might be precise enough, also we could take instDireciton into account to be closer to the normal behavior of isFullyDefined:
| && tvar.hasLowerBound => | |
| tvar.instantiate(fromBelow = true) | |
| => | |
| val direction = instDirection(tvar.origin) | |
| tvar.instantiate(fromBelow = direction < 0 || tvar.hasLowerBound) |
There was a problem hiding this comment.
I am not sure that will help. The situation is that we try to select a member that does not exist on the type variable. Instantiating to the upper bound would not help in this case. If the member existed on the upper bound, it would also exist on the variable itself.
There was a problem hiding this comment.
If the member existed on the upper bound, it would also exist on the variable itself.
Ah indeed, because goParam in findMember will recurse on the upper bound, but isn't that actually problematic? If I have a selection x.foo where the type of x is a type variable ?T <: B, we'll end up selecting some overload of B#foo, but it's possible that ?T is then instantiated to some more precise type which contains another overload of foo (or an override with a more precise result type), meaning that the member selected is not preserved by retyping (in practice our ReTyper will keep the original selection, but this still seems like a source of confusion since the meaning of the code is not apparent from the types the user sees).
There was a problem hiding this comment.
This could be a problem. But (1) it's not related to this issue.(2) I don't know how to solve it.
There was a problem hiding this comment.
I think the change I'm proposing together with removing the recursion on the upper bound in goParam would solve that issue, but I agree this can be treated separately from this PR.
Try to fully define qualifier type before emitting an error for
a typed Select.
The one possible objection to doing this is that it might hide other
errors where a type was not sufficiently well defined before we
select on it. On the other hand:
shows that the problem is more widespread than just a single case.
many cases. But interpolation is meant as an optimization only, to
keep constraint sets smaller. So, we should be prepared that interpolation
will not do its job. The fix in this commit is a way to do so.