Fix/bounds propagation v2 #238
Conversation
|
Supersedes #232 |
| case tp: RefinedType => dependsOnRefinedThis(tp.refinedInfo) || dependsOnRefinedThis(tp.parent) | ||
| case tp: TypeBounds => dependsOnRefinedThis(tp.lo) || dependsOnRefinedThis(tp.hi) | ||
| case tp: AnnotatedType => dependsOnRefinedThis(tp.underlying) | ||
| case tp: AndOrType => dependsOnRefinedThis(tp.tp1) || dependsOnRefinedThis(tp.tp2) |
There was a problem hiding this comment.
Should this also treat SuperType? I would think that should have either:
case tp: SuperType => dependsOnRefinedThis(tp.thistpe) || dependsOnRefinedThis(tp.supertpe)or maybe just:
case tp: SuperType => dependsOnRefinedThis(tp.thistpe)Here's a test case:
trait T { type X }
"": (T { type X = Int; type U = super[T].X })#Usandbox/test.scala:16: error: type mismatch:
found : String("")
required: Super(C.this.T{X = Int; U = super.X}).X
"": (T { type X = Int; type U = super[T].X })#U
^
I also tried this somewhat silly example, not sure what to make of the results.
"": ({ type U = this.type })#Usandbox/test.scala:18: error: type mismatch:
found : String("")
required: Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
Object{
U =
...(
...
)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}(this)
}#U
"": ({ type U = this.type })#U
^
There was a problem hiding this comment.
I pushed a commit which rules out the silly example as deprecated.
Regarding SuperTypes: I believe we do not want to allow them as prefixes of TypeRefs. I.e. only allow them as prefixes of termRefs. Then we do not need a case in lookupRefined for them. I'll open a separate issue for that.
|
This seems to evade the cyclic reference checker in dotty. Scalac does not allow it: class T {
type X = (U, U)
type U = X with Int // allowed in dotty
}
"": (T {})#U // allowedThe during completion of for Maybe this is intentional? def typeDefSig(tdef: TypeDef, sym: Symbol)(implicit ctx: Context): Type = {
completeParams(tdef.tparams)
sym.info = TypeBounds.empty // avoid cyclic reference errors for F-bounds
As context: I was looking for a way to form a cycle in the types that might lead to infinite recursion in |
|
I'm still working my way up to review of the meat of this PR, 5222c11. |
|
Encouragingly, Dotty compiles all of the /*
public class Exist<T extends String> {
// java helpfully re-interprets Exist<?> as Exist<? extends String>
public Exist<?> foo() { throw new RuntimeException(); }
}
*/
class Exist[T <: String] {
def foo: Exist[_] = null
}
/*
public class ExistF<T extends ExistF<T>> {
// java helpfully re-interprets ExistF<?> as ExistF<?0 extends ExistF<?0>>
public ExistF<?> foo() { throw new RuntimeException(); }
}
*/
class ExistF[T <: ExistF[T]] {
def foo: ExistF[_] = null
}
/*
public class ExistIndir<T extends String, U extends T> {
// java helpfully re-interprets ExistIndir<?> as ExistIndir<? extends String>
public ExistIndir<?, ?> foo() { throw new RuntimeException(); }
}
*/
class ExistIndir[T <: String, U <: T] {
def foo: ExistIndir[_, _] = null
}
class Test {
class MyExist extends ExistF[MyExist]
// SI-8197, SI-6169: java infers the bounds of existentials, so we have to as well now that SI-1786 is fixed...
def stringy: Exist[_ <: String] = (new Exist[String]).foo
// def fbounded: (ExistF[t] forSome {type t <: ExistF[t] }) = (new MyExist).foo
def indir: ExistIndir[_ <: String, _ <: String] = (new ExistIndir[String, String]).foo
}
/*
public abstract class OP<T> { }
public interface Skin<C extends Skinnable> { }
public interface Skinnable {
OP<Skin<?>> skinProperty();
}
*/
class OP[T]
trait Skin[C <: Skinnable]
trait Skinnable {
def skinProperty: OP[Skin[_]]
}
object ObjectProperty {
implicit def jfxObjectProperty2sfx[T](p: OP[T]): ObjectProperty[T] = new ObjectProperty[T](p)
}
class ObjectProperty[T](val delegate: OP[T])
trait TestWildcardBoundInference {
def delegate: Skinnable
def skin: ObjectProperty[Skin[_ /* inferred: <: Skinnable */]] = ObjectProperty.jfxObjectProperty2sfx(delegate.skinProperty)
skin: ObjectProperty[Skin[_ <: Skinnable]]
def skinCheckInference = delegate.skinProperty
skinCheckInference: ObjectProperty[Skin[_ <: Skinnable]]
} |
|
In scalac, the bounds propagation was added to |
An example where this helps:
Previously, the private value `mnemonics` in Coder.scala was fof the form
Lambda$IP { ... } # Apply
It now simplifies to a Map[...] type.
Needed some fixes to lookup refined. The potential alias type is now calculated by taking the member of the original refined type, instead of by simply following the refined info. This takes into account refinements that were defined after the refinement type that contains the alias. The change amde another test (transform) hit the deep subtype limit, which is now disabled.
We need to adapt type parameter bounds with an as-ssen-from to the prefix of the type constructor. Makes pos/boundspropagation pass.
Avoid the crash if origin is not associated with a bound in the current constraint.
Avoids cyclic references caused by forcing info too early.
Now: The underlying refined type. Was: The parent of the type. We need the change because RefinedThis is used as a narrowed version of the underlying refinedType (e.g. in TypeComparer rebase), and the old scheme would lose a binding of that type.
The previous scheme did not propagate bounds correctly. More generally,
given a comparison
T { X <: A } <: U { X <: B }
it would errenously decompose this to
T <: U, A <: B
But we really need to check whether the total constraint for X in T { X <: A }
subsumes the total constraint for X in T { X <: B }
The new scheme propagates only if the binding in the lower type is an alias.
E.g.
T { X = A } <: Y { X <: B }
decomposes to
T { A = A } <: U, A <: B
The change uncovered another bug, where in the slow path we too a member relative to a refined type;
We need to "narrow" the type to a RefinedThis instead. (See use of "narrow" in TypeComparer).
That change uncovered a third bug concerning the underlying type of a RefinedThis. The last bug was fixed in a previous commit (84f32cd).
Two tests (1048, 1843) which were pos tests for scalac but failed compling in dotc have
changed their status and location. They typecheck now, but fail later. They have been
moved to pending.
There's a lot of diagnostic code in TypeComparer to figure out the various problems. I left it in
to be able to come back to the commit in case there are more problems. The checks and diagnostics
will be removed in a subsequent commit.
Need to account for the fact that some argument types may be TypeBoudns themselves. The change makes Jason's latest example work.
Move core logic to TypeOps, only leave error reporting in Checking. That way, we have the option of re-using the code as a simple test elsewhere.
We used to approximate these by their bounds, but this is confusing. See comment in printbounds.scala.
Toucher checks, but only deprecated warnings instead of errors.
to reflect last commit.
More robust cyclicity check which does not depend on source positions.
odersky
force-pushed
the
fix/bounds-propagation-v2
branch
from
89e251d to
9d6c104
Compare
|
Force pushed after rebasing to current master |
Now detects the cycles reported by @retronym
|
Last commit detects the cycle between |
Turns out that the last commit was a red herring. None of the hoops it jumped though was necessary. Instead there was a bug in isRef which caused `&` to erroneously compute T & Int as Int. The bug was that we always approximated alias types by their high bound. But in the present case, this leads to errors because U gets 'bounds >: Nothing <: Any', but it was still an alias type (i.e. its Deferred flag is not set). The fix dereferences aliases only if their info is a TypeAlias.
|
@odersky Nice solution! Am I correct in assuming that any reference to those dummy symbols that is retained after typechecking would represent a failure in cycle detection? Worth adding a corresponding assertion in to |
|
@smarter re: indentation. It might not be common, but I think it's natural: If the comment fits on one |
|
@retronym See last commit. We don't need the more elaborate solution, after all. |
|
The end result here LGTM, in that I wasn't able to break it and it handles a number of test cases that have troubled scalac with far fewer special cases and concerns about cycles or non-determinism. You might consider a rebase to fix a few typos in commit messages and to squash some of the reverted experiments out of the history. I would appreciate an positive and negative concrete example to explain:
This wording would be useful to include in the code comments in One spot that I was was curious about is: def qualifies(m: SingleDenotation) = isSubType(m.info, tp2.refinedInfo)Is there any need to use as-seen-from here? If not, why not? |
|
I'll add those tests. Regarding |
|
In fact, I just found out that adding the tests properly requires another fix, concerning how refined types are elaborated. Because that fix is likely involved it warrants a separate PR. So the tests will come, but not in this PR. |
Backport "Allow private members when computing the denotation of a NamedType" to 3.3 LTS
Adds full bounds propagation and fixes quite a few other problems that were detected in the progress. Review by @retronym please.
Note: The encoding of parameterized types as refinement types requires bounds propagation for dotty. It's not optional, nor can it be restricted to pattern matching. In fact, there's nothing to propagate. Instead, when comparing refined types, we always need to take all bindings into account, which gives us bounds propagation for free.