Skip to content

[incrParse] Skip missing node in SyntaxData::getNextNode() #19951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions include/swift/Syntax/SyntaxData.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,15 +133,16 @@ class SyntaxData final
}

public:
/// Get the node immediately before this current node. Return 0 if we cannot
/// find such node.
/// Get the node immediately before this current node that does contain a
/// non-missing token. Return nullptr if we cannot find such node.
RC<SyntaxData> getPreviousNode() const;

/// Get the node immediately after this current node. Return 0 if we cannot
/// find such node.
/// Get the node immediately after this current node that does contain a
/// non-missing token. Return nullptr if we cannot find such node.
RC<SyntaxData> getNextNode() const;

/// Get the first token node in this tree
/// Get the first non-missing token node in this tree. Return nullptr if this
/// node does not contain non-missing tokens.
RC<SyntaxData> getFirstToken() const;

~SyntaxData() {
Expand Down
3 changes: 2 additions & 1 deletion lib/Parse/SyntaxParsingCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ bool SyntaxParsingCache::nodeCanBeReused(const Syntax &Node, size_t NodeStart,
if (auto NextNode = Node.getData().getNextNode()) {
auto NextLeafNode = NextNode->getFirstToken();
auto NextRawNode = NextLeafNode->getRaw();
assert(NextRawNode->isPresent());
NextLeafNodeLength += NextRawNode->getTokenText().size();
for (auto TriviaPiece : NextRawNode->getLeadingTrivia()) {
NextLeafNodeLength += TriviaPiece.getTextLength();
Expand Down Expand Up @@ -66,7 +67,7 @@ llvm::Optional<Syntax> SyntaxParsingCache::lookUpFrom(const Syntax &Node,
size_t ChildStart = NodeStart;
for (size_t I = 0, E = Node.getNumChildren(); I < E; ++I) {
llvm::Optional<Syntax> Child = Node.getChild(I);
if (!Child.hasValue()) {
if (!Child.hasValue() || Child->isMissing()) {
continue;
}
auto ChildEnd = ChildStart + Child->getTextLength();
Expand Down
29 changes: 19 additions & 10 deletions lib/Syntax/SyntaxData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ RC<SyntaxData> SyntaxData::getPreviousNode() const {
if (hasParent()) {
for (size_t I = N - 1; ; I--) {
if (auto C = getParent()->getChild(I)) {
return C;
if (C->getRaw()->isPresent() && C->getFirstToken())
return C;
}
if (I == 0)
break;
Expand All @@ -73,27 +74,35 @@ RC<SyntaxData> SyntaxData::getNextNode() const {
if (hasParent()) {
for (size_t I = getIndexInParent() + 1, N = Parent->getNumChildren();
I != N; I++) {
if (auto C = getParent()->getChild(I))
return C;
if (auto C = getParent()->getChild(I)) {
if (C->getRaw()->isPresent() && C->getFirstToken())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we skip over missing nodes here, I think we should do the same in getPreviousNode. Also I think it's worth documenting that getNextNode will return the next node that does contain a non-missing token.

return C;
}
}
return Parent->getNextNode();
}
return nullptr;
}

RC<SyntaxData> SyntaxData::getFirstToken() const {
if (getRaw()->isToken()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, I think it's worth adding a doc comment mentioning that we return the first non-missing token.

// Get a reference counted version of this
assert(hasParent() && "The syntax tree should not conisist only of the root");
return getParent()->getChild(getIndexInParent());
}

for (size_t I = 0, E = getNumChildren(); I < E; ++I) {
if (auto Child = getChild(I)) {
if (!Child->getRaw()->isMissing()) {
return Child->getFirstToken();
if (Child->getRaw()->isMissing())
continue;
if (Child->getRaw()->isToken()) {
return Child;
} else if (auto Token = Child->getFirstToken()) {
return Token;
}
}
}

// Get a reference counted version of this
assert(getRaw()->isToken() && "Leaf node that is no token?");
assert(hasParent() && "The syntax tree should not conisist only of the root");
return getParent()->getChild(getIndexInParent());
return nullptr;
}

AbsolutePosition SyntaxData::getAbsolutePositionBeforeLeadingTrivia() const {
Expand Down
7 changes: 7 additions & 0 deletions test/incrParse/add-else-to-ifconfig.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// RUN: %empty-directory(%t)
// RUN: %validate-incrparse %s --test-case ADD_ELSE

func container() {
#if false
<<ADD_ELSE<|||#else>>>

10 changes: 10 additions & 0 deletions test/incrParse/add-space-at-missing-brace.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// RUN: %empty-directory(%t)
// RUN: %validate-incrparse %s --test-case INSERT_SPACE

class AnimationType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing the RUN line for this test case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this change doesn't fix this test case. Sorry, I will retry fixing.

func foo(x: Blah) {
switch x {
case (.

extension AnimationType {
public<<INSERT_SPACE<||| >>>