Skip to content

Cyclic reference involving implicit value #4709

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

Closed
allanrenucci opened this issue Jun 22, 2018 · 6 comments · Fixed by #4729
Closed

Cyclic reference involving implicit value #4709

allanrenucci opened this issue Jun 22, 2018 · 6 comments · Fixed by #4729

Comments

@allanrenucci
Copy link
Contributor

class Context
class ContextBase { def settings = 1 }

class Test {
  implicit def toBase(ctx: Context): ContextBase = ???

  def test(ctx0: Context) = {
    implicit val ctx = { ctx0.settings; ??? }
  }
}

This code snippet above fails to compile:

-- [E047] Syntax Error: tests/allan/Test.scala:8:25 ----------------------------
8 |    implicit val ctx = { ctx0.settings; ??? }
  |                         ^
  |                         cyclic reference involving implicit value ctx

Adding an explicit type annotation to val ctx solves the issue

@allanrenucci
Copy link
Contributor Author

Maybe related to #3253

allanrenucci added a commit to dotty-staging/dotty that referenced this issue Jun 22, 2018
@odersky
Copy link
Contributor

odersky commented Jun 24, 2018

The cyclic reference is real. The only possible improvement I see is to change the error message to say that an explicit return type should be added.

@allanrenucci
Copy link
Contributor Author

Can you explain what the cyclic reference is

@odersky
Copy link
Contributor

odersky commented Jun 25, 2018

settings is not a member of ctx, so we need to look for an implicit reference that makes it one. ctx is implicit, so we need its type to determine whether it is a suitable candidate -> cycle.

@Blaisorblade
Copy link
Contributor

@allanrenucci pointed out that here you can't actually use ctx in itself because it's not a field (and uses will get flagged earlier), so it's not clear why it should be considered by implicit search.

It's weird, in fact, that some vals (including implicits) are in scope when they can't be used. You need to shadow earlier references to keep compatibility with the current semantics, but why have them in scope?

More surprising puzzlers we found include the following (uncategorized):

pgiarrusso@SilenceOfTheLambdas:~/git/scala$ /usr/local/bin/dotr
Starting dotty REPL...
scala> def f: Unit = { implicitly[Int]; implicit val i: Int = 1 }
1 |def f: Unit = { implicitly[Int]; implicit val i: Int = 1 }
  |                ^^^^^^^^^^^^^^^
  |                a pure expression does nothing in statement position
1 |def f: Unit = { implicitly[Int]; implicit val i: Int = 1 }
  |                ^^^^^^^^^^^^^^^
  |           `i` is a forward reference extending over the definition of `i` // this error mentions generated code
scala> def f: Unit = { implicitly[Int]; implicit lazy val i: Int = 1 }
def f: Unit
scala> def f: Unit = { implicitly[Int]; implicit val i = 1 }
1 |def f: Unit = { implicitly[Int]; implicit val i = 1 }
  |                ^^^^^^^^^^^^^^^
  |                a pure expression does nothing in statement position
1 |def f: Unit = { implicitly[Int]; implicit val i = 1 }
  |                ^^^^^^^^^^^^^^^
  |           `i` is a forward reference extending over the definition of `i`
scala> def f: Unit = { implicitly[Int]; implicit val i = implicitly[Int] } // OK, it's fine to have an error here
1 |def f: Unit = { implicitly[Int]; implicit val i = implicitly[Int] }
  |                                                  ^^^^^^^^^^^^^^^
  |                               cyclic reference involving implicit value i
pgiarrusso@SilenceOfTheLambdas:~/git/dotty-master$ scala
Welcome to Scala 2.12.6 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_172).
Type in expressions for evaluation. Or try :help.

scala> def f: Unit = { implicitly[Int]; implicit val i: Int = 1 }
<console>:11: error: forward reference extends over definition of value i
       def f: Unit = { implicitly[Int]; implicit val i: Int = 1 }
                                 ^

scala> def f: Unit = { implicitly[Int]; implicit lazy val i: Int = 1 }
f: Unit

scala> def f: Unit = { implicitly[Int]; implicit val i = implicitly[Int] }
<console>:11: error: could not find implicit value for parameter e: Int
       def f: Unit = { implicitly[Int]; implicit val i = implicitly[Int] }
                                 ^
<console>:11: error: could not find implicit value for parameter e: Int
       def f: Unit = { implicitly[Int]; implicit val i = implicitly[Int] }
                                                                   ^

