Merge pull request scala#10858 from lrytz/pr10856light

Relax PhaseAssembly about loose constraints

src/compiler/scala/tools/nsc/PhaseAssembly.scala

@@ -33,7 +33,8 @@ trait PhaseAssembly {
       phasesSet.add(terminal)
       reporter.warning(NoPosition, "Added default terminal phase")
     }
-    val graph = DependencyGraph(phasesSet)
+    val warn = !settings.isScaladoc || settings.isDebug || settings.showPhases.value
+    val graph = DependencyGraph(phasesSet, warn)
     for (n <- settings.genPhaseGraph.valueSetByUser; d <- settings.outputDirs.getSingleOutput if !d.isVirtual)
       DependencyGraph.graphToDotFile(graph, Path(d.file) / File(s"$n.dot"))
     graph.compilerPhaseList().tap(_ => graph.warnings.foreach(msg => reporter.warning(NoPosition, msg)))
@@ -84,8 +85,10 @@ class DependencyGraph(order: Int, start: String, val components: Map[String, Sub
     }
   }
 
-  // input must be acyclic and only one FollowsNow edge is allowed between a pair of vertices
-  private def validate(): Unit = {
+  /** Find unreachable vertices.
+   *  Input must be acyclic and a vertex can have only one outgoing FollowsNow edge.
+   */
+  private def validate(warn: Boolean): Set[String] = if (order == 1) Set.empty else {
     def checkFollowsNow(v: Int): Unit =
       adjacency(v).foldLeft(-1) { (w, e) =>
         if (e.weight != FollowsNow) w
@@ -122,6 +125,10 @@ class DependencyGraph(order: Int, start: String, val components: Map[String, Sub
       }
     }
     walk()
+    names.iterator.zipWithIndex.collect { case (n, i) if !seen(i) =>
+      if (warn) warning(s"Dropping phase ${names(i)}, it is not reachable from $start")
+      n
+    }.toSet
   }
 
   def compilerPhaseList(): List[SubComponent] = if (order == 1) List(components(start)) else {
@@ -210,7 +217,6 @@ class DependencyGraph(order: Int, start: String, val components: Map[String, Sub
         }
       }
     }
-    validate()
     relax()
     traverse()
   }
@@ -229,34 +235,41 @@ object DependencyGraph {
    *  A component must be declared as "initial".
    *  If no phase is "initial" but a phase is named "parser", it is taken as initial.
    *  If no phase is "terminal" but a phase is named "terminal", it is taken as terminal.
-   *  Empty constraints are ignored.
+   *  Warnings are issued for invalid constraints (runsAfter / runsRightAfter / runsBefore) if `warn` is true.
+   *  Components without a valid "runsAfter" or "runsRightAfter" are dropped with an "unreachable" warning.
    */
-  def apply(phases: Iterable[SubComponent]): DependencyGraph = {
+  def apply(phases: Iterable[SubComponent], warn: Boolean = true): DependencyGraph = {
     val start = phases.find(_.initial)
       .orElse(phases.find(_.phaseName == Parser))
       .getOrElse(throw new AssertionError("Missing initial component"))
     val end = phases.find(_.terminal)
       .orElse(phases.find(_.phaseName == Terminal))
       .getOrElse(throw new AssertionError("Missing terminal component"))
-    new DependencyGraph(phases.size, start.phaseName, phases.map(p => p.phaseName -> p).toMap).tap { graph =>
-      for (p <- phases) {
-        require(p.phaseName.nonEmpty, "Phase name must be non-empty.")
-        def checkConstraint(name: String, constraint: String): Boolean =
-          graph.components.contains(name).tap(ok => if (!ok) graph.warning(s"No phase `$name` for ${p.phaseName}.$constraint"))
-        for (after <- p.runsRightAfter if after.nonEmpty && checkConstraint(after, "runsRightAfter"))
-          graph.addEdge(after, p.phaseName, FollowsNow)
-        for (after <- p.runsAfter if after.nonEmpty && !p.runsRightAfter.contains(after) && checkConstraint(after, "runsAfter"))
-          graph.addEdge(after, p.phaseName, Follows)
-        for (before <- p.runsBefore if before.nonEmpty && checkConstraint(before, "runsBefore"))
-          graph.addEdge(p.phaseName, before, Follows)
-        if (p != start && p != end)
-          if (p.runsRightAfter.forall(_.isEmpty) && p.runsAfter.forall(_.isEmpty))
-            graph.addEdge(start.phaseName, p.phaseName, Follows)
-        if (p != end || p == end && p == start)
-          if (p.runsBefore.forall(_.isEmpty))
-            graph.addEdge(p.phaseName, end.phaseName, Follows)
+    val graph = new DependencyGraph(phases.size, start.phaseName, phases.map(p => p.phaseName -> p).toMap)
+    def phaseTypo(name: String) =
+      if (graph.components.contains(name)) ""
+      else graph.components.keysIterator.filter(util.EditDistance.levenshtein(name, _) < 3).toList match {
+        case Nil => ""
+        case close => s" - did you mean ${util.StringUtil.oxford(close, "or")}?"
       }
+    for (p <- phases) {
+      require(p.phaseName.nonEmpty, "Phase name must be non-empty.")
+      def checkConstraint(name: String, constraint: String): Boolean =
+        graph.components.contains(name).tap(ok => if (!ok && warn) graph.warning(s"No phase `$name` for ${p.phaseName}.$constraint${phaseTypo(name)}"))
+      for (after <- p.runsRightAfter if after.nonEmpty && checkConstraint(after, "runsRightAfter"))
+        graph.addEdge(after, p.phaseName, FollowsNow)
+      for (after <- p.runsAfter if after.nonEmpty && !p.runsRightAfter.contains(after) && checkConstraint(after, "runsAfter"))
+        graph.addEdge(after, p.phaseName, Follows)
+      for (before <- p.runsBefore if before.nonEmpty && checkConstraint(before, "runsBefore"))
+        graph.addEdge(p.phaseName, before, Follows)
+      // Add "runsBefore terminal" to phases without (or with invalid) runsBefore
+      if (p != end || p == end && p == start)
+        if (!p.runsBefore.exists(graph.components.contains))
+          graph.addEdge(p.phaseName, end.phaseName, Follows)
     }
+    val unreachable = graph.validate(warn)
+    if (unreachable.isEmpty) graph
+    else apply(phases.filterNot(p => unreachable(p.phaseName)), warn).tap(res => graph.warnings.foreach(res.warning))
   }
 
   /** Emit a graphviz dot file for the graph.

src/partest/scala/tools/partest/DirectTest.scala

@@ -53,12 +53,15 @@ abstract class DirectTest {
   // a default Settings object using only extraSettings
   def settings: Settings = newSettings(tokenize(extraSettings))
   // settings factory using given args and also debug settings
-  def newSettings(args: List[String]) = (new Settings).tap { s =>
-    val allArgs = args ++ tokenize(debugSettings)
+  def newSettings(args: List[String]): Settings = newBaseSettings().tap { s =>
+    val allArgs = debugSettings.pipe(db => if (db.isEmpty) args else args ++ tokenize(db))
     log(s"newSettings: allArgs = $allArgs")
     val (success, residual) = s.processArguments(allArgs, processAll = false)
     assert(success && residual.isEmpty, s"Bad settings [${args.mkString(",")}], residual [${residual.mkString(",")}]")
   }
+  // scaladoc has custom settings
+  def newBaseSettings(): Settings = new Settings
+
   // new compiler using given ad hoc args, -d and extraSettings
   def newCompiler(args: String*): Global = {
     val settings = newSettings(tokenize(s"""-d "${testOutput.path}" ${extraSettings}""") ++ args.toList)

src/partest/scala/tools/partest/ScaladocModelTest.scala

@@ -12,13 +12,13 @@
 
 package scala.tools.partest
 
-import scala.sys.process.{Parser => CommandLineParser}
 import scala.tools.nsc._
+import scala.tools.nsc.doc.{DocFactory, Universe}
 import scala.tools.nsc.doc.base.comment._
 import scala.tools.nsc.doc.model._
 import scala.tools.nsc.doc.model.diagram._
-import scala.tools.nsc.doc.{DocFactory, Universe}
 import scala.tools.nsc.reporters.ConsoleReporter
+import scala.util.chaining._
 
 /** A class for testing scaladoc model generation
  *   - you need to specify the code in the `code` method
@@ -87,20 +87,21 @@ abstract class ScaladocModelTest extends DirectTest {
 
   private[this] var docSettings: doc.Settings = null
 
+  // custom settings, silencing "model contains X documentable templates"
+  override def newBaseSettings(): doc.Settings = new doc.Settings(_ => ()).tap(_.scaladocQuietRun = true)
+  override def newSettings(args: List[String]): doc.Settings = super.newSettings(args).asInstanceOf[doc.Settings]
+  override def settings: doc.Settings = newSettings(tokenize(s"$extraSettings $scaladocSettings"))
+
   // create a new scaladoc compiler
   def newDocFactory: DocFactory = {
-    docSettings = new doc.Settings(_ => ())
-    docSettings.scaladocQuietRun = true // yaay, no more "model contains X documentable templates"!
-    val args = extraSettings + " " + scaladocSettings
-    new ScalaDoc.Command((CommandLineParser tokenize (args)), docSettings) // side-effecting, I think
-    val docFact = new DocFactory(new ConsoleReporter(docSettings), docSettings)
-    docFact
+    docSettings = settings
+    new DocFactory(new ConsoleReporter(docSettings), docSettings)
   }
 
   // compile with scaladoc and output the result
   def model: Option[Universe] = newDocFactory.makeUniverse(Right(code))
 
-  // finally, enable easy navigation inside the entities
+  // enable easy navigation inside the entities
   object access {
 
     implicit class TemplateAccess(tpl: DocTemplateEntity) {

src/partest/scala/tools/partest/nest/DirectCompiler.scala

@@ -55,9 +55,8 @@ class DirectCompiler(val runner: Runner) {
 
 
   /** Massage args to merge plugins and fix paths.
-   *  Plugin path can be relative to test root, or cwd is out.
-   *  While we're at it, mix in the baseline options, too.
-   *  That's how ant passes in the plugins dir.
+   *  Plugin path can be relative to test root, or cwd (".") means use output dir and copy scalac-plugin.xml there.
+   *  Mix in the baseline options from the suiteRunner (scalacOpts, scalacExtraArgs).
    */
   private def updatePluginPath(args: List[String], out: AbstractFile, srcdir: AbstractFile): Seq[String] = {
     val dir = runner.suiteRunner.pathSettings.testRoot
@@ -87,6 +86,11 @@ class DirectCompiler(val runner: Runner) {
 
     runner.suiteRunner.scalacExtraArgs ++ filteredOpts ++ others ++ Xplugin
   }
+  private def updatePluginPath(args: List[String]): Seq[String] = {
+    import runner.testInfo.testFile
+    val srcDir = if (testFile.isDirectory) testFile else Path(testFile).parent.jfile
+    updatePluginPath(args, AbstractFile.getDirectory(runner.outDir), AbstractFile.getDirectory(srcDir))
+  }
 
   def compile(opts0: List[String], sources: List[File]): TestState = {
     import runner.{sources => _, _}
@@ -104,8 +108,7 @@ class DirectCompiler(val runner: Runner) {
 
     val testSettings = new TestSettings(FileManager.joinPaths(classPath), s => parseArgErrors += s)
     val logWriter    = new FileWriter(logFile)
-    val srcDir       = if (testFile.isDirectory) testFile else Path(testFile).parent.jfile
-    val opts         = updatePluginPath(opts0, AbstractFile.getDirectory(outDir), AbstractFile.getDirectory(srcDir))
+    val opts         = updatePluginPath(opts0)
     val command      = new CompilerCommand(opts.toList, testSettings)
     val reporter     = ExtConsoleReporter(testSettings, new PrintWriter(logWriter, true))
     val global       = newGlobal(testSettings, reporter)

src/scaladoc/scala/tools/nsc/doc/ScaladocGlobal.scala

@@ -54,8 +54,6 @@ class ScaladocGlobal(settings: doc.Settings, reporter: Reporter) extends Global(
     phasesSet += analyzer.namerFactory
     phasesSet += analyzer.packageObjects
     phasesSet += analyzer.typerFactory
-    phasesSet += patmatSentinel
-    phasesSet += erasureSentinel
     phasesSet += terminal
   }
 
@@ -65,27 +63,6 @@ class ScaladocGlobal(settings: doc.Settings, reporter: Reporter) extends Global(
       override def platformPhases = Nil // used by computePlatformPhases
     }
 
-  // Placeholders for plugins who wish to declare runsBefore patmat or erasure.
-  // A bit deceptive for plugins that run after them, as scaladoc ought to -Ystop-before:patmat
-  lazy val patmatSentinel: SubComponent = new { val global = self } with SubComponent {
-    val phaseName = "patmat"
-    val runsAfter = "typer" :: Nil
-    val runsRightAfter = None
-    def newPhase(prev: Phase): Phase = new Phase(prev) {
-      val name = phaseName
-      def run() = ()
-    }
-  }
-  lazy val erasureSentinel: SubComponent = new { val global = self } with SubComponent {
-    val phaseName = "erasure"
-    val runsAfter = "patmat" :: Nil
-    val runsRightAfter = None
-    def newPhase(prev: Phase): Phase = new Phase(prev) {
-      val name = phaseName
-      def run() = ()
-    }
-  }
-
   override def createJavadoc = if (settings.docNoJavaComments.value) false else true
 
   override lazy val analyzer =

test/files/neg/t7494-before-parser.check

@@ -1 +1,7 @@
-fatal error: Phases form a cycle: parser -> beforeparser -> parser
+warning: Dropping phase beforeparser, it is not reachable from parser
+sample_2.scala:8: error: type mismatch;
+ found   : String("")
+ required: Int
+  def f: Int = ""
+               ^
+1 error

test/files/neg/t7494-before-parser/sample_2.scala

@@ -3,4 +3,7 @@ package sample
 
 // just a sample that is compiled with the sample plugin enabled
 object Sample extends App {
+  // because `-Werror` doesn't work; after phase assembly warnings are issued,
+  // Run.compileUnits resets the reporter (and its warning count)
+  def f: Int = ""
 }

test/files/neg/t8755-regress-a.check

@@ -0,0 +1,28 @@
+    phase name  id  description
+    ----------  --  -----------
+        parser   1  parse source into ASTs, perform simple desugaring
+         namer   2  resolve names, attach symbols to named trees
+packageobjects   3  load package objects
+         typer   4  the meat and potatoes: type the trees
+            C8   0  C8 makes C7 reachable
+superaccessors   6  add super accessors in traits and nested classes
+            C7   0  C7 has only a before constraint
+    extmethods   8  add extension methods for inline classes
+       pickler   9  serialize symbol tables
+     refchecks  10  reference/override checking, translate nested objects
+        patmat  11  translate match expressions
+       uncurry  12  uncurry, translate function values to anonymous classes
+        fields  13  synthesize accessors and fields, add bitmaps for lazy vals
+     tailcalls  14  replace tail calls by jumps
+    specialize  15  @specialized-driven class and method specialization
+ explicitouter  16  this refs to outer pointers
+       erasure  17  erase types, add interfaces for traits
+   posterasure  18  clean up erased inline classes
+    lambdalift  19  move nested functions to top level
+  constructors  20  move field definitions into constructors
+       flatten  21  eliminate inner classes
+         mixin  22  mixin composition
+       cleanup  23  platform-specific cleanups, generate reflective calls
+    delambdafy  24  remove lambdas
+           jvm  25  generate JVM bytecode
+      terminal  26  the last phase during a compilation run

test/files/neg/t8755-regress-a/ploogin_1.scala

@@ -0,0 +1,40 @@
+
+package t8755
+
+import scala.tools.nsc, nsc.{Global, Phase, plugins}, plugins.{Plugin, PluginComponent}
+
+class P(val global: Global) extends Plugin {
+  override val name = "Testing phase assembly"
+  override val description = "C7 is not dropped even though it has no runs[Right]After"
+  override val components = List[PluginComponent](
+    component7,
+    component8,
+  )
+
+  object component7 extends PluginComponent {
+    override val global = P.this.global
+    override val phaseName = "C7"
+    override val description = "C7 has only a before constraint"
+    override val runsRightAfter = None
+    override val runsAfter = Nil
+    override val runsBefore = List("patmat")
+    override def newPhase(prev: Phase) = new phase(prev)
+    class phase(prev: Phase) extends Phase(prev) {
+      override val name = s"phase $phaseName"
+      override def run() = println(name)
+    }
+  }
+  object component8 extends PluginComponent {
+    override val global = P.this.global
+    override val phaseName = "C8"
+    override val description = "C8 makes C7 reachable"
+    override val runsRightAfter = None
+    override val runsAfter = List("typer")
+    override val runsBefore = List("C7") // component name, not phase name!
+    override def newPhase(prev: Phase) = new phase(prev)
+    class phase(prev: Phase) extends Phase(prev) {
+      override val name = s"phase $phaseName"
+      override def run() = println(name)
+    }
+  }
+}

test/files/neg/t8755-regress-a/sample_2.scala

@@ -0,0 +1,6 @@
+//> using options -Xplugin:. -Xplugin-require:"Testing phase assembly" -Vphases -Werror
+package sample
+
+// just a sample that is compiled with the sample plugin enabled
+object Sample extends App {
+}

test/files/neg/t8755-regress-a/scalac-plugin.xml

@@ -0,0 +1,5 @@
+<plugin>
+  <name>Testing phase assembly</name>
+  <classname>t8755.P</classname>
+</plugin>
+

test/files/neg/t8755.check

@@ -0,0 +1,29 @@
+warning: No phase `refchicks` for ploogin.runsAfter - did you mean refchecks?
+warning: No phase `java` for ploogin.runsBefore - did you mean jvm?
+warning: Dropping phase ploogin, it is not reachable from parser
+    phase name  id  description
+    ----------  --  -----------
+        parser   1  parse source into ASTs, perform simple desugaring
+         namer   2  resolve names, attach symbols to named trees
+packageobjects   3  load package objects
+         typer   4  the meat and potatoes: type the trees
+superaccessors   5  add super accessors in traits and nested classes
+    extmethods   6  add extension methods for inline classes
+       pickler   7  serialize symbol tables
+     refchecks   8  reference/override checking, translate nested objects
+        patmat   9  translate match expressions
+       uncurry  10  uncurry, translate function values to anonymous classes
+        fields  11  synthesize accessors and fields, add bitmaps for lazy vals
+     tailcalls  12  replace tail calls by jumps
+    specialize  13  @specialized-driven class and method specialization
+ explicitouter  14  this refs to outer pointers
+       erasure  15  erase types, add interfaces for traits
+   posterasure  16  clean up erased inline classes
+    lambdalift  17  move nested functions to top level
+  constructors  18  move field definitions into constructors
+       flatten  19  eliminate inner classes
+         mixin  20  mixin composition
+       cleanup  21  platform-specific cleanups, generate reflective calls
+    delambdafy  22  remove lambdas
+           jvm  23  generate JVM bytecode
+      terminal  24  the last phase during a compilation run

test/files/neg/t8755/ploogin_1.scala

@@ -0,0 +1,31 @@
+
+package t8755
+
+import scala.tools.nsc.{Global, Phase}
+import scala.tools.nsc.plugins.{Plugin, PluginComponent}
+import scala.reflect.io.Path
+import scala.reflect.io.File
+
+/** A test plugin.  */
+class Ploogin(val global: Global) extends Plugin {
+  import global._
+
+  val name = "ploogin"
+  val description = "A sample plugin for testing."
+  val components = List[PluginComponent](TestComponent)
+
+  private object TestComponent extends PluginComponent {
+    val global: Ploogin.this.global.type = Ploogin.this.global
+    override val runsBefore = List("java")
+    val runsAfter = List("refchicks")
+    val phaseName = Ploogin.this.name
+    override def description = "A sample phase that doesn't know when to run."
+    def newPhase(prev: Phase) = new TestPhase(prev)
+    class TestPhase(prev: Phase) extends StdPhase(prev) {
+      override def description = TestComponent.this.description
+      def apply(unit: CompilationUnit): Unit = {
+        // kewl kode
+      }
+    }
+  }
+}