SI-10159: Don't report cyclic reference errors found in isCyclicOrErroneous.#5656
Conversation
…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.
|
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? |
|
My understanding of |
|
That's a good point, about the caching. Currently, adding I'm not quite sure what can be done about that: the most direct fix I can think of would be to not cache Also, I cannot get your test case to compile. On 2.12.1, the cyclic reference error appears, but with other errors as well: 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 => |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There's a lot of noise on the duration of tests. We do have benchmarking infra coming up soon /cc @retronym
|
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 the example compiles without error, even though that should be an equivalent formulation (it works because implicit search is correctly disabled when typing Secondly, and sadly, I think neither version should compile, as the /cc @odersky for advice on wwdd (what would dotty do) |
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) |
|
@adriaanm has looked deeper and found a much nicer solution... closing for now |
Previously, while the implicit would rightly be discarded,
typedInternalwould catch the CyclicReference error and callreportTypeErrorbefore rethrowing, which would cause compilation to fail.This should help fix milessabin/shapeless#679.
Review by @adriaanm, @retronym