Allow defining alignment in indent queries

[Triton171]

Add an @align indent capture that aligns everything inside some node to a given @anchor. I added indent queries for aligning function parameters in C/C++ and python. In C, this makes the following possible (in python, it's essentially the same):

void func_with_many_args(int first_argument, int second_argument, |int third_argument);
// Pressing Enter results in the following, correctly aligned code:
void func_with_many_args(int first_argument, int second_argument,
                         int third_argument);

// The correct indentation is also computed when the last argument hasn't been added yet.
// Pressing Enter here results in the same indentation:
void func_with_many_args(int first_argument, int second_argument,|);

This should (partially) resolve #3196. I tried using the @align capture to write indent queries for haskell but this is a lot more complicated than for other languages. It should be possible without adding anything else to the indentation system but there's still some work to do. It's probably best to do that in a separate PR.

I also tweaked the rust indent queries a bit so that they now correctly guess the indentation for helix-term/src/commands.rs (which is nice since that's a large and fairly complicated file). It doesn't seem to require any alignment any more, probably due commands.rs having been changed since I last tested the indent queries on it. The changes to the queries are not too complicated but if it makes it easier to review, I can of course split them into a separate PR.

[teburd]

I guess my questions here are...

can this be used to align struct members in C, escapes in multiline macros, and not indent ifdefs and such.

Some complex C macros confuse even emacs which is annoying, for example:
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/rtio/rtio.h#L426

But otherwise alignment of not only the first item on the line but also alignment of the escape for multi-line macros is quite nice to have (githubs viewer fails to show reality, but this macro has its backslashes aligned given an appropriate tab spacing)

https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/rtio/rtio.h#L410

[Triton171]

Aligning struct members should work; for example with the following query:

(struct_specifier
    (field_declaration_list
        .
        (field_declaration) @anchor)) @align

Setting the indent for ifdefs to 0 is not yet possible. The issue is that (in this implementation) we can only align to other tree-sitter nodes. One could write queries that align preproc_ifdef to its surrounding translation_unit but this would have to be done for every nesting level, so it would fail for a sufficiently deep syntax tree. We could of course allow to specify some constant alignment to solve this issue, or maybe only add some special capture for zero alignment.

Aligning escapes in multiline macros is also not possible and definitely the trickiest to implement: Currently the indentation system only handles leading whitespace in lines, so we would need to add a way to specify alignment inside a line (probably a new capture type). The issue is that at the moment the indentation queries are only run when inserting a new line. To allow for alignment, they would have to be run every time the syntax tree changes in a significant way. I've thought about doing something that in order to retroactively update incorrect indentation but there's some challenges to this. At the moment, it would probably be easier to implement the alignment of macro escapes in an external formatter.

[Triton171]

text_to_visual_pos would definitely be a simpler solution. I decided against it because it breaks alignment for different tab-widths:

class foo {
    // Assume that this is indented with a single tab
    void bar(int parm1,
             int parm2);
}

We want int parm1 and int parm2 to align here. If someone with a different (visual) tab width setting edits the code, it should still be aligned for them (if we assume that everyone always uses the same tab width, we might as well use spaces for indentation). This means that in this case the whitespace before int parm2 should consist of a single tab, followed by 9 spaces.

If we also want this to work when the previous line starts with an arbitrary sequence of tabs and spaces, we could change the type of Indentation::align to Option<(String, usize)>: The string simply stores the leading whitespace of the line that we want to align to and the usize stores the number of spaces we have to add after it. To make it even simpler, it could also just be an Option<String> where the spaces are already included. It's a bit less efficient since we need to allocate a String but that shouldn't really be a problem (we only allocate a single string if we want to align to something, which isn't the case for most indent queries).

[pascalkuthe]

Ah then I would go with a String you have to create a String when inserting the edit anyway so you need to alloc either way

[Triton171]

The new commits should now align correctly even for arbitrary sequences of tabs & spaces. This requires us to take Indentation by value (to avoid cloning the string) and IndentCaptureType is no longer Copy for the same reason. Because we only iterate over each Capture once, it's not really an issue.

[pascalkuthe]

except for one minor comment this looks pretty good now to me 👍

Btw. I am a bit hesitant about including copies of commands.rs and similar as testcases. Couldn't we just check that we indent the entire helix codebase correctly without keeping a copy of that file? RA has a similar testcases where they make sure that RA cad parse itself.

[Triton171]

Btw. I am a bit hesitant about including copies of commands.rs and similar as testcases. Couldn't we just check that we indent the entire helix codebase correctly without keeping a copy of that file?

Both approaches have advantages and disadvantages imo. Copying the file is of course a bit redundant because we store almost the same file twice. On the other hand, if we use files for testing without copying them, this means that anyone changing these files can get test failures if the indent queries are incorrect on the new version (even if their changes are perfectly reasonable). Since we'll probably never get 100% accuracy on the indent queries, this is likely to happen at some point.

In fact, this has already happened since there are some test failures in the new version of commands.rs. I'll try to fix the queries but I'm still not sure if we should really always use the latest version of these files for indent testing.

[pascalkuthe]

Btw. I am a bit hesitant about including copies of commands.rs and similar as testcases. Couldn't we just check that we indent the entire helix codebase correctly without keeping a copy of that file?

Both approaches have advantages and disadvantages imo. Copying the file is of course a bit redundant because we store almost the same file twice. On the other hand, if we use files for testing without copying them, this means that anyone changing these files can get test failures if the indent queries are incorrect on the new version (even if their changes are perfectly reasonable). Since we'll probably never get 100% accuracy on the indent queries, this is likely to happen at some point.

In fact, this has already happened since there are some test failures in the new version of commands.rs. I'll try to fix the queries but I'm still not sure if we should really always use the latest version of these files for indent testing.

Yeah that's definitely a concern too. Since we have git as a built requirement you could instead read the files from git:

Simply read the stdout from git cat-file blob <commit-hash>:<path>.I think running on the entire codebase would in fact be very useful. For example it looks like the new test case actually caught a regression in the rust query changes you made (at least the line that shows up in the test failure indents correctly on master).

Having a large test case like this that runs on a large evolving real-world codebase has the potential to nicely flush out edge cases. We can bump the pinned commit from time to time and if there are some code that truly can't be indented correctly we could probably add a way some known-bad lines

[pascalkuthe]

This looks really great, thanks for going the extra mile with the testing! Looks like you found quite a few edgecases in the rust queries

[Triton171]

Thanks for the reviews 😄. The combination of call_expression and field_expression still needs a bit of work for some rare edge cases, but other than that the rust queries seem pretty robust now.

I just saw #7817 and added a commit that fixes it, since it needed only one simple pattern.

[Triton171]

Rebased it on top of #6768 to fix the conflicts in helix-core/src/indent.rs, all other files had no/only trivial conflicts. I also cleaned up my commit history a bit while I was at it, so this should be ready to merge now.

[pascalkuthe]

It looks like some of the yaml tests are still failing in CI

[archseer]

Great work! LGTM