Skip to content

Commit 218f490

Browse files
author
Nathan Hawes
committed
[IDE] Fix SyntaxModel crash due to out-of-order walking of EnumElementDecls
ModelASTWalker was previously constructing SyntaxNodes for EnumElementDecls manually when visiting their associated EnumCaseDecl so that they would appear as children rather than siblings. It wasn't actually walking these nodes though, so missed handling some things, e.g. closures passed as default argument values. These were also still being visited later, and because the first visit consumed all the associated TokenNodes, this was triggering an assertion due to the associated TokenNodes not matching expectations.
1 parent ab995ec commit 218f490

File tree

8 files changed

+108
-51
lines changed

8 files changed

+108
-51
lines changed

‎lib/IDE/SyntaxModel.cpp

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ class ModelASTWalker : public ASTWalker {
248248

249249
Optional<SyntaxNode> parseFieldNode(StringRef Text, StringRef OrigText,
250250
SourceLoc OrigLoc);
251-
llvm::DenseSet<ASTNode> VisitedNodesInsideIfConfig;
251+
llvm::DenseSet<ASTNode> NodesVisitedBefore;
252252
/// When non-zero, we should avoid passing tokens as syntax nodes since a parent of several tokens
253253
/// is considered as one, e.g. object literal expression.
254254
uint8_t AvoidPassingSyntaxToken = 0;
@@ -310,8 +310,8 @@ class ModelASTWalker : public ASTWalker {
310310
bool searchForURL(CharSourceRange Range);
311311
bool findFieldsInDocCommentLine(SyntaxNode Node);
312312
bool findFieldsInDocCommentBlock(SyntaxNode Node);
313-
bool isVisitedBeforeInIfConfig(ASTNode Node) {
314-
return VisitedNodesInsideIfConfig.count(Node) > 0;
313+
bool isVisitedBefore(ASTNode Node) {
314+
return NodesVisitedBefore.count(Node) > 0;
315315
}
316316
};
317317

@@ -417,7 +417,7 @@ void ModelASTWalker::visitSourceFile(SourceFile &SrcFile,
417417
}
418418

419419
std::pair<bool, Expr *> ModelASTWalker::walkToExprPre(Expr *E) {
420-
if (isVisitedBeforeInIfConfig(E))
420+
if (isVisitedBefore(E))
421421
return {false, E};
422422

423423
auto addCallArgExpr = [&](Expr *Elem, TupleExpr *ParentTupleExpr) {
@@ -568,7 +568,7 @@ Expr *ModelASTWalker::walkToExprPost(Expr *E) {
568568
}
569569

570570
std::pair<bool, Stmt *> ModelASTWalker::walkToStmtPre(Stmt *S) {
571-
if (isVisitedBeforeInIfConfig(S)) {
571+
if (isVisitedBefore(S)) {
572572
return {false, S};
573573
}
574574
auto addExprElem = [&](SyntaxStructureElementKind K, const Expr *Elem,
@@ -710,7 +710,7 @@ Stmt *ModelASTWalker::walkToStmtPost(Stmt *S) {
710710
}
711711

712712
bool ModelASTWalker::walkToDeclPre(Decl *D) {
713-
if (isVisitedBeforeInIfConfig(D))
713+
if (isVisitedBefore(D))
714714
return false;
715715
if (D->isImplicit())
716716
return false;
@@ -850,7 +850,7 @@ bool ModelASTWalker::walkToDeclPre(Decl *D) {
850850
} else {
851851
Element.get<Decl*>()->walk(*this);
852852
}
853-
VisitedNodesInsideIfConfig.insert(Element);
853+
NodesVisitedBefore.insert(Element);
854854
}
855855
}
856856

@@ -888,13 +888,6 @@ bool ModelASTWalker::walkToDeclPre(Decl *D) {
888888
SourceRange NameRange = SourceRange(EnumElemD->getNameLoc(),
889889
ParamList->getSourceRange().End);
890890
SN.NameRange = charSourceRangeFromSourceRange(SM, NameRange);
891-
892-
for (auto Param : ParamList->getArray()) {
893-
auto TL = Param->getTypeLoc();
894-
CharSourceRange TR = charSourceRangeFromSourceRange(SM,
895-
TL.getSourceRange());
896-
passNonTokenNode({SyntaxNodeKind::TypeId, TR});
897-
}
898891
} else {
899892
SN.NameRange = CharSourceRange(EnumElemD->getNameLoc(),
900893
EnumElemD->getName().getLength());
@@ -906,7 +899,8 @@ bool ModelASTWalker::walkToDeclPre(Decl *D) {
906899
charSourceRangeFromSourceRange(SM, ElemRange));
907900
}
908901
pushStructureNode(SN, EnumElemD);
909-
popStructureNode();
902+
EnumElemD->walk(*this);
903+
NodesVisitedBefore.insert(EnumElemD);
910904
}
911905
}
912906
} else if (auto *TypeAliasD = dyn_cast<TypeAliasDecl>(D)) {
@@ -1039,12 +1033,17 @@ bool ModelASTWalker::handleSpecialDeclAttribute(const DeclAttribute *D,
10391033
if (!Arg->walk(*this))
10401034
return false;
10411035
}
1042-
} else {
1036+
} else if (!TokenNodes.empty()) {
10431037
auto Next = TokenNodes.front();
1044-
TokenNodes = TokenNodes.drop_front();
1045-
assert(Next.Range.getStart() == D->getRangeWithAt().Start);
1046-
if (!passNode({SyntaxNodeKind::AttributeBuiltin, Next.Range}))
1047-
return false;
1038+
if (Next.Range.getStart() == D->getRangeWithAt().Start) {
1039+
TokenNodes = TokenNodes.drop_front();
1040+
if (!passNode({SyntaxNodeKind::AttributeBuiltin, Next.Range}))
1041+
return false;
1042+
} else {
1043+
assert(0 && "Attribute's TokenNodes already consumed?");
1044+
}
1045+
} else {
1046+
assert(0 && "No TokenNodes?");
10481047
}
10491048
if (!passTokenNodesUntil(D->getRange().End, PassNodesBehavior::IncludeNodeAtLocation))
10501049
return false;
@@ -1177,7 +1176,7 @@ bool ModelASTWalker::passNonTokenNode(const SyntaxNode &Node) {
11771176
// such as multiple PatternBindingDecl in code like: var a, b : Int. Which
11781177
// would cause us to report the TypeRepr twice.
11791178
if (!SM.isBeforeInBuffer(LastLoc, Node.Range.getStart()))
1180-
return false;
1179+
return true;
11811180

11821181
if (!passTokenNodesUntil(Node.Range.getStart(), DisplaceNodeAtLocation))
11831182
return false;

‎test/IDE/coloring.swift

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ struct S {
1818
var a, b : Int
1919
}
2020

21+
enum EnumWhereCaseHasADefaultedFunctionTypeParam {
22+
// CHECK: <kw>enum</kw> EnumWhereCaseHasADefaultedFunctionTypeParam {
23+
case foo(x: () -> () = {
24+
// CHECK: <kw>case</kw> foo(x: () -> () = {
25+
func inner(x: S) {}
26+
// CHECK: <kw>func</kw> inner(x: <type>S</type>) {}
27+
})
28+
}
29+
2130
enum EnumWithDerivedEquatableConformance : Int {
2231
// CHECK-LABEL: <kw>enum</kw> EnumWithDerivedEquatableConformance : {{(<type>)}}Int{{(</type>)?}} {
2332
case CaseA

‎test/IDE/coloring_invalid.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// RUN: %target-swift-ide-test -syntax-coloring -source-filename %s | %FileCheck %s
2+
3+
public enum Result {
4+
// CHECK: <attr-builtin>public</attr-builtin> <kw>enum</kw> Result {
5+
case success(a b = {
6+
// CHECK: <kw>case</kw> success(a b = {
7+
8+
@available(*)
9+
// CHECK: <attr-builtin>@available</attr-builtin>(*)
10+
func materialize
11+
// CHECK: <kw>func</kw> materialize

‎test/IDE/structure.swift

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,3 +293,15 @@ myFunc(foo: 0,
293293
bar: baz == 0)
294294
// CHECK: <call><name>myFunc</name>(<arg><name>foo</name>: 0</arg>,
295295
// CHECK: <arg><name>bar</name>: baz == 0</arg>)</call>
296+
297+
298+
enum FooEnum {
299+
// CHECK: <enum>enum <name>FooEnum</name> {
300+
case blah(x: () -> () = {
301+
// CHECK: <enum-case>case <enum-elem><name>blah(<param><name>x</name>: <type>() -> ()</type> = <closure><brace>{
302+
@Tuples func foo(x: MyStruc) {}
303+
// CHECK: @Tuples <ffunc>func <name>foo(<param><name>x</name>: <type>MyStruc</type></param>)</name> {}</ffunc>
304+
})
305+
// CHECK: }</brace></closure></param>)</name></enum-elem></enum-case>
306+
}
307+
// CHECK: }</enum>

‎test/SourceKit/DocumentStructure/structure.swift.empty.response

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,18 +1269,20 @@
12691269
key.offset: 2011,
12701270
key.length: 13,
12711271
key.nameoffset: 2011,
1272-
key.namelength: 13
1272+
key.namelength: 13,
1273+
key.substructure: [
1274+
{
1275+
key.kind: source.lang.swift.decl.var.parameter,
1276+
key.name: "arg",
1277+
key.offset: 2015,
1278+
key.length: 8,
1279+
key.typename: "Int",
1280+
key.nameoffset: 2015,
1281+
key.namelength: 3
1282+
}
1283+
]
12731284
}
12741285
]
1275-
},
1276-
{
1277-
key.kind: source.lang.swift.decl.var.parameter,
1278-
key.name: "arg",
1279-
key.offset: 2015,
1280-
key.length: 8,
1281-
key.typename: "Int",
1282-
key.nameoffset: 2015,
1283-
key.namelength: 3
12841286
}
12851287
]
12861288
},

‎test/SourceKit/DocumentStructure/structure.swift.foobar.response

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,18 +1269,20 @@
12691269
key.offset: 2011,
12701270
key.length: 13,
12711271
key.nameoffset: 2011,
1272-
key.namelength: 13
1272+
key.namelength: 13,
1273+
key.substructure: [
1274+
{
1275+
key.kind: source.lang.swift.decl.var.parameter,
1276+
key.name: "arg",
1277+
key.offset: 2015,
1278+
key.length: 8,
1279+
key.typename: "Int",
1280+
key.nameoffset: 2015,
1281+
key.namelength: 3
1282+
}
1283+
]
12731284
}
12741285
]
1275-
},
1276-
{
1277-
key.kind: source.lang.swift.decl.var.parameter,
1278-
key.name: "arg",
1279-
key.offset: 2015,
1280-
key.length: 8,
1281-
key.typename: "Int",
1282-
key.nameoffset: 2015,
1283-
key.namelength: 3
12841286
}
12851287
]
12861288
},

