Golang indentation broken for type declarations and switch statements

[GeorgeNewby]

Reproduction steps

Type Declaration

// Before
type Test struct {|}

// After
type Test struct {
        |
    }

// Expected
type Test struct {
    |
}

Switch Statement

// Before
func main() {
    switch nil {|}
}

// After
func main() {
    switch nil {
    |
}
}

// Expected
func main() {
    switch nil {
        |
    }
}

Environment

• Platform: Linux
• Helix version: helix v0.5.0-373-g3a34036

Explanation

I've been looking into the issue quite a bit and I think it's an issue with the choice of nodes inindents.toml and some limitations of https://github.com/tree-sitter/tree-sitter-go.

The type declaration is it indented because of the following nodes: type_declaration, type_spec and field_declaration_list in indents.toml. I don't think we should be using the field_declaration_list as an indent as instead use the actual struct_spec node. However this causes issues with type grouping and nested structs.

I don't think we can ever do perfect indenting with just changes to indent.toml as the following example has identical treesitter nodes yet different indentations.

type (
   test struct {
       field int
       nested struct {
           inner string
       }
   }
)

type test struct {
   field int
   nested struct {
       inner string
   }
}

source_file [0, 0] - [15, 0]
  type_declaration [0, 0] - [7, 1]
    type_spec [1, 3] - [6, 4]
      name: type_identifier [1, 3] - [1, 7]
      type: struct_type [1, 8] - [6, 4]
        field_declaration_list [1, 15] - [6, 4]
          field_declaration [2, 7] - [2, 16]
            name: field_identifier [2, 7] - [2, 12]
            type: type_identifier [2, 13] - [2, 16]
          field_declaration [3, 7] - [5, 8]
            name: field_identifier [3, 7] - [3, 13]
            type: struct_type [3, 14] - [5, 8]
              field_declaration_list [3, 21] - [5, 8]
                field_declaration [4, 11] - [4, 23]
                  name: field_identifier [4, 11] - [4, 16]
                  type: type_identifier [4, 17] - [4, 23]
  type_declaration [9, 0] - [14, 1]
    type_spec [9, 5] - [14, 1]
      name: type_identifier [9, 5] - [9, 9]
      type: struct_type [9, 10] - [14, 1]
        field_declaration_list [9, 17] - [14, 1]
          field_declaration [10, 3] - [10, 12]
            name: field_identifier [10, 3] - [10, 8]
            type: type_identifier [10, 9] - [10, 12]
          field_declaration [11, 3] - [13, 4]
            name: field_identifier [11, 3] - [11, 9]
            type: struct_type [11, 10] - [13, 4]
              field_declaration_list [11, 17] - [13, 4]
                field_declaration [12, 7] - [12, 19]
                  name: field_identifier [12, 7] - [12, 12]
                  type: type_identifier [12, 13] - [12, 19]

Maybe one way to handle this is to ignore consecutive indents on the same line e.g. type_declaration and struct_spec should only apply 1 increment_from_line instead of 2 (in helix-core/src/indent.rs).

I'm currently looking into a solution and will create PR if I can solve it

[Triton171]

You're right, the current indentation system has some limitations. I'm currently rewriting it for that reason (see #1440). Unless i can think of some situation where this goes wrong, only applying indent once per line seems like a good idea. Basically if there are 2 indents and 1 outdent on the same line we currently see that as a net indent which we shouldn't do, right?

As for the switch statement, couldn't that just be solved by adding the expression_switch_statement node to the indent list? I'm not familiar with Go so maybe that breaks something else but at least in this case that should work.

[archseer]

How did this behave before #1341 ?

[Triton171]

I think the first example should've worked correctly before but it's hard to say because there were some weird edge cases. In any case, what @GeorgeNewby suggested should be a simple fix for it.

The switch statement seems like a simple problem with the indents.toml for Go, that was probably always broken.

[GeorgeNewby]

How did this behave before #1341 ?

I just tested using helix v0.5.0-326-g8f2af71 which is the the commit before #1341. It seems to work better, the first type deceleration example now works. It still falls apart a bit with nested type definitions. I think it works better because it completely ignores any nodes that start on the same line.

type Test struct {|}

On newline insertion, the following nodes are queried: field_declaration_list, struct_type, type_spec, type_definition and source_file. All nodes apart from source_file are on the same line so don't get counted.

What's interesting is the fn calculate_indentation still returns 1 because the initial node field_declaration_list is in the indent list.

    // if we're calculating indentation for a brand new line then the current node will become the
    // parent node. We need to take it's indentation level into account too.
    let node_kind = node.kind();
    if newline && query.indent.contains(node_kind) {
        increment += 1;
    }

The new version #1341 calculates the indent on the } node instead and returns also returns a 1 for the indent level which is incorrect and causes the unwanted indentation.

When entering a newline, should it be calculating the indent for the line directly underneath or the closing bracket 2 lines down?

[Triton171]

When entering a newline, should it be calculating the indent for the line directly underneath or the closing bracket 2 lines down?

I might be biased as I changed it to calculate the indent of the closing brace but I think it makes more sense:

• Before Add basic indentation handling as a fallback for treesitter queries #1341, we calculated the indent of the position before the cursor. This means that when inserting a new line, we didn't calculate the indent for the beginning of that new line but for the end of the previous one. This causes problems when there are no auto-pairs (for example it indents one too much every time you want to move a closing brace to a new line, as in the first example in Add basic indentation handling as a fallback for treesitter queries #1341).
• I'm not sure if there is a reliable way to compute the indent for the line in between the auto pairs: When we have to compute it, this line doesn't yet exist. The workaround was to take the indent of the opening brace but there are likely languages/treesitter grammars for which those are not the same. For example depending on the grammar definition it might make sense to add { to the outdent list (nvim-treesitter does this for rust).