Skip to content

Commit

Permalink
Dependency tracking between nodes is too coarse grained (enso-org#11428)
Browse files Browse the repository at this point in the history
close enso-org#11237

Changelog:
- update: implement special case for a line removal when calculating the changeset

# Important Notes
Note that the graph is still re-calculated when the node is re-added (by pressing `ctrl-z`). The reason is that the engine processes edits on the textual level and there is not enough information to do similar workarounds. The issue becomes irrelevant when we switch to the direct tree manipulation in Ydoc.

https://github.com/user-attachments/assets/c85afde8-6386-44df-82b5-6fb0cca5205b
  • Loading branch information
4e6 authored Oct 29, 2024
1 parent 15575b4 commit 74220f2
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,19 @@ final class ChangesetBuilder[A: TextEditor: IndexedSource](
val edit = edits.dequeue()
val locationEdit = ChangesetBuilder.toLocationEdit(edit, source)
var invalidatedSet =
ChangesetBuilder.invalidated(tree, locationEdit.location, true)
ChangesetBuilder.invalidated(
tree,
locationEdit.location,
locationEdit.isNodeRemoved,
true
)
if (invalidatedSet.isEmpty) {
invalidatedSet =
ChangesetBuilder.invalidated(tree, locationEdit.location, false)
invalidatedSet = ChangesetBuilder.invalidated(
tree,
locationEdit.location,
locationEdit.isNodeRemoved,
false
)
}
val newTree = ChangesetBuilder.updateLocations(tree, locationEdit)
val newSource = TextEditor[A].edit(source, edit)
Expand Down Expand Up @@ -260,8 +269,13 @@ object ChangesetBuilder {
*
* @param location the location of the edit
* @param length the length of the inserted text
* @param isNodeRemoved the flag indicating that the edit removes a node
*/
private case class LocationEdit(location: Location, length: Int) {
private case class LocationEdit(
location: Location,
length: Int,
isNodeRemoved: Boolean
) {

/** The difference in length between the edited text and the inserted text.
* Determines how much the rest of the text will be shifted after applying
Expand Down Expand Up @@ -409,19 +423,50 @@ object ChangesetBuilder {
/** Calculate the invalidated subset of the tree affected by the edit by
* comparing the source locations.
*
* The `isNodeRemoved` argument covers the case when the user removes a
* single line, for example:
*
* {{{
* 0|main =
* |
* 1| x = 0
* | ^^^^^^
* 2| y = 1
* |^^^^
* 3| y
* }}}
*
* In this case, when removing the line (1) `x = 0`, the expression `y = 1`
* on the line (2) should not be affected by the edit because it causes
* invalidation of all the subsequent expressions in the body of `main`
* function. Instead, the algorithm detects that only the `x` variable name
* was changed and later finds all its usages through the `DataflowAnalysis`
* metadata. Also note that we ignore the right hand side of the `x = ...`
* binding because the removal of rhs expression does not affect other
* expressions in the `main` body, while the usage of a common symbol, i.e.
* `foo`:
* {{{
* x = foo
* y = foo
* }}}
* will lead to the invalidation of the `y` expression as well (when looking
* for dynamic usages of the `foo` symbol) which is unwanted.
*
* @param tree the source tree
* @param edit the location of the edit
* @param isNodeRemoved flag indicating that the edit removes a single node
* @return the invalidated nodes of the tree
*/
private def invalidated(
tree: Tree,
edit: Location,
isNodeRemoved: Boolean,
onlyLeafs: Boolean
): Tree = {
val invalidated = mutable.TreeSet[ChangesetBuilder.Node]()
tree.iterator.foreach { node =>
if (!onlyLeafs || node.leaf) {
if (intersect(edit, node)) {
if (intersect(edit, node, isNodeRemoved)) {
invalidated += node
tree -= node
}
Expand All @@ -438,12 +483,14 @@ object ChangesetBuilder {
*/
private def intersect(
edit: Location,
node: ChangesetBuilder.Node
node: ChangesetBuilder.Node,
isNodeRemoved: Boolean
): Boolean = {
intersect(edit, node.location)
if (isNodeRemoved) intersectWhenNodeRemoved(edit, node.location)
else intersect(edit, node.location)
}

/** Check if the node location intersects the edit location.
/** Check if the node location intersects or borders with the edit location.
*
* @param edit location of the edit
* @param node location of the node
Expand All @@ -456,7 +503,23 @@ object ChangesetBuilder {
inside(edit.end, node)
}

/** Check if the character position index is inside the location.
/** Check if the node location intersects the edit that removes the line.
*
* In this case we assume that the edit removes the binding
* `name = expression`, and we only interested in detecting the `name` part.
*
* @param edit location of the edit
* @param node location of the node
* @return true if the node and edit locations are intersecting
*/
private def intersectWhenNodeRemoved(
edit: Location,
node: Location
): Boolean = {
node.start == edit.start && node.end < edit.end
}

/** Check if the character position index is inside or on the border of the location.
*
* @param index the character position
* @param location the location
Expand All @@ -476,7 +539,12 @@ object ChangesetBuilder {
edit: TextEdit,
source: A
): LocationEdit = {
LocationEdit(toLocation(edit, source), edit.text.length)
def isSameOffset: Boolean =
edit.range.end.character == edit.range.start.character
def isAcrossLines: Boolean =
edit.range.end.line > edit.range.start.line
val isNodeRemoved = edit.text.isEmpty && isSameOffset && isAcrossLines
LocationEdit(toLocation(edit, source), edit.text.length, isNodeRemoved)
}

/** Convert [[TextEdit]] location to [[Location]] in the provided source.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,27 @@ class ChangesetBuilderTest
)
}

"multiline remove node" in {
val code =
"""x ->
| y = foo 5
| z = foo 7
| y + x""".stripMargin.linesIterator.mkString("\n")
val edit = TextEdit(Range(Position(2, 4), Position(3, 4)), "")

val ir = code
.preprocessExpression(freshInlineContext)
.get
.asInstanceOf[Function.Lambda]

val secondLine = ir.body.children()(1).asInstanceOf[Expression.Binding]
val zName = secondLine.name

invalidated(ir, code, edit) should contain theSameElementsAs Seq(
zName.getId
)
}

"multiline insert line 1" in {
val code =
"""x ->
Expand Down Expand Up @@ -434,6 +455,7 @@ class ChangesetBuilderTest
atCode
)
}

}

def findIR(ir: IR, uuid: String): IR = {
Expand Down

0 comments on commit 74220f2

Please sign in to comment.