Skip to content

[Syntax] Don't rebuild Syntax with RawSyntax in SyntaxVisitor #14057

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
Jan 22, 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
5 changes: 4 additions & 1 deletion include/swift/Syntax/Syntax.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ class Syntax {
/// Return the parent of this node, if it has one.
llvm::Optional<Syntax> getParent() const;

/// Return the root syntax of this node.
Syntax getRoot() const;

/// Returns the child index of this node in its parent,
/// if it has one, otherwise 0.
CursorIndex getIndexInParent() const;
Expand Down Expand Up @@ -181,7 +184,7 @@ class Syntax {

/// Get the absolute position of this raw syntax: its offset, line,
/// and column.
AbsolutePosition getAbsolutePosition(SourceFileSyntax Root) const;
AbsolutePosition getAbsolutePosition() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call.


// TODO: hasSameStructureAs ?
};
Expand Down
5 changes: 3 additions & 2 deletions include/swift/Syntax/SyntaxVisitor.h.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ struct SyntaxVisitor {
void visit(Syntax node);

void visitChildren(Syntax node) {
for (auto C: node.getRaw()->getLayout())
visit(make<Syntax>(C));
for (unsigned i = 0, e = node.getNumChildren(); i != e; ++i) {
visit(node.getChild(i));
}
}
};
}
Expand Down
47 changes: 16 additions & 31 deletions lib/Syntax/Syntax.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,53 +81,38 @@ llvm::Optional<Syntax> Syntax::getParent() const {
};
}

Syntax Syntax::getRoot() const {
return { Root, Root.get() };
}

size_t Syntax::getNumChildren() const {
size_t NonTokenChildren = 0;
for (auto Child : getRaw()->getLayout()) {
if (!Child->isToken()) {
++NonTokenChildren;
}
}
return NonTokenChildren;
return Data->getNumChildren();
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any reason this skips token children.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember writing this and we had some tests that, for some reason, expected it to skip token children. Didn't want to mess with it at the time, because I was doing a behavior-preserving refactor. As far as I'm concerned, it should not skip tokens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Originally, we defined children as something indicating recursive structure in the tree but I agree, this should not skip tokens. This may indicate the name children isn't quite right. FWIW, Roslyn likes to use the term slots, similar to how we also use the term layout. Whether you can descend into a node is irrelevant to the number of items in the syntax's layout.

}

Syntax Syntax::getChild(const size_t N) const {
// The actual index of the Nth non-token child.
size_t ActualIndex = 0;
// The number of non-token children we've seen.
size_t NumNonTokenSeen = 0;
for (auto Child : getRaw()->getLayout()) {
// If we see a child that's not a token, count it.
if (!Child->isToken()) {
++NumNonTokenSeen;
}
// If the number of children we've seen indexes the same (count - 1) as
// the number we're looking for, then we're done.
if (NumNonTokenSeen == N + 1) { break; }

// Otherwise increment the actual index and keep searching.
++ActualIndex;
}
return Syntax { Root, Data->getChild(ActualIndex).get() };
return Syntax { Root, Data->getChild(N).get() };
}

