Fix python indentation

[Triton171]

This introduces 2 new capture types for indent queries. With this, the python indent queries have been improved and re-enabled. It should fix most of the issues from #763. We still don't update the indentation retroactively, so there is still some room for improvements:

if condition:
    do_smth()
    else:

Here the else should ideally be outdented automatically, once one has finished typing it. This is tricky for 2 reasons:

• We have to figure out when and for which lines to run indentation queries. For performance reasons, we don't want to do it too often and it also shouldn't "correct" the indentation after the user has manually changed it.
• With python, the tree-sitter grammar doesn't even recognize the else statement since it is indented (see the discussion in Python file indentation is wrong #763). Either the tree-sitter grammar needs to improve here or we need a special case to recognize this and similar situations.

If you have ideas for this, please let me know. Also, if there's other languages than python that can be fixed with the new captures (i.e. languages where functions, classes, etc. end simply where the indentation level decreases), I'm happy to update them as well

[AceofSpades5757]

It's also causing an issue where it'll automatically update at the line before indentation, such as if you put your cursor at the beginning of if condition: in your example and press enter to move it down a line.

P.S. This may have also been in the current implementation. I don't usually code Python with Helix due to the really annoying indentation issues. And thanks for your work!

[Triton171]

@AceofSpades5757 good find! The issue already existed in the old version but it's a somewhat rare edge case, so that's probably why it hasn't popped up before. The latest commit should fix it.

[AceofSpades5757]

No problem. I've had this version installed while working with Python to test it out. Python is a real sore spot for Helix. It feels like I'm constantly fighting it.

Hopefully this PR will be able to change that 😄

[AceofSpades5757]

This has been working pretty well, except for an issue where dedenting with < will be multiple indents rather than a single one. It sounds like some other people have this happen with other languages as well.

[Triton171]

That's interesting, the code for dedenting is pretty simple and doesn't use the indentation heuristic. If you can reproduce this, it'd probably be best to create a new issue for this.

[AceofSpades5757]

@Triton171 I haven't been able to reproduce it and I've been using it pretty extensively. I may have accidentally re-installed master as my default Helix. Everything has been going swimmingly recently but if something reproducible comes up, I'll comment here (or make a new issue).

[Dispersia]

Scala has the same issue, and this appears to work mostly correct for it too, though I am getting a strange behavior

When trying to add the indent functionality to scala's indents.scm, i'm currently adding

[
    (function_definition)
    (val_definition)
] @extend-indented

with just this, it seems to work correctly, however when trying to add stop-extend

[
    (identifier)
] @stop-extend

it seems to not work at all and was like its previous behavior. Even having an empty @stop-extend like [ ] @stop-extend does this, which I would expect does nothing

[Triton171]

Could you provide an example where the current indentation for Scala is incorrect? I'm not familiar with the language but from a quick look it seems like Scala uses blocks (delimited by curly braces) to define the range of functions. Because of this, I don't think we need @extend-indented here. Adding (function_definition) @extend-indented would cause indented lines that come after the function definition to be considered part of the function, which is incorrect in this case.

EDIT: I just saw that Scala 3 has multi-line methods without blocks. In this case, we do need the @extend-indented the way you used it. The issue with your @stop-extend query is that not every identifier marks the end of a function. I think we only need to add "}" @stop-extend, which should prevent any issues with the block syntax. We can't use stop-extend the same way I did for Python since Scala doesn't have a dedicated return statement.

I'm also not sure if the Tree-Sitter grammar actually supports multi-line methods: Judging from the syntax highlighting and the :tree-sitter-scopes command, there seems to be some weirdness going on there (the val keyword is parsed as an identifier). Maybe you could check that this actually works correctly. I have never used Scala so maybe I'm just misinterpreting something about the syntax.

[Dispersia]

No you're correct, that syntax for scala 3 is new and not supported by the current tree-sitter syntax, I will be working on that next, I'm just trying to get it to work for what is the current scala 2+ syntax. In scala, you will typically write a one line function as lamda's in C#, something like

val mappedUnit =
    ZIO.succeed(ZStream.fromStream(stream))

etc, where its identifier will go on a new line, not on its current, so it should always be indented. If the user chooses to wrap it in a block, e.g.

val mappedUnit = {
    ZIO.succeed(ZStream.fromStream(stream))
}

it should still count as only one indentation, which is what currently happens correctly. However, when I add even an empty stop-extend, the original extend-indented stops working completely, if I put that line at all in the indents file the val mappedUnit =\n doesn't indent anymore (or if i put a random value, just incase it doesn't like being empty). I am trying to make it work like how current neovim does, which will end the indentation after the first line with a new identifier, effectively making the first statement "return" even if it isnt.

How I might try to achieve this in the long run is the last identifier for tree-sitter will be replaced with a wrapped return type, and then i could do that behavior in here, but im more worried about the common case (as multi line blocks for classes/objects/functions/vals aren't common yet)

[Triton171]

The weird behavior with @stop-extend happens because you're writing an invalid indent query: Tree-Sitter doesn't allow empty alternations (for a description of the query syntax, see their documentation). Unfortunately, this currently fails silently and simply uses the fallback method of copying the indentation of the current line.

For getting the indentation right, you should definitely try to improve the Tree-Sitter grammar before tinkering with it in Helix. Once multiline functions are parsed correctly, you could also use something like (function_definition (identifier) @stop-extend), which would only match an identifier node nested directly inside a function. This prevents e.g. an identifier that's part of an assignment expression to be matched. Basically, you have to think about what exactly makes an expression a "return expression" after which the function ends. Tree-Sitter queries are surprisingly powerful, so once you have an idea of what exactly you want in your head, it often isn't too difficult to write a query for it.

If you manage to fix the grammar and need help with indent queries in Helix, feel free to ping me.

[the-mikedavis]

Unfortunately, this currently fails silently and simply uses the fallback method of copying the indentation of the current line.

Recently we added an error log message for this. There isn't a visual indication as there is for syntax highlighting (i.e. no highlights) but you should be able to find out if your indent queries are invalid either by checking the log file or running cargo xtask query-check if you are building from source and the modified injections are under this repo's runtime/ dir

[the-mikedavis]

I think this block can be simplified a bit - query_range can be calculated after prev_child is determined with a map+unwrap_or. So the whole let (query_range, prev_child) is not needed

[Triton171]

Agreed. I also noticed that the treesitter parser was borrowed unnecessarily early, so I fixed that too.

[the-mikedavis]

I think the names could use some workshopping - I like "extend" but @stop-extend doesn't convey well that it only stops the extension once

[the-mikedavis]

Maybe @indent.extend and then @indent.extend.pause? Or @extend and @extend.pause-once? I also liked "after" from the discussion in the issue

[Triton171]

@extend sounds good. In case we add more "extend" captures types, we can still rename it to something more specific later. Maybe @extend.prevent-once is a more accurate description of what the other capture does? Or @extend.end-once?

[zyklotomic]

Just a question as a reader, why is it called @extend.prevent-once? More specifically, I'm not sure I understand how it is semantically "once". If I understand correctly, the capture type is supposed to convey that the extended range of the most immediate ancestor ends. Would @extend.end-immediate-ancestor be more apt albeit verbose?

[Triton171]

I think the wording in the docs might be a bit confusing: It prevents the first extension of an ancestor, but this ancestor doesn't necessarily have to be immediate. Rather, this refers to the first ancestor that is captured by an @extend capture.

The last sentence in the docs ("All other ancestors are unaffected (regardless of whether the innermost ancestor would actually have been extended)") could probably be explained a bit better. What it means is that the first ancestor that is captured by an @extend may not actually cause any extension (for example because the lines after it are not indented enough). In this case, @extend.prevent-once "prevents" this extension, but since the extension wouldn't have happened anyways, it has no effect. In particular, if some more distant ancestor is actually extended, this extension is not prevented.

For python, these subtleties don't really make a difference but I still think it's important to document them, since for other languages they might be important. If you find a better name than @extend.prevent-once or a better wording for the docs, feel free to open a PR.

[zyklotomic]

Ah thank you for the explanation! So technically, we can say the most immediate ancestor captured by @extend capture.

Would it be easier to simply think of @extend and @extend.prevent-once as opposite operations, like opening and closing parentheses? We could then rename them into @extend.begin-once and @extend.end-once, or @extend.open and @extend.close to emphasize this; something with symmetry in the naming. Is this a correct characterization?

[Triton171]

@extend and @extend.prevent-once are sort of opposites but it's not completely symmetric. With parentheses, there's usually exactly one closing parenthesis for each opening parenthesis. Here, this is not the case: There can be multiple @extend captures without any @extend.prevent-once (in which case the innermost one takes precedence) and there might be multiple @extend.prevent-once captures without any @extend (although they have no effect in this situation). Also, the @extend.prevent-once doesn't appear after the @extend but rather as a descendant of it, in other words, it is contained in it.

I'm not sure if there is a better name for this, but the documentation could definitely be improved. Maybe a link to the python indent queries could also be helpful as an example for how to use the captures in practice.

[the-mikedavis]

This block is a bit long and seems to be somewhat well-contained. It may clean this function up a bit to split this into its own function. This function is already quite large and hard to follow

[the-mikedavis]

I'm curious if we can fork tree-sitter-python and have the function_definition and other rules that end in : consume the newline. That way the cursor would belong to the indented node and I think we might not need the extend system. I'll try it out

[Triton171]

I think the behavior would need to be a bit more complicated: It doesn't just have to consume the newline but also any following indented lines. Additionally, this should not happen when the function ends with for example a return expression.

If all this was implemented in the grammar, it would make this PR redundant but I'm not sure if it's worth the effort of forking the tree-sitter grammars for all languages that are affected by this.

[Triton171]

I've found an additional issue when there are multiple nested @extend-indented nodes:

def func():
    if condition:
        return
    | # <--- Cursor

In this situation, pressing enter would result in a line that's not indented at all. This happens since we're not inside the if_statement, so the extension prevented by the return_statement is the extension of the function. The latest commit slightly changes the behavior of the @stop-extend capture to fix this.

[dead10ck]

Sorry, this is probably just due to unfamiliarity, but I'm not understanding how you can have the cursor on a node with children that happen before the cursor. Could you please give an example of this?

[Triton171]

I think my comments are somewhat misleading: By "cursor" I mean the position of the indent query (not the tree-sitter cursor). Usually, this will just be the position of the text cursor when starting a new line. I'll change the comments to clarify that.

An example then looks like this:

def func(): # <-- The function_definition node ends here
    
    # <--- The indent is queried for this position

The function_definition node ends before the position of the indent query (but since it directly precedes the query position, it is considered for extension).

[archseer]

Thanks for working on this! There's been a few testers that are using this branch already so I think this is fine to merge 🎉