intellij icon indicating copy to clipboard operation
intellij copied to clipboard

Rust Support

Open alexjpwalker opened this issue 3 years ago • 8 comments

Checklist

  • [x] I have filed an issue about this change and discussed potential changes with the maintainers.
  • [ ] I have received the approval from the maintainers to make this change.
  • [x] This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See the Contributions section in the README for more details.

Discussion thread for this change

Issue number: https://github.com/bazelbuild/intellij/issues/675

Description of this change

This integration depends on https://github.com/intellij-rust/intellij-rust/pull/8491.

The IntelliJ Rust plugin is built to work with Cargo projects, so to make it work with Bazel projects, we generate Cargo manifest files using Bazel.

This change generates a Cargo.toml (and a RustIdeInfo file) in the Bazel sandbox for every rust_binary and rust_library target in the project.

rust_test targets need to be handled a little differently. Normally, in Cargo, we specify test targets as part of the same Cargo.toml file that contains the library (or binary) that we're writing tests for. However, Aspects can't modify files generated by, say, rust_library while processing a rust_test. So instead, we force IntelliJ to detect the test target by writing a blank file to the Bazel sandbox with the same name and path as the rust_test's source file.

An example project, and demo of the plugin's capabilities, is available here: https://github.com/vaticle/bazel-intellij-rust-example

https://user-images.githubusercontent.com/14253102/151391801-a7551fcb-0322-4ed5-adc1-f11784a3b106.mov

alexjpwalker avatar Jan 31 '22 15:01 alexjpwalker

Hey @alexjpwalker, thank you for this PR!

I'd like to know your opinion on one idea. If I understand correctly, it seems that the Rust Toml file generated here is not used anywhere inside IntelliJ Bazel Plugin code, it is utilized in IntelliJ Rust Plugin. I'm wondering if we could make aspects execution logic extensible, so it could be delegated to the Rust Plugin. This could simplify future development, as we wouldn't have to synchronize both sides. I don't know how to do it yet, just asking what you think about it.

tpasternak avatar Aug 11 '22 14:08 tpasternak

it seems that the Rust Toml file generated here is not used anywhere inside IntelliJ Bazel Plugin code, it is utilized in IntelliJ Rust Plugin. I'm wondering if we could make aspects execution logic extensible, so it could be delegated to the Rust Plugin.

@tpasternak : Your understanding is correct! The Bazel plugin does not use the generated Cargo.toml files, these are used by the Rust plugin to identify project structure and the set of files to provide code analysis over.

The Rust plugin currently assumes that every Rust project is a Cargo project:

Each Rust project in IDE consists of multiple CargoProjects.

CargoProject corresponds to a Cargo.toml file inside a project source folder that is linked to the current IDE project via attachCargoProject.

(source: https://github.com/intellij-rust/intellij-rust/blob/e8b5860f6f31f520bd54b75d62fcbb917888e4e1/ARCHITECTURE.md#project-model)

Because of this assumption, one of the key goals of this PR has been to generate Cargo.toml files using the intellij_info_aspect. This does introduce coupling between the Bazel and Rust plugins.

alexjpwalker avatar Aug 15 '22 17:08 alexjpwalker

@tpasternak However - do correct me if I'm wrong:

  • The only consumer of this plugin will be the Rust IntelliJ plugin;
  • There is only one Rust IntelliJ plugin at the time of writing, and any future IntelliJ Rust work would ideally contribute to the existing plugin, rather than creating a new one;
  • Even if a new Rust plugin was created, it would, almost certainly, assume the use of Cargo as the build system.

Therefore, creating a coupling between the two seems like it would be acceptable, as the two will always be coupled in practice.

Is there a specific reason why we need a solution that is not tied to IntelliJ Rust plugin(s)? And if so, what would that solution look like?

alexjpwalker avatar Aug 22 '22 11:08 alexjpwalker

You're right, but by decoupling we could avoid necessity to have synchronous releases. Also, in my opinion, it will be easier if the rules_rust team review and merge changes related to cargo toml creation.

tpasternak avatar Aug 24 '22 09:08 tpasternak

cc @mai93

tpasternak avatar Aug 24 '22 10:08 tpasternak

It is true that the current approach is "needed because of the Rust plugin's expectation that we are working in a Cargo project". Generating Cargo.toml was a trade-off made to avoid an unknown, potentially large amount of development work on the Rust plugin side. If we follow the same approach as in Java, this is how I (approximately) foresee it looking:

Bazel

  • intellij_info_aspect generates IDE metadata for each rust_library, rust_binary (and perhaps rust_test). It specifies the exact paths to the sources of all of its dependencies. These paths may be in the project, or in bazel-bin (in the case of generated sources), or in bazel-{project_name} (in the case of external crates fetched via crate_universe/cargo raze/etc.)

Rust

  • In addition to detecting Cargo projects (Cargo.toml files), also detect Bazel projects (IDE metadata files inside bazel-bin).
  • Parse the Bazel IDE metadata to construct the project structure. (Perhaps by generating Cargo.toml.)

alexjpwalker avatar Aug 24 '22 11:08 alexjpwalker

Hello @alexjpwalker, Could you please fix the build kite failures to move this PR for further good state. Thanks!

sgowroji avatar Sep 06 '22 04:09 sgowroji

@sgowroji : Tests failed because previously we'd added Rust as a "default supported language", when really it should be optionally supported, similarly to Python and Kotlin. By adding the AlwaysPresentRustSyncPlugin and the new WorkspaceType RUST, the behaviour is now in line with other optional languages:

  • issues an IDE warning if the Rust plugin is not installed, which is clickable and prompts the user to install it;
  • prompts the user to enable Bazel plugin rust support on detecting rust_* targets in the project, and only once that is enabled, flags those targets for syncing

As a result, the plugin now behaves properly and passes BuildKite's checks.

alexjpwalker avatar Sep 08 '22 15:09 alexjpwalker

The way that this PR currently functions is to generate Cargo.toml files and trick the Rust plugin into using them. This is not in line with other languages in the Bazel plugin. Moreover, I would expect the complexity of implementing a new language such as Rust correctly (ie: not through hacks) to be very high, requiring considerable knowledge of IntelliJ extensions and the internals of the Bazel and Rust plugins which I don't have myself.

As such, we've changed course on Bazel + IntelliJ + Rust support and put together a script that runs a standalone Bazel aspect that generates metadata similar to Cargo.toml, then recursively scans bazel-bin and generates Cargo.toml files in the project directory to construct a Cargo project. We've retained some modifications to the Rust plugin, namely detecting the Rust toolchain and stdlib in $(bazel info output_base)/external, but we are able to proceed with this strategy with no code changes to the Bazel plugin at all, making this PR redundant.

I've created a copy of the branch this PR is based off, which can be found here: https://github.com/vaticle/bazel-intellij/tree/rust-support-archive

alexjpwalker avatar Oct 14 '22 09:10 alexjpwalker

Thank you, that seems like a better approach for a workaround until we get a more "proper" implementation. Does that mean we can close this PR?

jastice avatar Oct 14 '22 09:10 jastice

Yes, let's close this PR, @jastice . Thank you to everyone who's put in their time to review it.

alexjpwalker avatar Oct 14 '22 10:10 alexjpwalker