Skip to content
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

Fix improper usage of constrained breaking type inference #13779

Merged
merged 1 commit into from Nov 3, 2021

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 20, 2021

In multiple places, we had code equivalent to the following pattern:

val (tl2, targs) = constrained(tl)
tl2.resultType <:< ...

which lead to subtype checks directly involving the TypeParamRefs of the
constrained type lambda. This commit uses the following pattern instead:

val (tl2, targs) = constrained(tl)
tl2.instantiate(targs.map(_.tpe)) <:< ...

which substitutes the TypeParamRefs by the corresponding TypeVars in the
constraint. This is necessary because when comparing
TypeParamRefs in isSubType:

  • we only recurse on the bounds of the TypeParamRef using
    isSubTypeWhenFrozen which prevents further constraints from being
    added (see the added stm.scala test case for an example where this
    matters).
  • if the corresponding TypeVar is instantiated and the TyperState has
    been gc()'ed, there is no way to find the instantiation corresponding
    to the current TypeParamRef anymore.

There is one place where I left the old logic intact:
TrackingTypeComparer#matchCase because the match type caching
logic (in MatchType#reduced) conflicted with the use of TypeVars since
it retracts the current TyperState.

This change breaks a test which involves an unlikely combination of
implicit conversion, overloading and apply insertion. Given that there
is always a tension between type inference and implicit conversion, and
that we're discouraging uses of implicit conversions, I think that's an
acceptable trade-off.

In multiple places, we had code equivalent to the following pattern:

    val (tl2, targs) = constrained(tl)
    tl2.resultType <:< ...

which lead to subtype checks directly involving the TypeParamRefs of the
constrained type lambda. This commit uses the following pattern instead:

    val (tl2, targs) = constrained(tl)
    tl2.instantiate(targs.map(_.tpe)) <:< ...

which substitutes the TypeParamRefs by the corresponding TypeVars in the
constraint. This is necessary because when comparing
TypeParamRefs in isSubType:
- we only recurse on the bounds of the TypeParamRef using
  `isSubTypeWhenFrozen` which prevents further constraints from being
  added (see the added stm.scala test case for an example where this
  matters).
- if the corresponding TypeVar is instantiated and the TyperState has
  been gc()'ed, there is no way to find the instantiation corresponding
  to the current TypeParamRef anymore.

There is one place where I left the old logic intact:
`TrackingTypeComparer#matchCase` because the match type caching
logic (in `MatchType#reduced`) conflicted with the use of TypeVars since
it retracts the current TyperState.

This change breaks a test which involves an unlikely combination of
implicit conversion, overloading and apply insertion. Given that there
is always a tension between type inference and implicit conversion, and
that we're discouraging uses of implicit conversions, I think that's an
acceptable trade-off.
@odersky odersky merged commit fa56a4e into scala:master Nov 3, 2021
@odersky odersky deleted the constrain-finalResultType-2 branch November 3, 2021 10:42
@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants