pre-2.13.15 regression: phase assembly related failure during Scaladoc generation in Scala.js repo

[SethTisue]

as reported to me by @sjrd

repro steps: clone scala-js/scala-js

apply this patch in order not to run afoul of https://github.com/scala/scala/pull/10727:

--- library/src/main/scala/scala/scalajs/js/Error.scala
+++ library/src/main/scala/scala/scalajs/js/Error.scala
@@ -46,7 +46,7 @@ object Error extends js.Object {
  */
 @js.native
 @JSGlobal
-class AggregateError(errors: js.Iterable[scala.Any], message: String = "") extends js.Error {
+class AggregateError(_errors: js.Iterable[scala.Any], message: String = "") extends js.Error {

then:

• start sbt
• set ThisBuild / resolvers += "integration" at "https://scala-ci.typesafe.com/artifactory/scala-integration/"
• set ThisBuild / cross213ScalaVersions += "2.13.15-bin-52b2013"
• library2_13/doc

and you get:

[info] Main Scala API documentation to /Users/tisue/scala-js/library/.2.13/target/scala-2.13/api...
[error] No phase `pickler` for jsinterop.runsBefore
[error] one error found
[error] stack trace is suppressed; run last library2_13 / Compile / doc for the full output
[error] (library2_13 / Compile / doc) Scaladoc generation failed

fyi @lrytz @som-snytt — looks like we need to do some further adjusting in the vein of https://github.com/scala/scala/pull/10807

Seb knows how to work around it for Scala.js in particular if he must, but I'm proposing we consider it a blocker for the release because other compiler plugins might be affected (we do have the most popular ones in the community build, but lord knows what else is out there) and because it actually is a bug on our end

[SethTisue]

as for how we didn't notice this until so late,

• Scala.js isn't in the Scala 2 community build, not even a subset. Without sinking too much time into it, I will try again to add enough of it for this sort of problem to turn up. (Perhaps I'll fail again; I know I've tried before, but it was a long time ago.)
• Scala.js does run their CI against Scala 2 nightlies, but GitHub Actions eventually stops doing those runs if there haven't been any commits on that branch in the Scala.js repo, and that's what happened here

regardless, at least we caught it during our RC testing period!

[lrytz]

I'd suggest to pull out the list here

https://github.com/scala/scala/blob/v2.13.15-M1/src/compiler/scala/tools/nsc/Global.scala#L702-L725

to make it accessible to ScaladocGlobal and then generate generate phase "sentinels" (like in https://github.com/scala/scala/pull/10807) for all of them.

[lrytz]

... or maybe: make the phase assembly aware of being in scaladoc and drop the phases with bad constraints? what happens in 2.13.14?

[sjrd]

Scala 2.13.14 and all previous releases seem to ignore the bad constraints themselves, but not the phases.

In the particular case of jsinterop in Scala.js, we actually want that phase to run for Scaladoc. (ideally) This is what happens with the current nightly if I remove the bad constraint myself.

[som-snytt]

Today I'll try the lrytz suggestion of "add all the phase names", perhaps also names referenced by plugins, in scaladoc. I specifically did not want to burden build files with turning off plugins that reference absent phases. "Syntax" phases matter for scaladoc 2.

(I knew about the pickler constraint but did not think of it later. I'll also try "ignore spurious constraints under scaladoc", which is what we're really saying. The goal is to avoid warning, but it could still warn for typers [sic], if I'm inclined to add "did you mean".)

[SethTisue]

ignore spurious constraints under scaladoc

Is there any possibility that ignoring a spurious constraint could cause the phase to land in some unexpected and wrong place, a different place than where it landed during normal compilation?

[som-snytt]

Scaladoc 2 only runs thru typer, so any other phase is irrelevant.

Either the phase name is misspelled, or the phase is dropped by scaladoc. To run before or after a dropped phase means run after typer.

For scaladoc 2, I think it's reasonable even to assume no misspellings because compilation succeeds.

[sjrd]

It seems though that so far the jsinterop phase has been running between typer and whatever Scaladoc is doing, because the effects of the phase can be seen in the docs. Is that something we can guarantee/declare a constraint for?

[som-snytt]

Yes, I'll add a test. I misspoke and should have said Scaladoc only constructs phases through typer, but runs whatever winds up in the slurry of phases, including plugins. (IIRC it used to contribute backend phases that were omitted from the phase assembly.

I'll check if I fixed this nuisance:

➜  ~ scaladoc -Vphases
warning: Phases are restricted when using Scaladoc

)

[lrytz]

apply this patch in order not to run afoul of scala/scala#10727:

This change was moved behind -Xsource (https://github.com/scala/scala/pull/10846), so the patch should not be needed. Unless I missed something, Scala.js is not using -Xsource:3 and this is another bug we should address before the release.

Other observations

• The No phase `pickler` for jsinterop.runsBefore message is a warning, scala-js uses -Xfatal-warnings
• The constraint was misspelled until recently (https://github.com/scala-js/scala-js/commit/aa84f641c653fbbfc1b14378329cdf89cf1c5564) - it's worth comparing 2.13.14 and 2.13.15 for this case
• Scaladoc does run all assembled phases over the sources: https://github.com/scala/scala/blob/v2.13.15-M1/src/scaladoc/scala/tools/nsc/doc/DocFactory.scala#L54

I'm playing around with a simple hello world plugin

• val runsAfter = List("typer") works as expected in 2.13.14 and current 2.13.x, both scalac and scaladoc
• val runsAfter = List("pickler") works in scalac 2.13.14 and 2.13.x
  • scaladoc 2.13.14 says dropping dependency on node with no phase object: pickler and doesn't run the phase
  • scaladoc 2.13.x says No phase `pickler` for el-plugin.runsAfter. The phase ends up at the top, before parser (consistent with -Vphases)
• val runsAfter = List("typo"): 2.13.14 scalac&scaladoc warn and drop the phase. 2.13.x scalac&scaladoc warn and insert the phase at the beginning - is that intentional?

I'm also wondering

• From the hello world test, it seems 2.13.14 drops phases with bad constraints. Didn't that affect Scala.js before the typo was fixed, is the phase not needed, or did it somehow run anyway?
• 2.13.14 warns when encountering a bad constraint and dropping a phase. Why does that warning not show up in the Scala.js build (when running library2_13/doc), or for Scala.js users?

[sjrd]

The No phase `pickler` for jsinterop.runsBefore message is a warning, scala-js uses -Xfatal-warnings

Ah that's good to know. I had not realized because usually warnings-turned-errors are presented as [warning] (in red) then "no warning can be ... under -Xfatal-warnings" is emitted.

Although the same issue applies for all users of Scala.js, and that includes libraries who are likely building with -Xfatal-warnings as well.

The constraint was misspelled until recently (scala-js/scala-js@aa84f64) - it's worth comparing 2.13.14 and 2.13.15 for this case

Yes. The report above is while using the new, correct spelling. With the incorrect spelling everything was broken, not just Scaladoc.

From the hello world test, it seems 2.13.14 drops phases with bad constraints. Didn't that affect Scala.js before the typo was fixed, is the phase not needed, or did it somehow run anyway?

Hum, I don't think I specifically tried that. I checked with 2.12, though, and it did not drop the phase. You can on the published doc at https://www.scala-js.org/api/scalajs-library/latest/scala/scalajs/js/Any.html that there is the @JSType annotation mentioned. That annotation is added by the jsinterop phase, so it's evidence that the phase was run.

2.13.14 warns when encountering a bad constraint and dropping a phase. Why does that warning not show up in the Scala.js build (when running library2_13/doc), or for Scala.js users?

That is very surprising. I don't know how Scala.js users wouldn't have run into this.

[som-snytt]

I fixed the case of the unconstrained phase scheduled before the initial phase; and also with multiple nonexistent phases, it currently throws (wrongly); the minimal constraint is after initial or before terminal, but that was not enforced in that case. As usual, the hard part was writing the test.

[lrytz]

This change was moved behind -Xsource (scala/scala#10846), so the patch should not be needed. Unless I missed something, Scala.js is not using -Xsource:3 and this is another bug we should address before the release.

Submitted https://github.com/scala/scala/pull/10855, @som-snytt

[lrytz]

To answer my questions:

• 2.13.14 drops phases with bad constraints. Didn't that affect Scala.js before the typo was fixed?

No because the typo was in runsBefore. 2.13.14 silently ignores invalid runsBefore constraints.

• 2.13.14 warns when encountering a bad constraint and dropping a phase. Why does that warning not show up in the Scala.js build (when running library2_13/doc)

Again, because the missing "pickler" phase (with correct spelling) is in the runsBefore constraint.