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

LSP auto-import mangles templ files #135

Closed
jwhitaker-swiftnav opened this issue Aug 31, 2023 · 8 comments
Closed

LSP auto-import mangles templ files #135

jwhitaker-swiftnav opened this issue Aug 31, 2023 · 8 comments

Comments

@jwhitaker-swiftnav
Copy link

jwhitaker-swiftnav commented Aug 31, 2023

Heya, thanks heaps for building this, it is I think the best high-level design for how templating should work I've ever seen.

I'm just seen some funniness in the VSCode extension (have been for ages, it's not introduced recently) - accepting the suggestion for an auto-import seems to mangle things up.

Video (two examples, one at start one at end):

Screencast.from.31-08-23.08.28.44.webm.mp4

Exact behaviour seems to be affected by whether the generated code is up to date, but there is some level of mangling in either case.

@aggroot
Copy link
Contributor

aggroot commented Aug 31, 2023

I was looking to open a ticket for the same issue. Other than this, everything is awesome.
Thank you for such a great work!

Update: I did some digging and I found two possible reasons why this bug is happening:

  • the default imports added by the generator automatically are not available in the sourcemap because they don t exists in *.templ files and because of this the mapping of ranges for text edits when a completion that modifies imports is received will always fail
  • also it seems that the textEdits sent by gopls will always try to change the format of imports to
import (
    "github.com/a-h/templ"
    "context"
    "io"
    "bytes"
)

and couldn t find a way to disable this. There are some ways to disable reorganization of imports only on file save, but not on autocomplete

I m trying to find a way to fix this but it seems a tricky one

@a-h
Copy link
Owner

a-h commented Sep 3, 2023

Thanks for the nice feedback, and great bug report @jwhitaker-swiftnav, and thanks for looking into the issue @agalbenus

Looking at the LSP logs when it happened didn't tell me much, but I added some extra logging to print out the full object model of the autocompletion options.

The completion items include "additionalTextEdits" - i.e. they change text that isn't just the bit you're autocompleting - selecting the item also changes the import list.

templ isn't currently rewriting those additional text edits at all, but even if it was, as @agalbenus points out, the imports don't exist in the source file, so there's no proper mapping.

	p.Log.Info("completion: received items", zap.Int("count", len(result.Items)), zap.Any("items", result.Items))
	for i := 0; i < len(result.Items); i++ {
		item := result.Items[i]
		if item.TextEdit != nil {
			item.TextEdit.Range = p.convertGoRangeToTemplRange(templURI, item.TextEdit.Range)
		}
		result.Items[i] = item
	}

Here's the completion list.

