a migration conundrum (oldpatmat -> virtpatmat)

[Adriaan Moors]

[tagline: what to do about the old pattern matcher's GADT unsoundness that's relied on by, e.g., the std lib]

After spending a considerable amount banging my head against the wall and various pieces of office furniture,

I think I've convinced myself that some of the old typing rules for old pattern matching are wrong and that I need not (physically) abuse my head further.

I say let the code speak for itself (https://gist.github.com/1925479)

object Test extends App {
  class Wrapped[W](var x: W) // must be invariant (to trigger the bug)
  class AbsWrapperCov[+B] // must be covariant
  case class Wrapper[T](underlying: Wrapped[T]) extends AbsWrapperCov[T]


  def unwrap[A](it: AbsWrapperCov[A]): Wrapped[A] = it match {
    case Wrapper(wrapped) => wrapped // after inferTypedPattern: ?T <: A&0

    // -Yvirtpatmat says:
    // matchRes4 = <error: value wrapped>;
    // found   : Test.Wrapped[T] where type T <: A
    // required: Test.Wrapped[A]
    // which is the right thing to do, I'd say (try running this test)
    // however, the plain pattern matcher accepts this program
    // what's worse, the standard library relies on stuff like this type checking... 


      // from JavaConversions:
      //
      // implicit def asJavaIterator[A](it: Iterator[A]): ju.Iterator[A] = it match {
      //   case JIteratorWrapper(wrapped) => wrapped
      //   case _ => IteratorWrapper(it)
      // }


  }  



  class A { def imNotAB = println("notB")}
  class B

  val w = new Wrapped(new A)
  unwrap[Any](Wrapper(w)).x = new B

  w.x.imNotAB


}

(I hope I managed to preserve formatting this time)

so, this compiles on master (except when you use -Yvirtpatmat)

but I claim you don't want to set loose the resulting byte codes

the reason I'm contacting the list is that we've been relying on code like this compiling (e.g. JavaConversions),

and I'm sure that's not the only piece of code out there that would break if we're more strict/correct

should we gently wean off old code from its bad habits? give type errors outright?

am I missing something?

cheers

adriaan

[Paul Phillips]

On Mon, Feb 27, 2012 at 9:16 AM, Adriaan Moors <adriaa...@epfl.ch> wrote:
> I say let the code speak for itself (https://gist.github.com/1925479)

See also https://issues.scala-lang.org/browse/SI-5189

scala> case class Foo[T, U](f: T => U)
defined class Foo

// uh-oh, Any => Any should be Nothing => Any.
scala> def f(x: Any) = x match { case Foo(bar) => bar }
f: (x: Any)Any => Any

scala> def f(x: Any) = x match { case Foo(bar) => bar(5) }
f: (x: Any)Any

scala> f(Foo((x: String) => x))
java.lang.ClassCastException: java.lang.Integer cannot be cast to
java.lang.String
at $anonfun$1.apply(<console>:11)
at .f(<console>:9)
at .<init>(<console>:11)

[Josh Suereth]

Guys, we have to keep up with the modern hype of new languages.  Type-unsoundness is *in*.  Shave your beards and start using MacOS.

P.S.  Let's get this (fixed type system hole) in the next milestone so everyone can freak-out over the issue with a big sign demonstrating how terrible the below code can be that we're protecting people from.

[Simon Ochsenreither]

Agree. It is still early in the release cycle, so people who totally rely on it can either complain or fix their code ... (should I mention scala.dbc again? .. mhhh ... on.)
:-)

Bye and see you all in London!

Simon

[Daniel Sobral]

One question: how do we fix JavaConversions?

--
Daniel C. Sobral

I travel to the future all the time.

[Paul Phillips]

On Mon, Feb 27, 2012 at 9:16 AM, Adriaan Moors <adriaa...@epfl.ch> wrote:

>   class Wrapped[W](var x: W) // must be invariant (to trigger the bug)
>   class AbsWrapperCov[+B] // must be covariant   case class
> Wrapper[T](underlying: Wrapped[T]) extends AbsWrapperCov[T]
>
>   def unwrap[A](it: AbsWrapperCov[A]): Wrapped[A] = it match {     case
> Wrapper(wrapped) => wrapped // after inferTypedPattern: ?T <: A&0

So given:

class A[+T]
class B[T] extends A[T]

The pattern matcher allows people to treat the same term as an A[+T]
for the purposes of variance checks but a B[T] for the purposes of
member selection. There's probably no way to fix it without breaking
code. We can fix JavaConversions easily enough by casting if we can't
think of anything better.

[Adriaan Moors]

There's probably no way to fix it without breakingcode. 

We can fix JavaConversions easily enough by casting if we can't
think of anything better.

I for one can't. There's a reason people let unsoundness slip by: it only goes wrong when it goes wrong.

(but when it goes wrong....)

[Miles Sabin]

I vaguely remember that when I wrote an ancestor of that pattern match
I expected to have to cast, and was pleasantly surprised that I didn't
need to. In retrospect I should have hit the big red button.

FTR, I'd be delighted to see JavaConversions removed altogether.

Cheers,

Miles

--
Miles Sabin
tel: +44 7813 944 528
gtalk: mi...@milessabin.com
skype: milessabin
g+: http://www.milessabin.com
http://twitter.com/milessabin
http://www.chuusai.com/

[Adriaan Moors]

I think I have a (conceptual) fix for inferConstructorInstance. I've updated the gist https://gist.github.com/1925479 with the reasoning applied to a couple of examples (see below).

consider the example in TestPos2 (please see the gist -- copy/pasting messes up formatting)

we know that b: Base[T]  (foo's T)

and when we're in case C(x), we know that b's actual type is C[U], for some unknown U

how do we soundly relate U to foo's T?

we know that C[U] <: Base[T_actual], but since Base is covariant in its first type parameter, the T_actual could be any subtype of the statically known T,

so let's model that using an existential, reducing C[U] <: Base[_ <: T] to U =:= (X forSome {type X <: T})   (type equality since C is invariant)

[Adriaan Moors]

fwiw, my current attempt at implementing this: https://github.com/adriaanm/scala/commit/d0d45cc9d1083f87b4c301455384c5802846f426

it correctly rejects/accept my test, but fails on other stuff... haven't figured out why yet

[Burak Emir]

Hi Adriaan,

FWIW, to me it seems that the type-checking of the pattern *body* that
seems to be wrong (it should not matter what pattern matcher
implementation does or does not do with it)

In your example,

- `it' has type AbsWrapperCov[A]

- the pattern Wrapped(...) has type Wrapper[T0] <: AbsWrapperCov[A]
for some fixed T0 <: A

- consequently, `wrapped' has type Wrapped[T0] with the same fixed T0<A.

That is all fine. In the body, though,

- `wrapped' is returned in a place where we expect Wrapped[A].

This is what should fail, since Wrapped is invariant and we don't have
any evidence that T0 == A.

I am not sure I understand what your fix does, but judging from the
error output, it seems that `wrapped' has the right bound already.

BTW keep in mind it's been years, I may be missing a lot here : )

Hope this helps,
Burak

--
Burak Emir
--
http://burak.emir.googlepages.com

[Burak Emir]

[adding adriaan's correct email address]

[Adriaan Moors]

On Sun, Mar 4, 2012 at 12:29 PM, Burak Emir <burak...@gmail.com> wrote:

it seems that `wrapped' has the right bound already.

... but it didn't before the fix, as far as I can tell from the -Xprint:typer output of compiling https://github.com/adriaanm/scala/blob/6f5bbeb48104dd7d0ac13572feb33f3cf3773e57/test/files/neg/t5189b.scala

    def unwrap#7667[T#7668 >: scala#104.this.Nothing#3461 <: scala#104.this.Any#136](x#10164: TestPos#86.this.AbsWrapperCov#7661[T#7669&0]): T#7668 = x#10164 match {

      case (<param> x#11853: T#7669&0)TestPos#86.this.Wrapper#7663[T#7669&0]((x#11854 @ _)) => x#11854

    }

cheers

adriaan