Skip to content
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.

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

fix: handle more cases of editing subtrees that depend on column values #3257

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rooney
Copy link
Contributor

@rooney rooney commented Apr 4, 2024

Reopen #3218

Copy link
Contributor

@maxbrunsfeld maxbrunsfeld left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @rooney. I left some minor suggestions.

test/fixtures/test_grammars/depends_on_column/scanner.c Outdated Show resolved Hide resolved
test/fixtures/test_grammars/depends_on_column/grammar.json Outdated Show resolved Hide resolved
) && (
!ts_subtree_depends_on_column(*child) ||
!column_shifted ||
child_left.extent.row > edit.old_end.extent.row
Copy link
Contributor

Choose a reason for hiding this comment

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

This new condition makes good sense. Did you check if you can actually remove the previous condition? I would think that you'd only need to check depends_on_column in one spot. Do any tests fail if you remove the second part of this 3-part && expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you check if you can actually remove the previous condition?

Yea I did, and I can't: it would make the Haskell-like indentation test fail -- I originally thought that it's the test that were wrong.

I would think that you'd only need to check depends_on_column in one spot.

That would be the case if trees that depend-on-column make their supertree depend-on-column too. But that's not the case because currently the condition for setting the flag is:

// subtree.c line 394
    if (
      self.ptr->size.extent.row == 0 &&
      ts_subtree_depends_on_column(child)
    ) {
      self.ptr->depends_on_column = true;
    }

So it's possible for a tree to NOT depend on column, but it contains subtree that DOES. (I find that counterintuitive, but maybe there's a reason for that?).
That's why we need to check depends_on_column on two spots: one when iterating the bisecting-stack, and another when delving into the children -- the condition needs to be re-checked for each subtree.

Now, I did try changing the flag-setting code to:

// subtree.c line 394
    if (
      ts_subtree_depends_on_column(child)
    ) {
      self.ptr->depends_on_column = true;
    }
  • All the tests still pass after this change.
  • It's more intuitive: if a tree depends-on-column, then its supertree depends-on-column too.
  • On subtree-edit, we just need to check the flag at one spot only.

But is it OK? If so, I'll make the change.

Do any tests fail if you remove the second part of this 3-part && expression?

No, all tests still pass without the second condition -- it's an optimization for when the column positions does not change e.g. if the length of inserted_text == deleted_length. Do you think it's incorrect/premature? Or is it ok but I should add a test for that (in the form of a recorder reading fewer bytes)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, then almost the entire tree would depend on its column.

A tree only depends on its start column of any of its children that start on its first line depend on their column. For all subsequent children, the tree's start column is decoupled from their column.

cli/src/tests/parser_test.rs Outdated Show resolved Hide resolved
@ObserverOfTime ObserverOfTime added this to the 0.22 milestone Apr 11, 2024
@ObserverOfTime ObserverOfTime modified the milestones: 0.22, 0.24, 1.0 May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants