[libSyntax] Create deferred nodes in SyntaxParseActions #36277

ahoppen · 2021-03-04T09:44:32Z

Move the creation of deferred nodes from the ParsedRawSyntaxRecorder to SyntaxParseActions. By itself this does not offer any advantage, but from here we will be able to optimize the creation of deferred nodes inside the different SyntaxParseActions implementations.

All implementation that is currently in SyntaxParseActions is transitional. In the next PR these will be pushed to CLibParseActions (which will mostly match what’s currently in SyntaxParseActions) and SyntaxTreeCreator (which can avoid creating deferred nodes altogether by directly constructing RawSyntax nodes).

ahoppen · 2021-03-08T21:17:55Z

@swift-ci Please smoke test

include/swift/Parse/ParsedRawSyntaxNode.h

rintaro · 2021-03-09T05:12:20Z

include/swift/Parse/ParsedRawSyntaxNode.h

+  /// Returns the opaque data of this node. This must be interpreted by the
+  /// \c SyntaxParseAction, which likely also needs the node type to know
+  /// what type of node the data represents.
+  OpaqueSyntaxNode getData() const { return Data.getOpaque(); }
+
+  RecordedOrDeferredNode getRecordedOrDeferredNode() const { return Data; }


These methods (getData() and getRecordedOrDeferredNode()) breaks the "move-only" semantics of ParsedRawSyntaxNode. "Data" should not be extracted without resetting the ParsedRawSyntaxNode.
This is important to ensure not to discard parsed recorded nodes without putting them into the final tree.

Previously, we allowed copying via copyDeferred() method only if it's deferred.

We need some kind of “get data, but don’t take” method to retrieve the node’s children here.

I’ve renamed the method to getUnsafeOpaqueData and added a comment that the data returned by it should only be used to inspect the node, not to record it. Are you OK with that?

Also, while looking through the uses of this method, I actually found a case where we should have been using takeData instead, so good catch.

I modified getRecordedOrDeferredNode to takeRecordedOrDeferredNode.

rintaro · 2021-03-09T05:40:33Z

lib/Parse/SyntaxParseActions.cpp

+    case RecordedOrDeferredNode::Kind::Null:
+      break;
+    case RecordedOrDeferredNode::Kind::Recorded:
+      llvm_unreachable("Children of deferred nodes must also be deferred");


Do we have any comments why deferred nodes can't contain recorded nodes? If not, could you add some?

It’s because of the way we create the syntax tree. Once we put the SyntaxParsingContext in deferred node all (child) nodes are also deferred. If we chose to record child nodes of deferred nodes, we would also get into trouble should we decide to discard the deferred node, because now we’d also need to discard the recorded child node, which cannot be done. Please correct me if any of the assumptions I’m making here are wrong.

We don’t have a comment about this anywhere. I’ve added one here.

lib/Parse/SyntaxParseActions.cpp

ahoppen · 2021-03-09T12:42:11Z

@swift-ci Please smoke test

…yntaxRecorder This is an intermediate step in the effort to push deferred node handling all the way down to SyntaxParseActions.

By now ParsedRawSyntaxNode does not have any knowledge about deferred node data anymore, which frees up SyntaxParseActions (and, in particular its sublass SyntaxTreeCreator) to perform optimisations to more efficiently create and record deferred nodes.

…e as RecordedOrDeferredNode This is mostly a clean-up commit. ParsedRawSyntaxNode stores an OpaqueSyntaxNode and its DataKind so it might as well use a RecordedOrDeferredNode.

This requires a SyntaxParsingContext to be passed to the dump function and was thus disabled in an earlier commit

…ParsedRawSyntaxNode They are superseded by getData/takeData

ahoppen · 2021-03-09T15:56:56Z

@swift-ci Please smoke test

rintaro · 2021-03-09T16:25:56Z

lib/Parse/ParsedSyntaxNodes.cpp.gyb

-Parsed${child.type_name} Parsed${node.name}::getDeferred${child.name}() {
-  return Parsed${child.type_name} {getRaw().getDeferredChildren()[${node.name}::Cursor::${child.name}].copyDeferred()};
+Parsed${child.type_name} Parsed${node.name}::getDeferred${child.name}(const SyntaxParsingContext *SyntaxContext) {
+  return Parsed${child.type_name} {getRaw().getDeferredChild(${node.name}::Cursor::${child.name}, SyntaxContext).copyDeferred()};


copyDeferred() is not needed I believe

It’s still used in one place, I’ll remove it in 81f7d1e which is part of #36362

rintaro · 2021-03-09T16:27:03Z

lib/Parse/ParsedSyntaxNodes.cpp.gyb

-Parsed${node.name}::getDeferred${child.name}() {
-  auto &RawChild = getRaw().getDeferredChildren()[${node.name}::Cursor::${child.name}];
+Parsed${node.name}::getDeferred${child.name}(const SyntaxParsingContext *SyntaxContext) {
+  auto RawChild = getRaw().getDeferredChild(${node.name}::Cursor::${child.name}, SyntaxContext);
  if (RawChild.isNull())
    return None;
  return Parsed${child.type_name} {RawChild.copyDeferred()};


If you remove them, copyDeferred() is not used anymore...

The getData and getRecordedOrDeferredNode methods broke the move-only semantics of ParsedRawSyntaxNode because it allowed retrieving the data without resetting it. Change most uses to use takeData or takeRecordedOrDeferredNode instead of the get methods. To retrieve the children of a ParsedRawSyntaxNode, we still need a way to access the OpaqueSyntaxNode for inspection by the SyntaxParseActions without resetting it. For this, we still maintain a method with the semantics of getData, but it’s now called getUnsafeOpaqueData to make sure it’s not accidentally used.

…ntaxNodes

ahoppen · 2021-03-09T18:13:24Z

@swift-ci Please smoke test

ahoppen · 2021-03-09T19:26:09Z

@swift-ci Please smoke test Windows

ahoppen changed the title ~~Pr/deferred nodes in syntax parse actions~~ [libSyntax] Create deferred nodes in SyntaxParseActions Mar 4, 2021

ahoppen force-pushed the pr/deferred-nodes-in-syntax-parse-actions branch 4 times, most recently from 5164eb4 to 5f3abe3 Compare March 8, 2021 21:17

ahoppen requested a review from rintaro March 8, 2021 21:17

ahoppen marked this pull request as ready for review March 8, 2021 21:17

rintaro reviewed Mar 9, 2021

View reviewed changes

ahoppen force-pushed the pr/deferred-nodes-in-syntax-parse-actions branch 2 times, most recently from 0fa3e7e to 7cca0af Compare March 9, 2021 12:07

ahoppen mentioned this pull request Mar 9, 2021

[libSyntax] Minor cleanup in ParsedRawSyntaxNode #36362

Merged

ahoppen force-pushed the pr/deferred-nodes-in-syntax-parse-actions branch from 7cca0af to 0c136af Compare March 9, 2021 12:41

ahoppen added 5 commits March 9, 2021 14:26

[libSyntax] Create DeferredLayoutNode/DeferredTokenNode in ParsedRawS…

a5632b4

…yntaxRecorder This is an intermediate step in the effort to push deferred node handling all the way down to SyntaxParseActions.

[libSyntax] Store DataKind and OpaqueSyntaxNode in ParsedRawSyntaxNod…

c8acecd

…e as RecordedOrDeferredNode This is mostly a clean-up commit. ParsedRawSyntaxNode stores an OpaqueSyntaxNode and its DataKind so it might as well use a RecordedOrDeferredNode.

[libSyntax] Re-enable dumping of ParsedRawSyntaxNode children

439b44b

This requires a SyntaxParsingContext to be passed to the dump function and was thus disabled in an earlier commit

[libSyntax] Remove unnecessary getOpaqueNode and takeOpaqueNode from …

a38dc1a

…ParsedRawSyntaxNode They are superseded by getData/takeData

ahoppen force-pushed the pr/deferred-nodes-in-syntax-parse-actions branch from 0c136af to 1f695c9 Compare March 9, 2021 13:26

rintaro reviewed Mar 9, 2021

View reviewed changes

rintaro approved these changes Mar 9, 2021

View reviewed changes

ahoppen added 2 commits March 9, 2021 19:10

[libSyntax] Allow retrieval of a null child from deferred ParsedRawSy…

bf6aff9

…ntaxNodes

ahoppen force-pushed the pr/deferred-nodes-in-syntax-parse-actions branch from 1f695c9 to bf6aff9 Compare March 9, 2021 18:13

ahoppen merged commit beadd4a into swiftlang:main Mar 9, 2021

ahoppen deleted the pr/deferred-nodes-in-syntax-parse-actions branch March 9, 2021 21:58

[libSyntax] Create deferred nodes in SyntaxParseActions #36277

[libSyntax] Create deferred nodes in SyntaxParseActions #36277

Uh oh!

Conversation

ahoppen commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Mar 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rintaro Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Mar 9, 2021

Uh oh!

ahoppen commented Mar 9, 2021

Uh oh!

rintaro Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

rintaro Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

rintaro Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 9, 2021

Uh oh!

ahoppen commented Mar 9, 2021

Uh oh!

Uh oh!

ahoppen commented Mar 4, 2021 •

edited

Loading

ahoppen Mar 9, 2021 •

edited

Loading

ahoppen Mar 9, 2021 •

edited

Loading