Add LSP server #2662

ghost · 2023-12-13T18:27:50Z

Passes tests and seems to work so far, unimplemented APIs aside. Source code is partially reorganized to try to be a bit closer to the rest of Buf, but there's plenty that could still be done. We'll probably need to work out how much of that should block.

P.S.: I think the path handling will need to be cleaned up for Windows to work fully. Have yet to test this.

bufdev

Looking really good!

I can't comment, but why is there a protovalidate.zip checked in?

bufdev · 2023-12-13T20:23:41Z

private/buf/buflsp/buflsp.go

+	wellKnownTypesDescriptorProtoPath = normalpath.Join("google", "protobuf", "descriptor.proto")
+	// v3WellKnownTypesCacheRelDirPath is the relative path to the cache directory for materialized
+	// well-known types .proto files.
+	v3WellKnownTypesCacheRelDirPath = normalpath.Join("v3", "wkt")


I talked to @Alfus about this, but this implies it's the same cache directory as the module cache - is that correct? If so, this should be in a different location - buf lsp should not modify the module cache. We could have $BUF_CACHE_DIR/lsp/wkt/WKT_VERSION as an example

Putting the version there as a subdirectory seems like a good idea, though I don't see an obvious version in the generated datawkt package. Digest can easily work if that's sufficient/reasonable, though.

