Rework implicit search #3421

odersky · 2017-11-01T09:58:41Z

Fixes #3396. I am not sure what we should do to add this as a test case. I thought collection-strawman was already part of the community build? If yes, the contrib directory should be included, that would have shown the error,

Don't proceed with implicit search if result type cannot match - the search will likely by under-constrained, which means that an unbounded number of alternatives is tried. See strawman-contrib MapDecoratorTest.scala for an example where this happens.

nicolasstucki · 2017-11-01T10:17:53Z

The strawman is currently only a submodule used in the test suite and it has a fixed commit. We should probably update the strawman submodule as well.

It would be good to have it also on the community build to find issues faster.

allanrenucci · 2017-11-01T10:25:48Z

In the community build, we build a fixed commit of the projects. I can make it work to compile the latest commit on master. However, if the community build fails, we then don't know if it is due to a regression or newly added code.

nicolasstucki · 2017-11-01T10:37:29Z

It is at least a way to detect that there is some bug somewere.

nicolasstucki · 2017-11-01T10:41:57Z

I will update the strawman submodule.

odersky · 2017-11-01T10:45:51Z

Let's update the strawman semi-regularly as it develops, Say every 2 weeks. /cc @julienrf

nicolasstucki · 2017-11-01T11:38:26Z

Also, we do not compile the test sources from the strawan in the submodule

odersky · 2017-11-01T12:21:48Z

test performance please

dottybot · 2017-11-01T12:22:11Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-11-01T13:46:45Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3421/ to see the changes.

Benchmarks is based on merging with master (3a72da6)

This caused nested ambiguity errors with the next commit.

Propagate implicit ambiguity and divergence failures from implicit arguments to their callers. In particular, and ambiguity in an implicit argument means that the whole implicit search is ambiguous. Previously, the local ambiguity translated to an "implicit not found" on the next outer level, which meant that another implicit on that level could qualify and the ambiguity would be forgotten. The code is still a bit rough and could profit from some refactorings.

This is a temporary fix. iterator-from seems to cause non-deterministic ambiguity errors. Moving to pending until we have figured out what goes wrong.

odersky · 2017-11-03T18:26:51Z

The latest commits also fix #3430.

odersky · 2017-11-03T18:27:18Z

test performance please

dottybot · 2017-11-03T18:27:38Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-11-03T19:53:51Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3421/ to see the changes.

Benchmarks is based on merging with master (22ff8a4)

odersky · 2017-11-03T21:31:52Z

test performance please

dottybot · 2017-11-03T21:32:08Z

performance test scheduled: 1 job(s) in queue, 0 running.

dottybot · 2017-11-03T22:57:46Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3421/ to see the changes.

Benchmarks is based on merging with master (22ff8a4)

odersky · 2017-11-04T09:34:06Z

What's the purpose of doing this? In scalac they're going in the opposite direction: completely get rid of implicit sorting because it made implicit search depend on compilation order: scala/scala#6083

In fact I think we go to some degree in the same way. I would like to complement the current behavior which depends on compilation history with criteria that depend only on the definitions themselves. I believe order in a file is significant. If we communicate that it means programmers can arrange their implicits in the order they should be tried, similarly to clauses in a pattern match. We probably should also look at nesting level (deeper takes precedence) and class hierarchy (definitions in subclasses take precedence). If we do that we might be able to drop the useCount-based criterion.

Another aspect worth noting is that with d7c8604 a divergent implicit will be issued (or not) independent of compilation order.

Refactoring of implicits with the aim of clearer code and better error messages. Here's an example: -- Error: implicitSearch.scala:15:12 ------------------------------------------- 15 | sort(xs) // error (with a partially constructed implicit argument shown) | ^ |no implicit argument of type Test.Ord[scala.collection.immutable.List[scala.collection.immutable.List[T]]] was found for parameter o of method sort in object Test. |I found: | | Test.listOrd[T](Test.listOrd[T](/* missing */implicitly[Test.Ord[T]])) | |But no implicit values were found that match type Test.Ord[T].

odersky · 2017-11-05T21:34:34Z

test performance please

dottybot · 2017-11-05T21:34:59Z

