Skip to content

Commit b1c8013

Browse files
author
Nathan Hawes
authored
Merge pull request #19782 from nathawes/incremental-parsing-bug
[incrParse] Fix bug mapping a node's location back to its location in the cached syntax tree
2 parents 28997eb + 9003d83 commit b1c8013

File tree

12 files changed

+304
-36
lines changed

12 files changed

+304
-36
lines changed

‎include/swift/Parse/SyntaxParsingCache.h

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
#include "llvm/Support/raw_ostream.h"
1919
#include <unordered_set>
2020

21-
namespace {
21+
namespace swift {
22+
23+
using namespace swift::syntax;
2224

2325
/// A single edit to the original source file in which a continuous range of
2426
/// characters have been replaced by a new string
@@ -38,16 +40,10 @@ struct SourceEdit {
3840
/// Check if the characters replaced by this edit fall into the given range
3941
/// or are directly adjacent to it
4042
bool intersectsOrTouchesRange(size_t RangeStart, size_t RangeEnd) {
41-
return !(End <= RangeStart || Start >= RangeEnd);
43+
return End >= RangeStart && Start <= RangeEnd;
4244
}
4345
};
4446

45-
} // anonymous namespace
46-
47-
namespace swift {
48-
49-
using namespace swift::syntax;
50-
5147
struct SyntaxReuseRegion {
5248
AbsolutePosition Start;
5349
AbsolutePosition End;
@@ -89,6 +85,13 @@ class SyntaxParsingCache {
8985
std::vector<SyntaxReuseRegion>
9086
getReusedRegions(const SourceFileSyntax &SyntaxTree) const;
9187

88+
/// Translates a post-edit position to a pre-edit position by undoing the
89+
/// specified edits.
90+
/// Should not be invoked externally. Only public for testing purposes.
91+
static size_t
92+
translateToPreEditPosition(size_t PostEditPosition,
93+
llvm::SmallVector<SourceEdit, 4> Edits);
94+
9295
private:
9396
llvm::Optional<Syntax> lookUpFrom(const Syntax &Node, size_t NodeStart,
9497
size_t Position, SyntaxKind Kind);

‎lib/Basic/SourceLoc.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,9 @@ llvm::Optional<unsigned> SourceManager::resolveFromLineCol(unsigned BufferId,
331331
return None;
332332
}
333333
Ptr = LineStart;
334-
for (; Ptr < End; ++Ptr) {
334+
335+
// The <= here is to allow for non-inclusive range end positions at EOF
336+
for (; Ptr <= End; ++Ptr) {
335337
--Col;
336338
if (Col == 0)
337339
return Ptr - InputBuf->getBufferStart();

‎lib/Parse/SyntaxParsingCache.cpp

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,21 @@ llvm::Optional<Syntax> SyntaxParsingCache::lookUpFrom(const Syntax &Node,
7979
return llvm::None;
8080
}
8181

82-
llvm::Optional<Syntax> SyntaxParsingCache::lookUp(size_t NewPosition,
83-
SyntaxKind Kind) {
84-
// Undo the edits in reverse order
85-
size_t OldPosition = NewPosition;
86-
for (auto I = Edits.rbegin(), E = Edits.rend(); I != E; ++I) {
82+
size_t SyntaxParsingCache::translateToPreEditPosition(
83+
size_t PostEditPosition, llvm::SmallVector<SourceEdit, 4> Edits) {
84+
size_t Position = PostEditPosition;
85+
for (auto I = Edits.begin(), E = Edits.end(); I != E; ++I) {
8786
auto Edit = *I;
88-
if (Edit.End <= OldPosition) {
89-
OldPosition =
90-
OldPosition - Edit.ReplacementLength + Edit.originalLength();
87+
if (Edit.End + Edit.ReplacementLength - Edit.originalLength() <= Position) {
88+
Position = Position - Edit.ReplacementLength + Edit.originalLength();
9189
}
9290
}
91+
return Position;
92+
}
93+
94+
llvm::Optional<Syntax> SyntaxParsingCache::lookUp(size_t NewPosition,
95+
SyntaxKind Kind) {
96+
size_t OldPosition = translateToPreEditPosition(NewPosition, Edits);
9397

9498
auto Node = lookUpFrom(OldSyntaxTree, /*NodeStart=*/0, OldPosition, Kind);
9599
if (Node.hasValue()) {

‎test/incrParse/Outputs/extend-identifier-at-eof.json

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
{
2+
"id": 39,
3+
"kind": "SourceFile",
4+
"layout": [
5+
{
6+
"id": 38,
7+
"kind": "CodeBlockItemList",
8+
"layout": [
9+
{
10+
"id": 13,
11+
"omitted": true
12+
},
13+
{
14+
"id": 35,
15+
"kind": "CodeBlockItem",
16+
"layout": [
17+
{
18+
"id": 34,
19+
"kind": "SequenceExpr",
20+
"layout": [
21+
{
22+
"id": 33,
23+
"kind": "ExprList",
24+
"layout": [
25+
{
26+
"id": 28,
27+
"kind": "DiscardAssignmentExpr",
28+
"layout": [
29+
{
30+
"id": 27,
31+
"tokenKind": {
32+
"kind": "kw__"
33+
},
34+
"leadingTrivia": [
35+
{
36+
"kind": "Newline",
37+
"value": 1
38+
},
39+
{
40+
"kind": "LineComment",
41+
"value": "\/\/ ATTENTION: This file is testing the EOF token. "
42+
},
43+
{
44+
"kind": "Newline",
45+
"value": 1
46+
},
47+
{
48+
"kind": "LineComment",
49+
"value": "\/\/ DO NOT PUT ANYTHING AFTER THE CHANGE, NOT EVEN A NEWLINE"
50+
},
51+
{
52+
"kind": "Newline",
53+
"value": 1
54+
}
55+
],
56+
"trailingTrivia": [
57+
{
58+
"kind": "Space",
59+
"value": 1
60+
}
61+
],
62+
"presence": "Present"
63+
}
64+
],
65+
"presence": "Present"
66+
},
67+
{
68+
"id": 30,
69+
"kind": "AssignmentExpr",
70+
"layout": [
71+
{
72+
"id": 29,
73+
"tokenKind": {
74+
"kind": "equal"
75+
},
76+
"leadingTrivia": [],
77+
"trailingTrivia": [
78+
{
79+
"kind": "Space",
80+
"value": 1
81+
}
82+
],
83+
"presence": "Present"
84+
}
85+
],
86+
"presence": "Present"
87+
},
88+
{
89+
"id": 32,
90+
"kind": "IdentifierExpr",
91+
"layout": [
92+
{
93+
"id": 31,
94+
"tokenKind": {
95+
"kind": "identifier",
96+
"text": "xx"
97+
},
98+
"leadingTrivia": [],
99+
"trailingTrivia": [],
100+
"presence": "Present"
101+
},
102+
null
103+
],
104+
"presence": "Present"
105+
}
106+
],
107+
"presence": "Present"
108+
}
109+
],
110+
"presence": "Present"
111+
},
112+
null,
113+
null
114+
],
115+
"presence": "Present"
116+
}
117+
],
118+
"presence": "Present"
119+
},
120+
{
121+
"id": 37,
122+
"tokenKind": {
123+
"kind": "eof",
124+
"text": ""
125+
},
126+
"leadingTrivia": [],
127+
"trailingTrivia": [],
128+
"presence": "Present"
129+
}
130+
],
131+
"presence": "Present"
132+
}

‎test/incrParse/extend-identifier-at-eof.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
// RUN: %incr-transfer-tree --expected-incremental-syntax-tree %S/Outputs/extend-identifier-at-eof.json %s
2+
3+
func foo() {}
4+
// ATTENTION: This file is testing the EOF token.
5+
// DO NOT PUT ANYTHING AFTER THE CHANGE, NOT EVEN A NEWLINE
6+
_ = x<<<|||x>>>

‎test/incrParse/multi-edit-mapping.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %validate-incrparse %s --test-case MULTI
3+
4+
let one: Int;<reparse MULTI>let two: Int; let three: Int; <<MULTI<||| >>><<MULTI<||| >>>let found: Int;</reparse MULTI>let five: Int;

‎test/incrParse/simple.swift

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,16 @@
1313
// RUN: %validate-incrparse %s --test-case LAST_CHARACTER_OF_STRUCT
1414
// RUN: %validate-incrparse %s --test-case ADD_ARRAY_CLOSE_BRACKET
1515
// RUN: %validate-incrparse %s --test-case ADD_IF_OPEN_BRACE
16+
// RUN: %validate-incrparse %s --test-case EXTEND_IDENTIFIER
1617

1718
func start() {}
1819

1920
<reparse REPLACE>
2021
func foo() {
2122
}
2223

23-
_ = <<REPLACE<6|||7>>></reparse REPLACE>
24-
_ = <<REPLACE_BY_LONGER<6|||"Hello World">>>
24+
_ = <<REPLACE<6|||7>>>
25+
_ = <<REPLACE_BY_LONGER<6|||"Hello World">>></reparse REPLACE>
2526
_ = <<REPLACE_BY_SHORTER<"Hello again"|||"a">>>
2627
<<INSERT<|||foo()>>>
2728
<<REMOVE<print("abc")|||>>>
@@ -52,3 +53,5 @@ var computedVar: [Int] {
5253
if true <<ADD_IF_OPEN_BRACE<|||{>>>
5354
_ = 5
5455
}
56+
57+
let y<<EXTEND_IDENTIFIER<|||ou>>> = 42

‎tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1996,7 +1996,7 @@ static unsigned resolveFromLineCol(unsigned Line, unsigned Col,
19961996
exit(1);
19971997
}
19981998
Ptr = LineStart;
1999-
for (; Ptr < End; ++Ptr) {
1999+
for (; Ptr <= End; ++Ptr) {
20002000
--Col;
20012001
if (Col == 0)
20022002
return Ptr - InputBuf->getBufferStart();

‎tools/swift-syntax-test/swift-syntax-test.cpp

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -246,11 +246,10 @@ struct ByteBasedSourceRange {
246246

247247
bool empty() { return Start == End; }
248248

249-
SourceRange toSourceRange(SourceManager &SourceMgr, unsigned BufferID) {
249+
CharSourceRange toCharSourceRange(SourceManager &SourceMgr, unsigned BufferID) {
250250
auto StartLoc = SourceMgr.getLocForOffset(BufferID, Start);
251-
// SourceRange includes the last offset, we don't. So subtract 1
252-
auto EndLoc = SourceMgr.getLocForOffset(BufferID, End - 1);
253-
return SourceRange(StartLoc, EndLoc);
251+
auto EndLoc = SourceMgr.getLocForOffset(BufferID, End);
252+
return CharSourceRange(SourceMgr, StartLoc, EndLoc);
254253
}
255254
};
256255

@@ -539,12 +538,11 @@ bool verifyReusedRegions(ByteBasedSourceRangeSet ExpectedReparseRegions,
539538
bool NoUnexpectedParse = true;
540539

541540
for (auto ReparseRegion : UnexpectedReparseRegions.Ranges) {
542-
auto ReparseRange = ReparseRegion.toSourceRange(SourceMgr, BufferID);
541+
auto ReparseRange = ReparseRegion.toCharSourceRange(SourceMgr, BufferID);
543542

544543
// To improve the ergonomics when writing tests we do not want to complain
545544
// about reparsed whitespaces.
546-
auto RangeStr =
547-
CharSourceRange(SourceMgr, ReparseRange.Start, ReparseRange.End).str();
545+
auto RangeStr = ReparseRange.str();
548546
llvm::Regex WhitespaceOnlyRegex("^[ \t\r\n]*$");
549547
if (WhitespaceOnlyRegex.match(RangeStr)) {
550548
continue;

‎unittests/Parse/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ add_swift_unittest(SwiftParseTests
22
BuildConfigTests.cpp
33
LexerTests.cpp
44
LexerTriviaTests.cpp
5+
SyntaxParsingCacheTests.cpp
56
TokenizerTests.cpp
67
)
78

0 commit comments

Comments
 (0)