SI-10154 Fix implicit search regression for term-owned objects #5654
Conversation
A recent change to fix lookup of companion implicits of term-owned classes (scala#5550) caused a regression in the enclosed test case. The previous approach of calling `Scope#lookup(companionName)` was replaced by a lookup of the scope entry of the original name followed by a narrower search lookup for the companion name, to ensure that it was a true companion, and not just a same-named module from defined at a different nested scope. However, module class symbols are not themselves entered into scopes, so the first part of the new scheme fails. We need to add a special case modules here. I've chosen to just call `.sourceModule` on module classes. For module classes in the current run (all term owned symbols will fall into this category), this amounts to using the value of the field `ModuleClassSymbol#module`.
|
Trying a release build (which should include the community build) |
This reverts commit 6d935d4. the problem Adriaan was investigating here turned out to be a Scala regression, not a Shapeless bug. see scala/scala#5654
|
in Jenkins the |
the upickle-pprint failure is almost certainly unrelated. the other two, it's harder to be sure. but, Shapeless being broken took a pretty big swath of the community build out for a couple months or so, so it wouldn't be surprising at all if a few projects randomly broke during that time for unrelated reasons and we're just learning about it now. |
|
@sbuzzard has experienced some regressions in 2.12.1 that he's documented in this gist: https://gist.github.com/sbuzzard/e43ddf1c4c7c3a85e192785fc27dc220 (thanks for the careful minimization!). Steve, can you test your code against this PR and see if it fixes your issues? you should be able to use: |
|
@fommil what do you make of the spray-json-shapeless failure...? |
|
I think the Argonaut failure cannot be this PR's fault — it's just failing to find a Scala-version-specific source directory. |
|
Still received the errors with this build. https://gist.github.com/sbuzzard/671bf1d7ee677151e52cedab94a8afed |
|
Hmm, I'll need to investigate further. This is almost certainly going to cause trouble for typeclass derivation, but I'd be amazed if sjs fails but something like circe passes. Could be I was relying on something really fragile? |
|
I will say that we have similar constructs throughout our code base, with implicits for both scodec Codecs and circe Encoders and Decoders in the companions. We also have hundreds of instances where we have ADTs defined in the same compilation unit. But in only a few cases do we see the direct known subclass error and only in one case do we get the no such element exception. Not sure what it is about the structure of my minimized test case that causes this. In addition to the work arounds in my original gist, if the maybeOptionCodec in the minimized reproduction is not defined in the object where it is used but elsewhere, the no such element exception does not occur. |
|
I've reduced @sbuzzard's test that results in the import scodec.Codec
sealed trait TestType
object TestType {
implicit val codec: Codec[TestType] = ???
}
case class Test(testType: TestType)
object Test {
private def crossTalk: Unit = {
shapeless.Lazy.mkLazy[Codec[TestType]]
()
}
implicit val codec: Codec[Test] = null
}
object Client {
def crossTalk /*inferred*/ = {
shapeless.Lazy.mkLazy[Codec[Test]]
()
}
}This expands to: package <empty> {
import scodec.Codec;
sealed abstract trait TestType extends scala.AnyRef;
object TestType extends scala.AnyRef {
private[this] val codec: scodec.Codec[TestType] = scala.Predef.???;
implicit <stable> <accessor> def codec: scodec.Codec[TestType] = TestType.this.codec
};
case class Test extends AnyRef with Product with Serializable {
...
};
object Test extends scala.AnyRef with Serializable {
private def crossTalk: Unit = {
{
val inst$macro$4: scodec.Codec[TestType] = {
final class anon$lazy$macro$3 extends AnyRef with Serializable {
<stable> <accessor> lazy val inst$macro$2: scodec.Codec[Test] = Test.this.codec.asInstanceOf[scodec.Codec[Test]];
<stable> <accessor> lazy val inst$macro$1: scodec.Codec[TestType] = TestType.codec.asInstanceOf[scodec.Codec[TestType]]
};
new anon$lazy$macro$3().inst$macro$1
};
shapeless.Lazy.apply[scodec.Codec[TestType]](inst$macro$4)
};
()
};
private[this] val codec: scodec.Codec[Test] = null;
implicit <stable> <accessor> def codec: scodec.Codec[Test] = Test.this.codec;
case <synthetic> def apply(testType: TestType): Test = new Test(testType);
case <synthetic> def unapply(x$0: Test): Option[TestType] = if (x$0.==(null))
scala.None
else
Some.apply[TestType](x$0.testType);
<synthetic> private def readResolve(): Object = Test
};
object Client extends scala.AnyRef {
def crossTalk: Unit = {
shapeless.Lazy.apply[scodec.Codec[Test]](inst$macro$2);
()
}
}
}The ill-scoped reference to I suspect this is a bug in the Shapeless |
|
@retronym it looks like it's an interaction between |
|
yep - we should be doing that anyway (at least in the cases where we've tripped over this). Adding the explicit result type works great to resolve the element not found error . There are still cases where I get the known direct subclass before subtype is registered with all obvious result types annotated. Thanks! |
|
Actually, this change in the compiler seems to be the most straightforward fix, diff --git src/compiler/scala/tools/nsc/typechecker/Namers.scala src/compiler/scala/tools/nsc/typechecker/Namers.scala
index 395bda234b..80730a83af 100644
--- src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -1856,6 +1856,7 @@ trait Namers extends MethodSynthesis {
val tree: Tree
override def forceDirectSuperclasses: Unit = {
tree.foreach {
+ case DefDef(_, _, _, _, TypeTree(), _) =>
case dt: DefTree => global.withPropagateCyclicReferences(Option(dt.symbol).map(_.maybeInitialize))
case _ =>
} |
| @@ -1194,7 +1194,8 @@ trait Contexts { self: Analyzer => | |||
| } | |||
|
|
|||
| final def lookupCompanionOf(original: Symbol): Symbol = { | |||
There was a problem hiding this comment.
My head started spinning with indirection, so I figure I'll rubber-duck-review this by doing some inlining...
I tried to push all logic into lookupCompanionOf:
final def lookupCompanionOf(original: Symbol): Symbol =
if (original == NoSymbol) NoSymbol else { // can we collapse the next few lines by using original.exists in the condition here?
original.failIfStub()
if (original.needsFlatClasses) original.info // use initialize() ?
val flatOwnerInfo = original.owner.rawInfo
// can we find it using the owner structure -- i.e., is it a member of some other symbol?
val member =
original match {
case mcs: ModuleClassSymbol if mcs.module ne null => mcs.module
case _ =>
flatOwnerInfo.decl(original.name.companionName).suchThat(sym =>
sym != original && (if (original.isClass) sym.isModule else sym.isClass) && sym.isCoDefinedWith(original)
)
}
// can we decide which branch to take here directly rather than by trying the search?
// i.e., is it a term-owned symbol??
if (member == NoSymbol)
lookupScopeEntry(original) match {
case null => NoSymbol
case entry => entry.owner.lookupCompanion(original)
}
else member
}
There was a problem hiding this comment.
can we decide which branch to take here directly rather than by trying the search? i.e., is it a term-owned symbol??
I think we also get into this code path for class-owned symbols where the class info is incomplete. So the check would need to be something like owner.isTerm || !owner.hasCompleteInfo
8b827e2 to
3b67c71
Compare
|
I would like to see this PR move forward, in order to fix the Scala 2.12 community build, in order to ensure the quality of the Scala 2.12.2 release, which is now probably only a few weeks off. @retronym, is there anything I can do to help? |
3b67c71 to
d230729
Compare
|
@adriaanm I pushed a fix for the build errors in the last commit. Do you prefer it to the original commit? |
d230729 to
1d41aef
Compare
|
I've moved the refactoring to #5700 so we can fast track the regression fix. |
A recent change to fix lookup of companion implicits of term-owned
classes (#5550) caused a regression in the enclosed test case. The
previous approach of calling
Scope#lookup(companionName)wasreplaced by a lookup of the scope entry of the original name followed
by a narrower search lookup for the companion name, to ensure
that it was a true companion, and not just a same-named module
from defined at a different nested scope.
However, module class symbols are not themselves entered into
scopes, so the first part of the new scheme fails. We need to
add a special case modules here.
I've chosen to just call
.sourceModuleon module classes.For module classes in the current run (all term owned symbols
will fall into this category), this amounts to using the value
of the field
ModuleClassSymbol#module.JIRA: https://issues.scala-lang.org/browse/SI-10154