‎test/SourceKit/DocumentStructure/structure.swift.response

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,18 +1269,20 @@
12691269
key.offset: 2011,
12701270
key.length: 13,
12711271
key.nameoffset: 2011,
1272-
key.namelength: 13
1272+
key.namelength: 13,
1273+
key.substructure: [
1274+
{
1275+
key.kind: source.lang.swift.decl.var.parameter,
1276+
key.name: "arg",
1277+
key.offset: 2015,
1278+
key.length: 8,
1279+
key.typename: "Int",
1280+
key.nameoffset: 2015,
1281+
key.namelength: 3
1282+
}
1283+
]
12731284
}
12741285
]
1275-
},
1276-
{
1277-
key.kind: source.lang.swift.decl.var.parameter,
1278-
key.name: "arg",
1279-
key.offset: 2015,
1280-
key.length: 8,
1281-
key.typename: "Int",
1282-
key.nameoffset: 2015,
1283-
key.namelength: 3
12841286
}
12851287
]
12861288
},

‎test/SourceKit/InterfaceGen/gen_swift_source.swift.response

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,26 @@ internal enum Colors {
595595
key.attribute: source.decl.attribute.internal
596596
}
597597
]
598+
},
599+
{
600+
key.kind: source.lang.swift.decl.var.instance,
601+
key.accessibility: source.lang.swift.accessibility.internal,
602+
key.setter_accessibility: source.lang.swift.accessibility.internal,
603+
key.name: "p2",
604+
key.offset: 559,
605+
key.length: 24,
606+
key.typename: "(Int, Int)",
607+
key.nameoffset: 568,
608+
key.namelength: 2,
609+
key.docoffset: 527,
610+
key.doclength: 19,
611+
key.attributes: [
612+
{
613+
key.offset: 550,
614+
key.length: 8,
615+
key.attribute: source.decl.attribute.internal
616+
}
617+
]
598618
}
599619
]
600620
},

0 commit comments

Comments
 (0)