AbsolutePosition Syntax::getAbsolutePosition(SourceFileSyntax Root) const {
AbsolutePosition Syntax::getAbsolutePosition() const {
assert(getRoot().is<SourceFileSyntax>() &&
"Absolute position can only be calculated for nodes which has "
"SourceFile root");
AbsolutePosition Pos;

/// This visitor collects all of the nodes before this node to calculate its
/// offset from the begenning of the file.
class Visitor: public SyntaxVisitor {
AbsolutePosition &Pos;
RawSyntax *Target;
const SyntaxData *Target;
bool Found = false;

public:
Visitor(AbsolutePosition &Pos, RawSyntax *Target): Pos(Pos),
Target(Target) {}
Visitor(AbsolutePosition &Pos, const SyntaxData *Target)
: Pos(Pos), Target(Target) {}
~Visitor() { assert(Found); }
void visitPre(Syntax Node) override {
// Check if this node is the target;
Found |= Node.getRaw().get() == Target;
Found |= Node.getDataPointer() == Target;
}
void visit(TokenSyntax Node) override {
// Ignore missing node and ignore the nodes after this node.
Expand All @@ -136,7 +121,7 @@ AbsolutePosition Syntax::getAbsolutePosition(SourceFileSyntax Root) const {
// Collect all the offsets.
Node.getRaw()->accumulateAbsolutePosition(Pos);
}
} Calculator(Pos, getRaw().get());
} Calculator(Pos, getDataPointer());

/// This visitor visit the first token node of this node to accumulate its
/// leading trivia. Therefore, the calculated absolute location will point
Expand All @@ -157,7 +142,7 @@ AbsolutePosition Syntax::getAbsolutePosition(SourceFileSyntax Root) const {
} FTFinder(Pos);

// Visit the root to get all the nodes before this node.
Root.accept(Calculator);
getRoot().accept(Calculator);

// Visit this node to accumulate the leading trivia of its first token.
const_cast<Syntax*>(this)->accept(FTFinder);
Expand Down
12 changes: 4 additions & 8 deletions lib/Syntax/SyntaxParsingContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,16 +217,14 @@ RC<RawSyntax> bridgeAs(SyntaxContextKind Kind, ArrayRef<RC<RawSyntax>> Parts) {

/// This verifier traverses a syntax node to emit proper diagnostics.
class SyntaxVerifier: public SyntaxVisitor {
SourceFileSyntax Root;
RootContextData &RootData;
template<class T>
SourceLoc getSourceLoc(T Node) {
return RootData.SourceMgr.getLocForOffset(RootData.BufferID,
Node.getAbsolutePosition(Root).getOffset());
Node.getAbsolutePosition().getOffset());
}
public:
SyntaxVerifier(SourceFileSyntax Root, RootContextData &RootData) :
Root(Root), RootData(RootData) {}
SyntaxVerifier(RootContextData &RootData) : RootData(RootData) {}
void visit(UnknownDeclSyntax Node) override {
RootData.Diags.diagnose(getSourceLoc(Node), diag::unknown_syntax_entity,
"declaration");
Expand Down Expand Up @@ -275,10 +273,8 @@ void finalizeSourceFile(RootContextData &RootData,

if (SF.getASTContext().LangOpts.VerifySyntaxTree) {
// Verify the added nodes if specified.
SyntaxVerifier Verifier(SF.getSyntaxRoot(), RootData);
for (auto RawNode: Parts) {
Verifier.verify(make<Syntax>(RawNode));
}
SyntaxVerifier Verifier(RootData);
Verifier.verify(SF.getSyntaxRoot());
}
}
} // End of anonymous namespace
Expand Down
4 changes: 2 additions & 2 deletions lib/Syntax/SyntaxVisitor.cpp.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ void swift::syntax::SyntaxVisitor::visit(Syntax node) {
SWIFT_DEFER { visitPost(node); };
switch (node.getKind()) {
case SyntaxKind::Token:
visit(make<TokenSyntax>(node.getRaw()));
visit(node.castTo<TokenSyntax>());
return;
% for node in SYNTAX_NODES:
% if is_visitable(node):
case SyntaxKind::${node.syntax_kind}:
visit(make<${node.name}>(node.getRaw()));
visit(node.castTo<${node.name}>());
return;
% end
% end
Expand Down
2 changes: 1 addition & 1 deletion tools/swift-syntax-test/swift-syntax-test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ int dumpEOFSourceLoc(const char *MainExecutablePath,
auto BufferId = *SF->getBufferID();
SyntaxPrintOptions Opts;
auto Root = SF->getSyntaxRoot();
auto AbPos = Root.getEOFToken().getAbsolutePosition(Root);
auto AbPos = Root.getEOFToken().getAbsolutePosition();

SourceManager &SourceMgr = SF->getASTContext().SourceMgr;
auto StartLoc = SourceMgr.getLocForBufferStart(BufferId);
Expand Down
2 changes: 2 additions & 0 deletions unittests/Syntax/UnknownSyntaxTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ using llvm::SmallString;
using namespace swift;
using namespace swift::syntax;

/*
SymbolicReferenceExprSyntax getCannedSymbolicRef() {
// First, make a symbolic reference to an 'Array<Int>'
auto Array = SyntaxFactory::makeIdentifier("Array", {}, {});
Expand Down Expand Up @@ -170,3 +171,4 @@ TEST(UnknownSyntaxTests, EmbedUnknownStmt) {
// TODO
}

*/