feat: add api-linter (powered by buf CLI)

[fredrikaverpil]

This adds the excellent "api-linter" from Google, which lints for so-called AIPs in .proto files: https://github.com/googleapis/api-linter

Notes

• You must have buf on $PATH.
• There is a built in function (for the --descriptor-set-in argument) which attempts to search for a buf.yaml (or buf.yml) file. This search upwards from the opened buffer (the .proto file) until hitting $HOME, where it stops and fails. But this function may not be sufficient for everyone's use case. It's important that for such users, this function can be overridden and right now it seems like the only way is to fully replace all arguments with opts.linters["api-linter"] = { args = {...} }, which is totally fine I guess.
• The repo (and proto) from the screenshot: https://github.com/fredrikaverpil/go-microservice/blob/main/proto/gomicroservice/v1/user_service.proto
• I've chosen to include a few --disable-rule directives as otherwise you'll get these java warnings (and you may not even be using java). See below:

[mfussenegger]

Is this common to generate the descriptor-set-in file like this?

Looking at the other integrations mentioned in googleapis/api-linter#764 they don't appear to be doing anything like this as far as I can tell.

[fredrikaverpil]

@mfussenegger I can't speak for those other IDE plugins but you have to somehow tell api-linter where it can find all imports that might be used by the proto file that you are linting, or api-linter will simply fail linting.

$ api-linter gomicroservice/v1/user_service.proto
2024/09/26 14:19:02 proto/gomicroservice/v1/user_service.proto:5:8: open buf/validate/validate.proto: no such file or directory

Here's the user_service.proto used in the command above. The command is failing because it doesn't find the protos from https://github.com/bufbuild/protoc-gen-validate. But if I instead use the built descriptor file (which knows about the validate dependency since it's specified in my buf.yaml config), it will succeed:

$ buf build -o d.pb
$ api-linter --descriptor-set-in=d.pb gomicroservice/v1/user_service.proto 
- file_path: gomicroservice/v1/user_service.proto
  problems:
    - message: Proto files must set `option java_package`.
      location:
        start_position:
            line_number: 3
            column_number: 1
        end_position:
            line_number: 3
            column_number: 26
        path: gomicroservice/v1/user_service.proto
      rule_id: core::0191::java-package
      rule_doc_uri: https://linter.aip.dev/191/java-package
    - message: Proto files must set `option java_multiple_files = true;`
      location:
        start_position:
            line_number: 3
            column_number: 1
        end_position:
            line_number: 3
            column_number: 26
        path: gomicroservice/v1/user_service.proto
      rule_id: core::0191::java-multiple-files
      rule_doc_uri: https://linter.aip.dev/191/java-multiple-files
    - message: Proto files should set `option java_outer_classname = "UserServiceProto"`.
      location:
        start_position:
            line_number: 3
            column_number: 1
        end_position:
            line_number: 3
            column_number: 26
        path: gomicroservice/v1/user_service.proto
      rule_id: core::0191::java-outer-classname
      rule_doc_uri: https://linter.aip.dev/191/java-outer-classname

... and in this case it will complain about the core::0191 rules.

I believe you can also do it via -I arguments, which you then need to point to other proto files (which correspond to your imports). They do it in one of their tests googleapis/api-linter@6ef36a6/cmd/api-linter/cli_test.go#L21-L24.

So, one may traverse the current project for protos to solve that, but where would I traverse or look for imports from protovalidate (and other downloaded buf dependencies)?
On my machine they are downloaded by buf into ~/.cache/buf but I'm not sure that would be the case for everyone?
It would also take considerable amount of code to figure out whether to traverse that cache to find the appropriate protos, as they are versioned and also stored in a custom cache structure. It's not as simple as doing a find operation based on the name of the import in the proto I want to lint, or appending -I ~/.cache/buf.

But by building this descriptor file, all such dependencies are sucked in (by buf itself) and redefined in the descriptor file, making it a lot easier to point api-linter to this derivative file. This basically leverages whatever buf does to perform the codegen based on the protos. As a bonus here, buf will adhere to the buf lockfile, using the versions of the protos you intend.