{
  "level": "info",
  "ts": "2023-09-03T18:34:45+01:00",
  "caller": "proxy/server.go:335",
  "msg": "completion: received items",
  "count": 7,
  "items": [
    {
      "additionalTextEdits": [
        {
          "range": {
            "start": {
              "line": 6,
              "character": 7
            },
            "end": {
              "line": 6,
              "character": 7
            }
          },
          "newText": "(\n\t"
        },
        {
          "range": {
            "start": {
              "line": 6,
              "character": 8
            },
            "end": {
              "line": 6,
              "character": 10
            }
          },
          "newText": "by"
        },
        {
          "range": {
            "start": {
              "line": 6,
              "character": 11
            },
            "end": {
              "line": 9,
              "character": 1
            }
          },
          "newText": "es\"\n\t\"context\"\n\t\"io\"\n\t\"strconv\"\n\n\t\"github.com/a-h/te"
        },
        {
          "range": {
            "start": {
              "line": 9,
              "character": 3
            },
            "end": {
              "line": 9,
              "character": 13
            }
          },
          "newText": "l"
        },
        {
          "range": {
            "start": {
              "line": 9,
              "character": 14
            },
            "end": {
              "line": 9,
              "character": 14
            }
          },
          "newText": "\n)"
        }
      ],
      "detail": "\"strconv\"",
      "documentation": {
        "kind": "markdown",
        "value": ""
      },
      "filterText": "strconv",
      "insertTextFormat": 2,
      "kind": 9,
      "label": "strconv",
      "preselect": true,
      "sortText": "00000",
      "textEdit": {
        "range": {
          "start": {
            "line": 24,
            "character": 21
          },
          "end": {
            "line": 24,
            "character": 25
          }
        },
        "newText": "strconv"
      }
    },
...
}

I think the way to solve this might be to get templ to do its own addition of import statements. The LSP returns an enum called https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemKind and export const Module = 9;.

      "filterText": "strconv",
      "insertTextFormat": 2,
      "kind": 9,

So it might be a case of if kind == 9 in the message received from gopls, and additionalTextEdits are present, remove them and replace them with a plain Go import statement in the templ file.

@a-h
Copy link
Owner

a-h commented Sep 3, 2023

Modifying server.go in the LSP section shows good promise.

        // Rewrite the result positions.
-       p.Log.Info("completion: received items", zap.Int("count", len(result.Items)))
+       p.Log.Info("completion: received items", zap.Int("count", len(result.Items)), zap.Any("items", result.Items))
        for i := 0; i < len(result.Items); i++ {
                item := result.Items[i]
                if item.TextEdit != nil {
                        item.TextEdit.Range = p.convertGoRangeToTemplRange(templURI, item.TextEdit.Range)
                }
+               if item.Kind == lsp.CompletionItemKindModule && len(item.AdditionalTextEdits) > 0 {
+                       item.AdditionalTextEdits = []lsp.TextEdit{
+                               {
+                                       Range: lsp.Range{
+                                               Start: lsp.Position{Line: 1, Character: 0},
+                                               End:   lsp.Position{Line: 1, Character: 0},
+                                       },
+                                       NewText: fmt.Sprintf(`import "%s"`, item.Label),
+                               },
+                       }
+               }
                result.Items[i] = item
        }

This change is really basic - it just puts an import statement in as the first line in the file - it doesn't format the code in that section, e.g. insert appropriate newlines, which I'd really want it to do.

import

@a-h
Copy link
Owner

a-h commented Sep 3, 2023

This is more like it...

import
import2

@a-h
Copy link
Owner

a-h commented Sep 3, 2023

OK, @jwhitaker-swiftnav, @agalbenus - if you have a chance to check over the changes in that PR, I'd appreciate it!

@a-h
Copy link
Owner

a-h commented Sep 4, 2023

The PR is #136

I've just realised that it does a good job of the import, but if you import a function after the package, it still mangles it. I'll take a look at that before I merge.

@aggroot
Copy link
Contributor

aggroot commented Sep 4, 2023

@a-h I just added a PR to fix imports on completions that contain functions/structs and so on #138. Basically, all completions that require package imports have the import embedded in the detail field

@a-h a-h closed this as completed in 86d87b4 Sep 5, 2023
@jwhitaker-swiftnav
Copy link
Author

this looks fab, thanks both!

a-h added a commit that referenced this issue Sep 24, 2023
…#136)

* fix(lsp): file incorrectly updated when importing modules, fixes #135

* refactor: reduce time spent iterating through files, and reduce risk of incorrectly identifying an import statement

* refactor: add tests for import management

* refactor: rename field

* fix: use the detail section, because the label only includes the last part of the name

* fix: solve auto-import for completions that require them (#138)

* fixed autoimport for other completion kinds that require import

* Apply suggestions from code review

remove unused parameter

Co-authored-by: Adrian Hesketh <[email protected]>

* Apply suggestions from code review

fix naming typo

Co-authored-by: Adrian Hesketh <[email protected]>

* changed how a kind that requires import is identified

* remove obsolete regexp

---------

Co-authored-by: agalbenus <[email protected]>
Co-authored-by: Adrian Hesketh <[email protected]>

* refactor: minor restructure, and additional test

---------

Co-authored-by: Adrian Galbenus <[email protected]>
Co-authored-by: agalbenus <[email protected]>
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

No branches or pull requests

3 participants