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

nathawes · 2018-10-08T23:28:28Z

Also fix Edit::intersectsOrTouchesRange check only returning true when the ranges overlapped, rather than when they overlapped or touched. This was causing an edit that extended an existing identifier, for example, to reuse the existing node, and parse the remaining characters as a separate node in the tree.

Resolves rdar://problem/45108439

… the cached syntax tree Also fix Edit::intersectsOrTouchesRange check only returning true when the ranges overlapped, rather than when they overlapped or 'touched'. Resolves rdar://problem/45108439

nathawes · 2018-10-08T23:28:48Z

@swift-ci please smoke test

nkcsgexi

Good catch! Does the stress tester test other mutation patterns than extending a token?

ahoppen · 2018-10-09T16:18:43Z

I'd like to take a closer look at this since I know that I had to switch around some < and <= in my initial implementation as well. I think I'll get to it tomorrow morning CEST.

nathawes · 2018-10-09T18:05:40Z

That would be great, thanks @ahoppen!

Here's the commit where the new -> old location computation's condition was changed from < to <= if that helps: b26dd11

ahoppen

A couple comments inline that fix some more underlying bugs that have just surfaced.

ahoppen · 2018-10-10T07:03:03Z

include/swift/Parse/SyntaxParsingCache.h

@@ -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;


Good catch!

ahoppen · 2018-10-10T07:10:29Z

lib/Parse/SyntaxParsingCache.cpp

