Skip to content

[incrParse] Fix bug mapping a node's location back to its location in the cached syntax tree #19782

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

Merged
merged 8 commits into from
Oct 12, 2018
Merged

[incrParse] Fix bug mapping a node's location back to its location in the cached syntax tree #19782

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5c568af
[incrParse] Fix bug mapping a node's location back to its location in…
Oct 6, 2018
63becb5
[incrParse] Fix bug in translation of post-edit to pre-edit positions
ahoppen Oct 11, 2018
9d9bb53
[incrParse] Fix issue that did not report the first reparsed characte…
ahoppen Oct 11, 2018
d6fc080
[incrParse] Adjust expected reparse region for simple test case
ahoppen Oct 11, 2018
8baba29
[incrParse] Add warning not to add anything to the identifier-at-eof …
ahoppen Oct 11, 2018
0152e23
[incrParse] Fix expected syntax tree json to account for added comment
Oct 11, 2018
efa2c54
[incrParse] Update text-util.py to handle multiple edits and reparse …
Oct 11, 2018
9003d83
[gardening] convert ByteBasedSourceRange to CharSourceRange rather th…
Oct 11, 2018
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
2 changes: 1 addition & 1 deletion include/swift/Parse/SyntaxParsingCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ struct SourceEdit {
/// Check if the characters replaced by this edit fall into the given range
/// or are directly adjacent to it
bool intersectsOrTouchesRange(size_t RangeStart, size_t RangeEnd) {
return !(End <= RangeStart || Start >= RangeEnd);
return End >= RangeStart && Start <= RangeEnd;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

}
};

