Conversation
This ports our image building path to use the new compiler and handles all the necessary test changes, etc. The new compiler brings a lot of improvements: - Richer diagnostics and error reporting - Support for Editions 2024 features - Incremental compilation (although this is mostly for the LSP, which is already using the new compiler)
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
This adds edition 2024 support to the LSP, notably in completion, semantic tokens, and hover. Intended to be a fast-follow to #4407.
This adds edition 2024 support to the LSP, notably in completion, semantic tokens, and hover. Intended to be a fast-follow to #4407.
| fmt.Sprintf( | ||
| `%s:5:1:detected cyclic import while importing "a/a.proto"`, | ||
| filepath.FromSlash("testdata/cyclicimport/b/b.proto"), | ||
| ), |
There was a problem hiding this comment.
I don't love this diagnostic, and the fact that the diagnostics in general are unable to provide the more robust details in a single FileAnnotation... e.g. this is the full report shows a much clearer cycle: https://github.com/bufbuild/protocompile/blob/main/experimental/ir/testdata/imports/cycle.proto.yaml.stderr.txt
That being said, if the user is printing the output to text, then they'll get the full information. This is only on the individual FileAnnotation (which would be used for things like in-line github annotations, the json output for analysis, etc.) -- and would provide a clear place where the import cycle was detected (although more details, in this case in particular would be nice). Will noodle this one a little.
This adds edition 2024 support to the LSP, notably in completion, semantic tokens, and hover. Intended to be a fast-follow to #4407.
stefanvanburen
left a comment
stefanvanburen
left a comment
There was a problem hiding this comment.
just a couple minor things, this is looking good!
| alreadySeen, | ||
| nonImportFilenames, | ||
| seen, | ||
| xslices.ToStructMap(paths), |
There was a problem hiding this comment.
nit: call this once outside the loop?
| defer func() { | ||
| if retErr != nil { | ||
| retErr = errors.Join(retErr, moduleFile.Close()) | ||
| } | ||
| }() |
There was a problem hiding this comment.
I think we want to close the moduleFile regardless of if we have an error, because readObjectCloserToSourceFile will copy over its contents but not close it.
| defer func() { | |
| if retErr != nil { | |
| retErr = errors.Join(retErr, moduleFile.Close()) | |
| } | |
| }() | |
| defer moduleFile.Close() |
| } | ||
|
|
||
| if len(targetFileInfos) == 0 { | ||
| // If we had no no target files within the module after path filtering, this is an error. |
There was a problem hiding this comment.
| // If we had no no target files within the module after path filtering, this is an error. | |
| // If we had no target files within the module after path filtering, this is an error. |
Just since we're nearby.
2 participants
This ports our image building path to use the new compiler and handles all the necessary test changes, etc.
The new compiler brings a lot of improvements:
This PR is in draft:
Fixes #4193