Fix bounds propagation #232
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When instantiating with a wildcard argument, take formal parameter bounds into account.
|
LGTM, but needs rebasing. |
|
Here's a tougher example. I was motivated to try out when I saw the call to // sandbox/test.scala
class Base {
type N
class Tree[-T >: N]
def f(x: Any): Tree[N] = x match {
case y: Tree[_] => y
}
}
class Derived extends Base {
def g(x: Any): Tree[N] = x match {
case y: Tree[_] => y // fails in both scalac and dotc
}
} |
|
Here's patch that makes this typecheck under scalac. I was using a bit of trial and error to get the right diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index a182a2e..dffed83 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -4923,8 +4923,10 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
val TypeBounds(lo1, hi1) = tbounds.subst(tparams, argtypes)
val lo = lub(List(lo0, lo1))
val hi = glb(List(hi0, hi1))
- if (!(lo =:= lo0 && hi =:= hi0))
- asym setInfo logResult(s"Updating bounds of ${asym.fullLocationString} in $tree from '$abounds' to")(TypeBounds(lo, hi))
+ if (!(lo =:= lo0 && hi =:= hi0)) {
+ val boundsSeenFrom = TypeBounds(lo, hi).asSeenFrom(tpt1.tpe.prefix, tparam.enclClass.owner.enclClass)
+ asym setInfo logResult(s"Updating bounds of ${asym.fullLocationString} in $tree from '$abounds' to")(boundsSeenFrom)
+ }
}
if (asym != null && asym.isAbstractType) {
arg match {/cc @adriaanm |
|
I've logged this as https://issues.scala-lang.org/browse/SI-8991 |
|
Here is an example that typechecks in class Base {
type N
class Tree[-S, -T >: Option[S]]
def g(x: Any): Tree[_, _ <: Option[N]] = x match {
case y: Tree[_, _] => y
}
}I believe this is due to a missing substitution, as is done in val TypeBounds(lo1, hi1) = tbounds.subst(tparams, argtypes) |
|
I would also be pretty cautious in propagating bounds to wildcards in non-pattern contexts. Paul tried that in the lead up to 2.11, but we ended up reverting in scala/scala#3509. IIRC, there were two problems. First, the propagation was conditional on the type parameter having a complete info to avoid cycles. This leads to compilation-order dependent instability of inferred types. Second, inferring more specific types was not source compatible. Here's an example of that which uses the |
|
@retronym: "Here is an example that typechecks in scalac but fails to do so in dotc." I don't see why it should typecheck. The Tree bound is a lower bound but the required bound is an upper bound. Why would the |
|
@retronym I think you are right about the dangers of &'ing with the bound eagerly. I'll abandon the current approach and open a new PR with a different scheme. |
@odersky This example is less cluttered. class Tree[S, T <: S]
class Base {
def g(x: Any): Tree[_, _ <: Int] = x match {
case y: Tree[Int @unchecked, _] => y
}
} |
This was referenced Nov 21, 2014
odersky
added a commit
to dotty-staging/dotty
that referenced
this pull request
Nov 26, 2014
Previously, a double definition errorfor `T` was produced in a case like this:
type T1 = C { T <: A }
type T2 = T1 { T <: B }
This was caused by the way T1 was treated in the refinement class
that is used to typecheck the type. Desugaring of T2 with `refinedTypeToClass`
would give
trait <refinement> extends T1 { type T <: B }
and `normalizeToClassRefs` would transform this to:
trait <refinement> extends C { type T <: A; type T <: B }
Hence the double definition. The new scheme desugars the rhs of `T2` to:
trait <refinement> extends C { this: T1 => type T <: B }
which avoids the problem.
Also, added tests that scala#232 (fix/boundsPropagation) indeed considers all refinements
together when comparing refined types.
odersky
added a commit
to dotty-staging/dotty
that referenced
this pull request
Nov 26, 2014
Previously, a double definition errorfor `T` was produced in a case like this:
type T1 = C { T <: A }
type T2 = T1 { T <: B }
This was caused by the way T1 was treated in the refinement class
that is used to typecheck the type. Desugaring of T2 with `refinedTypeToClass`
would give
trait <refinement> extends T1 { type T <: B }
and `normalizeToClassRefs` would transform this to:
trait <refinement> extends C { type T <: A; type T <: B }
Hence the double definition. The new scheme desugars the rhs of `T2` to:
trait <refinement> extends C { this: T1 => type T <: B }
which avoids the problem.
Also, added tests that scala#232 (fix/boundsPropagation) indeed considers all refinements
together when comparing refined types.
OlivierBlanvillain
pushed a commit
to OlivierBlanvillain/dotty
that referenced
this pull request
Dec 8, 2016
OlivierBlanvillain
pushed a commit
to OlivierBlanvillain/dotty
that referenced
this pull request
Dec 12, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When instantiating with a wildcard argument, take formal parameter
bounds into account. Review by @adriaanm @DarkDimius