@@ -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) {


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.

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.

ahoppen · 2018-10-10T07:11:12Z

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

+// RUN: %incr-transfer-tree --expected-incremental-syntax-tree %S/Outputs/extend-identifier-at-eof.json %s
+
+func foo() {}
+_ = x<<<|||x>>>


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

ahoppen · 2018-10-10T07:39:08Z

test/incrParse/simple.swift

-_ = <<REPLACE<6|||7>>></reparse REPLACE>
-_ = <<REPLACE_BY_LONGER<6|||"Hello World">>>
+_ = <<REPLACE<6|||7>>>
+_ = </reparse REPLACE><<REPLACE_BY_LONGER<6|||"Hello World">>>


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.

ahoppen · 2018-10-10T18:07:27Z

That should be taken care of by traversing the edits in reverse, which we are doing. Unless I’m completely mistaking.

…

On 10. Oct 2018, at 20:05, Nathan Hawes ***@***.***> wrote: @nathawes commented on this pull request. In lib/Parse/SyntaxParsingCache.cpp: > @@ -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) { 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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

nathawes · 2018-10-10T18:17:37Z

Ok, I might be misunderstanding how multiple edits are meant to be understood. If I we have two edits in source order is the second edit positioned:

in terms of the original file, or
in terms of the original file after the first edit was applied?

I was thinking it was (1), where I think doing it in reverse wouldn't help, but if it's (2) you're right.

…r as reparsed

…test

ahoppen · 2018-10-11T16:06:45Z

All changes are in terms of the original file. But because we are now speaking in terms of the post-edit position space, we need to traverse the edits in normal order again (not reverse order) to get the correct transformation. I extracted the logic, wrote a couple test cases for it and pushed it into this PR. While I was at it, I also addressed my other review comments.

Sorry for pushing into your branch, but it felt like this is cleaner than creating a new PR on which you would then need to rebase.

ahoppen · 2018-10-11T16:06:51Z

@swift-ci Please test

swift-ci · 2018-10-11T16:08:56Z

Build failed
Swift Test OS X Platform
Git Sha - 5c568af

swift-ci · 2018-10-11T16:09:31Z

Build failed
Swift Test Linux Platform
Git Sha - 5c568af

nathawes · 2018-10-11T19:28:17Z

No problem about pushing into the PR and thanks for adding the unit tests! The transformation looks good to me now, too. I came up with a different solution yesterday:

  size_t AccumulatedShift = 0;
  for (auto I = Edits.begin(), E = Edits.end(); I != E; ++I) {
    auto Edit = *I;
    size_t Shift = Edit.ReplacementLength - Edit.originalLength();
    if (Edit.End + AccumulatedShift + Shift <= NewPosition) {
      AccumulatedShift += Shift;
    } else {
      break;
    }
   }
  return /*OldPosition =*/ NewPosition - AccumulatedShift;

But they're equivalent, anyway (this one just applies the previous edits and current edit on the left hand side of the <= comparison, while yours applies the current edit on the left hand side, and un-applies the previous edits on the right hand side).

The test failure is just the expected syntax tree json needing to be updated for the 'don't add a new line' comment Alex added in one of the tests. I'll update it shortly and merge if all goes well.

…ranges on the same line It wasn't accounting for the prefix length before a reparse tag previously when keeping track of pre_column_offset and post_column_offset.

…an SourceRange The only client of the 'toSourceRange' method immediately constructs a CharSourceRange from it, and ByteBasedSourceRange's EndLoc isn't the start of the last token in the range (as SourceRange expects).

nathawes · 2018-10-11T21:52:23Z

@swift-ci please test

swift-ci · 2018-10-11T21:54:11Z

Build failed
Swift Test Linux Platform
Git Sha - 8baba29

swift-ci · 2018-10-11T21:54:12Z

Build failed
Swift Test OS X Platform
Git Sha - 8baba29

nathawes · 2018-10-12T02:09:19Z

@swift-ci please test

swift-ci · 2018-10-12T03:23:23Z

Build failed
Swift Test Linux Platform
Git Sha - 9003d83

swift-ci · 2018-10-12T04:16:40Z

Build failed
Swift Test OS X Platform
Git Sha - 9003d83

After swiftlang/swift#19782 is merged, statements adjacent to an edit will no longer be reused. Hence the REPLACE_BY_LONGER line is expected to be reparsed.

ahoppen · 2018-10-12T12:55:13Z

We need to adjust a test case in Swift Syntax to expect the extended expected reparse range.

Please test with following pull request:
swiftlang/swift-syntax#18

@swift-ci Please test

swift-ci · 2018-10-12T14:09:22Z

Build failed
Swift Test Linux Platform
Git Sha - 9003d83

ahoppen · 2018-10-12T16:08:18Z

Linux test failure seems unrelated.

nathawes · 2018-10-12T21:31:30Z

Please test with following pull request:
swiftlang/swift-syntax#18

@swift-ci Please smoke test Linux

[incrParse] Fix bug mapping a node's location back to its location in…

5c568af

… the cached syntax tree Also fix Edit::intersectsOrTouchesRange check only returning true when the ranges overlapped, rather than when they overlapped or 'touched'. Resolves rdar://problem/45108439

nathawes requested review from ahoppen and nkcsgexi October 8, 2018 23:28

nkcsgexi approved these changes Oct 8, 2018

View reviewed changes

ahoppen requested changes Oct 10, 2018

View reviewed changes

ahoppen added 4 commits October 11, 2018 17:54

[incrParse] Fix bug in translation of post-edit to pre-edit positions

63becb5

[incrParse] Fix issue that did not report the first reparsed characte…

9d9bb53

…r as reparsed

[incrParse] Adjust expected reparse region for simple test case

d6fc080

[incrParse] Add warning not to add anything to the identifier-at-eof …

8baba29

…test

Nathan Hawes added 3 commits October 11, 2018 14:17

[incrParse] Fix expected syntax tree json to account for added comment

0152e23

[incrParse] Update text-util.py to handle multiple edits and reparse …

efa2c54

…ranges on the same line It wasn't accounting for the prefix length before a reparse tag previously when keeping track of pre_column_offset and post_column_offset.

ahoppen approved these changes Oct 12, 2018

View reviewed changes

ahoppen mentioned this pull request Oct 12, 2018

Extend the expected reparse range for REPLACE test case swiftlang/swift-syntax#18

Merged

nathawes merged commit b1c8013 into swiftlang:master Oct 12, 2018

nathawes deleted the incremental-parsing-bug branch October 12, 2018 22:44

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

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

Uh oh!

Conversation

nathawes commented Oct 8, 2018

Uh oh!

nathawes commented Oct 8, 2018

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Oct 9, 2018

Uh oh!

nathawes commented Oct 9, 2018

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

ahoppen Oct 10, 2018

Choose a reason for hiding this comment

Explanation

Uh oh!

nathawes Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

ahoppen Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

ahoppen Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Oct 10, 2018 via email

Uh oh!

nathawes commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Oct 11, 2018

Uh oh!

ahoppen commented Oct 11, 2018

Uh oh!

swift-ci commented Oct 11, 2018

Uh oh!

swift-ci commented Oct 11, 2018

Uh oh!

nathawes commented Oct 11, 2018

Uh oh!

nathawes commented Oct 11, 2018

Uh oh!

swift-ci commented Oct 11, 2018

Uh oh!

swift-ci commented Oct 11, 2018

Uh oh!

nathawes commented Oct 12, 2018

Uh oh!

swift-ci commented Oct 12, 2018

Uh oh!

swift-ci commented Oct 12, 2018

Uh oh!

ahoppen commented Oct 12, 2018

Uh oh!

swift-ci commented Oct 12, 2018

Uh oh!

ahoppen commented Oct 12, 2018

Uh oh!

nathawes commented Oct 12, 2018

Uh oh!

Uh oh!

nathawes commented Oct 10, 2018 •

edited

Loading