Expand Down
4 changes: 3 additions & 1 deletion lib/Basic/SourceLoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,9 @@ llvm::Optional<unsigned> SourceManager::resolveFromLineCol(unsigned BufferId,
return None;
}
Ptr = LineStart;
for (; Ptr < End; ++Ptr) {

// The <= here is to allow for non-inclusive range end positions at EOF
for (; Ptr <= End; ++Ptr) {
--Col;
if (Col == 0)
return Ptr - InputBuf->getBufferStart();
Expand Down
2 changes: 1 addition & 1 deletion lib/Parse/SyntaxParsingCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ llvm::Optional<Syntax> SyntaxParsingCache::lookUp(size_t NewPosition,
size_t OldPosition = NewPosition;
for (auto I = Edits.rbegin(), E = Edits.rend(); I != E; ++I) {
auto Edit = *I;
if (Edit.End <= OldPosition) {
if (Edit.End < OldPosition) {
Copy link
Member

Choose a reason for hiding this comment

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

After some more consideration this should be

if (Edit.End + Edit.ReplacementLength - Edit.originalLength() <= OldPosition) {

Explanation

With <= we don't catch the following case:

Original text: foobar
Change foo to fo resulting in fobar

The edit range is 1-4 with original length 3 and replacement length 2.
If I now try to look up bar, we start with OldPosition = NewPosition = 3. Now we need to see if that position needs to be adjusted because of the edit (which it obviously needs to be), but we get Edit.End (4) <= OldPosition (3) does not hold.

The issue is that we are comparing OldPosition which is still in the position space of the post-edit file and Edit.End which is in the position space of the pre-edit file, hence we need to translate Edit.End to the post-position space as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right! Ok, so this should make it correct for a single edit, but if there are multiple I think we'll need to account for the original-vs-replacement length differences of any edits earlier in the file too. I'll give that a go and update shortly.

OldPosition =
OldPosition - Edit.ReplacementLength + Edit.originalLength();
}
Expand Down
116 changes: 116 additions & 0 deletions test/incrParse/Outputs/extend-identifier-at-eof.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
{
"id": 39,
"kind": "SourceFile",
"layout": [
{
"id": 38,
"kind": "CodeBlockItemList",
"layout": [
{
"id": 13,
"omitted": true
},
{
"id": 35,
"kind": "CodeBlockItem",
"layout": [
{
"id": 34,
"kind": "SequenceExpr",
"layout": [
{
"id": 33,
"kind": "ExprList",
"layout": [
{
"id": 28,
"kind": "DiscardAssignmentExpr",
"layout": [
{
"id": 27,
"tokenKind": {
"kind": "kw__"
},
"leadingTrivia": [
{
"kind": "Newline",
"value": 1
}
],
"trailingTrivia": [
{
"kind": "Space",
"value": 1
}
],
"presence": "Present"
}
],
"presence": "Present"
},
{
"id": 30,
"kind": "AssignmentExpr",
"layout": [
{
"id": 29,
"tokenKind": {
"kind": "equal"
},
"leadingTrivia": [],
"trailingTrivia": [
{
"kind": "Space",
"value": 1
}
],
"presence": "Present"
}
],
"presence": "Present"
},
{
"id": 32,
"kind": "IdentifierExpr",
"layout": [
{
"id": 31,
"tokenKind": {
"kind": "identifier",
"text": "xx"
},
"leadingTrivia": [],
"trailingTrivia": [],
"presence": "Present"
},
null
],
"presence": "Present"
}
],
"presence": "Present"
}
],
"presence": "Present"
},
null,
null
],
"presence": "Present"
}
],
"presence": "Present"
},
{
"id": 37,
"tokenKind": {
"kind": "eof",
"text": ""
},
"leadingTrivia": [],
"trailingTrivia": [],
"presence": "Present"
}
],
"presence": "Present"
}
4 changes: 4 additions & 0 deletions test/incrParse/extend-identifier-at-eof.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// RUN: %incr-transfer-tree --expected-incremental-syntax-tree %S/Outputs/extend-identifier-at-eof.json %s

func foo() {}
_ = x<<<|||x>>>
Copy link
Member

Choose a reason for hiding this comment

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

I would add comment to this file explaining that no trailing newline should be added to the end of it.

7 changes: 5 additions & 2 deletions test/incrParse/simple.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,16 @@
// RUN: %validate-incrparse %s --test-case LAST_CHARACTER_OF_STRUCT
// RUN: %validate-incrparse %s --test-case ADD_ARRAY_CLOSE_BRACKET
// RUN: %validate-incrparse %s --test-case ADD_IF_OPEN_BRACE
// RUN: %validate-incrparse %s --test-case EXTEND_IDENTIFIER

func start() {}

<reparse REPLACE>
func foo() {
}

_ = <<REPLACE<6|||7>>></reparse REPLACE>
_ = <<REPLACE_BY_LONGER<6|||"Hello World">>>
_ = <<REPLACE<6|||7>>>
_ = </reparse REPLACE><<REPLACE_BY_LONGER<6|||"Hello World">>>
Copy link
Member

Choose a reason for hiding this comment

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

Putting the </reparse REPLACE> before the 6 seemed really odd to me and it turns out there is another off-by-one error here, where we don't report the last unexpectedly reparsed character.

Investigating further, I noticed that we need to remove the - 1 in swift-syntax-test.cpp:252 plus its corresponding comment. I don't know if that's related to some of your changes on SourceLoc or has been a bug ever since, but it fixed the issue. After that the end repairs marker also needs to go at the end of the line.

_ = <<REPLACE_BY_SHORTER<"Hello again"|||"a">>>
<<INSERT<|||foo()>>>
<<REMOVE<print("abc")|||>>>
Expand Down Expand Up @@ -52,3 +53,5 @@ var computedVar: [Int] {
if true <<ADD_IF_OPEN_BRACE<|||{>>>
_ = 5
}

let y<<EXTEND_IDENTIFIER<|||ou>>> = 42
2 changes: 1 addition & 1 deletion tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1996,7 +1996,7 @@ static unsigned resolveFromLineCol(unsigned Line, unsigned Col,
exit(1);
}
Ptr = LineStart;
for (; Ptr < End; ++Ptr) {
for (; Ptr <= End; ++Ptr) {
--Col;
if (Col == 0)
return Ptr - InputBuf->getBufferStart();
Expand Down