[Syntax] support nul character as garbage text trivia in libSyntax #14962

omochi · 2018-03-04T05:42:22Z

In previously, Lexer does not handle nul character in lexTrivia function.
So libSyntax lose information about nul character and can not achieve round trip translation.

With this PR, I update lexTrivia and libSyntax handles nul character as garbage text trivia.

I split my work into 2 commits.

In first commit, I introduce Lexer::NulCharacterKind enum to refactor Lexer code.
There are 3 types of meaning about nul character in Lexer.

Embedded nul character
CodeCompletion marker
String buffer terminator

We need to handle correctly and carefully these cases when see nul character in Lexer.
To clear this task and prevent to forget about this, this enum is useful.

How do you think?

In second commit, I update lexTrivia.
This commit achieve main purpose of this PR.
And add test cases to check round trip of source file with nul, trivia construction, diagnostics about embedded nul character.

A new test case Syntax/tokens_nul.swift covers all consideration of Syntax/lexer_invalid_nul.swift which is added by me in previous PR #14954
So I removed this.

xwu · 2018-03-04T06:09:12Z

include/swift/Parse/Lexer.h

@@ -461,6 +461,9 @@ class Lexer {
  };

 private:
+  /// nul character meaning kind


Drive-by nit: Please capitalize and punctuate comments.

omochi · 2018-03-04T06:54:26Z

@xwu Thanks, I updated.

rintaro · 2018-03-04T09:35:45Z

lib/Parse/Lexer.cpp

@@ -1857,6 +1869,16 @@ bool Lexer::tryLexConflictMarker(bool EatNewline) {
  return false;
 }

