[libSyntax] Syntax coloring using libSyntax #16636

ahoppen · 2018-05-15T19:26:51Z

This is a WIP to perform syntax coloring using libSyntax. With #16340 it should allow incremental syntax coloring.

ahoppen · 2018-06-11T21:27:16Z

@swift-ci Please smoke test

ahoppen · 2018-06-12T22:32:26Z

@swift-ci Please smoke test

ahoppen · 2018-06-12T22:52:13Z

@swift-ci Please smoke test

ahoppen · 2018-06-13T22:23:41Z

@swift-ci Please smoke test

ahoppen · 2018-06-18T17:57:05Z

@swift-ci Please smoke test

ahoppen · 2018-06-18T21:35:20Z

@swift-ci Please smoke test

ahoppen · 2018-06-19T16:30:43Z

@swift-ci Please smoke test

ahoppen · 2018-06-25T18:39:30Z

@swift-ci Please smoke test

ahoppen · 2018-06-26T00:13:12Z

@swift-ci Please smoke test

ahoppen · 2018-06-26T16:49:58Z

@swift-ci Please smoke test

nkcsgexi

Looking good over all! Some minor comments so far.

nkcsgexi · 2018-06-26T18:06:25Z

include/swift/Syntax/RawSyntax.h

@@ -270,13 +277,17 @@ class RawSyntax final
  }

  /// Constructor for creating layout nodes
+  /// If NodeId is 0, the next free NodeId is used, if it is passed, the caller


Isn't Optional<unsigned> more readable to describe these two situations?

Yeah, that's probably better. I'll change it.

nkcsgexi · 2018-06-26T18:07:55Z

include/swift/Basic/JSONSerialization.h

+  /// thus pass serialization options
+  enum OutputKind {
+    Normal,
+    SyntaxTree


Can we have some documentation here for the differences? a.k.a. why do we need node id in one case however not in the other.

I added some documentation. I don't want to go into detail on how to the subclasses here because it's not really the point where I'd look for it.

nkcsgexi · 2018-06-26T18:09:09Z

include/swift/Syntax/SyntaxClassifier.h.gyb

+}%
+//// Automatically Generated From SyntaxVisitor.h.gyb.
+//// Do Not Edit Directly!
+//===---------------- SyntaxVisitor.h - SyntaxVisitor definitions ---------===//


Nit: the type should be updated and the year.

nkcsgexi · 2018-06-26T18:10:27Z

include/swift/Syntax/SyntaxClassifier.h.gyb

+
+  /// First is the classification all identifiers shall inherit, if seconds is
+  /// set to \c true, all tokens will be forced to receive that classification
+  using ContextStackEntry = std::pair<SyntaxClassification, bool>;


Can we define a new struct here instead of using std::pair?

nkcsgexi · 2018-06-26T18:12:41Z

lib/Syntax/RawSyntax.cpp

  assert(Kind != SyntaxKind::Token &&
         "'token' syntax node must be constructed with dedicated constructor");
+  if (NodeId) {


Again, using Optional<unsigned> for NodeId can be more self-explanatory.

nkcsgexi · 2018-06-26T18:13:27Z

lib/Syntax/SyntaxClassifier.cpp.gyb

+}%
+//// Automatically Generated From SyntaxFactory.cpp.gyb.
+//// Do Not Edit Directly!
+//===--------- SyntaxFactory.cpp - Syntax Factory implementations ---------===//


Nit: update title, years and documentation in the comments.

nkcsgexi · 2018-06-26T18:14:41Z

lib/Syntax/SyntaxClassifier.cpp.gyb

+/// inside a special context. Returns \c None if the token has no nativie
+/// classification and should always inherit from the context.
+llvm::Optional<SyntaxClassification>
+getNativeClassificationForToken(TokenSyntax TokenNode) {


Can we rename it to getContextFreeClassificationForToken?

Yeah, that name is better. Changed it.

ahoppen · 2018-06-26T19:48:01Z

@swift-ci Please smoke test

ahoppen · 2018-06-26T20:26:46Z

@swift-ci Please smoke test

The id is meant to be stable across incremental parses

Since nodes have unique IDs now, we cannot reuse the nodes at multiple locations in the source file

ahoppen · 2018-06-26T21:05:51Z

@swift-ci Please smoke test

…n expression as interpolation anchor

ahoppen · 2018-06-26T22:42:38Z

@swift-ci Please test

swift-ci · 2018-06-26T22:44:23Z

Build failed
Swift Test OS X Platform
Git Sha - f1acebc34037f6b3187bd7f321b4e4c4fa590906

swift-ci · 2018-06-26T22:44:28Z

Build failed
Swift Test Linux Platform
Git Sha - f1acebc34037f6b3187bd7f321b4e4c4fa590906

nkcsgexi · 2018-06-26T23:13:26Z

tools/SourceKit/tools/sourcekitd/lib/API/Requests.cpp

+                                         /*EnableDiagnostics=*/false,
+                                         /*EnableSyntaxTree=*/false,
+                                         /*EnableSyntaxReuseInfo=*/false,
+                                         /*SyntacticOnly=*/false);


