Skip to content

Conversation

@milessabin
Copy link
Contributor

@milessabin milessabin commented Oct 11, 2017

The implementation of ContainsCollector overly aggressively normalizes away aliases resulting in symbols corresponding to higher-kinded type variables being missed. This shows up in existentialAbstraction where a type like [F[_]]C[F] which should be taken to C[F] forSome { type F[_] } is instead taken to C[F] (where F has escaped its quantifier). This is responsible for errors of the form reported in scala/bug#10545 because implicit selection is driven by a specificity test which in turn relies on existentialAbstraction.

I've made the smallest possible change that fixes this problem, however I think that ContainsCollector is ambivalent about its use of normalize and should probably be reworked to either fully normalize (in which case Type#contains will see through all aliases) or to not normalize at all apart from the case where it's required to handle refined types correctly.

Fixes scala/bug#10545. All (par)tests pass.

@milessabin milessabin requested a review from retronym October 11, 2017 09:29
@scala-jenkins scala-jenkins added this to the 2.12.5 milestone Oct 11, 2017
@milessabin
Copy link
Contributor Author

Relevant prior commit: a56afcd.

Nb. I based the unit test on BytecodeTesting rather than include it in TypesTest because the contains failure doesn't show up in the latter case ... a limitation of SymbolTableForUnitTesting I don't understand. The test also exercises existentialAbstraction and isStrictlyMoreSpecific directly which are only accessible given an instance of Inferencer.

@retronym
Copy link
Member

See also scala/scala-dev#207

@milessabin
Copy link
Contributor Author

Thanks for the pointer ... I'll run a community build when this goes green.

@milessabin
Copy link
Contributor Author

The community build fails with scalaz: the change produces some new implicit ambiguities ... similar situation to the SI-2712 fix.

@retronym
Copy link
Member

I'm going to review this shortly by:

  • sketching out a spectrum of ways we could implement Type.contains (in addition to the status quo and this patch, I could imagine a version that unpeels alias chains)
  • cataloging the callers of contains
  • showing how correctness of those callers would by improved or broken by the different implementations.

@milessabin
Copy link
Contributor Author

milessabin commented Oct 12, 2017

I've added a neg test which illustrates the sort of revealed ambiguity failures we get with this fix in place.

class Foo[F[_]]
object Foo {
  // Prior to this fix these two are ambiguous
  implicit def fooF0[F[_]]: Foo[F] = new Foo[F]
  implicit def fooF1: Foo[Option] = new Foo[Option]
}

class Bar[F[_]]
object Bar extends Bar0 {
  // Prior to this fix these two aren't selected because there is no
  // Foo[F] due to the ambiguity above
  // After this fix these two are ambiguous
  implicit def barF0[F[_]](implicit fooF: Foo[F]): Bar[F] = new Bar[F]
  implicit def barF1[F[_]](implicit fooF: Foo[F]): Bar[F] = new Bar[F]
}

trait Bar0 {
  // Prior to this fix we fall back to here
  implicit def barF2[F[_]]: Bar[F] = new Bar[F]
}

object Test {
  // Prior to this fix Bar.barF2[Option]
  // After this fix,
  // error: ambiguous implicit values:
  //   both method barF0 in object Bar of type [F[_]](implicit fooF: Foo[F])Bar[F]
  //   and method barF1 in object Bar of type [F[_]](implicit fooF: Foo[F])Bar[F]
  //   match expected type Bar[Option]
  //    implicitly[Bar[Option]]
  //              ^
  // one error found
  implicitly[Bar[Option]]
}

@milessabin
Copy link
Contributor Author

@adriaanm I'd be happy to target this at 2.13.x too.

@milessabin milessabin changed the base branch from 2.12.x to 2.13.x October 17, 2017 12:39
@milessabin
Copy link
Contributor Author

Rebased to 2.13.x ...

@milessabin milessabin modified the milestones: 2.12.5, 2.13.0-M3 Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect implementation of Type containment of Symbol frustrates implicit selection

3 participants