Don't reject views with result types which are TypeVars #7295

milessabin · 2018-10-01T17:32:14Z

On the matchesPtInst fast path views are pruned without first being applied. This can result in a false negative in HasMethodMatching if the view has a result type which is a not fully instantiated TypeVar. The fix is to fall back to the slow path in that case.

Fixes scala/bug#11174.

On the matchesPtInst fast path views are pruned without first being applied. This can result in a false negative in HasMethodMatching if the view has a result type which is a not fully instantiated TypeVar. The fix is to fall back to the slow path in that case. Fixes scala/bug#11174.

adriaanm

Nice minimization, now let's generalize what could possibly go wrong a bit :-)

adriaanm · 2018-10-02T08:07:23Z

src/compiler/scala/tools/nsc/typechecker/Implicits.scala

+
+            if(isView || isViewLike) {
+              tpInstantiated match {
+                case MethodType(_, tv: TypeVar) if !tv.instValid =>


I think this check could be more general. What if it's a conversion like def foo(x: From): x.To? The result type would also be undetermined, right? Or it could be an intersection type of a type variable with some other type.

Could you add a few test cases along these lines and make sure we catch those as well?

Implicits to satisfy views are matched against a search template. To match correctly against the template, TypeVars in the candidates type are replaced by their upper bounds once those bounds have been solved as far as possible against the template.

milessabin · 2018-10-02T21:05:42Z

Trying to get to the bottom of this ...

The issue boils down to the way that views are searched for when we're trying to adapt to a member. What we search for is a function from the adaptee to a synthetic refinement of a wildcard ? { def name: ? } constructed by HasMember. In this case we'll be looking for a view matching Test.c.type => ? { def foo: ? }.

When we call matchesPt attempting to match the type of toComponentCtor against this template we end up in isSubType via normSubType. Here everything goes fine until we compare the result type of the method (?CT[Int], a not yet fully instantiated TypeVar with an upper bound of Props) against the result type of the template (? { def foo: ? }) . This fails because we end up trying to register ? { def foo: ? } as a bound on CT and this is rejected because CT is higher kinded whereas ? { def foo: ? } isn't. A variant of this test case which has a result type of kind * succeeds, I think incorrectly, because the attempt to register the result type of the template as a bound is accepted thanks to the kinds lining up (see test/files/pos/t11174c.scala).

The fix in my first commit skirts around this problem by bailing to the slow path straight away. The new fix continues with the fast path, but replaces any uninstantiated TypeVars in the candidate type by their upper bounds before calling matchesPt. Consequently when we reach the previously problematic result type comparison it's now Props[Int] against ? { def foo: ? } which succeeds as expected.

I understand your concern about views of other forms, but I think that this really is an interaction between the mechanism for matching against view templates in the presence of type variables. I've added a test case along the lines you suggested anyway ... it passes, but it's not really relevant to the problem.

Addressed feedback/taking a different approach

dwijnand · 2018-10-03T07:57:06Z

src/compiler/scala/tools/nsc/typechecker/Implicits.scala

+      }
+
+      if (StatisticsStatics.areSomeColdStatsEnabled) statistics.incCounter(matchesPtInstCalls)
+        info.tpe match {


The indentation looks off here (almost looks like this block is dependant on if StatisticsStatics.areSomeColdStatsEnabled)

Yes there is. I'll deal with that in a separate commit.

adriaanm

I like the direction, but I'm not sure we're there yet. How much is there to gain in pruning polymorphic views? Perhaps we can just keep it simple and take the slow path for them?

adriaanm · 2018-10-03T09:16:44Z

src/compiler/scala/tools/nsc/typechecker/Implicits.scala

@@ -571,14 +571,36 @@ trait Implicits {
        case _ => false
      }

+      object tvarToHiBoundMap extends TypeMap {


Approximating a type variable by its upper bound is only sound when it occurs in a covariant position, right? See ExistentialExtrapolation for an analogous type map.

(Also, wildcardExtrapolation)

That occurred to me too, but see also dominates: https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/typechecker/Implicits.scala#L455-L456.

Yeah, that's not strictly right either, but has less impact since it's not about conformance.

milessabin · 2018-10-03T10:21:57Z

How much is there to gain in pruning polymorphic views?

I'm not sure. It won't make much difference in "large induction" type cases, but it might have an impact where lots of syntax is involved (ie. extension methods with polymorphic carriers).

adriaanm · 2018-10-03T12:57:52Z

So, this is intriguing :-) I ran this simplified example through the debugger, and it looks like an issue with type constructor variables

trait SomeTC[TC[_]]

class Tycon[T] { def foo(props: T): T = ??? }

class DontPruneTyconView {
  implicit def toTycon[TC[_]](c: SomeTC[TC]): TC[Int] = ???

  (??? : SomeTC[Tycon]).foo(23)
}

adriaanm · 2018-10-03T13:21:37Z

Concretely, there's no case in registerBound to deal with the situation where an applied typeconstructor variable is compared against a bound that's not also a type constructor. It makes perfect sense to register the bound [T >: Int <: Int] ?{def foo: ?} on the ?TC type var, for its occurrence in the type application ?TC[Int] where it encountered the kind-* bound ?{def foo: ?}.

milessabin · 2018-10-03T13:28:37Z

Yup, that's what I was trying to say earlier ;-)

This fails because we end up trying to register ? { def foo: ? } as a bound on CT and this is rejected because CT is higher kinded whereas ? { def foo: ? } isn't.

But I think we shouldn't be trying to do this in the first place, hence substituting with the bound.

How about if I make the replacement respect variance?

adriaanm · 2018-10-03T13:29:19Z

Apologies, I missed that in your comment.

adriaanm · 2018-10-03T13:31:07Z

I do think that missing case is a bug. We should allow a type constructor var that's applied to type arguments (thus of kind *) to register kind-* bounds. In the implementation, this tracking has to be done "inside out" -- sketching a solution:

      def unifyApplied(tp: Type) = this match {
        case atv: AppliedTypeVar =>
          addBound(genPolyType(params, tp))
          true
        case _ => false
      }

and patch this in here: unifySimple || unifyFull(tp) || unifyApplied(tp) || (

milessabin · 2018-10-03T13:32:34Z

If you try with test/files/pos/t11174c.scala, which is an example with a result type of kind *, you'll see that the bound registration does go through. But ? { def foo: ? } doesn't really make sense as a bound ... as I said, I don't think we should be trying this with uninstantiated TypeVars.

adriaanm · 2018-10-03T13:34:14Z

Why does it not make sense as a bound?

adriaanm · 2018-10-03T13:34:40Z

Also, the other case[*] is routinely comparing type vars against other types, so I'm not sure I follow the other point.

[*] if(!matchesPt(tpInstantiated, wildPt, allUndetparams)) {

milessabin · 2018-10-03T13:46:52Z

Why does it not make sense as a bound?

Well, maybe it does.

But I'm slightly puzzled that { def foo(props: T): T } can be a subtype of ? { foo: ? } given that foo has no argument list in the latter. Maybe that's not a problem given the role that it's playing here.

adriaanm · 2018-10-03T13:51:45Z

The syntax is confusing, but it just means there's a member named foo of any type, which subsumes a val or a method of any arity (it would have a MethodType as info that encodes its argument list, but we leave the info as a wildcard)

adriaanm · 2018-10-03T13:54:44Z

The refinement type is created by HasMember

adriaanm · 2018-10-03T13:56:30Z

Btw, there's a very similar test already in our suite as test/files/pos/t7785.scala

adriaanm · 2018-10-03T14:00:21Z

All this aside, it's most likely too risky to change type inference to fix this. My curiosity just got piqued...

milessabin · 2018-10-03T14:11:46Z

All this aside, it's most likely too risky to change type inference to fix this.

Agreed for 2.12.x. But what about for 2.13.x?

milessabin · 2018-10-03T14:14:01Z

How do you want to proceed from here? Fix the variance issue with the "replace by bounds" version?

adriaanm · 2018-10-03T14:28:56Z

I don't know -- it's tricky :-) I'm worried about regressions, but also concerned with the asymmetry between views and regular implicit search. The type variable bound bug we both discovered could also bite in a regular implicit search. (EDIT: though I can't think of an example, because it could crucially rely on type inference being partially guided by the source type of the implicit conversion)

milessabin · 2018-10-03T14:39:45Z

This non-view looks comparable and isn't problematic,

object Test {
  implicit def mkF[F[_]]: F[Int] = ???
  implicitly[Option[Int]]
}

adriaanm · 2018-10-03T14:47:04Z

This uses the existing unification mechanisms. I'm looking for an example that decides that ?F[Int] could not plausibly be a subtype (doesn't unify) with some kind-* type which in fact could be a supertype.

adriaanm · 2018-10-03T14:55:40Z

Given the complexity of it all, I would be inclined to just bail on this for 2.12.8 and use the slow path for polymorphic views.

milessabin · 2018-10-03T15:02:25Z

Given the complexity of it all, I would be inclined to just bail on this for 2.12.8 and use the slow path for polymorphic views.

OK, I'll do that (and fix the whitespace bug in a separate commit).

Incorporated feedback

milessabin · 2018-10-04T10:13:25Z

I've taken views off the fast path, and also tentatively removed the isViewLike logic as well. I needed that to get the community build passing previously, but I suspect that bailing on all views might make it unnecessary. If a community build run fails without it then it can be reinstated.

joroKr21 · 2018-10-04T10:48:30Z

This uses the existing unification mechanisms. I'm looking for an example that decides that ?F[Int] could not plausibly be a subtype (doesn't unify) with some kind-* type which in fact could be a supertype.

