Avoid creating Typeable for general type projections

[joroKr21]

Unless:

• the outer type is final and not parametrized
• the inner type is a case class

All other type projections are potentially unsound because the
inner type can capture type members of the outer type.

Closes #579
Closes #848

[milessabin]

Could you add a test that None is yielded appropriately?

[joroKr21]

You mean for an unrelated type?

[milessabin]

I'm looking at the comment above the code you replaced,

There is potential unsoundness if we allow a simple cast between two unparameterized types, if they contain values of an abstract type variable from outside of their definition. Therefore, check to see if any values have types that look different from the inside and outside of the type.

I'd like to see a test which verifies that unsound cases are still rejected.

[milessabin]

Something like this maybe?

trait Outer {
  type T
  class Inner(val t: T)
  def boom(a: Any): Option[Inner] = a.cast[Inner]
}
val i = new Outer { type T = Int }
val ii = new i.Inner(23)
val s = new Outer { type T = String }
s.boom(ii) // If Outer compiles then this should be None

[joroKr21]

There are existing tests for type projections in testNested. The example you described should be better handled in #798 but to test this I need to combine it with this PR or wait for one or the other to be merged.

Or maybe something like Typeable[Outer#Inner] - that doesn't fix T though.

[joroKr21]

I found an unsound example:

  @Test
  def testTypeProjection(): Unit = {
    trait Outer {
      type T
      class Inner(val t: T)
    }

    trait OuterInt extends Outer {
      type T = Int
    }

    trait OuterStr extends Outer {
      type T = String
    }

    val typ = Typeable[OuterInt#Inner]
    val outStr = new OuterStr { }
    assertEquals(None, typ.cast(new outStr.Inner("boom")))
  }

However, note that the check in master is not enough to catch all such cases. It doesn't deal with: inheritance, methods, even more nested classes, etc. That's a very deep rabbit hole.

One option would be to just reject all type projections.

[milessabin]

When you say "type projection" do you mean "abstract type member"?

[milessabin]

Ahh, you mean type projections as the target of the cast?

[joroKr21]

Yes, I mean instances of Typeable[Type#Projection] where Projection is not a case class.

[joroKr21]

Apropo, regular pattern matching has the same problem. It will warn for erasure on case _: Outer[Int]#Inner, but not on case _: OuterInt#Inner

[milessabin]

I just checked my earlier example,

@Test
def testRefined: Unit = {
  trait Outer {
    type T
    class Inner(val t: T)
    def boom(a: Any): Option[Inner] = a.cast[Inner]
  }
  val i = new Outer { type T = Int }
  val ii = new i.Inner(23)
  val s = new Outer { type T = String }
  //assert(!s.boom(ii).isDefined) // If Outer compiles then this should be None}
  s.boom(ii).map(_.t.length) // CCE
}

This blows up with a ClassCastException.

Conclusion: there should be no Typeable instances for types with abstract type members.

Conclusion: there should be no Typeable instances for types which contain references to abstract types.

[milessabin]

Interestingly, in your example, I think there should be an instance for Typeable[OuterInt#Inner] because as seen from OuterInt T is not abstract.

[joroKr21]

@milessabin in your example we don't have a type projection, but a path dependent type. As such, the outer reference should be compared for equality (see #798).

Yes, I guess my example should have a Typeable instance according to the comments on master (although it is still unsound), but it doesn't. Looking more closely at the code rtpe.asSeenFrom(tpe, tpe.typeSymbol.owner) is nonsensical. We can't replace references to Outer.this.type with OuterInt#Inner because Inner is not a subclass of Outer. In effect removing this call to asSeenFrom would change nothing.

But what if there is a third class nested inside Inner that captures type variables? Should we check all nested decls? What about inheriting captured variables from a superclass? Check all nested members? Even if all cases are covered, the error won't make much sense to the user any more.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@0a94fcf). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #803   +/-   ##
=========================================
  Coverage          ?   87.99%           
=========================================
  Files             ?       64           
  Lines             ?     1499           
  Branches          ?        4           
=========================================
  Hits              ?     1319           
  Misses            ?      180           
  Partials          ?        0

Impacted Files	Coverage Δ
core/src/main/scala/shapeless/typeable.scala	0% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a94fcf...3ed669c. Read the comment docs.

[milessabin]

I'm going to defer to your judgement on this one.

[joroKr21]

If you approve I will merge it 😄 - it's the last of a series of fixes in Typeable and doesn't make much sense to leave it for later.

[milessabin]

LGTM!