performance test scheduled: 1 job(s) in queue, 0 running.

Turns out it's not transitive when combined with the old scheme. So it should be one or the other. Conservatively, this commit reverts to the old scheme.

If we hit a divergent implicit during a contextual search, we should continue with a type-based search. Only ambiguities are globally fatal. The removed test that prevented this logic was introduced when I experimented with fatal divergence. It was left in by oversight.

smarter · 2017-11-14T17:13:12Z

scalacheck compiles with the latest commit.

smarter · 2017-11-14T18:10:35Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+
+    def msg(implicit ctx: Context): Message = explanation
+
+    /** If search was a for an implicit conversion, a note describing the failure


typo: was a -> was

Replace the use of ambiguous implicits by the `scala.implicits.Not` class introduced in the PR. Running `sbt dottyThing/compile` crashes with: [info] exception occurred while compiling /home/smarter/opt/shims/dottyThing/src/main/scala/shims/dotty/test.scala java.lang.AssertionError: assertion failed at dotty.DottyPredef$.assertFail(DottyPredef.scala:38) at dotty.tools.dotc.core.TyperState.commit(TyperState.scala:135) at dotty.tools.dotc.typer.Implicits.op$12(Implicits.scala:737) at dotty.tools.dotc.typer.Implicits.op$2(Implicits.scala:732) at dotty.tools.dotc.typer.Implicits.inferImplicit(Implicits.scala:728) at dotty.tools.dotc.typer.Implicits.inferImplicitArg(Implicits.scala:615) at dotty.tools.dotc.typer.Typer.implicitArgs$2(Typer.scala:2020) at dotty.tools.dotc.typer.Typer.implicitArgs$2(Typer.scala:2030) at dotty.tools.dotc.typer.Typer.addImplicitArgs$1(Typer.scala:2033) at dotty.tools.dotc.typer.Typer.adaptNoArgsImplicitMethod$2(Typer.scala:2085) at dotty.tools.dotc.typer.Typer.adaptNoArgs$1(Typer.scala:2201) at dotty.tools.dotc.typer.Typer.adaptInterpolated(Typer.scala:2346) at dotty.tools.dotc.typer.Typer.adaptInterpolated(Typer.scala:2335) at dotty.tools.dotc.typer.Typer.op$82(Typer.scala:1872) at dotty.tools.dotc.typer.Typer.op$42(Typer.scala:1868) at dotty.tools.dotc.typer.Typer.adapt(Typer.scala:1867) at dotty.tools.dotc.typer.Typer.op$40(Typer.scala:1706) at dotty.tools.dotc.typer.Typer.typed(Typer.scala:1702) ....

smarter · 2017-11-14T21:08:56Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+        case _: SearchFailure => alt2
+      }
+
+      def healAmbiguous(pending: List[Candidate], fail: SearchFailure) = {


I would add a doc comment for this method and for rank below.

smarter · 2017-11-14T21:09:12Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+      def disambiguate(alt1: SearchResult, alt2: SearchSuccess) = alt1 match {
+        case alt1: SearchSuccess =>
+          val diff = compareCandidate(alt1, alt2.ref, alt2.level)
+          assert(diff <= 0)


Why is this assert always true?

A added the comment: diff > 0 candidates should already have been eliminated in rank

smarter · 2017-11-14T22:06:59Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+          tryImplicit(cand) match {
+            case fail: SearchFailure =>
+              if (isNot)
+                SearchSuccess(ref(defn.Not_value), defn.Not_value.termRef, 0)(ctx.typerState)


I don't have a minimized example to reproduce this but there is something wrong here: The ctx used here comes from the ImplicitSearch class implicit parameter. There is no guarantee that the TyperState of this Context is committable (this can be checked by adding assert(ctx.typerState.isCommittable)). This is bad because in inferImplicit we do result.tstate.commit() which crashes when the TyperState is not committable. This is only a problem here because the other call to SearchSuccess is done in typedImplicit which takes its own implicit ctx (the only call to typedImplicit is typedImplicit(cand)(nestedContext().setNewTyperState().setSearchHistory(history)), the setNewTyperState() ensures the state is always committable).

I found this out while trying to get the usage of ambiguous implicits in shims to work, see https://github.com/smarter/shims/commits/dotty-not

Well spotted! Yes, we need to give it a nested committable typerstate.

odersky · 2017-11-15T08:40:52Z

I found another problem with the treatment of Not: We need to continue in each case as if it was a normal success/failure, instead of returning directly from rank.

OlivierBlanvillain · 2017-11-15T10:48:08Z

I tried compiling my library with this PR:

Everything works as it used to under -language:Scala2
I could replace all my NotX by implicits.Not[X] and compile without -language:Scala2!

Replace the use of ambiguous implicits by the `scala.implicits.Not` class introduced in the PR.

smarter

LGTM, https://github.com/smarter/shims/commits/dotty-not compiles now too!

smarter · 2017-11-15T16:03:35Z

@milessabin: This PR changes the behavior of implicit search, I believe this sort of things is relevant to your interests :). In particular, an ambiguous implicit now invalidates the whole search instead of just the current branch, see #3421 (comment). To compensate for this, negation of implicits is now explictly supported through https://github.com/dotty-staging/dotty/blob/fix-strawan/library/src/scala/implicits/Not.scala. Let us know if you can think of other patterns this may break.

PR scala#3421 changed implicit search: In the old scheme, a method with an implicit argument and a default value would give you the default value if called with an ambiguous implicit argument. The scheme emits an error. We now provide an implicit instance of `Show[Nothing]` in order to avoid ambiguous implicit error in the REPL for expressions like: ```scala scala> List() val res0: List[Nothing] = List() scala> Option.empty val res1: Option[Nothing] = None scala> Map() val res2: Map[Nothing, Nothing] = Map() ```

PR scala#3421 changed implicit search: In the old scheme, a method with an implicit argument and a default value would give you the default value if called with an ambiguous implicit argument. The new scheme emits an error. We make `Show[T]` invariant in order to avoid ambiguous implicit errors in the REPL for expressions like: ```scala scala> List() val res0: List[Nothing] = List() scala> Option.empty val res1: Option[Nothing] = None scala> Map() val res2: Map[Nothing, Nothing] = Map() ```

PR scala#3421 changed implicit search: In the old scheme, a method with an implicit argument and a default value would give you the default value if called with an ambiguous implicit argument. The new scheme emits an error. We make `Show[T]` invariant and provide a low priority default implicit in order to avoid ambiguous implicit errors in the REPL for expressions like: ```scala scala> List() val res0: List[Nothing] = List() scala> Option.empty val res1: Option[Nothing] = None scala> Map() val res2: Map[Nothing, Nothing] = Map() ```

odersky requested a review from smarter November 1, 2017 09:58

Fix typo in comment

fd70c61

odersky added 4 commits November 3, 2017 17:39

Reflect nesting of lazy_implicit values in their levels

2d4efa0

This caused nested ambiguity errors with the next commit.

Move iterator-from test to pending

c311bf3

This is a temporary fix. iterator-from seems to cause non-deterministic ambiguity errors. Moving to pending until we have figured out what goes wrong.

Add test

beb90b4

Refine order in which implicit candidates are tried

c1c9198

odersky added 2 commits November 5, 2017 21:30

Add @sharable to global data

e1cf2c9

Disable refined preference scheme for implicits

7da033b

Turns out it's not transitive when combined with the old scheme. So it should be one or the other. Conservatively, this commit reverts to the old scheme.

smarter changed the title ~~Fix #3396: Abort implicit search if result does not match~~ Rework implicit search Nov 14, 2017

smarter reviewed Nov 14, 2017

View reviewed changes

smarter requested changes Nov 14, 2017

View reviewed changes

Address reviewers comments

39b4ec2

odersky added 3 commits November 15, 2017 09:45

Fix typo

66b0d9a

Avoid infinite loop

0da21e1

Simplify control flow in rank method

db1b09a

smarter added a commit to smarter/shims that referenced this pull request Nov 15, 2017

Make this repo work with scala/scala3#3421

f8f940f

Replace the use of ambiguous implicits by the `scala.implicits.Not` class introduced in the PR.

smarter approved these changes Nov 15, 2017

View reviewed changes

odersky merged commit 745fae6 into scala:master Nov 15, 2017

odersky mentioned this pull request Nov 16, 2017

Compiler crash due to memory exhaustion #3430

Closed

This was referenced Nov 20, 2017

Add test cases from #3452 #3505

Merged

IndexOutOfBoundsException in implicit search #3452

Closed

allanrenucci mentioned this pull request Nov 27, 2017

Fix ambiguous implicit for Show[Nothing] #3568

Merged

adriaanm mentioned this pull request Dec 1, 2017

Narrow implicit scope scala/scala-dev#446

Closed

allanrenucci deleted the fix-strawan branch December 14, 2017 19:21

smarter mentioned this pull request Oct 10, 2018

Not not working; alternative definition doesn't need to be intrinsic? #5234

Closed

rssh mentioned this pull request Feb 6, 2021

call of inline method of result of other inline method produce a tree with incorrect ownity, (crashed with -Ycheck:macros) #11251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework implicit search #3421

Rework implicit search #3421

odersky commented Nov 1, 2017 •

edited by smarter

nicolasstucki commented Nov 1, 2017

allanrenucci commented Nov 1, 2017 •

edited

nicolasstucki commented Nov 1, 2017

nicolasstucki commented Nov 1, 2017

odersky commented Nov 1, 2017

nicolasstucki commented Nov 1, 2017

odersky commented Nov 1, 2017

dottybot commented Nov 1, 2017

dottybot commented Nov 1, 2017

odersky commented Nov 3, 2017

odersky commented Nov 3, 2017

dottybot commented Nov 3, 2017

dottybot commented Nov 3, 2017

odersky commented Nov 3, 2017

dottybot commented Nov 3, 2017

dottybot commented Nov 3, 2017

odersky commented Nov 4, 2017

odersky commented Nov 5, 2017

dottybot commented Nov 5, 2017

smarter commented Nov 14, 2017

smarter Nov 14, 2017

smarter Nov 14, 2017

smarter Nov 14, 2017

odersky Nov 15, 2017

smarter Nov 14, 2017

odersky Nov 15, 2017

odersky commented Nov 15, 2017

OlivierBlanvillain commented Nov 15, 2017

smarter left a comment

smarter commented Nov 15, 2017


		def msg(implicit ctx: Context): Message = explanation

		/** If search was a for an implicit conversion, a note describing the failure

Rework implicit search #3421

Rework implicit search #3421

Conversation

odersky commented Nov 1, 2017 • edited by smarter

nicolasstucki commented Nov 1, 2017

allanrenucci commented Nov 1, 2017 • edited

nicolasstucki commented Nov 1, 2017

nicolasstucki commented Nov 1, 2017

odersky commented Nov 1, 2017

nicolasstucki commented Nov 1, 2017

odersky commented Nov 1, 2017

dottybot commented Nov 1, 2017

dottybot commented Nov 1, 2017

odersky commented Nov 3, 2017

odersky commented Nov 3, 2017

dottybot commented Nov 3, 2017

dottybot commented Nov 3, 2017

odersky commented Nov 3, 2017

dottybot commented Nov 3, 2017

dottybot commented Nov 3, 2017

odersky commented Nov 4, 2017

odersky commented Nov 5, 2017

dottybot commented Nov 5, 2017

smarter commented Nov 14, 2017

smarter Nov 14, 2017

Choose a reason for hiding this comment

smarter Nov 14, 2017

Choose a reason for hiding this comment

smarter Nov 14, 2017

Choose a reason for hiding this comment

odersky Nov 15, 2017

Choose a reason for hiding this comment

smarter Nov 14, 2017

Choose a reason for hiding this comment

odersky Nov 15, 2017

Choose a reason for hiding this comment

odersky commented Nov 15, 2017

OlivierBlanvillain commented Nov 15, 2017

smarter left a comment

Choose a reason for hiding this comment

smarter commented Nov 15, 2017

odersky commented Nov 1, 2017 •

edited by smarter

allanrenucci commented Nov 1, 2017 •

edited