May be related to #6573

milessabin · 2018-10-05T15:41:01Z

Community build run in progress here.

milessabin · 2018-10-06T10:48:41Z

The community build is almost green. The two failures are classutil and enumeratum.

The classutil failure is due to a runtime test that the Scala release version is 2.12.6. So, unrelated to this PR or implicit pruning.

I'm not quite sure what the enumeratum failure is, but it's runtime again and doesn't seem obviously related to this PR or implicit pruning in general. The enumeratum build seems quite flaky and I'm not having a lot of luck getting it to build locally even with vanilla 2.12.7 due to Scala.js version slippage.

@SethTisue any thoughts on that?

adriaanm · 2018-10-08T09:25:25Z

The "build history" search box for https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/ reveals two prior occurrences: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3061/ and https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3559/

SethTisue · 2018-10-09T23:10:43Z

agree w/ your & Adriaan's assessment of the results: it's fine

milessabin · 2018-10-10T08:47:32Z

@adriaanm is this good to go?

It'll need carrying over to 2.13.x ... I'd be happy to look at making the fix in TypeVar if that works for you.

adriaanm · 2018-10-10T09:07:00Z

It would be good to investigate a better fix, but I'm not sure it'll be safe enough to include in 2.13. I guess it depends on the solution, but 2.13 at this point isn't much more permissive of regressions than 2.12. I don't want to distract from the focus of polish and stability for the collections.

