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 #4031, v3: Check arguments of dependent methods for realizability #5559

Closed
wants to merge 12 commits into from

Conversation

Blaisorblade
Copy link
Contributor

Fix #4031. This PR builds off #5558 and otherwise rebases the core changes related to that in #4036 (which were mostly about using realizability, not changing it).

There's still a fishy part from #4031 to make tests/pos/i4031.scala work:

def upcast(a: A, x: Any): a.L = x
def coerce3(x: Any): Any = upcast(p, x) // p is unstable!

When typechecking coerce3, since p is unstable, the return type cannot be p.L; so safeSubstParam eliminates references to p. Currently that produces Any#L; so we either produce something better, or we add some kludge so that Any#L <: Any. In both cases, per test i4031-anysel.scala, the user must still be forbidden from writing Any#L. There are two solutions here (from @odersky):

  1. either change derivedSelect(tp, pre = Any) to give Any, so that Any#L gives Any.
  2. or just make Any a supertype of all types, similarly to AnyKind, but keeping Any kind-monomorphic (I think?) thanks to isLambdaSub.

Blaisorblade and others added 12 commits December 2, 2018 17:40
I had a TypeError crash in refchecks after screwing up a typeclass encoding in a particularly bad way.
This commit reports an error instead.
A TermRef is stable if its underlying type is stable. Realizability should
behave the same way.
Part 1 of 9125f58.
I wrote this because I feared (incorrectly) exponential slowdowns in
realizability checking for this code. But debugging suggests that the
complexity of realizability checking is constant in the size of these
expressions (even after I disable caching of `Stable`).

Beware 1: these expressions almost smash the stack by sheer size.

Beware 2: this fails with `-Yno-deep-subtypes`, but simply because the checking
heuristics assumes people don't try to do this.
The first attempt required changing z1720.scala; but that became unnecessary
after aligning isRealizable with isStable.
A TermRef is stable if its underlying type is stable. Realizability should
behave the same way.
After porting the commit from scala#4036, this line newly gives an error. Might be
OK, investigate.
Add the rule T <: Any for any *-Type T. This was not include fully before. We
did have the rule that T <: Any, if Any is in the base types of T. However,
it could be that the base type wrt Any does not exist. Example:

    Any#L <: Any

yielded false before, now yields true. This error manifested itself in i4031.scala.

With the new rule, we can drop again the special case in derivedSelect.
@Blaisorblade Blaisorblade changed the title Fix #4031, 3rd attempt: Fix #4031, 3rd attempt: Check arguments of dependent methods for realizability Dec 2, 2018
@Blaisorblade Blaisorblade changed the title Fix #4031, 3rd attempt: Check arguments of dependent methods for realizability Fix #4031, v3: Check arguments of dependent methods for realizability Dec 2, 2018
@Blaisorblade
Copy link
Contributor Author

Blaisorblade commented Dec 2, 2018

The failure appears genuine, in the interaction between staging and dependent types. The error in #5557 suggests this already happens when rebasing #4036 directly and without most of my changes to realizability. @odersky can you take a look? The error is in a call to dependent method dynamicApply[This <: NonEmptyTuple] (self: This, n: Int): Elem[This, n.type], so this seems to take ~n as unstable?!?

sbt:dotty> ; dotty-bootstrapped/clean; dotty-library-bootstrapped/compile
[...]
[info] Compiling 62 Scala sources and 136 Java sources to /Users/pgiarrusso/git/dotty/library/../out/bootstrap/dotty-library-bootstrapped/scala-0.12/classes ...
[error] -- [E007] Type Mismatch Error: /Users/pgiarrusso/git/dotty/library/src-scala3/scala/StagedTuple.scala:125:35
[error] 125 |    if (!specialize) '(dynamicApply(~tup, ~n))
[error]     |                       ^^^^^^^^^^^^^^^^^^^^^^
[error]     |                       found:    Tuple.Elem[Tup, Any]
[error]     |                       required: Tuple.Elem[Tup, N]
[error] -- [E007] Type Mismatch Error: /Users/pgiarrusso/git/dotty/library/src-scala3/scala/StagedTuple.scala:129:35
[error] 129 |        case None => '(dynamicApply(~tup, ~n))
[error]     |                       ^^^^^^^^^^^^^^^^^^^^^^
[error]     |                       found:    Tuple.Elem[Tup, Any]
[error]     |                       required: Tuple.Elem[Tup, N]
[error] two errors found
[error] (dotty-library-bootstrapped / Compile / compileIncremental) Compilation failed
[error] Total time: 12 s, completed Dec 2, 2018 6:40:01 PM

@Blaisorblade
Copy link
Contributor Author

FWIW, the problem seems to arise from widening Tuple.Elem[This, n.type] to N in safeSubstParamN is clearly unrealizable.
The result of widen seems wrong, and Tuple.Elem[This, n.type] seems already widened, so safeSubstParam should not widen it again (or the caller shouldn't widen it).

tp = Tuple.Elem[This, n.type] / AppliedType(TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class scala)),module Tuple),type Elem),List(TypeVar(TypeParamRef(This)), TermParamRef(n))), widenedArgType = N / TypeRef(NoPrefix,type N)
diff --git compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
index 38b348227..a27369583 100644
--- compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
+++ compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
@@ -359,6 +359,7 @@ trait TypeAssigner {
       if (realizability(widenedArgType) == Realizable)
         tp.substParam(pref, SkolemType(widenedArgType))
       else {
+        Console.err.println(s"tp = ${tp.show} / $tp, widenedArgType = ${widenedArgType.show} / $widenedArgType")
         val avoidParam = new ApproximatingTypeMap {
           variance = initVariance
           def apply(t: Type) = if (t `eq` pref) emptyRange else mapOver(t)```

@odersky
Copy link
Contributor

odersky commented Dec 4, 2018

I had a long and hard look at this, but did not come up with ideas. I don't see a reason to reject the failing examples, they should pass. We need to arrive at a version of realizability and safeSubstParams that allows this. It's really safeSubstParams that's the culprit here.

So what happens is this:

  • ~n is not recognized as realizable since its underlying type N is not concrete.
  • safeSubstParams then decides that it cannot give dynamicApply(~tup, ~n) the type Tuple.Elem[This, N(?1)] and it gives it the type Tuple.Elem[This, Any] instead.
  • That logic is clearly too conservative. There's no problem with Tuple.Elem[This, N(?1)]

So it seems we need to work on safeSubstParams to make this pass. It's instructive to compare with #4031, where we should get an error:

  • p is not recognized as realizable since its underlying type Test.A{L <: Nothing} has problem bounds.
  • safeSubstParams then decides that it cannot give upcast(p, x) the type ?1.L where ?1 would
    be a skolem of type Test.A{L <: Nothing}. and it gives it the type Any#L instead which leads to an error.

I believe the difference is where the replacement for the unrealizable type is subsequently used in the substitution. For #4031 is appears left of a # where we expect a realizable type. But for StagedTuple it appears as a type argument where realizability does not matter. It seems we need to distinguish these two cases in the logic of safeSubstParams.

@odersky
Copy link
Contributor

odersky commented Dec 5, 2018

Closing since #4031 is not a bug that can be fixed right now.

@odersky odersky closed this Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants