Skip to content

Fix member lookups for inherited members #11821

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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions scaladoc-testcases/src/tests/links.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ object AnObject:
/**
* Broken link, that should result a warning not break compilation
* [[tests.links.AnObject]]

*/
class LinksTest:
def verifyIfLinksTestIsGenerated(b: Int): Int
= 123
= 123
20 changes: 20 additions & 0 deletions scaladoc-testcases/src/tests/lookupInheritedMembers.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package tests
package lookupInheritedMembers.pack1 {
class A:
def x = 1
val y = 1
type MyType

}
package lookupInheritedMembers.pack2 {
class B extends tests.lookupInheritedMembers.pack1.A
}

package lookupInheritedMembers {
/**
* [[tests.lookupInheritedMembers.pack2.B.x]]
* [[tests.lookupInheritedMembers.pack2.B.y]]
* [[tests.lookupInheritedMembers.pack2.B.MyType]]
*/
class LookupInheritedMembers
}
5 changes: 5 additions & 0 deletions scaladoc/src/dotty/tools/scaladoc/tasty/SymOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -190,3 +190,8 @@ class SymOps[Q <: Quotes](val q: Q) extends JavadocAnchorCreator with Scaladoc2A
// For some reason it contains `$$$` instrad of symbol name
s"${sym.name}${sym.fullName}/${sym.signature.resultSig}/[${sym.signature.paramSigs.mkString("/")}]"
)

def driInContextOfInheritingParent(par: Symbol)(using dctx: DocContext): DRI = sym.dri.copy(
location = par.dri.location,
externalLink = None
)
8 changes: 6 additions & 2 deletions scaladoc/src/dotty/tools/scaladoc/tasty/TastyParser.scala
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,12 @@ case class ScaladocTastyInspector()(using ctx: DocContext) extends DocTastyInspe
def driFor(link: String): Option[DRI] =
val symOps = new SymOps[q.type](q)
import symOps._
Try(QueryParser(link).readQuery()).toOption.flatMap(q =>
MemberLookup.lookupOpt(q, None).map{ case (sym, _) => sym.dri}
Try(QueryParser(link).readQuery()).toOption.flatMap(query =>
MemberLookup.lookupOpt(query, None).map {
case (sym, _, inheritingParent) => inheritingParent match
case Some(parent) => sym.driInContextOfInheritingParent(parent)
case None => sym.dri
}
)

ctx.staticSiteContext.foreach(_.memberLinkResolver = driFor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ abstract class MarkupConversion[T](val repr: Repr)(using DocContext) {

object SymOps extends SymOps[qctx.type](qctx)
export SymOps.dri
export SymOps.driInContextOfInheritingParent

def resolveLink(queryStr: String): DocLink =
if SchemeUri.matches(queryStr) then DocLink.ToURL(queryStr)
Expand All @@ -94,8 +95,11 @@ abstract class MarkupConversion[T](val repr: Repr)(using DocContext) {
DocLink.UnresolvedDRI(queryStr, msg)
case Right(query) =>
MemberLookup.lookup(using qctx)(query, owner) match
case Some((sym, targetText)) =>
DocLink.ToDRI(sym.dri, targetText)
case Some((sym, targetText, inheritingParent)) =>
var dri = inheritingParent match
case Some(parent) => sym.driInContextOfInheritingParent(parent)
case None => sym.dri
DocLink.ToDRI(dri, targetText)
case None =>
val txt = s"No DRI found for query"
val msg = s"$txt: $queryStr"
Expand Down
44 changes: 32 additions & 12 deletions scaladoc/src/dotty/tools/scaladoc/tasty/comments/MemberLookup.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,22 @@ import scala.quoted._

trait MemberLookup {

def memberLookupResult(using Quotes)(
symbol: quotes.reflect.Symbol,
label: String,
inheritingParent: Option[quotes.reflect.Symbol] = None
): (quotes.reflect.Symbol, String, Option[quotes.reflect.Symbol]) =
(symbol, label, inheritingParent)

def lookup(using Quotes, DocContext)(
query: Query,
owner: quotes.reflect.Symbol,
): Option[(quotes.reflect.Symbol, String)] = lookupOpt(query, Some(owner))
): Option[(quotes.reflect.Symbol, String, Option[quotes.reflect.Symbol])] = lookupOpt(query, Some(owner))

def lookupOpt(using Quotes, DocContext)(
query: Query,
ownerOpt: Option[quotes.reflect.Symbol],
): Option[(quotes.reflect.Symbol, String)] =
): Option[(quotes.reflect.Symbol, String, Option[quotes.reflect.Symbol])] =
try
import quotes.reflect._

Expand All @@ -26,7 +33,7 @@ trait MemberLookup {
def nearestMembered(sym: Symbol): Symbol =
if sym.isClassDef || sym.flags.is(Flags.Package) then sym else nearestMembered(sym.owner)

val res: Option[(Symbol, String)] = {
val res: Option[(Symbol, String, Option[Symbol])] = {
def toplevelLookup(querystrings: List[String]) =
downwardLookup(querystrings, defn.PredefModule.moduleClass)
.orElse(downwardLookup(querystrings, defn.ScalaPackage))
Expand All @@ -38,7 +45,7 @@ trait MemberLookup {
val nearest = nearestMembered(owner)
val nearestCls = nearestClass(owner)
val nearestPkg = nearestPackage(owner)
def relativeLookup(querystrings: List[String], owner: Symbol): Option[Symbol] = {
def relativeLookup(querystrings: List[String], owner: Symbol): Option[(Symbol, Option[Symbol])] = {
val isMeaningful =
owner.exists
// those are just an optimisation, they can be dropped if problems show up
Expand All @@ -56,26 +63,27 @@ trait MemberLookup {

query match {
case Query.StrictMemberId(id) =>
localLookup(id, nearest).nextOption.map(_ -> id)
localLookup(id, nearest).nextOption.map(memberLookupResult(_, id))
case Query.QualifiedId(Query.Qual.This, _, rest) =>
downwardLookup(rest.asList, nearestCls).map(_ -> rest.join)
downwardLookup(rest.asList, nearestCls).map(memberLookupResult(_, rest.join, _))
case Query.QualifiedId(Query.Qual.Package, _, rest) =>
downwardLookup(rest.asList, nearestPkg).map(_ -> rest.join)
downwardLookup(rest.asList, nearestPkg).map(memberLookupResult(_, rest.join, _))
case query =>
val ql = query.asList
toplevelLookup(ql)
.orElse(relativeLookup(ql, nearest))
.map(_ -> query.join)
.map(memberLookupResult(_, query.join, _))
}

case None =>
toplevelLookup(query.asList).map(_ -> query.join)
toplevelLookup(query.asList).map(memberLookupResult(_, query.join, _))
}
}

// println(s"looked up `$query` in ${owner.show}[${owner.flags.show}] as ${res.map(_.show)}")

res

catch
case e: Exception =>
// TODO (https://github.com/lampepfl/scala3doc/issues/238): proper reporting
Expand Down Expand Up @@ -169,11 +177,13 @@ trait MemberLookup {
}
}

private def downwardLookup(using Quotes)(query: List[String], owner: quotes.reflect.Symbol): Option[quotes.reflect.Symbol] = {
private def downwardLookup(using Quotes)(
query: List[String], owner: quotes.reflect.Symbol
): Option[(quotes.reflect.Symbol, Option[quotes.reflect.Symbol])] = {
import quotes.reflect._
query match {
case Nil => None
case q :: Nil => localLookup(q, owner).nextOption
case q :: Nil => localLookup(q, owner).nextOption.map((_, None))
case q :: qs =>
val lookedUp =
localLookup(q, owner).toSeq
Expand All @@ -191,8 +201,18 @@ trait MemberLookup {
else if s.flags.is(Flags.Module) then tm = Some(s)
else if s.isClassDef || s.isTypeDef then tp = Some(s)
}

def downwardLookUpForInheritedMembers(qs: List[String], owner: Symbol): Option[(Symbol, Option[Symbol])] =
downwardLookup(qs, owner).orElse(owner.memberMethod(qs.head).headOption.map((_, Some(owner)))).orElse {
val symbol = owner.memberType(qs.head)
Option.when(symbol != dotty.tools.dotc.core.Symbols.NoSymbol)(symbol).map((_, Some(owner)))
}.orElse {
val symbol = owner.memberField(qs.head)
Option.when(symbol != dotty.tools.dotc.core.Symbols.NoSymbol)(symbol).map((_, Some(owner)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it seems that the code we had previously didn't actually look up inherited members at all? In that case it'll have to be changed. We don't want to simply go through Scala Reflect functions here, we also need to take into account absent symbols (see hackIsNotAbsent).

This code also doesn't seem to work when the inherited symbol is in the middle of the query. In general it'd be better to modify localLookup instead of downwardLookup.


pk.flatMap(downwardLookup(qs, _))
.orElse(tp.flatMap(downwardLookup(qs, _)))
.orElse(tp.flatMap(downwardLookUpForInheritedMembers(qs, _)))
.orElse(tm.flatMap(downwardLookup(qs, _)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Inherited symbols can also appear in term symbols (objects, that is).

}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class LookupTestCases[Q <: Quotes](val q: Quotes) {
cases.foreach { case (query, Sym(sym)) =>
val lookupRes = MemberLookup.lookupOpt(parseQuery(query), None)
assertTrue(s"Couldn't look up: $query", lookupRes.nonEmpty)
val Some((lookedUp, _)) = lookupRes
val Some((lookedUp, _, _)) = lookupRes
assertSame(query, sym, lookedUp)
}
}
Expand Down Expand Up @@ -89,7 +89,7 @@ class LookupTestCases[Q <: Quotes](val q: Quotes) {
)

cases.foreach { case ((Sym(owner), query), Sym(target)) =>
val Some((lookedUp, _)) = MemberLookup.lookup(parseQuery(query), owner)
val Some((lookedUp, _, _)) = MemberLookup.lookup(parseQuery(query), owner)
assertSame(s"$owner / $query", target, lookedUp)
}
}
Expand Down