+Lexer::NulCharacterKind Lexer::getNulCharacterKind(const char *Ptr) const {
+  assert(Ptr != nullptr);


Could you add && *Ptr == 0?

Oh, good idea. I will fix it.

rintaro · 2018-03-04T09:41:05Z

include/swift/Parse/Lexer.h

+  /// There are 3 kinds of meaning about nul character in Lexer.
+  /// 1. BufferEnd is string buffer terminator.
+  /// 2. Embedded is embedded nul character.
+  /// 3. CodeCompletion is code completion marker.
  enum class NulCharacterKind { BufferEnd, Embedded, CodeCompletion };


Please make this:

/// Nul character meaning kind. enum class NulCharacterKind { /// String buffer terminator. BufferEnd, /// Embedded is embedded nul character. Embedded, ...

numbered list (1,2,3) might be confusable with step-by-step procedure.

Okey, I will fix it.

omochi · 2018-03-04T13:15:26Z

Ah, I mistook to apply changes to correct commit.
I will fix history again.

omochi · 2018-03-04T13:24:02Z

Ok, please review it.

harlanhaskins · 2018-03-04T21:03:54Z

lib/Parse/Lexer.cpp

        diagnoseEmbeddedNul(Diags, CurPtr-1);
+        LLVM_FALLTHROUGH;
+      case NulCharacterKind::CodeCompletion:
+        goto LoopStart;


Can this be replaced with a continue?

Oh, thanks.
I overlooked continue in switch is bound to outer while not to switch what is different from break.
Interesting...
I will fix it so.

harlanhaskins · 2018-03-04T21:04:01Z

lib/Parse/Lexer.cpp

+        diagnoseEmbeddedNul(Diags, CurPtr - 1);
+        LLVM_FALLTHROUGH;
+      case NulCharacterKind::CodeCompletion:
+        goto LoopStart;


Same as above

omochi · 2018-03-04T23:53:20Z

I updated in response to review.

rintaro · 2018-03-05T02:02:24Z

lib/Parse/Lexer.cpp

@@ -353,8 +353,13 @@ void Lexer::skipToEndOfLine(bool EatNewline) {
    case 0:
      // If this is a random nul character in the middle of a buffer, skip it as
      // whitespace.


This comment should be moved into case NulCharacterKind::Embedded: branch.

rintaro · 2018-03-05T02:08:21Z

lib/Parse/Lexer.cpp

+        LLVM_FALLTHROUGH;
+      case NulCharacterKind::CodeCompletion:
+        continue;
+      case NulCharacterKind::BufferEnd:
        break;


Could you move the task in the case of NulCharacterKind::BufferEnd into this branch? i.e.

case NulCharacterKind::BufferEnd: // The last line of the file does not have a newline. --CurPtr; return; }

rintaro · 2018-03-05T02:08:29Z

lib/Parse/Lexer.cpp

+        LLVM_FALLTHROUGH;
+      case NulCharacterKind::CodeCompletion:
+        continue;
+      case NulCharacterKind::BufferEnd:
        break;


rintaro · 2018-03-05T02:08:41Z

lib/Parse/Lexer.cpp

      diagnoseEmbeddedNul(Diags, CurPtr-1);
      goto Restart;
+    case NulCharacterKind::BufferEnd:
+      break;


omochi · 2018-03-05T05:48:58Z

I updated.
@rintaro Thanks, I intended to reduce diff at first time. But I agree your review.

rintaro · 2018-03-05T06:17:52Z

lib/Parse/Lexer.cpp

+        LLVM_FALLTHROUGH;
+      case NulCharacterKind::CodeCompletion:
+        continue;
+      case NulCharacterKind::BufferEnd:


The last one request from me.
Please enclose this particular branch with braces case NulCharacterKind::BufferEnd: { ... }. Lacking braces make us difficult to add another case (or re-order cases).

omochi · 2018-03-05T07:54:16Z

updated 🙂

rintaro · 2018-03-05T08:38:27Z

@swift-ci Please smoke test

xwu reviewed Mar 4, 2018

View reviewed changes

omochi force-pushed the syntax-nul-trivia branch from e0ff301 to 5dc1f47 Compare March 4, 2018 06:53

rintaro reviewed Mar 4, 2018

View reviewed changes

omochi force-pushed the syntax-nul-trivia branch from 5dc1f47 to 0f0fbfa Compare March 4, 2018 12:13

omochi mentioned this pull request Mar 4, 2018

[Do not merge] [Syntax] support invalid characters as trivia #14967

Closed

omochi force-pushed the syntax-nul-trivia branch from 0f0fbfa to 71a13b9 Compare March 4, 2018 13:21

harlanhaskins reviewed Mar 4, 2018

View reviewed changes

omochi force-pushed the syntax-nul-trivia branch from 71a13b9 to ecaa097 Compare March 4, 2018 23:46

rintaro reviewed Mar 5, 2018

View reviewed changes

omochi force-pushed the syntax-nul-trivia branch from ecaa097 to 7d3aab1 Compare March 5, 2018 05:41

rintaro reviewed Mar 5, 2018

View reviewed changes

omochi added 2 commits March 5, 2018 16:53

[Parse] refactor Lexer by made NulCharacterKind type

58857fa

[Syntax] support nul character as garbage trivia

190af6c

omochi force-pushed the syntax-nul-trivia branch from 7d3aab1 to 190af6c Compare March 5, 2018 07:53

rintaro approved these changes Mar 5, 2018

View reviewed changes

rintaro merged commit db47cb1 into swiftlang:master Mar 5, 2018

omochi deleted the syntax-nul-trivia branch March 7, 2018 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Syntax] support nul character as garbage text trivia in libSyntax #14962

[Syntax] support nul character as garbage text trivia in libSyntax #14962

omochi commented Mar 4, 2018

xwu Mar 4, 2018

omochi commented Mar 4, 2018

rintaro Mar 4, 2018

omochi Mar 4, 2018

rintaro Mar 4, 2018

omochi Mar 4, 2018

omochi commented Mar 4, 2018

omochi commented Mar 4, 2018

harlanhaskins Mar 4, 2018

omochi Mar 4, 2018

harlanhaskins Mar 4, 2018

omochi Mar 4, 2018

omochi commented Mar 4, 2018

rintaro Mar 5, 2018

rintaro Mar 5, 2018

rintaro Mar 5, 2018

rintaro Mar 5, 2018

omochi commented Mar 5, 2018

rintaro Mar 5, 2018 •

edited

Loading

omochi commented Mar 5, 2018

rintaro commented Mar 5, 2018

[Syntax] support nul character as garbage text trivia in libSyntax #14962

[Syntax] support nul character as garbage text trivia in libSyntax #14962

Conversation

omochi commented Mar 4, 2018

Choose a reason for hiding this comment

omochi commented Mar 4, 2018

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

omochi commented Mar 4, 2018

omochi commented Mar 4, 2018

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

omochi commented Mar 4, 2018

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

omochi commented Mar 5, 2018

rintaro Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

omochi commented Mar 5, 2018

rintaro commented Mar 5, 2018

rintaro Mar 5, 2018 •

edited

Loading