scala-jenkins added this to the 2.12.8 milestone Oct 1, 2018

milessabin requested a review from adriaanm October 1, 2018 17:32

milessabin mentioned this pull request Oct 1, 2018

2.12.7 regression wrt implicit conversions in super-class companion objects scala/bug#11174

Closed

adriaanm previously requested changes Oct 2, 2018

View reviewed changes

dwijnand reviewed Oct 3, 2018

View reviewed changes

adriaanm previously requested changes Oct 3, 2018

View reviewed changes

Take views off the fast path

5933c1a

adriaanm merged commit 2f42b90 into scala:2.12.x Oct 10, 2018

SethTisue added the release-notes worth highlighting in next release notes label Oct 10, 2018

Don't reject views with result types which are TypeVars #7295

Don't reject views with result types which are TypeVars #7295

Conversation

milessabin commented Oct 1, 2018

adriaanm left a comment

Choose a reason for hiding this comment

adriaanm Oct 2, 2018

Choose a reason for hiding this comment

milessabin commented Oct 2, 2018

dwijnand Oct 3, 2018 • edited

Choose a reason for hiding this comment

milessabin Oct 3, 2018

Choose a reason for hiding this comment

adriaanm left a comment

Choose a reason for hiding this comment

adriaanm Oct 3, 2018

Choose a reason for hiding this comment

adriaanm Oct 3, 2018

Choose a reason for hiding this comment

milessabin Oct 3, 2018

Choose a reason for hiding this comment

adriaanm Oct 3, 2018

Choose a reason for hiding this comment

milessabin commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018 • edited

milessabin commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018 • edited

milessabin commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018 • edited

milessabin commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018

milessabin commented Oct 3, 2018

milessabin commented Oct 3, 2018

adriaanm commented Oct 3, 2018 • edited

milessabin commented Oct 3, 2018

adriaanm commented Oct 3, 2018

adriaanm commented Oct 3, 2018

milessabin commented Oct 3, 2018

milessabin commented Oct 4, 2018 • edited

joroKr21 commented Oct 4, 2018

milessabin commented Oct 5, 2018

milessabin commented Oct 6, 2018

adriaanm commented Oct 8, 2018

SethTisue commented Oct 9, 2018

milessabin commented Oct 10, 2018

adriaanm commented Oct 10, 2018

dwijnand Oct 3, 2018 •

edited

adriaanm commented Oct 3, 2018 •

edited

adriaanm commented Oct 3, 2018 •

edited

adriaanm commented Oct 3, 2018 •

edited

adriaanm commented Oct 3, 2018 •

edited

milessabin commented Oct 4, 2018 •

edited