Snippet compiler #11582

pikinier20 · 2021-03-03T10:37:21Z

No description provided.

romanowski · 2021-03-03T18:07:39Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetCompiler.scala

+  target: AbstractFile = new VirtualDirectory("(memory)")
+):
+
+  private def newDriver: InteractiveDriver = {


In future we can probably stop after the given phases since we probably do not need to produce bytecode.

romanowski · 2021-03-03T18:47:53Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetCompiler.scala

+
+  def compile(
+    snippets: List[String]
+  ): Either[SnippetCompilerException, AbstractFile] = {


What about warning from successfull compilation?

I guess we will need to return something like SnippetCompilationResult:

case class SnippetCompilationResult(result: Option[AbstractFile], messages: Seq[SnippetCompilerMessage])

romanowski · 2021-03-03T18:52:06Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetChecker.scala

+):
+  def checkSnippet(snippet: String) = {
+    val wrapped = wrapper.wrap(snippet)
+    compiler.compile(snippet)


we should compile wrapped, shouldn't we?

romanowski · 2021-03-22T11:33:06Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetChecker.scala

+  private val sep = System.getProperty("path.separator")
+  private val cp = System.getProperty("java.class.path") + sep +
+      Paths.get(ctx.args.classpath).toAbsolutePath + sep +
+      ctx.args.tastyDirs.map(_.getAbsolutePath()).mkString(sep)


(Seq(System.getProperty("java.class.path"), Paths.get(ctx.args.classpath).toAbsolutePath) ++ ctx.args.tastyDirs.map(_.getAbsolutePath())).mkString(sep)

romanowski · 2021-03-22T11:35:41Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetChecker.scala

+  }
+
+  def summary: String = s"""
+  |Snippet compiler summary:


formatting is strange

romanowski · 2021-03-22T11:35:54Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetChecker.scala

+      ctx.args.tastyDirs.map(_.getAbsolutePath()).mkString(sep)
+  private val compiler: SnippetCompiler = SnippetCompiler(classpath = cp)
+  private val wrapper: SnippetWrapper = SnippetWrapper()
+  var warningsCount = 0


it should be private

romanowski · 2021-03-22T11:36:05Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetCompilationResult.scala

+package dotty.tools.scaladoc
+package snippets
+
+import dotty.tools.io.{ AbstractFile }


remove braces

romanowski · 2021-03-22T11:37:02Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetCompiler.scala

+import dotty.tools.dotc.interfaces.Diagnostic._
+
+class SnippetCompiler(
+  classpath: String = System.getProperty("java.class.path"), //Probably needs to be done better


Do we need that default here? it looks a bit denegerous.

b-studios · 2021-04-27T09:37:55Z

Hey @pikinier20, is this PR ready to be reviewed again?

pikinier20 · 2021-04-27T09:56:25Z

Hey @pikinier20, is this PR ready to be reviewed again?

Yes, it is, apart from the fact that I need to resolve conflicts.

tdudzik · 2021-04-27T12:34:13Z

scaladoc/src/dotty/tools/scaladoc/tasty/comments/markdown/SnippetRenderer.scala

+    case MessageLevel.Error => "snippet-error"
+    case MessageLevel.Debug => "snippet-debug"
+
+  private def cutBetwenSymbols[A](


Suggested change

private def cutBetwenSymbols[A](

private def cutBetweenSymbols[A](

smarter · 2021-05-17T10:24:19Z

project/Build.scala

      // TODO add versions etc.
      def srcManaged(v: String, s: String) = s"out/bootstrap/stdlib-bootstrapped/scala-$v/src_managed/main/$s-library-src"
      def scalaSrcLink(v: String, s: String) = s"-source-links:$s=github://scala/scala/v$v#src/library"
      def dottySrcLink(v: String, s: String) = s"-source-links:$s=github://lampepfl/dotty/$v#library/src"
+      def bootclasspath: Seq[String] = if(addBootclasspath) Seq(
+        "-bootclasspath",


Why do you need a separate bootclasspath given these libraries will be on the classpath anyway?

We need to somehow pass path to standard library to the snippet compiler, otherwise we get MissingCoreLibraryException. I've seen that sbt in task doc automatically passes this path in bootclasspath arg so I added support for this arg in scaladoc. Then I realized that in our sbt project we don't pass explicit bootclasspath to generateDocumentation task and snippet compiler didn't work. Previously I wanted to just pass System.getProperty("java.class.path") but it led to errors because we got sbt libs on classpath.

OK, but you should be able to use the regular -classpath and not necessarily -bootclasspath (which isn't really needed anymore)

But -classpath doesn't contain or at least it didn't contain stdlib entry when this was implemented and without stdlib entry I can't instantiate Context. Or maybe is there other way to obtain path to stdlib?

Which -classpath? It looks like -classpath isn't passed at all to scaladoc here, so I was just suggesting using that instead of -bootclasspath to pass the stdlib

Actually maybe the issue is in the scaladoc shell script? Unlike the scalac shell scrpt it doesn't pass the -Dscala.usejavacp=true flag so that the JVM classpath is available to the compiler: https://github.com/lampepfl/dotty/blob/83e17f110bfaa2fe876bacc90eca208f23930f98/dist/bin/scalac#L77-L78 vs https://github.com/lampepfl/dotty/blob/83e17f110bfaa2fe876bacc90eca208f23930f98/dist/bin/scaladoc#L129

that's a good point, gonna check that

smarter · 2021-05-17T10:24:59Z

scaladoc/src/dotty/tools/scaladoc/ScaladocSettings.scala

+    MultiStringSetting("-snippet-compiler", "snippet-compiler", snippets.SnippetCompilerArgs.usage)
+
+  val snippetCompilerDebug: Setting[Boolean] =
+    BooleanSetting("-snippet-compiler-debug", snippets.SnippetCompilerArgs.debugUsage, false)


Flags which are for debugging purpose only and not intended for others to use should start with -Y

I wasn't aware of this. Thanks!

smarter · 2021-05-20T16:24:46Z

scaladoc/src/dotty/tools/scaladoc/DocContext.scala

+  lazy val snippetChecker = snippets.SnippetChecker(
+    args.classpath, 
+    args.bootclasspath, 
+    args.tastyFiles, 
+    compilerContext.settings.scalajs.value(using compilerContext),
+    compilerContext.settings.usejavacp.value(using compilerContext)
+  )


There might be more settings needed to compile snippets (for example, if a compiler plugin is used), and adding them one by one isn't going to scale, could you pass all of compilerContext.settings instead?

Yes, you're right and ofc I can do that, but I'm afraid of passing options that can totally break snippet compiler. I don't have knowledge of all possible settings but e.g. what will the compiler do with -from-tasty setting?

what will the compiler do with -from-tasty setting?

It will unpickle the .tasty files and compile them to .class files.

smarter · 2021-05-23T17:15:25Z

dist/bin/scaladoc

@@ -127,7 +127,6 @@ eval "\"$JAVACMD\"" \
     ${JAVA_OPTS:-$default_java_opts} \
     "${java_args[@]}" \
     "${jvm_cp_args-}" \
-     -Dscala.usejavacp=true \


Any particular reason for removing this flag? It's in dist/bin/scala and dist/bin/scalac, so it makes sense to have it here too.

Yes, I removed this flag intentionally. It helped me a lot and made most of things working but then I realized that with this flag set, generating standard library documentation is failing. I'm not sure about the exact reason but I believe it's because we've got then two standard libraries on classpath and there was some conflict between them. Instead of setting the flag there, I set it directly in tasks that needed it.

Ok, but that means the scaladoc binary isn't really usable for external users. I believe the best solution is to not call the scaladoc script from Build.scala at all. Instead you should be able to run the scaladoc main class from the sbt project directly: (Compile / runMain).toTask("-foo -bla -bar")

Caution ! scaladoc binary MUST BE usable for external users, as are javadoc and haddock for instance.

So probably I need to implement @smarter solution. The funny part is that the -usejavacp option is not there on master, I added it some commits before in this PR, but now I realized it breaks doc generation for one task.

Also, can you provide me more information about usage of this binary? Let's say I've got a example scala3 project and I want to call scaladoc on it without sbt. Where should I look for binary and how to use it?

smarter · 2021-05-23T17:23:35Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetChecker.scala

+  private val snippetCompilerSettings: Seq[SnippetCompilerSetting[_]] = cctx.settings.userSetSettings(cctx.settingsState).filter(_ != cctx.settings.classpath).map( s =>
+    SnippetCompilerSetting(s, s.valueIn(cctx.settingsState))
+  ) :+ SnippetCompilerSetting(cctx.settings.classpath, fullClasspath)


What is the purpose of SnippetCompilerSetting? If it's just storing settings it seems that the existing SettingsState would suffice and you could write something like :

private val snippetCompilerSettings = cctx.settings.classpath.updateIn(cctx.settingsState, fullClasspath)

If we change classpath of compiler context used to load TASTY to produce documentation from, won't it affect loading process?

Since updateIn returns a new SettingsState, I was expecting it to not mutate the existing settingsState of the compiler context, but in fact it's slightly more tricky than that, it will be mutated if no one tried to read a setting previously: https://github.com/lampepfl/dotty/blob/e2e77b56d012b00faf16a86ad1c8437404bb0812/compiler/src/dotty/tools/dotc/config/Settings.scala#L36-L40 (this is a really bad idea and that code that should be replaced)

smarter · 2021-05-23T17:29:21Z

scaladoc/src/dotty/tools/scaladoc/snippets/SnippetCompiler.scala

+      rootCtx.setSetting(rootCtx.settings.YretainTrees, true)
+      rootCtx.setSetting(rootCtx.settings.YcookComments, true)
+      rootCtx.setSetting(rootCtx.settings.YreadComments, true)
+      val defaultFlags =
+        List("-color:never", "-unchecked", "-deprecation", "-Ximport-suggestion-timeout", "0")


Some settings are set with setSetting and some are strings passed to setup, it would be nicer and more type-safe to always use setSetting:

rootCtx.setSetting(rootCtx.settings.color, "never") .setSetting(...) // etc

Also it seems weird to explicitly enable -unchecked and -deprecation here, why those settings in particular? If the user wants them they can add them in scalacOptions themselves.

The reason behind exactly these flags is that mdoc uses them. But I think it's OK to remove them and let user choose.

… renderers

…t scaladoc compiler

…o generateDocumentation task

smarter · 2021-05-31T21:08:15Z

project/Build.scala

@@ -1243,7 +1243,7 @@ object Build {
      libraryDependencies += ("org.scala-js" %%% "scalajs-dom" % "1.1.0").cross(CrossVersion.for3Use2_13)
    )

-  def generateDocumentation(targets: Seq[String], name: String, outDir: String, ref: String, params: Seq[String] = Nil) =
+  def generateDocumentation(targets: Seq[String], name: String, outDir: String, ref: String, params: Seq[String] = Nil, usingScript: Boolean = true) =


This can be fixed later but do you know why we need usingScript in some situations and couldn't just always use Compile/run ?

It was my idea to have some coverage for scripts. Beside that there are no blockers to switch all cases to running Compile/run

I see, for consistency I suggest always using Compile/run and instead test the scaladoc binary from https://github.com/lampepfl/dotty/blob/master/project/scripts/cmdTests

@pikinier20 can we change that in follow up PR?

@pikinier20 can we change that in follow up PR?

Sure

romanowski requested changes Mar 3, 2021

View reviewed changes

pikinier20 force-pushed the snippet-compiler branch from 46208b0 to a8a39f8 Compare March 5, 2021 10:09

pikinier20 force-pushed the snippet-compiler branch 3 times, most recently from 853317f to 04ec4cb Compare March 22, 2021 08:49

pikinier20 marked this pull request as ready for review March 22, 2021 08:49

romanowski approved these changes Mar 22, 2021

View reviewed changes

pikinier20 changed the title ~~Snippet compiler PoC~~ Snippet compiler Mar 22, 2021

pikinier20 force-pushed the snippet-compiler branch from 70d5815 to 61a74c5 Compare March 26, 2021 15:32

b-studios assigned pikinier20 Apr 27, 2021

tdudzik reviewed Apr 27, 2021

View reviewed changes

pikinier20 force-pushed the snippet-compiler branch from e3f171a to 2d4e88b Compare April 29, 2021 12:37

pikinier20 requested a review from romanowski April 30, 2021 06:55

pikinier20 force-pushed the snippet-compiler branch 3 times, most recently from 799e543 to 2d1d670 Compare May 17, 2021 09:54

smarter reviewed May 17, 2021

View reviewed changes

smarter reviewed May 20, 2021

View reviewed changes

pikinier20 force-pushed the snippet-compiler branch 2 times, most recently from 6c1086c to 01dacd4 Compare May 21, 2021 12:12

pikinier20 requested a review from smarter May 21, 2021 13:59

smarter requested changes May 23, 2021

View reviewed changes

smarter reviewed May 23, 2021

View reviewed changes

BarkingBad linked an issue May 26, 2021 that may be closed by this pull request

Scaladoc text breaking bug for narrow screens #11613

Closed

pikinier20 added 3 commits May 31, 2021 09:16

Snippet compiler PoC

f7c8f52

Reporter fix. Message levels

80a4014

Fix snippet wrapper. Integrate snippet checker with Wiki and Markdown…

825f6c3

… renderers

KacperFKorban and others added 20 commits May 31, 2021 09:16

Apply review suggestions from PR scala#11822

82dad66

Add returning real line of error in source file of snippet for snippe…

b1cc5da

…t scaladoc compiler

Add failing flag

56e8b0c

Add debug flag.

b3b7d0b

Add addition operation for snippet compiler arg

78676bc

Refactor snippet compiler args

2fe91f3

Move snippet compiler logic to parsing phase.

e8a5df9

Refactor debug flag

ac76ac6

Display snippet compilation messages in scaladoc

94f77cb

Apply review changes

bcea268

Run snippet compiler on static sites

8ddb298

Further enhancements of snippet contexts

3e8995c

Filter non-scala snippets. Report errors in parsing override flags

d27e377

Fix positions in static sites. Enchance snippet wrapping

bb07b46

CRs

30eec69

Add and update tests for snippet compiler. Bugfixes found during testing

d0bf391

Wrap compiler messages into Try block to avoid lazy exceptions

3b0e11e

Update branch

975065b

Fix classpath problems. Add support for scala.js. Add bootclasspath t…

7d0e477

…o generateDocumentation task

Temporarily turn off snippet checking for scala3 library

ac8531d

pikinier20 force-pushed the snippet-compiler branch from 5c7a83d to 263b484 Compare May 31, 2021 21:04

pikinier20 requested a review from smarter May 31, 2021 21:04

smarter approved these changes May 31, 2021

View reviewed changes

romanowski approved these changes Jun 1, 2021

View reviewed changes

Change requests from reviews

946752f

pikinier20 force-pushed the snippet-compiler branch from 263b484 to 946752f Compare June 2, 2021 07:57

pikinier20 enabled auto-merge June 2, 2021 08:14

pikinier20 merged commit 5ac62a1 into scala:master Jun 2, 2021

raboof mentioned this pull request Jul 13, 2021

DottyDoc doesn't take into account '-release' to set the bootclasspath #13061

Closed

Kordyjan added this to the 3.0.2 milestone Aug 2, 2023

	private def cutBetwenSymbols[A](
	private def cutBetweenSymbols[A](

Snippet compiler #11582

Snippet compiler #11582

Conversation

pikinier20 commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-studios commented Apr 27, 2021

pikinier20 commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pikinier20 May 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pikinier20 May 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanowski Jun 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pikinier20 May 17, 2021 •

edited

pikinier20 May 24, 2021 •

edited

romanowski Jun 1, 2021 •

edited