Can we define a struct like EditorConsumerOptions to incorporate all these boolean values? Stacking them up like this can be error-prone.

…ormation is present

nkcsgexi · 2018-06-27T19:08:34Z

include/swift/Basic/JSONSerialization.h

+  /// reused since the last serialization.
+  enum OutputKind {
+    Normal,
+    SyntaxTree


It's still kind of weird that the serialization util knows syntax tree :( . Can we define a 32-bit field in Output whose meaning can be interpreted while serializing? For instance, we can use the first bit to indicate whether shouldIncludeNodeIds.

ahoppen · 2018-06-27T21:04:02Z

@swift-ci Please test

swift-ci · 2018-06-27T21:05:43Z

Build failed
Swift Test Linux Platform
Git Sha - 3c4fcc1485a83314eee775689cc787107efa6f65

swift-ci · 2018-06-27T21:05:52Z

Build failed
Swift Test OS X Platform
Git Sha - 3c4fcc1485a83314eee775689cc787107efa6f65

ahoppen · 2018-06-27T22:08:53Z

@swift-ci Please test

swift-ci · 2018-06-27T22:11:08Z

Build failed
Swift Test Linux Platform
Git Sha - 70ff7919cf6703a6ff47e28e252ced46b138011e

swift-ci · 2018-06-27T22:12:10Z

Build failed
Swift Test OS X Platform
Git Sha - 70ff7919cf6703a6ff47e28e252ced46b138011e

rintaro · 2018-06-28T01:07:06Z

I'll review this today.

rintaro · 2018-06-28T10:09:16Z

test/IDE/coloring.swift

@@ -163,18 +168,18 @@ func foo(n: Float) -> Int {
 }

 ///- returns: single-line, no space
-// CHECK: ///- <doc-comment-field>returns</doc-comment-field>: single-line, no space
+// CHECK-OLD: ///- <doc-comment-field>returns</doc-comment-field>: single-line, no space


Could you add CHECK-NEW check?

rintaro · 2018-06-28T10:13:29Z

test/IDE/coloring.swift

@@ -592,7 +611,8 @@ func keywordAsLabel4(_: Int) {}
 func keywordAsLabel5(_: Int, for: Int) {}
 // CHECK: <kw>func</kw> keywordAsLabel5(<kw>_</kw>: <type>Int</type>, for: <type>Int</type>) {}
 func keywordAsLabel6(if let: Int) {}
-// CHECK: <kw>func</kw> keywordAsLabel6(if <kw>let</kw>: <type>Int</type>) {}
+// CHECK-OLD: <kw>func</kw> keywordAsLabel6(if <kw>let</kw>: <type>Int</type>) {}
+// CHECK-NEW: <kw>func</kw> keywordAsLabel6(<kw>if</kw> <kw>let</kw>: <type>Int</type>) {}


Coloring as <kw>if</kw> here doesn't look right to me. (<kw>let</kw> too).

rintaro · 2018-06-28T10:27:21Z

Sorry, this PR is too big to review.
Is it possible to implement "Syntax coloring using libSyntax" without incremental parsing first, then make it incremental?

ahoppen · 2018-06-28T22:33:23Z

@rintaro I've split the PR will be delivering them in pieces now.

ahoppen force-pushed the libsyntax-coloring branch 2 times, most recently from 76aae2d to df62f67 Compare May 22, 2018 16:37

ahoppen force-pushed the libsyntax-coloring branch from df62f67 to 2c59d60 Compare June 11, 2018 21:26

ahoppen force-pushed the libsyntax-coloring branch from fc5320b to 9efff16 Compare June 12, 2018 22:52

ahoppen force-pushed the libsyntax-coloring branch from 9efff16 to f1acebc Compare June 13, 2018 22:23

ahoppen changed the title ~~[WIP] Syntax coloring using libSyntax~~ [libSyntax] Syntax coloring using libSyntax Jun 13, 2018

ahoppen force-pushed the libsyntax-coloring branch from edf8ad5 to 032904c Compare June 25, 2018 18:39

ahoppen force-pushed the libsyntax-coloring branch from d2ebc11 to ff88907 Compare June 26, 2018 16:48

nkcsgexi reviewed Jun 26, 2018

View reviewed changes

ahoppen force-pushed the libsyntax-coloring branch from ff88907 to 5ba3ee1 Compare June 26, 2018 19:47

ahoppen force-pushed the libsyntax-coloring branch from 5ba3ee1 to a6c94dd Compare June 26, 2018 20:26

ahoppen added 4 commits June 26, 2018 14:05

[incrParse] Add a stable id to the syntax nodes

275821a

The id is meant to be stable across incremental parses

[libSyntax] Add syntax coloring based on the syntax tree

9e0195b

[libSyntax] Disable caching of token nodes

3505596

Since nodes have unique IDs now, we cannot reuse the nodes at multiple locations in the source file

[libSyntax] Classify unterminated string literals as string literals

6833540

ahoppen force-pushed the libsyntax-coloring branch from a6c94dd to af18ead Compare June 26, 2018 21:05

[libSyntax] Classify the opening parenthesis of a string interpolatio…

6b03383

…n expression as interpolation anchor

ahoppen force-pushed the libsyntax-coloring branch from af18ead to 3c4fcc1 Compare June 26, 2018 22:42

nkcsgexi reviewed Jun 26, 2018

View reviewed changes

nkcsgexi requested a review from rintaro June 26, 2018 23:40

ahoppen added 4 commits June 27, 2018 10:23

[libSyntax] Teach SyntaxToSyntaxMapConverter to convert trivia

c9f215c

[libSyntax] Classify identifier tokens as identifiers if no other inf…

0500e65

…ormation is present

[libSyntax] Classify pound directives as build config keywords

e7fe537

[libSyntax] Add test variants for building the syntax map via libSyntax

4b506b2

ahoppen force-pushed the libsyntax-coloring branch from 3c4fcc1 to 4b506b2 Compare June 27, 2018 17:24

nkcsgexi reviewed Jun 27, 2018

View reviewed changes

nkcsgexi approved these changes Jun 27, 2018

View reviewed changes

[SourceKit] Move all options into a common options struct

8b037fc

ahoppen force-pushed the libsyntax-coloring branch from 70ff791 to 8b037fc Compare June 27, 2018 22:08

rintaro self-assigned this Jun 28, 2018

rintaro reviewed Jun 28, 2018

View reviewed changes

rintaro removed their assignment Jun 28, 2018

ahoppen closed this Jun 28, 2018

ahoppen mentioned this pull request Jun 29, 2018

[libSyntax] Syntax colouring based on the syntax tree #17621

Merged

ahoppen deleted the libsyntax-coloring branch July 19, 2018 16:21

[libSyntax] Syntax coloring using libSyntax #16636

[libSyntax] Syntax coloring using libSyntax #16636

Uh oh!

Conversation

ahoppen commented May 15, 2018

Uh oh!

ahoppen commented Jun 11, 2018

Uh oh!

ahoppen commented Jun 12, 2018

Uh oh!

ahoppen commented Jun 12, 2018

Uh oh!

ahoppen commented Jun 13, 2018

Uh oh!

ahoppen commented Jun 18, 2018

Uh oh!

ahoppen commented Jun 18, 2018

Uh oh!

ahoppen commented Jun 19, 2018

Uh oh!

ahoppen commented Jun 25, 2018

Uh oh!

ahoppen commented Jun 26, 2018

Uh oh!

ahoppen commented Jun 26, 2018

Uh oh!

nkcsgexi 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!

ahoppen commented Jun 26, 2018

Uh oh!

ahoppen commented Jun 26, 2018

Uh oh!

ahoppen commented Jun 26, 2018

Uh oh!

ahoppen commented Jun 26, 2018

Uh oh!

swift-ci commented Jun 26, 2018

Uh oh!

swift-ci commented Jun 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jun 27, 2018

Uh oh!

swift-ci commented Jun 27, 2018

Uh oh!

swift-ci commented Jun 27, 2018

Uh oh!

ahoppen commented Jun 27, 2018

Uh oh!

swift-ci commented Jun 27, 2018

Uh oh!

swift-ci commented Jun 27, 2018

Uh oh!

rintaro commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

rintaro commented Jun 28, 2018 •

edited

Loading