scala> def f: Unit = { implicit val i = implicitly[Int] }
<console>:11: error: could not find implicit value for parameter e: Int
       def f: Unit = { implicit val i = implicitly[Int] }
                                                  ^

scala> def f: Unit = { implicit val i: Int = implicitly[Int] }
<console>:11: error: forward reference extends over definition of value i
       def f: Unit = { implicit val i: Int = implicitly[Int] }
                                                       ^

@Blaisorblade
Copy link
Contributor

Discussion revealed that the fix Martin proposed could probably be localized to https://github.com/lampepfl/dotty/blob/1dce6772d0a4128e87e7038ac5da5d9e91c1c458/compiler/src/dotty/tools/dotc/core/TypeErrors.scala#L101-L124 — if we're producing CyclicReferenceInvolvingImplicit but we have cx.mode is Mode.InferringReturnType, it seems we could produce as error a mixture of RecursiveValueNeedsResultType and CyclicReferenceInvolvingImplicit with the desired error.

Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jun 27, 2018
Now we use the same logic as for recursive values to check we're indeed dealing
with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so I'm still not sure what's best to do.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jun 27, 2018
Now we use the same logic as for recursive values to check we're indeed dealing
with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so I'm still not sure what's best to do.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jun 27, 2018
Now we use the same logic as for recursive values to check we're indeed dealing
with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jun 27, 2018
This improves the error message, and also puts more effort into diagnosing the
cycle.

Now we use the same logic as for recursive values to check we're indeed dealing
with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.
allanrenucci added a commit to dotty-staging/dotty that referenced this issue Jun 29, 2018
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jul 1, 2018
…better

- Record in CyclicReference if it's emitted during implicit search.
- Before checking if the error is on an implicit, check if we have an error
  during implicit search that happens while inferring a return type; that can
  happen whether the member we're typechecking is implicit or not, and needs the
  same cure.

This improves the error message, and also puts more effort into diagnosing the
cycle: we now use similar logic as for recursive/overloaded members to check
we're indeed dealing with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jul 7, 2018
…better

- Record in CyclicReference if it's emitted during implicit search.
- Before checking if the error is on an implicit, check if we have an error
  during implicit search that happens while inferring a return type; that can
  happen whether the member we're typechecking is implicit or not, and needs the
  same cure.
- Add ErrorMessagesTests as requested

This improves the error message, and also puts more effort into diagnosing the
cycle: we now use similar logic as for recursive/overloaded members to check
we're indeed dealing with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jul 24, 2018
…better

- Record in CyclicReference if it's emitted during implicit search.
- Before checking if the error is on an implicit, check if we have an error
  during implicit search that happens while inferring a return type; that can
  happen whether the member we're typechecking is implicit or not, and needs the
  same cure.
- Add ErrorMessagesTests as requested

This improves the error message, and also puts more effort into diagnosing the
cycle: we now use similar logic as for recursive/overloaded members to check
we're indeed dealing with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.

*However*, this does not yet handle mutually recursive methods in a reasonable
way.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Jul 25, 2018
…better

- Record in CyclicReference if it's emitted during implicit search.
- Before checking if the error is on an implicit, check if we have an error
  during implicit search that happens while inferring a return type; that can
  happen whether the member we're typechecking is implicit or not, and needs the
  same cure.
- Add ErrorMessagesTests as requested

This improves the error message, and also puts more effort into diagnosing the
cycle: we now use similar logic as for recursive/overloaded members to check
we're indeed dealing with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.

*However*, this does not yet handle mutually recursive methods in a reasonable
way.
Blaisorblade added a commit to dotty-staging/dotty that referenced this issue Aug 9, 2018
…better

- Record in CyclicReference if it's emitted during implicit search.
- Before checking if the error is on an implicit, check if we have an error
  during implicit search that happens while inferring a return type; that can
  happen whether the member we're typechecking is implicit or not, and needs the
  same cure.
- Add ErrorMessagesTests as requested

This improves the error message, and also puts more effort into diagnosing the
cycle: we now use similar logic as for recursive/overloaded members to check
we're indeed dealing with a (potentially) recursive implicit.

Remaining `CyclicReferenceInvolvingImplicit` are unexpected and might involve
bugs, so make the error message and explanation more tentative, following
`CyclicReferenceInvolving`.

*However*, this does not yet handle mutually recursive methods in a reasonable
way.
Blaisorblade added a commit that referenced this issue Aug 11, 2018
Fix #4709: report if local implicits need type annotation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants