Separate parsing into its own Phase #13173

adampauls · 2021-07-27T22:43:44Z

First-time contributor. This is a resurrection of #5613. I've left a few inline comments about any confusion I had along the way.
Motivations:

Reuse phase infrastructure (e.g. -Ystop-after:parser)
Make it easier to replace the parser (useful in the REPL for example)

adampauls · 2021-07-30T23:21:29Z

compiler/src/dotty/tools/dotc/core/Phases.scala

@@ -64,7 +66,6 @@ object Phases {
                           YCheckAfter: List[String])(using Context): List[Phase] = {
      val fusedPhases = ListBuffer[Phase]()
      var prevPhases: Set[String] = Set.empty
-      val YCheckAll = YCheckAfter.contains("all")


This was redundant because containsPhase already checks for "all"

adampauls · 2021-07-30T23:21:38Z

compiler/src/dotty/tools/dotc/core/Phases.scala

@@ -106,7 +107,7 @@ object Phases {
              phase
            }
          fusedPhases += phaseToAdd
-          val shouldAddYCheck = YCheckAfter.containsPhase(phaseToAdd) || YCheckAll
+          val shouldAddYCheck = filteredPhases(i).exists(_.isCheckable) && YCheckAfter.containsPhase(phaseToAdd)


Check for isCheckable.

adampauls · 2021-07-30T23:21:49Z

compiler/src/dotty/tools/dotc/core/Phases.scala

@@ -402,9 +407,17 @@ object Phases {
    final def iterator: Iterator[Phase] =
      Iterator.iterate(this)(_.next) takeWhile (_.hasNext)

+    final def monitor(doing: String)(body: => Unit)(using Context): Unit =


Moved up from FrontEnd

adampauls · 2021-07-30T23:22:02Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -573,7 +573,7 @@ object SymDenotations {
      case _ =>
        // Otherwise, no completion is necessary, see the preconditions of `markAbsent()`.
        (myInfo `eq` NoType)
-        || is(Invisible) && !ctx.isAfterTyper
+        || is(Invisible) && ctx.isTyper


I believe this is the intent of this line.

adampauls · 2021-07-30T23:23:16Z

compiler/src/dotty/tools/dotc/transform/Pickler.scala

@@ -113,7 +115,7 @@ class Pickler extends Phase {
      val ctx2 = ctx.fresh.setSetting(ctx.settings.YreadComments, true)
      testUnpickler(
        using ctx2
-            .setPeriod(Period(ctx.runId + 1, FirstPhaseId))
+            .setPeriod(Period(ctx.runId + 1, ctx.base.typerPhase.id))


This was needed to fix the pickling tests. I don't understand why.

adampauls · 2021-07-31T17:02:50Z

docs/docs/contributing/testing.md

@@ -164,3 +164,10 @@ The test suite will create a new file if it detects any difference, which can be
 original expect file, or if the user wants to globally replace all expect files for semanticdb they can use
 `scala3-compiler-bootstrapped/test:runMain dotty.tools.dotc.semanticdb.updateExpect`, and compare the changes via version
 control.
+
+## Troubleshooting


Wrote up some advice I got from gitter.

smarter · 2021-08-02T16:20:25Z

This is a resurrection #5613.

Cool, /cc @allanrenucci

adampauls · 2021-08-02T18:28:01Z

I rebased so there are only two commits. I fixed a test failure. There's a compilation error that seems to be present on master

[error] -- [E007] Type Mismatch Error: /Users/adpauls/sm/git/dotty/scaladoc/src/dotty/tools/scaladoc/tasty/ClassLikeSupport.scala:258:97
[error] 258 |            ParametersList(paramList.params.map(mkParameter(_, memberInfo = memberInfo.paramLists(index))), paramListModifier(paramList.params))

griggt · 2021-08-02T20:13:21Z

[error] -- [E008] Not Found Error: /__w/dotty/dotty/community-build/community-projects/akka/plugins/serialversion-remover-plugin/src/main/scala/akka/Plugin.scala:14:30 
[error] 14 |import dotty.tools.dotc.typer.FrontEnd
[error]    |                              ^^^^^^^^
[error]    |                value FrontEnd is not a member of dotty.tools.dotc.typer

There is an open ticket upstream to remove this compiler plugin: akka/akka#30245

adampauls · 2021-08-02T20:15:16Z

compiler/src/dotty/tools/dotc/typer/TyperPhase.scala

  val name: String = "typer"
 }
+
+@deprecated(message = "FrontEnd has been split into TyperPhase and Parser. Refer to one or the other.")
+object FrontEnd {


This is a little sketchy, but should silence the error in the community build.

adampauls · 2021-08-02T23:44:31Z

Also, /cc @biboudis

dwijnand

I'm no expert, but the diff seems reasonable to me. Left some non-major comments, questions and suggestions.

compiler/src/dotty/tools/dotc/typer/TyperPhase.scala

dwijnand · 2021-08-03T09:06:58Z

compiler/src/dotty/tools/dotc/typer/TyperPhase.scala

-      unit.untpdTree.checkPos(nonOverlapping = !unit.isJava && !ctx.reporter.hasErrors)
-  }
+  // Run regardless of parsing errors
+  override def isRunnable(implicit ctx: Context): Boolean = true


This seems wrong, no? Then again no checkfile has failed so perhaps I'm wrong.

I lifted that from the original PR. Without it, at least dotty.tools.dotc.CompilationTests fail.

Yes, even in case of errors in parsing, we run the typer phase, this is particularly useful for IDEs since it means the presentation compiler is usable even when your code doesn't typecheck.

compiler/src/dotty/tools/dotc/parsing/ParserPhase.scala

compiler/src/dotty/tools/dotc/typer/Checking.scala

compiler/src/dotty/tools/dotc/typer/TyperPhase.scala

dwijnand

LGTM.

I'm not sure if Nico's just putting names to PRs or if he wants Guilliaume to sign off on merging this.

adampauls · 2021-08-03T20:37:22Z

Thanks for the quick review!

adampauls · 2021-08-03T22:04:57Z

I'll hold off until merging until @smarter has approved.

adampauls · 2021-08-05T16:43:50Z

Should I continue to wait or just go ahead and merge?

smarter · 2021-08-05T16:48:28Z

I should be able to review this tomorrow.

smarter

Otherwise LGTM.

smarter · 2021-08-06T12:49:36Z

docs/docs/internals/overall-structure.md

@@ -97,7 +97,8 @@ phases. The current list of phases is specified in class [Compiler] as follows:

  /** Phases dealing with the frontend up to trees ready for TASTY pickling */
  protected def frontendPhases: List[List[Phase]] =
-    List(new FrontEnd) ::           // Compiler frontend: scanner, parser, namer, typer
+    List(new Parser) ::             // scanner, parser, namer, typer


Suggested change

List(new Parser) :: // scanner, parser, namer, typer

List(new Parser) :: // scanner, parser

Done, thanks

smarter · 2021-08-06T12:53:01Z

compiler/src/dotty/tools/dotc/typer/TyperPhase.scala

+        // TODO without this test, dotty.tools.repl.ScriptedTests fails. Not sure why.
+        if (this.start > Periods.FirstPhaseId)


Rather than using this to indirectly check if we're in the REPL, I suggest defining class TyperPhase(val addRootImports: Boolean = true) and setting the parameter to false in ReplCompiler.

PR comments #2

…res.

fommil · 2022-02-03T11:16:24Z

@smarter does this mean we can have pre-typer plugins now? 😬 As per https://contributors.scala-lang.org/t/pre-typer-syntactic-plugins-in-scala-3/5565

smarter · 2022-02-03T11:18:21Z

No, pre-typer plugin are still intentionally restricted to research plugins.

adampauls marked this pull request as draft July 27, 2021 22:45

adampauls commented Jul 30, 2021

View reviewed changes

adampauls changed the title ~~[WIP] Separate parsing into its own Phase~~ Separate parsing into its own Phase Jul 31, 2021

adampauls marked this pull request as ready for review July 31, 2021 02:16

adampauls commented Jul 31, 2021

View reviewed changes

adampauls force-pushed the wip_separate_parser_phase branch from d68d3f3 to adc8f44 Compare August 2, 2021 18:26

adampauls force-pushed the wip_separate_parser_phase branch from adc8f44 to 024b0ad Compare August 2, 2021 20:14

adampauls commented Aug 2, 2021

View reviewed changes

dwijnand reviewed Aug 3, 2021

View reviewed changes

nicolasstucki assigned smarter Aug 3, 2021

adampauls requested a review from dwijnand August 3, 2021 19:29

dwijnand approved these changes Aug 3, 2021

View reviewed changes

adampauls requested a review from smarter August 4, 2021 01:45

smarter requested changes Aug 6, 2021

View reviewed changes

smarter assigned adampauls and unassigned smarter Aug 6, 2021

adampauls added 2 commits August 6, 2021 09:35

Split FrontEnd into Parser and TyperPhase

c4e5303

PR comments #2

Add some documentation about troubleshooting spurious unit test failu…

bd08f7e

…res.

adampauls force-pushed the wip_separate_parser_phase branch from 87456bb to bd08f7e Compare August 6, 2021 16:36

smarter approved these changes Aug 6, 2021

View reviewed changes

smarter enabled auto-merge August 6, 2021 16:42

smarter merged commit 58d1b10 into scala:master Aug 6, 2021

tgodzik mentioned this pull request Sep 14, 2021

Completions causing AssertionError with missing implicit parameter #13467

Closed

adampauls mentioned this pull request Oct 18, 2021

[WIP] Separate the namer out from the typer. #13762

Closed

SethTisue mentioned this pull request Oct 27, 2021

in REPL, -Vprint should support the new "parser" phase #13835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate parsing into its own Phase #13173

Separate parsing into its own Phase #13173

adampauls commented Jul 27, 2021 •

edited

Loading

adampauls Jul 30, 2021

adampauls Jul 30, 2021

adampauls Jul 30, 2021

adampauls Jul 30, 2021

adampauls Jul 30, 2021

adampauls Jul 31, 2021

smarter commented Aug 2, 2021

adampauls commented Aug 2, 2021 •

edited

Loading

griggt commented Aug 2, 2021

adampauls Aug 2, 2021

adampauls commented Aug 2, 2021 •

edited

Loading

dwijnand left a comment

dwijnand Aug 3, 2021

adampauls Aug 3, 2021

smarter Aug 3, 2021

dwijnand left a comment

adampauls commented Aug 3, 2021

adampauls commented Aug 3, 2021

adampauls commented Aug 5, 2021

smarter commented Aug 5, 2021

smarter left a comment

smarter Aug 6, 2021

adampauls Aug 6, 2021

smarter Aug 6, 2021

adampauls Aug 6, 2021

fommil commented Feb 3, 2022

smarter commented Feb 3, 2022

	List(new Parser) :: // scanner, parser, namer, typer
	List(new Parser) :: // scanner, parser

		// TODO without this test, dotty.tools.repl.ScriptedTests fails. Not sure why.
		if (this.start > Periods.FirstPhaseId)

Separate parsing into its own Phase #13173

Separate parsing into its own Phase #13173

Conversation

adampauls commented Jul 27, 2021 • edited Loading

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

smarter commented Aug 2, 2021

adampauls commented Aug 2, 2021 • edited Loading

griggt commented Aug 2, 2021

Choose a reason for hiding this comment

adampauls commented Aug 2, 2021 • edited Loading

dwijnand left a 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

dwijnand left a comment

Choose a reason for hiding this comment

adampauls commented Aug 3, 2021

adampauls commented Aug 3, 2021

adampauls commented Aug 5, 2021

smarter commented Aug 5, 2021

smarter left a 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

fommil commented Feb 3, 2022

smarter commented Feb 3, 2022

adampauls commented Jul 27, 2021 •

edited

Loading

adampauls commented Aug 2, 2021 •

edited

Loading

adampauls commented Aug 2, 2021 •

edited

Loading