New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework implicit search #3421

Merged
merged 30 commits into from Nov 15, 2017

Conversation

Projects
None yet
7 participants
@odersky
Contributor

odersky commented Nov 1, 2017

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,

Fix #3396: Abort implicit search if result does not match
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.

@odersky odersky requested a review from smarter Nov 1, 2017

@nicolasstucki

This comment has been minimized.

Show comment
Hide comment
@nicolasstucki

nicolasstucki Nov 1, 2017

Contributor

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.

Contributor

nicolasstucki commented Nov 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@allanrenucci

allanrenucci Nov 1, 2017

Member

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.

Member

allanrenucci commented Nov 1, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolasstucki

nicolasstucki Nov 1, 2017

Contributor

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

Contributor

nicolasstucki commented Nov 1, 2017

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

@nicolasstucki

This comment has been minimized.

Show comment
Hide comment
@nicolasstucki

nicolasstucki Nov 1, 2017

Contributor

I will update the strawman submodule.

Contributor

nicolasstucki commented Nov 1, 2017

I will update the strawman submodule.

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 1, 2017

Contributor

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

Contributor

odersky commented Nov 1, 2017

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

@nicolasstucki

This comment has been minimized.

Show comment
Hide comment
@nicolasstucki

nicolasstucki Nov 1, 2017

Contributor

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

Contributor

nicolasstucki commented Nov 1, 2017

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

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 1, 2017

Contributor

test performance please

Contributor

odersky commented Nov 1, 2017

test performance please

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 1, 2017

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

dotty-bot commented Nov 1, 2017

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

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 1, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (3a72da6)

dotty-bot commented Nov 1, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (3a72da6)

odersky added some commits Nov 3, 2017

Reflect nesting of lazy_implicit values in their levels
This caused nested ambiguity errors with the next commit.
Propagate implicit ambiguity and divergence failures
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.
Move iterator-from test to pending
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

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 3, 2017

Contributor

The latest commits also fix #3430.

Contributor

odersky commented Nov 3, 2017

The latest commits also fix #3430.

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 3, 2017

Contributor

test performance please

Contributor

odersky commented Nov 3, 2017

test performance please

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 3, 2017

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

dotty-bot commented Nov 3, 2017

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

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 3, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (22ff8a4)

dotty-bot commented Nov 3, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (22ff8a4)

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 3, 2017

Contributor

test performance please

Contributor

odersky commented Nov 3, 2017

test performance please

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 3, 2017

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

dotty-bot commented Nov 3, 2017

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

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Nov 3, 2017

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

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

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 3, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (22ff8a4)

dotty-bot commented Nov 3, 2017

Performance test finished successfully:

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

Benchmarks is based on merging with master (22ff8a4)

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 4, 2017

Contributor

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.

Contributor

odersky commented Nov 4, 2017

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.

odersky added some commits Nov 5, 2017

The great implicits re-org
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

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 5, 2017

Contributor

test performance please

Contributor

odersky commented Nov 5, 2017

test performance please

@dotty-bot

This comment has been minimized.

Show comment
Hide comment
@dotty-bot

dotty-bot Nov 5, 2017

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

dotty-bot commented Nov 5, 2017

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

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

Fix handling divergent implicits
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

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Nov 14, 2017

Member

scalacheck compiles with the latest commit.

Member

smarter commented Nov 14, 2017

scalacheck compiles with the latest commit.

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

Attempt to make this work with lampepfl/dotty#3421
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 added a commit to smarter/shims that referenced this pull request Nov 14, 2017

Attempt to make this work with lampepfl/dotty#3421
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)
....
@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 15, 2017

Contributor

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.

Contributor

odersky commented Nov 15, 2017

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.

odersky added some commits Nov 15, 2017

@OlivierBlanvillain

This comment has been minimized.

Show comment
Hide comment
@OlivierBlanvillain

OlivierBlanvillain Nov 15, 2017

Contributor

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!
Contributor

OlivierBlanvillain commented Nov 15, 2017

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!

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

Make this repo work with lampepfl/dotty#3421
Replace the use of ambiguous implicits by the `scala.implicits.Not`
class introduced in the PR.
@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Nov 15, 2017

Member

@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.

Member

smarter commented Nov 15, 2017

@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.

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

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Nov 27, 2017

Fix ambiguous implicit for Show[Nothing]
PR lampepfl#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()
```

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Nov 27, 2017

Fix ambiguous implicit for Show[Nothing]
PR lampepfl#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()
```

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Nov 27, 2017

Fix ambiguous implicit for Show[Nothing]
PR lampepfl#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()
```

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Nov 28, 2017

Fix ambiguous implicit for Show[Nothing]
PR lampepfl#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()
```

allanrenucci added a commit to allanrenucci/dotty that referenced this pull request Nov 28, 2017

Fix ambiguous implicit for Show[Nothing]
PR lampepfl#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()
```

@adriaanm adriaanm referenced this pull request Dec 1, 2017

Open

Narrow implicit scope #446

@allanrenucci allanrenucci deleted the dotty-staging:fix-strawan branch Dec 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment