-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
WIP: merge Scala3doc #10081
WIP: merge Scala3doc #10081
Conversation
7c832dd
to
d27609f
Compare
d27609f
to
e9871ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in sbt plugin and build definition looks ok however I think it should be rebased on top of #10073 since there we've maintained a history of scala3doc
and use more recent version of code, without need for small Kotlin project build with gradle as well changes in GH actions
@@ -19,7 +19,8 @@ case class DottyDokkaConfig(docConfiguration: DocConfiguration) extends DokkaCon | |||
|
|||
private object OurConfig extends DokkaConfiguration.PluginConfiguration: | |||
override def getFqPluginName = "ExternalDocsTooKey" | |||
override def getSerializationFormat: DokkaConfiguration$SerializationFormat = DokkaConfiguration$SerializationFormat.JSON | |||
override def getSerializationFormat: DokkaConfiguration$SerializationFormat = | |||
DokkaConfiguration$SerializationFormat.JSON.asInstanceOf[DokkaConfiguration$SerializationFormat] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do need to cast here? Some change in dotty type interference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This became necessary when I bumped up Dotty compiler version to latest. I'm not sure if this is related to type inference, my first guess is that Kotlin was emitting some strange JVM bytecode and we changed how we translated that to Scala types. Another of the litany of things we should open an issue for in the Dotty repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will take a look
@@ -133,6 +133,37 @@ jobs: | |||
run: sbt ";sjsJUnitTests/test ;sjsCompilerTests/test" | |||
shell: cmd | |||
|
|||
build_docs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under impression that we want to have a dedicated fast running CI for scala3docs and having it as a step in main build will not be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it and I think something like what we have here is the cleanest way. build_docs
actually runs concurrently with other "jobs", so it finishes very quickly (in 4 minutes). We might still make it a separate workflow so that we can disable the primary workflow when only Scala3doc has changed.
@@ -1440,6 +1460,130 @@ object Build { | |||
settings(commonBenchmarkSettings). | |||
enablePlugins(JmhPlugin) | |||
|
|||
def asScala3doc: Project = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we should keep most of the settings in scala3doc/build.sbt, since if we move them to top level all changes here will requrie to run full community build (and we may not want that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's doable. The primary reason behind merging the two repos together is that we want their builds to be unified, so that Scala3doc can be easily compiled with latest compiler (which is practically necessary for quickly fixing compiler issues to help Scala3doc) and so that compiler sub-projects and community build projects can be easily documented with latest Scala3doc. I can't see any good way of doing that other than having Scala3doc be a sub-project here.
// Configuration for the doctool | ||
resolvers ++= Seq( | ||
Resolver.jcenterRepo, | ||
Resolver.bintrayRepo("kotlin", "kotlin-dev"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add those only if useScala3doc := true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll ask someone that understands how SBT works if that will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. I will try to make it work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was worried here about is when exactly the value of useScala3doc
is checked. It has to be checked after plugin users could have already set it, otherwise it will always be false. Also whatever we do here should work correctly with users adding their own resolvers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following will do what you want:
resolvers ++= {
if (useScala3doc.value) {
Seq(...)
} else Nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjrd Excellent, thanks for checking up on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will include that in #10092
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contents of docs where copied to
scala3doc/dotty-docs/docs. This makes it really hard to see any diffs. It would much simpler if we delete
docs` to make git handle these as file moves.
Scala3doc (name subject to change) is the documentation tool for | ||
[Dotty](https://github.com/lampepfl/dotty), which is scheduled to become | ||
Scala 3. It's based on [Dokka](https://github.com/Kotlin/dokka), the | ||
documentation tool for Kotlin. It uses Tasty-Reflect to access definitions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation tool for Kotlin. It uses Tasty-Reflect to access definitions, | |
documentation tool for Kotlin. It uses the TastyInspector to access definitions, |
@@ -0,0 +1,151 @@ | |||
--- | |||
layout: blog-page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we deleter the blog post file original blogpost file? This way git will consider it a move and we won't lose the history. Same for all other blog posts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't, these files are actually slightly different. We will do that when replacing Dottydoc as the tool for rendering the Dotty blog, which should be done soon-ish.
@@ -0,0 +1,32 @@ | |||
--- | |||
layout: main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete docs/blog/index.html
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as here: #10081 (comment)
self: TastyParser => | ||
import reflect._ | ||
|
||
object SymOps extends SymOps[reflect.type](reflect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is encoding what is described in #9530. We should improve extensions to support this use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was not happy when I needed to introduce Cake Patter that cause me so much pain in Scala 2
) | ||
|
||
/** Parses a single Tasty compilation unit. */ | ||
case class TastyParser(reflect: Reflection, inspector: DokkaBaseTastyInspector, config: DottyDokkaConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case class TastyParser(reflect: Reflection, inspector: DokkaBaseTastyInspector, config: DottyDokkaConfig) | |
case class TastyParser(qctx: QuoteContext, inspector: DokkaBaseTastyInspector, config: DottyDokkaConfig) |
We should always use the full path for reflection. Using Reflection
directly may not work in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case AppliedType(tpe, _) => inner(tpe) | ||
case tp @ TermRef(qual, typeName) => | ||
qual match | ||
case _: Type | _: NoPrefix => Some(tp.termSymbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case _: Type | _: NoPrefix => Some(tp.termSymbol) | |
case _: TypeRepr | _: NoPrefix => Some(tp.termSymbol) |
Need to rebase and rename reflection Type
to TypeRepr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result() | ||
} | ||
|
||
class InspectorDriver extends Driver: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to find a way to share this code. That can be done later.
override protected def newCompiler(implicit ctx: Context): Compiler = new TastyFromClass | ||
|
||
def run(filesToDocument: List[String])(implicit ctx: Context): Unit = | ||
doCompile(newCompiler, filesToDocument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What can filesToDocument
contain? .tasty
and .jar
file? If so it is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List of .tasty
files. We do support jars however we extract them to temporary directory and use extracted .tasy
files. I think we will change that once #10061 is ready
private val topLevels = Seq.newBuilder[Documentable] | ||
|
||
def processCompilationUnit(using ctx: quoted.QuoteContext)(root: ctx.reflect.Tree): Unit = | ||
val parser = new TastyParser(ctx.reflect, this, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should pass QuoteContext
rather than Reflection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
private val topLevels = Seq.newBuilder[Documentable] | ||
|
||
def processCompilationUnit(using ctx: quoted.QuoteContext)(root: ctx.reflect.Tree): Unit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def processCompilationUnit(using ctx: quoted.QuoteContext)(root: ctx.reflect.Tree): Unit = | |
def processCompilationUnit(using QuoteContext)(root: qctx.reflect.Tree): Unit = |
qctx
acts like the ctx
in the compiler. You have to import scala.quote._
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this relate to #10092? |
@bishabosha that one is this one rebased on top of Scala3doc's migrated Git history + some additional tweaks to the configuration + changes to the code which were already requested (such as removing Scala2 dependencies). |
@abgruszecki thank you for explaining, does this one need to remain open? |
|
Closing in favour of #10092. |
No description provided.