Skip to content

SI-10159: Don't report cyclic reference errors found in isCyclicOrErroneous.#5656

Closed
ghost wants to merge 2 commits into2.12.xfrom
unknown repository
Closed

SI-10159: Don't report cyclic reference errors found in isCyclicOrErroneous.#5656
ghost wants to merge 2 commits into2.12.xfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Jan 22, 2017

Previously, while the implicit would rightly be discarded, typedInternal would catch the CyclicReference error and call reportTypeError before rethrowing, which would cause compilation to fail.

This should help fix milessabin/shapeless#679.

Review by @adriaanm, @retronym

…roneous`.

Previously, while the implicit would rightly be discarded,
`typedInternal` would catch the CyclicReference error
and call `reportTypeError` before rethrowing, which would cause
compilation to fail.
@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Jan 22, 2017
@retronym
Copy link
Member

retronym commented Jan 22, 2017

I believe this is an analogous test case that doesn't involve macros and Dynamic.

class D[T <: Bound](t: T) { type TT = T}
object Test {
  object C extends D(1)
  implicit val unrelated: List[C.TT] = null // error: illegal cyclic reference involving object C
}
class Bound
object Bound {
  implicit def AnyToBound[T](t: T): Bound = new Bound
}

Could you check whether this PR fixes it, too, and add it as an extra test case?

@retronym
Copy link
Member

My understanding of ImplicitInfo#computeIsCyclicOrErroneous is that even after you've suppressed the unwanted reporting of the cyclic error, the implicit will still be out of service as the ImplicitInfo is cached. I think there are different instances of ImplicitInfo to represent the in-scope implicits (Context#implicitss) vs the companion scope implicits (Implicits#implicitsOfExpectedType), so in- or out-of-service might vary depending on how you consume the implicit.

@ghost
Copy link
Author

ghost commented Jan 23, 2017

That's a good point, about the caching. Currently, adding val i = implicitly[List[K]] to my test gives a "could not find implicit" error, but val i: List[K] = implicitly[List[K]] does not... it appears that the implicit cache gets cleared at some point.

I'm not quite sure what can be done about that: the most direct fix I can think of would be to not cache isCyclicOrErroneous for locked symbols, but that seems like it would come across as somewhat arcane. What do you think?

Also, I cannot get your test case to compile. On 2.12.1, the cyclic reference error appears, but with other errors as well:

<pastie>:13: error: inferred type arguments [Int] do not conform to class D's type parameter bounds [T <: Bound]
         object C extends D(1)
                          ^
<pastie>:13: error: type mismatch;
 found   : Int(1)
 required: T
         object C extends D(1)

However, with this change the cyclic reference error vanishes (but those two remain).

Currently, a propagated cyclic reference error in computing
ImplicitInfo#isCyclicOrErroneous will poison that entry forever.
If the cyclic error is produced by a symbol other than the one
being considered, it may become non-cyclic later on, and
should be kept around.
isCyclicOrErroneousCache = cyclicOrErroneous
cyclicOrErroneous
} catch {
case _: CyclicReference =>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this is a correct test. If it's unavoidably a cyclic reference, then sym should be locked... otherwise we probably should consider it next time around.

I'm going to compile shapeless w/ and w/o this patch to see if this will cause a major performance regression. I believe it won't, however.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. validate-test took somewhat longer on this commit than the previous... some others take longer... I can't tell if that's in normal variation or whatnot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of noise on the duration of tests. We do have benchmarking infra coming up soon /cc @retronym

@adriaanm adriaanm requested a review from retronym March 6, 2017 23:54
@retronym retronym self-assigned this Mar 6, 2017
@adriaanm
Copy link
Contributor

adriaanm commented Mar 7, 2017

I've been digging around a bit (apologies for taking so long!), and it looks like your PR reveals some much deeper issues! When changing the type alias to

  type K = Record.bip.type#T // was `Record.bip.T`

the example compiles without error, even though that should be an equivalent formulation (it works because implicit search is correctly disabled when typing Record.bip.type). That's the first issue (code duplication ftw): typing a Select(qual, name), where name.isTypeName, should behave the same as SelectFromTypeTree(SingletonTypeTree(qual), name).

Secondly, and sadly, I think neither version should compile, as the [select|apply]Dynamic expansion was never intended to happen in types... Without macros, it wasn't possible to escape the stable identifier requirement, so it didn't occur to me to check that explicitly.

/cc @odersky for advice on wwdd (what would dotty do)

@adriaanm
Copy link
Contributor

adriaanm commented Mar 7, 2017

Secondly, and sadly, I think neither version should compile,

To clarify, for backwards compatibility we won't disallow this in 2.12.x, but I expect this loophole to be closed. I have a PR in the making that implements your initial observation that implicit search should not be used while type checking the qualifier of a type selection. (Which makes your test case compile)

@ghost
Copy link
Author

ghost commented Mar 8, 2017

@adriaanm has looked deeper and found a much nicer solution... closing for now

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants