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

Consider actual indentation of previous line for smart indent #8307

Merged
merged 7 commits into from
Dec 15, 2023

Conversation

Triton171
Copy link
Contributor

@Triton171 Triton171 commented Sep 16, 2023

Essentially, to compute the indentation of a new line, we run an indent query both for the insertion position and for a non-empty line above. Then, we add the difference of these computed indents to the actual indentation of the line above. This makes indent queries a lot more resilient: Any error / inconsistency which applies to both lines will be canceled out, e.g.:

  • Incorrect / Incomplete indent queries. In particular, incomplete indent queries are no longer worse than no indent queries with this change. This should also make it easier to contribute new languages since one can start by adding a few simple captures and then gradually add more.
  • Incorrect parsing of the syntax tree by tree-sitter (e.g. if the code is incomplete)
  • Different formatting styles where only one can correctly be handled by indent queries. This is especially interesting for languages like Markdown and LaTex where indent queries weren't really feasible until now due to the variation in formatting.

The main thing still missing from this PR is some way to disable relative indent queries. While this shouldn't really be necessary when using Helix, relative indent computation makes debugging indent queries more difficult. Should this be an option? Or is there a better way to do it?
I also haven't tested my changes a lot, so I appreciate it if anyone wants to try it out. I'll also use it myself for some time before marking the PR ready for merging.

The only significant downside of this change is that we now compute 2 (or in rare cases 3) indent queries when inserting a new line. The time needed for this only becomes noticable when inserting a new line with many cursors at once. If this is an issue, we could disable smart indent when inserting many (e.g. > 1000) newlines at once. For now, I think it is fine though (I haven't seen any complaints about the current indent queries being slow with many cursors either).

Fixes #6235
Fixes #7206
Fixes (partially) #6939 and #1725

@Triton171 Triton171 marked this pull request as draft September 16, 2023 17:06
@Triton171 Triton171 changed the title DRAFT: Consider actual indentation of previous line for smart indent Consider actual indentation of previous line for smart indent Sep 16, 2023
@pascalkuthe pascalkuthe added E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. A-indent Area: Indentation labels Sep 16, 2023
@clo4
Copy link
Contributor

clo4 commented Sep 17, 2023

After I manually closed #8124, I realised it shouldn't have been because plain text is still an issue - this PR would also have closed that, given that it works great with plain text. Fantastic contribution, looking forward to when this is merged! It feels great to use.

@Triton171 Triton171 force-pushed the relative-indent-queries branch 2 times, most recently from 1834d8d to 0b7abe3 Compare September 19, 2023 14:16
@Triton171
Copy link
Contributor Author

I've tested the changes a bit and it seems to work fairly well. There are a few situations in which the new heuristic is worse than the old one, for example:

int main() {
    int var = complicated_expression_spanning_multiple_lines
              + which_is_aligned;
            // This is the indentation level of a newline here with the new heuristic
    // This is the expected indentation level (which the old heuristic correctly guesses)
}

This can only happen if the line above is not indented as expected and this error does not also apply to the new line. This means that it tends to happen in languages where our current indent queries are not very reliable. Since in these case the new heuristic is a lot more resilient overall, I think this is fine for now. In the future, we could think about using the syntax tree to decide which line should be used as a baseline for comparing the indent (in my implementation, this is just the previous non-blank line). It probably makes sense to do this in another PR though, since we might have more examples where my implementation is suboptimal by then.

Also, these issues can often be solved by adding missing @align captures to the indent queries, since we don't use a line for comparing the indentation if we see from the query that it has a different alignment.

book/src/configuration.md Outdated Show resolved Hide resolved
helix-core/src/indent.rs Outdated Show resolved Hide resolved
helix-core/src/indent.rs Outdated Show resolved Hide resolved
helix-core/src/indent.rs Outdated Show resolved Hide resolved
helix-core/src/indent.rs Outdated Show resolved Hide resolved
helix-core/src/indent.rs Show resolved Hide resolved
@pascalkuthe pascalkuthe added this to the next milestone Oct 19, 2023
@elrafoon
Copy link

elrafoon commented Dec 3, 2023

Works great for me!

i.e. also take into account the indentation of a previous
line when computing the indentation for a new line.
Add tests to ensure that relative & absolute indent computation are consistent.
Since the tree-sitter grammar is not very good
at parsing function calls while they're being written,
this is not yet super useful.
However, it prevents the new `hybrid` indent heuristic
from choosing these lines as a baseline, making it
more robust.
Increase hybrid indent heuristic attempt limit to 4.
Clarify the fallback logic in indent heuristic docs.
@Triton171
Copy link
Contributor Author

Thanks for the reminder that this PR still exists :D. I rebased it onto the latest master and added 2 alignment queries for C, mainly since they aid the new hybrid indent heuristic in choosing a suitable baseline. Since I've daily-driven this for the past few months at work (doing C++ development), I'm reasonably confident that the code shouldn't have any major bugs.

Some other languages might also still have missing alignment queries which could cause issues with the new indent heuristic. I think these should be rare, so we can just fix them whenever they are reported (there's not really a way to find them other than using Helix to edit code in the language for a significant amount of time).

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

As always, thanks for improving our indentation capabilities! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-indent Area: Indentation E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C++ indentation is wrong for access specifiers C/C++ namespace incorrect indentation
6 participants