Skip to content
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

recursive value xxx needs type #7486

Closed
scabug opened this issue May 16, 2013 · 19 comments
Closed

recursive value xxx needs type #7486

scabug opened this issue May 16, 2013 · 19 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented May 16, 2013

With 2.10.2-SNAPSHOT (2.10.2-20130515-155912-70e2ead142) compilation error is reported for the following code in akka-testkit.
https://github.com/akka/akka/blob/master/akka-testkit/src/test/scala/akka/testkit/AkkaSpecSpec.scala#L67

        implicit val davyJones = otherSystem.actorOf(Props(new Actor {
          def receive = {
            case m: DeadLetter  locker :+= m
            case "Die!"         sender ! "finally gone"; context.stop(self)
          }
        }), "davyJones")

[error] /Users/patrik/dev/akka/akka-testkit/src/test/scala/akka/testkit/AkkaSpecSpec.scala:68: recursive value davyJones needs type
[error]           def receive = {
[error]               ^

It compiles fine when I remove the implicit, which we actually don't need here.

This is compiling fine with 2.10.1.

@scabug
Copy link
Author

@scabug scabug commented May 16, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7486?orig=1
Reporter: Patrik Nordwall (patriknw)
Affected Versions: 2.10.2-RC1, 2.11.0-M3
Other Milestones: 2.11.0-M4

@scabug
Copy link
Author

@scabug scabug commented May 17, 2013

@retronym said:
I'm looking into this now to figure out what changed, and whether this blocks 2.10.2.

@scabug
Copy link
Author

@scabug scabug commented May 17, 2013

@retronym said:

object Test{
  var locker = 0
  // remove implicit, or change to `locker = locker + 1` to make it compile.
  implicit val davyJones0 = {
    locker += 0
    0
  }
}

My bisection tools seem to be failing me: they indicate that the first failing commit is https://github.com/scala/scala/commit/2.10.x~11%5E2, which was just a checkfile update.

@scabug
Copy link
Author

@scabug scabug commented May 17, 2013

@scabug
Copy link
Author

@scabug scabug commented May 17, 2013

@paulp said:
BTW when you use 2.10.x~XX syntax there's a trap: it's a moving target.

@scabug
Copy link
Author

@scabug scabug commented May 21, 2013

@retronym said:
The fix didn't fix this regression related to forward-references, implicits without return types, and fallback to the implicit scope exercised by ScalaTest. I haven't managed to extricate this from Scalatest.

package org.scalatest

import org.scalautils.Equality
import org.scalautils.NormalizingEquality
import org.scalautils.Normalization
import org.scalautils.StringNormalizations._
import SharedHelpers._

class ListShouldContainSpec extends Spec with Matchers {
  val some: Option[String] = Some("hi")

  {
    val wrap = ListShouldContainSpec.this.convertToAnyShouldWrapper[Option[String]](ListShouldContainSpec.this.some)
    // forward reference to `implicit val e` reported in 2.10.2, in 2.10.1 companion implicit for Equality[String] was used as the implicit argument to `being`.
    wrap.should(contain("HI"))(ListShouldContainSpec.this.after.being(org.scalautils.StringNormalizations.lowerCased)/*(implicitly[Equality[String]])*/)
    wrap.should(contain("HI"))(ListShouldContainSpec.this.after.being(org.scalautils.StringNormalizations.lowerCased)/*(implicitly[Equality[String]])*/)

    // annotate type and the forward reference error is issued under 2.10.1
    implicit val e /*Equality[String]*/ = new Equality[String] {
      def areEqual(a: String, b: Any): Boolean = a != b
    }
  }
}

@scabug
Copy link
Author

@scabug scabug commented May 22, 2013

@paulp said:
I admit I can't figure out what I'm supposed to think is the bug. As best I can understand your adjectives, you're saying it's a bug when the forward reference is reported. Is that correct? Is there something specified somewhere which would lead me to that?

It is not intuitive, and it will definitely not lead to a very consistent programming experience, that there can be an implicit in the same block which has the type being sought, and it will be leapfrogged to go searching in the companion scope, but only if it has no declared type. This seems like quite a perversion of the reason there is a difference, that being cycle avoidance.

Maybe there is some backstory here which I can't see because it's stored in a lead box at lex luthor's house.

@scabug
Copy link
Author

@scabug scabug commented May 22, 2013

@retronym said:
It changed in 2.10.2, and we didn't expect it to: that's the main problem.

As per the mail I just sent (which I'll reproduce here for the record), this facet of implicit search isn't specced, so the status quo is the spec. The implementation intends to exclude e from the implicit search as it doesn't have an explicit result type, and is defined after the point of the search. But after the change, the "doesn't-have-a-result-type" check fails, as e-s symbol has its type elaborated earlier.

I guess as the 2.10.2 guardian, James ought to make a call on what to do. Two questions to answer: 1) leave it or revert it; 2) invest time into pinning down the behaviour with tests or deem it effectively untestable.