(For now I've addressed this by adding the digest of the wkt module into the resulting path.)

ghost · 2023-12-13T20:37:27Z

Looking really good!

I can't comment, but why is there a protovalidate.zip checked in?

It's a terrible hack. Since we can't fetch modules yet due to missing APIs, the test literally pulls the zip as file data into an omni provider.

Obviously, it would be better to get rid of this. Any opinion on how? I think the most obvious paths forward are:

Remove dependency and allow it to be invalid. The LSP tests will still run, but some of the tests will need to be removed at least until fetching dependencies works.
Vendor protovalidate some other way. I don't have a huge opinion on how if so, the zip thing was just a very quick hack.
Adjust the tests/buftest to not depend on protovalidate to begin with. Fine with me, although that will mean we're not really testing dependencies outside of wkt. This might be OK since dependencies are mostly abstracted from the LSP.

bufdev · 2023-12-13T20:42:23Z

I would just check in protovalidate as individual files, not as a zip file - we do this in other places too, theres not many files. ie just explode the zip file into its constituent files and check those in.

ghost · 2023-12-13T20:46:27Z

Works for me, fixed.

…ck/buf-lsp-merge

ghost · 2023-12-14T21:11:17Z

Do let me know if there's any more work you think I should take a look at on this. For now, I'm at a bit of a stopping point as everything I'm able to test seems to work as expected and I think most of the low hanging fruit for cleaning up the code to Buf's standards/style are taken care of.

bufdev

Leaving code organization alone for now, but some structural questions. I believe most of these are for @Alfus and not @jchadwick-buf but both feel free to chime in here.

bufdev · 2023-12-19T18:05:36Z

private/buf/buflsp/buflsp_test.go

+	if err != nil {
+		return nil, err
+	}
+	omniProvider, err := bufmoduletesting.NewOmniProvider(


If we're providing protovalidate via this OmniProvider, why do we still need to depend on it via a BSR dependency in buftest? Is there some functionality under the hood we are testing? It's not immediately apparent to me that, if the *server is using the controller, which itself is just depending on the ModuleKeyProvider/ModuleDataProvider given via OmniProvider, why we also need to have a hard dependency on buf.build here.

If not, we could get rid of buftest entirely, and move all the lsp testing code into testdata local to this package.

OK, SGTM. Now buftest is no more, and I've reverted the changes made to the proto directory as well.

private/buf/buflsp/buflsp.go

bufdev · 2023-12-19T18:08:08Z

private/buf/buflsp/buflsp.go

+				if entry, ok := buflsp.fileCache[event.Name]; ok {
+					if entry.moduleSet != nil {
+						if err := buflsp.refreshImage(context.Background(), entry.moduleSet, entry.bucket); err != nil {
+							buflsp.logger.Sugar().Errorf("refreshImage error: %s", err)


This will be displayed to users, we should make sure this error is meaningful, which I don't think it is if we are referencing an internal method refreshImage.

How should errors that have no one to report them to be handled in general? This can probably just be removed

bufdev · 2023-12-19T18:08:49Z

private/buf/buflsp/buflsp.go

+		var err error
+		workspace, err := s.controller.GetWorkspace(ctx, params.TextDocument.URI.Filename())
+		if err != nil {
+			s.logger.Warn("could not determine workspace", zap.Error(err))


This will be displayed to the user, same comment above.

bufdev · 2023-12-19T18:09:08Z

private/buf/buflsp/buflsp.go

+	}
+
+	// Create a new file entry for the file
+	entry, err := s.createFileEntry(ctx, params.TextDocument, moduleSet)


moduleSet can be nil, are we making sure that createFileEntry is documented that one of its parameters may be nil?

I've eliminated the intentionally-nil module set pointers.

private/buf/buflsp/buflsp.go

bufdev · 2023-12-19T18:10:25Z

private/buf/buflsp/buflsp.go

+		bucket = s.wellKnownTypesBucket
+		file, err = bucket.GetFile(ctx, path)
+		if err != nil {
+			s.logger.Warn("could not resolve import", zap.String("path", path))


Why aren't we returning this error? We generally never want to using logging as a means of error propagation unless absolutely necessary.

It is going to be necessary to swallow some errors at some point because the LSP should provide some functionality even when the source code is partially broken, since the LSP will necessarily see source code as it is being typed. In this case, it doesn't actually matter since we bail out earlier in processText anyway, so there's really no point in swallowing this error, so now it does not do that.

bufdev · 2023-12-19T18:12:57Z

private/buf/buflsp/buflsp.go

+		workspace, err := s.controller.GetWorkspace(ctx, params.TextDocument.URI.Filename())
+		if err != nil {
+			s.logger.Warn("could not determine workspace", zap.Error(err))
+			// Continue anyways if this fails.


I believe the reason for this is that we can still provide useful features even if we can't get a workspace.

The previous statement (that we still provide useful features even without a workspace) is still true, but this hack is no longer necessary as I've deferred workspace resolution until later, so we don't need to swallow this error here anymore.

private/buf/buflsp/buflsp.go

…ck/buf-lsp-merge

bufdev · 2024-02-12T16:31:49Z

Pushed this work to https://github.com/bufbuild/buf/tree/do-not-delete-buf-lsp-merge for long-term storage for now. We talked offline and agreed we need to rework this, which we hope to pick up relatively soon. Closing this PR as it is.

jpaju · 2024-02-21T21:23:22Z

I'd love to see the LSP released someday 🚀

fira42073 · 2024-08-04T11:17:22Z

Hey! I wonder if there are any updates here? Buf is the de-facto standard for proto generation for many teams, and it would be nice to have a compatible lsp as well. There are other solutions available, but they usually lack maintenance and features. Anyway, it would be awesome if this would be revived C:
Thanks for all of your work!

bufdev · 2024-08-27T18:20:56Z

We're reviving this and working on it soon, cc @mcy

juanmarin-co · 2024-10-12T01:43:27Z

Is there a place to follow the progress on the server?

jpaju · 2024-10-12T07:04:03Z

The first version of the LSP is already released: #3316. I guess you can follow the PRs in this repo to see the progress

John Chadwick added 10 commits December 12, 2023 12:13

Add proto/buftest buf module

aa0efb1

Rename buf.lsp.test.v1 -> buf.lsp.test.v1alpha1

58e5ce5

Add internal buflsp library + dependencies

d2bf326

Add buf lsp command

9822920

checkpoint

f013ac0

checkpoint

6af137b

checkpoint

925f44c

Add missing newline in buf.yaml

e9997e1

Remove old v1 manifest/lockfile for buftest

b53028c

Remove mysterious copy of protovalidate?

0141ccb

ghost requested a review from bufdev as a code owner December 13, 2023 18:27

bufdev reviewed Dec 13, 2023

View reviewed changes

Move well-known types cache

2d258b1

Directly vendor protovalidate, no zip file

e3054a8

John Chadwick added 2 commits December 13, 2023 18:10

Add digest to wkt cache path.

187d0bb

Merge branch 'bufmod' of https://github.com/bufbuild/buf into jchadwi…

593c401

…ck/buf-lsp-merge

ghost requested a review from bufdev December 14, 2023 21:06

bufdev added 2 commits December 19, 2023 12:53

Merge branch 'bufmod' into jchadwick/buf-lsp-merge

86e35fd

fix

9617c78

bufdev reviewed Dec 19, 2023

View reviewed changes

bufdev and others added 6 commits December 27, 2023 00:00

merge

aa8fcd0

commit

8527981

fix

bd402bb

get make lint passing

ee4a508

Merge branch 'bufmod' of https://github.com/bufbuild/buf into jchadwi…

4c8c403

…ck/buf-lsp-merge

ModuleDigest -> Digest

7cab809

bufdev and others added 5 commits January 29, 2024 18:01

Merge branch 'bufmod' into jchadwick/buf-lsp-merge

6cb2d1d

Remove digest from cache path

e36b534

fix broken digest

c4efa7d

merge

6d3add2

fix

ade01ad

bufdev mentioned this pull request Feb 2, 2024

Move proto/buf to buf and add ioext.CompositeReadWriteCloser #2749

Merged

bufdev added 16 commits February 2, 2024 14:27

Merge branch 'bufmod' into jchadwick/buf-lsp-merge

8bcfd36

Move buf lsp to buf beta lsp

044ef15

Refactor cache dir path

46a8506

Rename func to match type

b22d85f

Make server a parameter to newFileEntry

97701fa

todos

801a4ce

todos

ccc081e

change diagnostic message

d643212

More cleanup

21d73e4

More cleanup

b342959

More cleanup

2b820b8

merge

226e9e6

Merge branch 'bufmod' into jchadwick/buf-lsp-merge

d486929

Merge branch 'bufmod' into jchadwick/buf-lsp-merge

2e2b72f

Fix

dc313e5

merge

9cc2d6c

bufdev closed this Feb 12, 2024

bufdev deleted the jchadwick/buf-lsp-merge branch March 19, 2024 21:12

Add LSP server #2662

Add LSP server #2662

Uh oh!

Conversation

ghost commented Dec 13, 2023

Uh oh!

bufdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 13, 2023

Uh oh!

bufdev commented Dec 13, 2023

Uh oh!

ghost commented Dec 13, 2023

Uh oh!

ghost commented Dec 14, 2023

Uh oh!

bufdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bufdev commented Feb 12, 2024

Uh oh!

jpaju commented Feb 21, 2024

Uh oh!

fira42073 commented Aug 4, 2024

Uh oh!

bufdev commented Aug 27, 2024

Uh oh!

juanmarin-co commented Oct 12, 2024

Uh oh!

jpaju commented Oct 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants