Reimplement SyntaxHighlighter using Scanner and Parser #4537

slothspot · 2018-05-16T20:09:22Z

WORK IN PROGRESS

Problems

annotations are not supported
comments are not supported
tests are broken
list of keywords which should not be highlighted is not defined
something else I forgot

Nice parts:

implementation is simpler than the current one
REPL highlights strings incorrectly when multiple non-null characters are in front of the string #4405 was fixed here

Next steps:
fix tests and reach features parity with current highlighter implementation

@allanrenucci I would appreciate your feedback on the current state of this experiment and clarification on comments highlighting and list of keywords which shouldn't be highlighted

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

allanrenucci

Great job! Once you're done, you can remove all the code from the old implementation

allanrenucci · 2018-05-18T08:38:57Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+
+    val sf = new SourceFile("<highlighting>", in.toCharArray)
+    val p = new Parser(sf)
+    val s = new Scanner(sf)


I think we could use better names: s/sf/source, s/p/parser, s/s/scanner, s/p/pos...

allanrenucci · 2018-05-18T08:39:46Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+
+    val outputH = Array.fill(in.length)(NoColor)
+
+    def highlightRange(p: Position, color: String): Unit = {


We usually don't type annotate inner methods

allanrenucci · 2018-05-18T09:36:39Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+    def highlightRange(p: Position, color: String): Unit = {
+      if(p.exists) {
+        for {
+          i <- p.start until math.min(p.end, outputH.length)


This will hide bugs with positions. Let's catch IndexOutOfBoundException and print something like:

Encountered tree with invalid position, please open an issue with the code snippet that caused the error

allanrenucci · 2018-05-18T09:36:59Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+    val outputH = Array.fill(in.length)(NoColor)
+
+    def highlightRange(p: Position, color: String): Unit = {
+      if(p.exists) {


missing space after if

allanrenucci · 2018-05-18T09:40:40Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+      if(p.exists) {
+        for {
+          i <- p.start until math.min(p.end, outputH.length)
+        } outputH(i) = color


Prefer syntax with parentheses:

for (i <- p.start until p.end) outputH(i) = color

allanrenucci · 2018-05-18T09:51:37Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+      }
+      sb.append(in(idx))
+    }
+    if(outputH.last != NoColor) {


What if the input is empty? I would test against this at the beginning

allanrenucci · 2018-05-18T09:52:13Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+    for {
+      idx <- outputH.indices
+    } {
+      if(idx == 0 || outputH(idx-1) != outputH(idx)){


If color at index 0 is NoColor, we should not add it

allanrenucci · 2018-05-18T10:03:09Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+      val offsetStart = s.offset
+
+      if(s.token == IDENTIFIER && s.name == nme.???) {
+        highlightRange(Position(s.offset, s.offset + s.name.length), Console.RED_B)


define highlightRange and highlightPosition as:

def highlightRange(from: Int, to: Int, color: String) = ... def highlightPosition(pos: Postition, color: String) = if (pos.exists) highlightRange(pos.start, pos.end, color)

allanrenucci · 2018-05-18T10:06:50Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+      sb.append(NoColor)
+    }
+
+    sb.mkString


Even if it does the same thing, we prefer calling toString on StringBuilder

allanrenucci · 2018-05-18T10:09:39Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+
+    val sf = new SourceFile("<highlighting>", in.toCharArray)
+    val p = new Parser(sf)
+    val s = new Scanner(sf)


Move the scanner declaration closer to use site.

slothspot · 2018-05-18T12:20:07Z

@allanrenucci PR was updated addressing your comments.
I would like to get some suggestions:
1/ How to match annotations? (Annotated node type doesn't work for me)
2/ What to add to the list of excluded keywords?

allanrenucci · 2018-05-18T22:17:23Z

1/ How to match annotations? (Annotated node type doesn't work for me)

You should be able to match on MemberDef. Then you can do .rawMods.annotations. Another (more limited) possibility would be to use the scanner and highlight Tokens.AT as well as the following identifier.

2/ What to add to the list of excluded keywords?

In Tokens.scala, we have keywords = alphaKeywords + symbolicKeywords, we could decide to only highlight alphaKeywords. So instead of doing isKeyword(scanner.token), you could do alphaKeywords.contains(scanner.token)

allanrenucci · 2018-05-18T22:17:57Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+          colorAt(i) = color
+      } catch {
+        case _: IndexOutOfBoundsException =>
+          ctx.error("Encountered tree with invalid position, please open an issue with the code snippet that caused the error")


ctx.error will not print anything since your are using a dummy reporter. Simply use println

allanrenucci · 2018-05-22T12:14:40Z

@slothspot Let's not bother about annotations in this PR. This can be fixed in a separate PR.

I'd like to get this in and close all bugs related to syntax highlighting. Can you update the tests cases and add @Ignore on the tests for annotations and comments that are currently failing.

slothspot · 2018-05-23T16:07:27Z

Preliminary support for annotations was added.
Have next problem:
this @deprecated class Zoo matches to ValOrDefDef with name <init> and namePos pointing to range [0..6] which results in first half of @deprecated highlighted with ValDefColor.

Questions:
1/ Shouldn't this be matched with TypeDef?
2/ Shouldn't namePos be <no position> in this case?

slothspot · 2018-05-23T16:14:28Z

And due to your last request:
1/ I may mark annotations and comments related tests as ignored and create separate PR to handle these.
2/ Is it ok to use LiteralColor for strings?
3/ I'm not happy with interpolated strings:
for s"Some str with $v inside" current implementation gives:
s<L|"Some str with> <V|$v> $<L|inside">
but new one gives:
s<L|"Some str with $>v <L|inside">
May try to fix this with this PR or create separate one

allanrenucci · 2018-05-23T16:52:13Z

this @deprecated class Zoo matches to ValOrDefDef with name and namePos pointing to range [0..6] which results in first half of @deprecated highlighted with ValDefColor.

You are matching the contstructor of the class. The position looks weird to me too. I will have to investigate. What you can try to do is not print synthetic positions: if (pos.exists && pos.isSourceDerived) highlightRange(...)

I may mark annotations and comments related tests as ignored and create separate PR to handle these

Yes, let's handle annoations in a separate PR. Please remove the code for annotations. You can keep the tests and mark them as Pending.

Is it ok to use LiteralColor for strings?

Yes, we can always change it later

I'm not happy with interpolated strings:

You could special case the tree InterpolatedString. Let's do this in another PR

allanrenucci · 2018-05-23T16:29:59Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

+
+      if (isKwd) {
+        val offsetEnd = scanner.lastOffset
+        highlightPosition(Position(offsetStart, offsetEnd), KeywordColor)


highlightRange?

allanrenucci · 2018-05-23T16:33:06Z

compiler/test/dotty/tools/dotc/printing/SyntaxHighlightingTests.scala

+        |}
+      """.stripMargin
+
+    test(source, expected)


Prefer simple single line test case:

test("@inline val bar = 42", ...)

allanrenucci · 2018-05-23T16:38:02Z

compiler/test/dotty/tools/dotc/printing/SyntaxHighlightingTests.scala

@@ -89,12 +111,12 @@ class SyntaxHighlightingTests {

  @Test
  def unionTypes = {


allanrenucci · 2018-05-23T16:52:06Z

compiler/src/dotty/tools/dotc/printing/SyntaxHighlighting.scala

-import util.Chars
+import util.{Chars, SourceFile}
+
+import scala.collection.mutable

 /** This object provides functions for syntax highlighting in the REPL */
 object SyntaxHighlighting {


Remove all the legacy code

Comments addressed

danielyli · 2018-06-20T21:55:11Z

@smarter Added more tests that catch current bugs. Please cherry-pick.

smarter · 2018-06-20T22:18:32Z

@danielyli Thanks! Pushed to the PR.

@inline

- Highlight is based on MemberDef with annotations present - move scanner highlighter before parser so that annotation @inline wholdn't be highlighted as keyword - annotations are highlighted when full member definition is available

Fix scala#4500

If we ignore trees named <error> and <init>, it is not so fragile after all...

@foo

Prior to this commit, an annotation with one or more parameter lists would have incorrect positions attached to each parameter list. For example, the annotation @foo("bar") would have the erroneous position `<4..10>` attached to its `Apply` node instead of the correct position `<4..11>`. This was caused by forget- ting to take into account the closing parenthesis.

These tests currently fail because of a bug in the syntax highlighter.

Test for `1Lx` currently fails; tests for float literals pass.

allanrenucci · 2018-06-29T13:33:20Z

I rebased the PR. The fix in d31f32e is not correct. It breaks a lot of stuff. The compiler does not bootstrap anymore

The end position was correctly set but the start position was wrong: the start position of `fn(args)` should be the same as the start position of `fn` itself.

allanrenucci · 2018-09-21T13:46:37Z

Closed in favor of #5137. Thanks @slothspot for the original PR

dottybot reviewed May 16, 2018

View reviewed changes

allanrenucci previously requested changes May 18, 2018

View reviewed changes

allanrenucci reviewed May 18, 2018

View reviewed changes

fabianpage mentioned this pull request May 23, 2018

Fix #4405 REPL highlight for StringLiteral #4531

Merged

allanrenucci reviewed May 23, 2018

View reviewed changes

slothspot force-pushed the new_highlighter branch 2 times, most recently from d4b4a92 to 53cf6ec Compare May 28, 2018 09:34

allanrenucci added the stat:wip label May 28, 2018

allanrenucci mentioned this pull request May 28, 2018

Fixes #3683 REPL: Incorrect pretty-printing of singleton types #4132

Closed

allanrenucci force-pushed the new_highlighter branch from 53cf6ec to 183ce6e Compare June 1, 2018 16:10

allanrenucci removed the stat:wip label Jun 1, 2018

allanrenucci requested a review from smarter June 1, 2018 16:15

allanrenucci force-pushed the new_highlighter branch from 0921b19 to fa0ea25 Compare June 1, 2018 18:45

smarter force-pushed the new_highlighter branch from 3f67337 to d6444e3 Compare June 20, 2018 22:18

slothspot added 7 commits June 29, 2018 14:37

Add UntypedTreeTraverser

1acd2ad

Introduce Parser and Scanner based highlighter

0350fa6

Small improvements in highlighter

2999dd2

Switch highlighting tests to new highlighter implementation

ec275f9

Address review comments

227f697

Highlight Ident as type when it is type

1f7acd7

Update expected highlight for union types

5074d86

slothspot and others added 8 commits June 29, 2018 14:37

Change ctx.error to println

067be4a

Add annotations highlight

253bf0c

- Highlight is based on MemberDef with annotations present - move scanner highlighter before parser so that annotation @inline wholdn't be highlighted as keyword - annotations are highlighted when full member definition is available

Remove old highlighter code and ignore tests with unsupported features

1f3f36c

Polishing

05aa49d

Fix scala#4500

Let's turn on debug mode by default

20f431b

If we ignore trees named <error> and <init>, it is not so fragile after all...

Add tests for mid-expression user inputs

a41d4f7

These tests currently fail because of a bug in the syntax highlighter.

Add tests for long and float literals

1380ad5

Test for `1Lx` currently fails; tests for float literals pass.

allanrenucci force-pushed the new_highlighter branch from d6444e3 to 4b08b9c Compare June 29, 2018 13:00

Fix rebase breakages

9ea233b

allanrenucci force-pushed the new_highlighter branch from 4b08b9c to 9ea233b Compare June 29, 2018 13:03

Fix position of parsed applications

47fb710

The end position was correctly set but the start position was wrong: the start position of `fn(args)` should be the same as the start position of `fn` itself.

allanrenucci closed this Sep 21, 2018


		val outputH = Array.fill(in.length)(NoColor)

		def highlightRange(p: Position, color: String): Unit = {

		@@ -89,12 +111,12 @@ class SyntaxHighlightingTests {

		@Test
		def unionTypes = {

Reimplement SyntaxHighlighter using Scanner and Parser #4537

Reimplement SyntaxHighlighter using Scanner and Parser #4537

Uh oh!

Conversation

slothspot commented May 16, 2018 • edited by smarter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slothspot commented May 18, 2018

Uh oh!

allanrenucci commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented May 22, 2018

Uh oh!

slothspot commented May 23, 2018

Uh oh!

slothspot commented May 23, 2018

Uh oh!

allanrenucci commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielyli commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Jun 20, 2018

Uh oh!

allanrenucci commented Jun 29, 2018

Uh oh!

allanrenucci commented Sep 21, 2018

Uh oh!

Uh oh!

slothspot commented May 16, 2018 •

edited by smarter

Loading

danielyli commented Jun 20, 2018 •

edited

Loading