On Wed, May 22, 2013 at 12:23 AM, Bill Venners <bill@artima.com> wrote:
Seems odd that the compiler would judge this code a forward reference error rather than decide it should look in the companions. If it doesn't find anything in the companions it makes sense it could issue an error that includes a note about the forward reference as a helpful clue, but not if there is something in the companion objects. In that case I'd expect it should use the implicit in the companion.

The way things are specced (which isn't a bible, but also shouldn't be ignored): dsfs
First, eligible are all identifiers x that can be accessed at the point of the method call without a prefix and that denote an implicit definition (§7.1) or an implicit parameter. An eligible identifier may thus be a local name, or a member of an enclosing template, or it may be have been made accessible without a prefix through an import clause (§4.7). If there are no eligible identifiers under this rule, then, second, eligible are also all implicit members of some object that belongs to the implicit scope of the implicit parameter’s type, T . 

A forward reference is still in-scope. Remember, that forward references are allowed in certain circumstances:

scala> def foo { println(x); def foo = 0; def x = "x" }

foo: Unit

scala> foo

x

I won't go into the exact rules here (it's spelled out in Chapter 4 of the Spec, if you're interested). Suffice to say that they are useful for creating mutually recursive sets of local defs or lazy vals.

But, I'm pretty sure we don't want to entangle these forward reference rules with implicit search. Not only is it hard (or maybe impossible) for the compiler to detect the forward reference errors *before* completing the implicit search, but how could someone reading the code reason about it?

As you can see, the implementation already extends beyond the spec in ruling out *potential* forward references that don't have an explicit result type. Why? Well, consider this code:

{

  implicit val y = /*: InferredY */ = { calyY }

  implicit def x /*: InferredX */ = { calcX } 

}

This introduces the possibility of cyclic dependency in the inferred types of x and y. The code the Paul posted attempts to rule that out. But it does so in a fairly fragile way, and the patches I linked have bumped that off kilter. We'll investigate exactly why, but I don't know if it will be a blocker for 2.10.2. I will probably argue in favour of a conservative approach of reverting those commits; but we are still in the untenable position of having compiler behaviour that we can barely write tests for. But such are the pleasures of our job!

In general, you're witnessing the downside of providing an API that essentially requires your users to understand the mechanics of implicit search. An alternative API would not place `Equality.default` in the companion object, and instead you would require users to manually import `Equality.Implicits.default` in places where they really wanted to use default equality.

In the meantime, I would strongly recommend that you annotate the types of your Equality type class, and update your examples and documentation to do the same. In doing so, you will not expose yourself and your users to the esoteric parts of implicit search.

If you want to restrict the scope of an implicit, simply introduce a new scope:

{

  // use Equality.default here

  {

    implicit val customEquality: Equality[A] = (....)

   // use customEquality

  }


@scabug
Copy link
Author

@scabug scabug commented May 22, 2013

@retronym said:
James, building trunk ScalaTest against our latest and greatest Scala versions showed up a variation of this bug. It's also sensitive to symbol initialiization order, so quite difficult to test (let alone spec.)

Assigning to you to make a call on the course of action.

@scabug
Copy link
Author

@scabug scabug commented May 22, 2013

@retronym said:
Reopening so we don't lose track of this.

@scabug
Copy link
Author

@scabug scabug commented May 22, 2013

@adriaanm said:
Bumping to RC2 since RC1 is already on maven

@scabug
Copy link
Author

@scabug scabug commented May 23, 2013

@JamesIry said:
I suspect we may have to revert unless somebody can come up with a reasonably safe fix.

@scabug
Copy link
Author

@scabug scabug commented May 23, 2013

@paulp said:
There is no such thing as a reasonably safe fix. This aspect of the compiler is ridiculous.

@scabug
Copy link
Author

@scabug scabug commented May 23, 2013

@retronym said:
I vote to revert for 2.10.2, but leave the change in master. We can poke and test things a bit more in master to understand the scope of the change (which does seem pretty small.) I suspect that if we had an INFERRED flag on symbols we could make this deterministic.

@scabug
Copy link
Author

@scabug scabug commented May 23, 2013

@JamesIry said:
Paul, do you want to own the revert?

@scabug
Copy link
Author

@scabug scabug commented May 23, 2013

@paulp said:
I can revert it as long as nobody is going to insist on a test. The difficulty of testing it is the thing which makes it "ridiculous" so my hands are tied. If jason can't extract a test from scalatest, I guarantee I can't.

@scabug
Copy link
Author

@scabug scabug commented May 24, 2013

@adriaanm said:
A l'impossible nul n'est tenu. It's the law. (In some places.)

@scabug scabug closed this May 27, 2013
@scabug
Copy link
Author

@scabug scabug commented May 29, 2013

@scabug
Copy link
Author

@scabug scabug commented Feb 3, 2015

@retronym said:
I've just been playing with an approach to issuing a warning for discarded implicits. After we finish typechecking the file, we can see if any discarded candidates turned out to be viable for the search. We can safely check this after the entire file has been typechecked.

Here's a prototype: https://github.com/retronym/scala/compare/topic/warn-discarded-implicit?expand=1

However, this does not take into account the way that implicit search participates in type inference. If the expected type of the implicit search contains an undetermined type variable, the discarded implicit might not appear viable after inference has occurred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants