intellij icon indicating copy to clipboard operation
intellij copied to clipboard

Intellij doesn't recognize imports from @com_google_protobuf

Open ronisec opened this issue 3 years ago • 8 comments

Despite multiple "Sync" runs, GoLand does not recognize its @com_google_protobuf dependencies. Example:

image

Running:

  1. GoLand 2021.2
  2. Bazel Plugin 2021.11.09.0.2-api-version-212
  3. Protocol Buffers Plugin 212.4746.57

The relevant target in our BUILD.bazel file:

load("@rules_proto//proto:defs.bzl", "proto_library")

proto_library(
    name = "pb_proto",
    srcs = ["mymessage.proto"],
    visibility = ["//visibility:public"],
    deps = [
        "@com_google_protobuf//:timestamp_proto",
        "@com_google_protobuf//:wrappers_proto",
    ],
)

The com_google_protobuf dependency from our WORKSPACE file:

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "com_google_protobuf",
    sha256 = "6b6bf5cd8d0cca442745c4c3c9f527c83ad6ef35a405f64db5215889ac779b42",
    strip_prefix = "protobuf-3.19.3",
    urls = ["https://github.com/protocolbuffers/protobuf/archive/v3.19.3.zip"],
)

load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")

protobuf_deps()

ronisec avatar Jan 26 '22 11:01 ronisec

It's worth mentioning that this resolution doesn't work in *.proto files but works in *.go files.

fkorotkov avatar Jan 31 '22 19:01 fkorotkov

That the resolution is only not working in *.proto files but successfully works in *.go files is an important detail. If *.go files were affected, we'd need to look for an issue with attaching the proto stubs generated by the Bazel build.

For imports in *.proto files, implementing the FileResolveProvider extension point from the Protobuf plugin would indeed be an option (as @fkorotkov suggested via chat). By default, the Protobuf plugin provides two implementations for FileResolveProvider. One of them resolves imports from the project roots. The other one uses the paths configured in the settings.

Out of the box, the Protobuf plugin doesn't seem to provide a way to resolve imports of protos which come in via dependencies. I'd assume that you would see the same issue in a Gradle- or Maven-based project. Did you test this?

That would also influence where an appropriate implementation needs to be placed. If it's possible to write a FileResolveProvider which locates a file in IntelliJ's attached libraries, it would be best placed in the Protobuf plugin as it would then be independent of the build language.

If that's not entirely possible, then the Bazel plugin should only bridge the remaining gap.

alice-ks avatar Feb 02 '22 14:02 alice-ks

Thanks for the responses! I tested this out only with Bazel (not with Gradle or Maven). Should I open an issue for this on the Protobuf plugin on this then?

ronisec avatar Feb 02 '22 15:02 ronisec

I don't think it's a Proto plugin issues since these external proto files are not in project scope (if you try "Go to File" action then you'll see that IntelliJ doesn't know about the files).

I'll open a PR shortly that adds integration with Protocol Buffers plugin to do the resolution.

fkorotkov avatar Feb 02 '22 15:02 fkorotkov

Opened #3228 draft which fixes the issue for my use case.

fkorotkov avatar Feb 02 '22 16:02 fkorotkov

Thanks for following up on this Fedor!

ronisec avatar Feb 03 '22 07:02 ronisec

I looked at https://github.com/bazelbuild/intellij/pull/3228 and this implementation is exactly what I meant that we should avoid. The implementation of course works as the Bazel plugin is explicitly gathering a reference to any sources it encounters (but does so outside of it's regular data management) and then resolves any references to other proto files queried by the Protobuf plugin directly against those sources. This is working completely around IntelliJ's project model and the design of the Bazel plugin, introduces hard dependencies between the plugins, and introduces a special purpose implementation. In addition, there's a large risk that the generic data the aspect gathers in that implementation will be quite large for large projects, resulting in performance issues.

I'm convinced that there's an alternative solution which avoids all/most of these disadvantages and lets the plugins stay within their respective scope. Instead of pulling in everything in the aspect, it would be necessary to figure out how to exactly reference the currently missing proto files/sources. Potentially, these sources are already gathered for some languages but seemingly hidden with other names. For that case, duplicate data mustn't be collected. A next step would be to figure out how to attach them to IntelliJ's project model such that they are indexed and that the Protobuf plugin can reference them (in an implementation of FileResolveProvider within the Protobuf plugin) without the two plugins knowing about each other. I would be surprised if this wasn't possible somehow. A positive consequence of this would be that users would be able to find those proto files via the "GoToFile" action, which I would expect as a user.

alice-ks avatar Feb 08 '22 19:02 alice-ks

figure out how to exactly reference the currently missing proto files/sources

I wasn't able to find any target that already contains this information. I inspected all the target infos that were presented for my test project. Maybe you can take a look and help us to figure this out?

figure out how to attach them to IntelliJ's project model such that they are indexed and that the Protobuf plugin can reference them

This is done BlazeProtoAdditionalLibraryRootsProvider in my PR. With this extension *.proto files are indexed and available for "GoToFile" action.

... without the two plugins knowing about each other. I would be surprised if this wasn't possible somehow.

It doesn't seem possible with the current two implementation of FileResolveProvider that the Proto plugin has out of the box as you mentioned yourself above.

Overall after digging into the code I don't see a way to get the external *.proto files into the project without BlazeProtoAdditionalLibraryRootsProvider. For which we need somehow to get the info from Bazel either via the generic info, separate proto configuration or some other way which I wasn't able to find.

The only part of the PR that IMO can be moved to the Proto plugin itself is the resoling part so just adding BlazeProtoAdditionalLibraryRootsProvider from the Bazel plugin side will be enough. But this still require the GenericIdeInfo thing. I though of making it as a separate proto language so you can add it to additional_languages to enable the integration but proto_library is a generic target already and seems pretty impossible to abstract out.

fkorotkov avatar Feb 09 '22 12:02 fkorotkov

Thank you for contributing to the IntelliJ repository! This issue has been marked as stale since it has not had any activity in the last 6 months. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-maintainer". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

github-actions[bot] avatar May 18 '23 02:05 github-actions[bot]

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

github-actions[bot] avatar Jun 01 '23 02:06 github-actions[bot]