Skip to content

Dottydoc: Show implicit modifier in implicit function types #4954

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

Merged
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ trait TypeLinker extends MemberLookup {
ref.copy(left = linkReference(ent, left, packs), right = linkReference(ent, right, packs))
case ref @ NamedReference(_, rf, _, _) =>
ref.copy(ref = linkRef(rf))
case ref @ FunctionReference(args, rv) =>
case ref @ FunctionReference(args, rv, _) =>
ref.copy(args = args.map(linkReference(ent, _, packs)), returnValue = linkReference(ent, rv, packs))
case ref @ TupleReference(args) =>
ref.copy(args = args.map(linkRef))
Expand Down
7 changes: 4 additions & 3 deletions doc-tool/src/dotty/tools/dottydoc/model/JavaConverters.scala
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,11 @@ object JavaConverters {
"scala" -> ref
).asJava

case FunctionReference(args, returnValue) => Map(
case FunctionReference(args, returnValue, isImplicit) => Map(
"kind" -> "FunctionReference",
"args" -> args.map(_.asJava).asJava,
"returnValue" -> returnValue.asJava,
"isImplicit" -> isImplicit,
"scala" -> ref
).asJava

Expand Down Expand Up @@ -273,15 +274,15 @@ object JavaConverters {
e.paramLists.map { paramList =>
Map(
"isImplicit" -> paramList.isImplicit,
"list" -> paramList.list.map(_.showReference).asJava
"list" -> paramList.list.map(_.asJava).asJava
).asJava
}
.asJava
}
)

def returnValue(e: ReturnValue) =
Map("returnValue" -> e.returnValue.showReference)
Map("returnValue" -> e.returnValue.asJava)

entity(e) ++ (e match {
case e: Package => members(e)
Expand Down
2 changes: 1 addition & 1 deletion doc-tool/src/dotty/tools/dottydoc/model/factories.scala
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ object factories {
val cls = tycon.typeSymbol

if (defn.isFunctionClass(cls))
FunctionReference(args.init.map(expandTpe(_, Nil)), expandTpe(args.last))
FunctionReference(args.init.map(expandTpe(_, Nil)), expandTpe(args.last), defn.isImplicitFunctionClass(cls))
else if (defn.isTupleClass(cls))
TupleReference(args.map(expandTpe(_, Nil)))
else {
Expand Down
41 changes: 1 addition & 40 deletions doc-tool/src/dotty/tools/dottydoc/model/references.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ object references {
final case class TypeReference(title: String, tpeLink: MaterializableLink, paramLinks: List[Reference]) extends Reference
final case class OrTypeReference(left: Reference, right: Reference) extends Reference
final case class AndTypeReference(left: Reference, right: Reference) extends Reference
final case class FunctionReference(args: List[Reference], returnValue: Reference) extends Reference
final case class FunctionReference(args: List[Reference], returnValue: Reference, isImplicit: Boolean) extends Reference
final case class TupleReference(args: List[Reference]) extends Reference
final case class BoundsReference(low: Reference, high: Reference) extends Reference
final case class NamedReference(title: String, ref: Reference, isByName: Boolean = false, isRepeated: Boolean = false) extends Reference
Expand All @@ -27,43 +27,4 @@ object references {
})
}
final case class NoLink(title: String, target: String) extends MaterializableLink

implicit class ReferenceShower(val ref: Reference) extends AnyVal {
Copy link
Contributor

@Blaisorblade Blaisorblade Aug 17, 2018

Choose a reason for hiding this comment

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

As an outsider: This PR mostly makes sense to me, but I'm not sure why ReferenceShower is removed here.

Not that I'm attached to it — it seems pretty-printing won't insert necessary extra parentheses — seems we'd have gotten A & B => C from both (A & B) => C and A & (B => C). But is this handled better by the new code? It seems harder to get right once you flatten everything into nested maps, as asJava seems to do.

For the record, PlainPrinter/RefinedPrinter start handling this correctly in #4072 (tho the basic infrastructure was in place): recursive calls must pass the current precedence and insert parentheses around nodes with a lower precedence than the current one.

But I wonder if DottyDoc should reimplement this logic instead of reusing RefinedPrinter, since each deviation will confuse users. The only different requirement seems that RefinedPrinter uses a pretty-printing library designed to insert line-breaks where needed, but flattening that to String without extra line-breaks should be feasible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it because the functionality is actually implemented twice: in ReferenceShower and RenderReference.

Instead of generating a string from the reference, we keep it in "java form" and use the tag to actually render it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, but the precedence bug seems still an issue; I've confirmed it (tho without this PR) and opened #4966.

def showReference: String = ref match {
case TypeReference(title, _, tparams) =>
title + {
if (tparams.nonEmpty) tparams.map(_.showReference).mkString("[", ",", "]")
else ""
}

case OrTypeReference(left, right) =>
left.showReference + " | " + right.showReference
case AndTypeReference(left, right) =>
left.showReference + " & " + right.showReference

case FunctionReference(args, ret) =>
if (args.isEmpty)
"() => " + ret.showReference
else if (args.tail.isEmpty)
args.head.showReference + " => " + ret.showReference
else
args.mkString("(", ",", s") => ${ret.showReference}")

case TupleReference(xs) =>
xs.mkString("(", ",", ")")

case BoundsReference(lo, hi) =>
lo.showReference + "<: " + hi.showReference

case NamedReference(title, ref, isByName, isRepeated) =>
val byName = if (isByName) "=> " else ""
val repeated = if (isRepeated) "*" else ""
s"$title: $byName${ref.showReference}$repeated"

case ConstantReference(title) => title
case EmptyReference =>
assert(false, "unexpected empty reference")
"<empty reference>"
}
}
}
5 changes: 3 additions & 2 deletions doc-tool/src/dotty/tools/dottydoc/staticsite/tags.scala
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,14 @@ object tags {
case AndTypeReference(left, right) =>
s"""${renderReference(left)}<span class="and-or-separator"> &amp; </span>${renderReference(right)}"""

case FunctionReference(args, returnValue) => {
case FunctionReference(args, returnValue, isImplicit) => {
val implicitPrefix = if (isImplicit) "implicit " else ""
val params =
if (args.isEmpty) "<span>() =&gt; </span>"
else if (args.tail.isEmpty) renderReference(args.head) + """<span class="right-arrow"> =&gt; </span>"""
else args.map(renderReference).mkString("<span>(</span>", "<span>, </span>", "<span>) =&gt; </span>")

params + renderReference(returnValue)
implicitPrefix + params + renderReference(returnValue)
}

case TupleReference(args) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ class JavaConverterTest {
case AndTypeReference(left, right) =>
assertSerializedCorrectly(left, actual.get("left"))
assertSerializedCorrectly(right, actual.get("right"))
case FunctionReference(args, returnValue) =>
case FunctionReference(args, returnValue, isImplicit) =>
assertEquals(isImplicit, actual.get("isImplicit"))
assertEach(args, actual.get("args")) { (exp, act) =>
assertSerializedCorrectly(exp, act)
}
Expand Down
51 changes: 51 additions & 0 deletions doc-tool/test/dotty/tools/dottydoc/TypeRendering.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package dotty.tools
package dottydoc

import dotty.tools.dotc.util.SourceFile
import dotty.tools.dottydoc.model._
import dotty.tools.dottydoc.model.internal._
import dotty.tools.dottydoc.model.references._

import org.junit.Test
import org.junit.Assert.{assertTrue, fail}

class TypeRenderingTestFromTasty extends TypeRenderingTest with CheckFromTasty
class TypeRenderingTestFromSource extends TypeRenderingTest with CheckFromSource
abstract class TypeRenderingTest extends DottyDocTest {
@Test def renderImplicitFunctionType = {
val source = new SourceFile(
"ImplicitFunctionType.scala",
"""
|package scala
|
|trait Test {
| def a: implicit Int => Int = ???
| def b(x: implicit Int => Int) = ???
| type c = implicit Int => Int
|}
""".stripMargin
)

def checkImplicitFunctionType(ref: Reference) = ref match {
case FunctionReference(_, _, isImplicit) =>
assertTrue("Should be an implicit function type", isImplicit)
case _ =>
fail("Unexpected: " + ref)
}

checkFromSource(source :: Nil) { case (_, packages) =>
packages("scala") match {
case PackageImpl(_, _, _, List(trt: Trait), _, _, _, _) =>
val List(a: Def, b: Def, c: TypeAlias) = trt.members.sortBy(_.name)
checkImplicitFunctionType(a.returnValue)
b.paramLists.head.list.head match {
case NamedReference("x", ref, _, _) =>
checkImplicitFunctionType(ref)
case _ =>
fail("Unexpected: " + b.paramLists.head.list.head)
}
checkImplicitFunctionType(c.alias.get)
}
}
}
}