Merge pull request #31685 from akyrtzi/libsyntax-incremental-parse-unreachable-fix

[Parser/libSyntax] Avoid doing lookup for a previous parsed node when we are in backtracking mode

Author: akyrtzi

include/swift/Parse/Parser.h

@@ -494,13 +494,7 @@ class Parser {
     ~BacktrackingScope();
     bool willBacktrack() const { return Backtrack; }
 
-    void cancelBacktrack() {
-      Backtrack = false;
-      SynContext->setTransparent();
-      SynContext.reset();
-      DT.commit();
-      TempReceiver.shouldTransfer = true;
-    }
+    void cancelBacktrack();
   };
 
   /// RAII object that, when it is destructed, restores the parser and lexer to

include/swift/Parse/SyntaxParsingContext.h

@@ -326,6 +326,9 @@ class alignas(1 << SyntaxAlignInBits) SyntaxParsingContext {
     IsBacktracking = true;
   }
 
+  /// Cancels backtracking state from the top of the context stack until `this` context.
+  void cancelBacktrack();
+
   bool isBacktracking() const { return IsBacktracking; }
 
   void setShouldDefer(bool Value = true) { ShouldDefer = Value; }

lib/Parse/ParseDecl.cpp

@@ -5531,7 +5531,14 @@ ParserStatus Parser::parseGetSet(ParseDeclOptions Flags,
       parseImplicitGetter();
       return makeParserSuccess();
     }
-    IsFirstAccessor = false;
+    if (IsFirstAccessor) {
+      // Continue parsing without backtracking so we can re-use previously
+      // parsed nodes for incremental re-parsing, but avoid destructing
+      // `backtrack` because its syntax context isn't at the top of the stack at
+      // this point.
+      backtrack->cancelBacktrack();
+      IsFirstAccessor = false;
+    }
 
     // For now, immediately reject illegal accessors in protocols just to
     // avoid having to deal with them everywhere.

lib/Parse/Parser.cpp

@@ -222,6 +222,19 @@ swift::Parser::BacktrackingScope::~BacktrackingScope() {
   }
 }
 
+void swift::Parser::BacktrackingScope::cancelBacktrack() {
+  if (!Backtrack)
+    return;
+
+  Backtrack = false;
+  SynContext->cancelBacktrack();
+  SynContext->setTransparent();
+  if (SynContext->isTopOfContextStack())
+    SynContext.reset();
+  DT.commit();
+  TempReceiver.shouldTransfer = true;
+}
+
 /// Tokenizes a string literal, taking into account string interpolation.
 static void getStringPartTokens(const Token &Tok, const LangOptions &LangOpts,
                                 const SourceManager &SM,

lib/Parse/SyntaxParsingContext.cpp

@@ -46,10 +46,39 @@ SyntaxParsingContext::SyntaxParsingContext(SyntaxParsingContext *&CtxtHolder,
   getStorage().reserve(128);
 }
 
+void SyntaxParsingContext::cancelBacktrack() {
+  SyntaxParsingContext *curr = CtxtHolder;
+  while (true) {
+    curr->IsBacktracking = false;
+    if (curr == this) {
+      break;
+    }
+    curr = curr->getParent();
+  }
+}
+
 size_t SyntaxParsingContext::lookupNode(size_t LexerOffset, SourceLoc Loc) {
   if (!Enabled)
     return 0;
 
+  // Avoid doing lookup for a previous parsed node when we are in backtracking
+  // mode. This is because if the parser library client give us a node pointer
+  // and we discard it due to backtracking then we are violating this invariant:
+  //
+  //   The parser guarantees that any \c swiftparse_client_node_t, given to the
+  //   parser by \c swiftparse_node_handler_t or \c swiftparse_node_lookup_t,
+  //   will be returned back to the client.
+  //
+  // which will end up likely creating a memory leak for the client because
+  // the semantics is that the parser accepts ownership of the object that the
+  // node pointer represents.
+  //
+  // Note that the fact that backtracking mode is disabling incremental parse
+  // node re-use is another reason that we should keep backtracking state as
+  // minimal as possible.
+  if (isBacktracking())
+    return 0;
+
   assert(getStorage().size() == Offset &&
          "Cannot do lookup if nodes have already been gathered");
   assert(Mode == AccumulationMode::CreateSyntax &&

test/incrParse/add-close-brace-to-property.swift

@@ -0,0 +1,9 @@
+// RUN: %empty-directory(%t)
+// RUN: %validate-incrparse %s --test-case REPLACE
+
+
+var value: Int {
+    get { fatalError() }
+<<REPLACE<|||}>>>
+
+let x = 10

test/incrParse/reuse.swift

@@ -1,11 +1,19 @@
 // RUN: %empty-directory(%t)
+// RUN: %validate-incrparse %s --test-case MODIFY_ACCESSOR
 // RUN: %validate-incrparse %s --test-case ADD_PROPERTY
 // RUN: %validate-incrparse %s --test-case WRAP_IN_CLASS
 // RUN: %validate-incrparse %s --test-case UNWRAP_CLASS
 // RUN: %validate-incrparse %s --test-case NEXT_TOKEN_CALCULATION
 
 func start() {}
 
+<reparse MODIFY_ACCESSOR>var someprop: Int {</reparse MODIFY_ACCESSOR>
+  <reparse MODIFY_ACCESSOR>get {</reparse MODIFY_ACCESSOR>
+    return 0
+  <reparse MODIFY_ACCESSOR>}</reparse MODIFY_ACCESSOR>
+  <reparse MODIFY_ACCESSOR>set { print(<<MODIFY_ACCESSOR<|||0>>>) }</reparse MODIFY_ACCESSOR>
+<reparse MODIFY_ACCESSOR>}</reparse MODIFY_ACCESSOR>
+
 <reparse ADD_PROPERTY>struct Foo {</reparse ADD_PROPERTY>
   let a: Int
   let b: Int