So in my opinion I'm doing the right thing here. And obviously you can override the arguments and provide different logic if you don't think this suits your needs.

This is api-linter --help that describes --descriptor-set-in vs -I:

❯ api-linter --help
Usage of api-linter:
      --config string                   The linter config file.
      --debug                           Run in debug mode. Panics will print stack.
      --descriptor-set-in stringArray   The file containing a FileDescriptorSet for searching proto imports.
                                        May be specified multiple times.
      --disable-rule stringArray        Disable a rule with the given name.
                                        May be specified multiple times.
      --enable-rule stringArray         Enable a rule with the given name.
                                        May be specified multiple times.
      --ignore-comment-disables         If set to true, disable comments will be ignored.
                                        This is helpful when strict enforcement of AIPs are necessary and
                                        proto definitions should not be able to disable checks.
      --list-rules                      Print the rules and exit.  Honors the output-format flag.
      --output-format string            The format of the linting results.
                                        Supported formats include "yaml", "json","github" and "summary" table.
                                        YAML is the default.
  -o, --output-path string              The output file path.
                                        If not given, the linting results will be printed out to STDOUT.
  -I, --proto-path stringArray          The folder for searching proto imports.
                                        May be specified multiple times; directories will be searched in order.
                                        The current working directory is always used.
      --set-exit-status                 Return exit status 1 when lint errors are found.
      --version                         Print version and exit.

[fredrikaverpil]

@mfussenegger I guess you can argue that this is solution is geared towards buf users. If you're not using buf, I'm not really sure how to solve this. But I think this is still a great starting point for people who do use buf.

I use this almost everyday and it's super helpful when working with "AIP-driven" APIs: https://github.com/fredrikaverpil/dotfiles/blob/main/nvim-fredrik/lua/lang/protobuf.lua

[fredrikaverpil]

@mfussenegger would you like me to implement two strategies?

1. buf CLI is available and buf.yaml is found; uses current implementation.
2. buf CLI may be available but buf.yaml is not found (indicates the user is not using buf); walk $cwd recursively for *.proto files and append each one using the -I argument.

Then I guess we can support (mostly) what the other IDEs are doing too.

Strategy 2 will be slower and inferior in the sense that it won't support knowledge of protos which reside outside your git repo project and if it encounters an import it cannot resolve, the api-linter will simply not lint and error. But at least you will get something that looks similar to what other IDEs are doing in case you don't use buf. I suspect that IDEs offer the ability for users to define custom filepaths to wherever they have protos stored which the api-linter need to be aware of.

I personally feel having these two strategies would be fine and then let someone who is a non-buf user extend this functionality to perhaps even better fit that use case.

[mfussenegger]

No strong opinion on this. I don't use api-linter.

But it is highly unusual that a linter requires to run another tool beforehand. I kinda think that whatever buf is doing should be something that api-linter can do out of the box too.

The descriptor_set_in function also uses several functions only available in nvim 0.10+ and is synchronous.
Not a blocker either, but also not really happy about this.

If the buf setup is common, would it make sense to create a buf-api-linter project, that basically does the pre-processing and then delegates to api-linter?

[fredrikaverpil]

This turned out to be a long post/reply. Sorry for that. I'm just trying to give as nuanced and honest picture of this as I can, so to help out in being able to take good decisions here. Maybe grab a cup or keg of your favorite beverage before reading. 😄

But it is highly unusual that a linter requires to run another tool beforehand. I kinda think that whatever buf is doing should be something that api-linter can do out of the box too.

I can only agree it is not common behavior for a linter. I searched the api-linter repo to see if I could get some answers on this myself (🕳️ 🐇). If you are inclined to skim through this GitHub issue, it may bring a little light to this topic as Googlers are here discussing how to make api-linter aware of imports (as it seems to be more akin to an LSP) and finally landing on a descriptor file (which you indeed have to generate beforehand).

My point is only that api-linter clearly need this input and won't work without it. The input being all your protos; either one by one or in the form of this binary descriptor file representation.

To be clear, you don't have to use the buf CLI to generate this descriptor file. But you somehow need to supply api-linter with all protos (even from imports made in the proto you wish to lint). Where I work, we've done this since beginning of 2022.

Alternatives to using the buf CLI could be (although I wouldn't consider them for nvim-lint):

• Clone down the third-party proto projects (that you use in a particular project) to disk and supply paths to api-linter for each one (using the -I argument).
• Use the tool mentioned in the GitHub issue from 2019 I linked to above: https://pkg.go.dev/github.com/jhump/protoreflect/desc/builder

The descriptor_set_in function also uses several functions only available in nvim 0.10+ and is synchronous.
Not a blocker either, but also not really happy about this.

I can change this code into supporting an older nvim version of your choice. Let me know which version I need to support or if you have a reference implementation in mind from another linter. No problem.

If the buf setup is common, [...]

Let me quickly just mention we use the buf CLI where I work because it enables an ecosystem of tooling around working with protos. They market themselves as being an "all-in-one protobuf toolchain" here. You can specify your dependencies, download them, upgrade them, create a lockfile etc, all using the buf CLI. And buf knows exactly which versions of your third-party dependencies you have in your lock file and reads those when building the descriptor file.

I've worked at companies that "were not yet on buf" but wanted to go there but had blocking issues letting them do so. So I'd say buf might be somewhat common but definitively desirable over previous solutions in my opinion. The buf CLI is free by the way, which I guess contributes to its popularity (~9k stars): https://github.com/bufbuild/buf
I would personally not reach for anything else when starting up a new gRPC project for managing protos.

To be super clear, you need third-party dependencies to e.g. generate python/java/go/etc code. It's not really feasible scenario that you don't use any third-party dependencies.

[...] would it make sense to create a buf-api-linter project, that basically does the pre-processing and then delegates to api-linter?

Sure, I guess that's one option, but in that case how would the "regular" nvim-lint api-linter would work by default?
Would it just walk the $CWD recursively for .proto files and append them (using the -I argument) to the command?
It would just take one third-party dependency and it wouldn't be able to run.
I've worked at three separate companies working on gRPC APIs and all of them make use of third-party dependencies stored outside of the project. Again, I haven't looked at how this is solved for other IDEs but I'm sure they need the user to manually enter paths to local directories where any such downloaded dependencies can be found.

However, I personally think it would be totally fine to require users to append additional filepaths to the "regular" api-linter args in that case.

I'd be happy to update the PR with a buf-api-linter as well, if you think this is the best way forward.

I would have personally just assumed most people (want to) use buf - and if not, they can overide the arguments themselves. This is one of the strengths of nvim-lint I've come to love!
But it's possible that this could be a short-sighted approach and that your idea of supplying two linters would be more sustainable long-term.

[mfussenegger]

Sure, I guess that's one option, but in that case how would the "regular" nvim-lint api-linter would work by default?
Would it just walk the $CWD recursively for .proto files and append them (using the -I argument) to the command?

Yes I'd prefer that, plus the buf-api-linter variant. (Or only the buf-api-linter variant if the above behavior is not all that useful)

[fredrikaverpil]

I think for most users, a "bare-bones" api-linter which wouldn't take any third-party plugins into account wouldn't be usable, really. So I opted for just adding a buf_api_linter here.

I also swapped out some newer functions for older and custom approaches. I'm not sure what you think about that. Anyway, I tested these changes out here on a couple of projects and it works fine for me.

[mfussenegger]

I think there was a bit of a misunderstanding. What I meant was not only changing this to buf_api_linter but also create a dedicated buf-api-linter project with an executable that combines buf and api-linter.

Then this integration could define the buf-api-linter cmd and remove the logic around calling buf.

This would have the advantage that integration in other editors would make use of it too.
(It's to me generally a bit of a miracle that we haven't managed to standardize output formats but need custom logic for each and every linter)

[fredrikaverpil]

Aha okay. Yes, I think I misunderstood you there.

So buf-api-linter should call try_lint("api-linter") but with custom logic around buf and with custom opts?

[bheadwhite]

bump

[fredrikaverpil]

@mfussenegger please have a look at what I did now. I broke apart the logic into two linters.

I know the interface doesn't specify supplying an example autocmd, but I felt I had to do this to show the usage intent due to the lack of cwd-as-a-function.