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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2018
Merged

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

merged 1 commit into from
Oct 25, 2018

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Oct 19, 2018

Parser complements missing } as missing token for code blocks.
Taking Missing node into account confuses reusability checking in incremental parsing.

rdar://problem/45215049 https://bugs.swift.org/browse/SR-8976
rdar://problem/45287031 https://bugs.swift.org/browse/SR-9006

@rintaro rintaro force-pushed the incrparse-missingnode branch from 588de88 to b5b0e41 Compare October 19, 2018 09:56
@rintaro
Copy link
Member Author

rintaro commented Oct 19, 2018

@swift-ci Please smoke test

ahoppen
ahoppen previously approved these changes Oct 19, 2018
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Missing the RUN line for the second test case. Otherwise LGTM.

test/incrParse/add-space-at-missing-brace.swift
@@ -0,0 +1,7 @@
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.

@rintaro rintaro dismissed ahoppen’s stale review October 19, 2018 10:49

The change doesn't solve the problem.

@rintaro rintaro force-pushed the incrparse-missingnode branch 3 times, most recently from 648ac00 to 7463cef Compare October 19, 2018 11:16
@rintaro
Copy link
Member Author

rintaro commented Oct 19, 2018

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Oct 19, 2018

@ahoppen Could you re-review this?

nathawes
nathawes approved these changes Oct 19, 2018
View reviewed changes
Copy link
Contributor

@nathawes nathawes left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this Rintaro! LGTM.

ahoppen
ahoppen reviewed Oct 20, 2018
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looking good overall. Two minor comments to add documentation that we skip over missing tokens and to make the behaviour of getNextNode consistent with getPreviousNode.

lib/Syntax/SyntaxData.cpp
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.

lib/Syntax/SyntaxData.cpp
}
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.

@rintaro
[incrParse] Skip missing node in getNextNode()
891cca1 
Taking Missing node into account confuses reusability checking in
incremental parsing.

rdar://problem/45215049 https://bugs.swift.org/browse/SR-8976
rdar://problem/45287031 https://bugs.swift.org/browse/SR-9006
@rintaro rintaro force-pushed the incrparse-missingnode branch from e4dac20 to 891cca1 Compare October 24, 2018 14:59
@rintaro
Copy link
Member Author

rintaro commented Oct 24, 2018

@swift-ci Please smoke test

@nkcsgexi
Copy link
Contributor

Probably we could add isImplicit function on every syntax node since essentially we have to accept its existence after this patch. Then we can rename getPreviousNode() to getPreviousNonImplicitNode()

@rintaro rintaro merged commit bf2cf99 into swiftlang:master Oct 25, 2018
@rintaro rintaro deleted the incrparse-missingnode branch October 25, 2018 01:44
@rintaro
Copy link
Member Author

rintaro commented Oct 25, 2018

Yeah, I'm also thinking about it.
Let's do that if we find actual use cases which requires to recognize synthesized nodes.

@rintaro rintaro mentioned this pull request Nov 7, 2018
Merged
This was referenced Nov 13, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen left review comments

@nathawes nathawes nathawes approved these changes

@nkcsgexi nkcsgexi Awaiting requested review from nkcsgexi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rintaro @nkcsgexi @ahoppen @nathawes