feat: Creating rust dependencies tree explorer #11557

bruno-ortiz · 2022-02-26T01:02:52Z

Hello!

I tried to implement a tree view that shows the dependencies of a project.

It allows to see all dependencies to the project and it uses cargo tree for it. Also it allows to click and open the files, the viewtree tries its best to follow the openned file in the editor.

Here is an example:

Any feedback is welcome since i have basically no professional experience with TS.

bruno-ortiz · 2022-03-01T14:55:03Z

Hey guys! I don't want to be a annoying or anything like that, but as this is my first PR here, do i need to do something else for the PR to be reviewed?

Veykril

I don't know my way around the client code too much, and I don't think any of the active people do either but overall this seems good to me.

Though I have two questions, how often does this invoke cargo (as in when) and we should ideally not have this code show error popups I think, if this code errors but the rest of r-a works fine I wouldn't personally want to bother users with it. So ideally errors should only be logged, and preferably in a way that is not too spammy.

Veykril · 2022-03-10T15:54:50Z

editors/code/src/ctx.ts

+        vscode.window.onDidChangeActiveTextEditor(e => {
+            if (e && isRustEditor(e)) {
+                execRevealDependency(e).catch(reason => {
+                    void vscode.window.showErrorMessage(`Dependency error: ${reason}`);
+                });
+            }
+        });


Just asking for my own understanding as I am not familiar with VSCodes api, but this is executed whenever the user switches the active editor to a rust file right? Why do we have to do this every time again, and secondly, if we do this on every change and an error does occur, the way this is written looks to me like we will spam the user with error messages whenever they switch their editor tab which sounds really unpleasant.

@Veykril Thanks for the review, i'll try to answer your questions:
1- It calls cargo tree is invoked every time the editor opens and after that for every "ok" that it receives from the server: The code is this:

switch (status.health) { case "ok": this.statusBar.color = undefined; this.dependencies.refresh(); // it tells the editor that something changed in the server and it should update the dependency tree break; case "warning": this.statusBar.tooltip += "\nClick to reload."; ....

2- I agree that we should not show the popup errors. I can change that, what are the best practices to logging in the editor?
3- This is executed every time, so the dependency tree can "follow" the user. So every time the user changes the active editor, the dependency tree "follows" the user and reveals the file in it.

The server has some (limited?) support for detecting when the dependencies (Cargo.lock or rust-project.json) have actually changed, it would be nice to hook into that instead of running cargo tree every time the active editor changes.

@lnicola it does not run cargo tree every time the editor changes, it runs when it receives a "status change" notification from the server. It checks that the status is "ok" and tries to reload the dependencies.
But as pointed in the other comments ill try to look into the dependency graph feature.

bjorn3 · 2022-03-10T16:09:42Z

Instead of using cargo tree should this use the dependency graph of rust-analyzer? That will also make it work when using rust-project.json instead of cargo and when using rust-analyzer.linkedProjects.

bruno-ortiz · 2022-03-10T17:56:40Z

@bjorn3 Can you please send me a reference or any example on how i can use the dependency graph? And is this blocking, can we make this change afterwards?

Thanks for the comments

lnicola · 2022-03-10T17:54:15Z

editors/code/src/commands.ts

+
+export function revealDependency(ctx: Ctx): Cmd {
+    return async (editor: RustEditor) => {
+        const rootPath = vscode.workspace.workspaceFolders![0].uri.fsPath;


Does this work for Code workspaces? We've historically had a lot of trouble with them.

I'm not too familiar with VSCode API, i'll look into that.

this won't really work for workspace folders—it'll only get this data for the first workspace, but you wouldn't be able to see dependencies from other workspaces.

lnicola · 2022-03-10T17:56:21Z

editors/code/src/dependencies_provider.ts

+        return [stdlib].concat(deps);
+    }
+
+    private async getStdLib(): Promise<Dependency> {


This duplicates the logic in the server. I think we should do as much as possible there instead of in the client code.

Sure, can you please point me out on where can i find this logic in the server? ty

There's https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide/src/view_crate_graph.rs for "View crate graph" and https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/project_model/src/workspace.rs#L140 takes care of loading a workspace. The sysroot is detected by Sysroot::load via Sysroot::discover.

This duplicates the logic in the server. I think we should do as much as possible there instead of in the client code.

I'm new here, but I disagree. The server-side implementation takes some of the burden off of the developers of non-VSCode clients.

@nemethf are you arguing for implementing it on the server, or client-side? I said I think it should be in the server, but maybe it wasn't very clear with all those prepositions and stuff.

I as well think it should be on the server side. I'm sorry if I misunderstood something or if I wasn't clear. Thanks.

lnicola · 2022-03-10T18:00:37Z

editors/code/src/ctx.ts

+        vscode.window.onDidChangeActiveTextEditor(e => {
+            if (e && isRustEditor(e)) {
+                execRevealDependency(e).catch(reason => {
+                    void vscode.window.showErrorMessage(`Dependency error: ${reason}`);
+                });
+            }
+        });


The server has some (limited?) support for detecting when the dependencies (Cargo.lock or rust-project.json) have actually changed, it would be nice to hook into that instead of running cargo tree every time the active editor changes.

bjorn3 · 2022-03-10T18:01:07Z

We will probably need an lsp extension to provide the dependency graph from the language server to the extension. @Veykril maybe you can explain how to do this?

lnicola · 2022-03-10T18:02:37Z

There is a "view dependency graph" command which could serve as inspiration.

Veykril · 2022-03-10T21:44:01Z

Ye, the server notifying the client here will have to be done as an lsp extension, a good inspiration for that would probably be the ServerStatus notification, as well as the view dependency graph as noted earlier.

bors · 2022-05-17T19:00:29Z

☔ The latest upstream changes (presumably #12294) made this pull request unmergeable. Please resolve the merge conflicts.

Veykril · 2022-07-17T16:06:21Z

Went ahead and rebased this, this still needs to use the server for querying, I added a small commit implementing the scaffolding part for the lsp extension for now.

bors · 2022-08-02T13:22:11Z

☔ The latest upstream changes (presumably #12919) made this pull request unmergeable. Please resolve the merge conflicts.

bruno-ortiz · 2023-04-03T01:12:00Z

@Veykril sorry for the long delay, I got swamped at work.

I implemented the server query, feel free to review the code again..

bruno-ortiz · 2023-04-03T02:28:49Z

crates/ide/src/fetch_crates.rs

+fn crate_path(db: &RootDatabase, data: &ide_db::base_db::CrateData, crate_name: &str) -> String {
+    let source_root_id = db.file_source_root(data.root_file_id);
+    let source_root = db.source_root(source_root_id);
+    let source_root_path = source_root.path_for_file(&data.root_file_id);
+    match source_root_path.cloned() {
+        Some(mut root_path) => {
+            let mut crate_path = "".to_string();
+            while let Some(vfs_path) = root_path.parent() {
+                match vfs_path.name_and_extension() {
+                    Some((name, _)) => {
+                        if name.starts_with(crate_name) {
+                            crate_path = vfs_path.to_string();
+                            break;
+                        }
+                    }
+                    None => break,
+                }
+                root_path = vfs_path;
+            }
+            crate_path
+        }
+        None => "".to_owned(),
+    }


I feel that this function is SUPER hacky, but I couldn't figure out another way around it.

I think that your instinct that this is a bit hacky is correct, but I'm struggling to fully understand this: are you trying to convert a URI to a path?

@davidbarsky this function tries to find a crate root dir in the filesystem, through the CrateGraph I can get the root file of a crate, for example, tokio's root file is /home/<user>/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.27.0/src/lib.rs. But I don't need the root file I need the root dir of a crate, so I have to convert this file path to /home/<user>/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.27.0.
If there is a cleaner way of doing it, I'll gladly change the implementation.

nemethf · 2023-04-04T10:40:10Z

docs/dev/lsp-extensions.md

+    crates: {
+        name: string;
+        version: string;
+        path: string;


Wouldn't it be more general (and more in line with the language server protocol) to use textDocument: TextDocumentIdentifier here instead of the current path: string?

And as a side note: why this is called a graph/tree, when this is just a list? It is not possible to construct a graph from the response. Ie.: visualize the the dependencies of a dependency.

@nemethf you're right about the naming, I changed from "graph" to "list". The tree is because the crates are displayed in a "ViewTree".
About the TextDocumentIdentifier , I tried using it, but it just made the code more complicated, because to reveal the items in the viewTree I need to work with the filesystem path, and the TextDocumentIdentifier inputs the "scheme" in the path.

About the TextDocumentIdentifier , I tried using it, but it just made the code more complicated, because to reveal the items in the viewTree I need to work with the filesystem path, and the TextDocumentIdentifier inputs the "scheme" in the path.

Looking over vscode.TreeItem, it seems like it requires you to operate in terms of URIs anyways, so I think it might clean up some of the code here if everything here operated in terms of URIs—the scheme can be dropped accordingly.

The tree is because the crates are displayed in a "ViewTree".

Thank you. I have no objections against it. However, it is a bit strange that a client-side implementation detail determines an abstract name in protocol specification. I can imagine some clients using this extension for something else, for example, to provide a "jump to dependency" functionality that implements an incremental search on the dependencies without showing them beforehand.

Also this fetchDependencyGraph reminds me of the rust-analyzer/viewCrateGraph LSP extension. In the long run, it might make sense to generalize both of them to send the same details. fetchDependencyGraph could list the dependencies of a dependency, and viewCrateGraph could be extended to have a uri/path for each dependency.

A TextDocumentIdentifier would indeed make more sense if possible

davidbarsky · 2023-04-04T17:02:51Z

editors/code/src/commands.ts

+
+export function revealDependency(ctx: Ctx): Cmd {
+    return async (editor: RustEditor) => {
+        const rootPath = vscode.workspace.workspaceFolders![0].uri.fsPath;


this won't really work for workspace folders—it'll only get this data for the first workspace, but you wouldn't be able to see dependencies from other workspaces.

davidbarsky · 2023-04-04T17:09:07Z

editors/code/src/commands.ts

+    return async (editor: RustEditor) => {
+        const rootPath = vscode.workspace.workspaceFolders![0].uri.fsPath;
+        const documentPath = editor.document.uri.fsPath;
+        if (documentPath.startsWith(rootPath)) return;


I'm not entirely sure why you'd want to do this. There's already non-workspace filtering done on the server; this just adds an additional check.

(This also breaks for monorepos where users open their editor at the monorepo root. Non-workspace crates are often stored in third-party/ but share the workspace root.)

The purpose of the function revealDependency is to make the TreeView "reveal" the opened file in the editor. Example:

But if I where to open a file that belongs to the local project, I dont want to trigger any "revealing"

The purpose of the function revealDependency is to make the TreeView "reveal" the opened file in the editor.

Gotcha, thanks for explaining—I've built the branch locally and usage made a bit more sense.

But if I where to open a file that belongs to the local project, I dont want to trigger any "revealing"

That makes sense and works for most default Cargo-based builds. However, using if (documentPath.startsWith(rootPath)) return; in situations where cargo vendor has been run or when a user opens their VS Code instance at the root of the monorepo (where a third-party/ folder exists and contains vendored Rust crates), this check will prevent this tree-based explorer from ever opening and it seems unfortunate that those users won't benefit from this feature¹. I don't have merge permissions on rust-analyzer, but I'd recommend removing this check, as fetch_crates in crates/ide/src/fetch_crates.rs already performs a similar check. I need to look over this PR a bit more, but if a user asks to reveal the dependency for a crate that is part of their editable workspace, then I would consider showing it. Alternatively, it might make sense to add another LSP extension to query the server itself to check whether a file is editable/part of the current editable workspace or not.

Footnotes

Personally, I'd love to build atop of this UI in order to allow users to incrementally add crates to their (editable) rust-analyzer workspace—it's a pretty common workflow in monorepos. ↩

@davidbarsky thanks for the reply, I just pushed a commit improving some pain points and hopefully giving it proper support to multiple workspace folders.
Could you point me at any github repos that make use of the features that you mentioned? I would love to test this cases because I suspect that they won't be a problem, because if they're already part of the project tree, the main treeView of vscode will already display those dependencies.

Just to add to my previous comment, I created a vendored project and everything seems fine.

Could you point me at any github repos that make use of the features that you mentioned?

@bruno-ortiz I can't think of any project off-hand, but it would be possible to take, e.g., the rust-analyzer repository and run cargo vendor and you'll see a subset of the behavior I'm mentioning.

I would love to test this cases because I suspect that they won't be a problem, because if they're already part of the project tree, the main treeView of vscode will already display those dependencies.

At the risk of being that person, but a smaller repository won't really demonstrate this issue—it only really becomes apparent once a monorepo has tens (or hundreds!) of millions of lines of code. While said dependencies are technically part of the primary explorer view, it's often infeasible to see them at a glance because of the sheer number of folders present in the default file explorer.

My mental model of this feature—at least in monorepos—is that it is a subset of the monorepo (think of it as a materialized view...) that corresponds to the portions of monorepo most relevant to the code being worked on at the moment.

The reason that I'm so adamant about requesting this is that I didn't really get the need/desire for it until I tried it. While it is technically possible to navigate through the primary explorer window to dependencies, it ends up taking closer to 30-60 seconds per dependency rather than something instantaneous.

davidbarsky · 2023-04-04T17:26:08Z

docs/dev/lsp-extensions.md

+    crates: {
+        name: string;
+        version: string;
+        path: string;


About the TextDocumentIdentifier , I tried using it, but it just made the code more complicated, because to reveal the items in the viewTree I need to work with the filesystem path, and the TextDocumentIdentifier inputs the "scheme" in the path.

Looking over vscode.TreeItem, it seems like it requires you to operate in terms of URIs anyways, so I think it might clean up some of the code here if everything here operated in terms of URIs—the scheme can be dropped accordingly.

davidbarsky · 2023-04-04T17:29:53Z

editors/code/src/commands.ts

+            do {
+                documentPath = path.dirname(documentPath);
+                parentChain.push({ id: documentPath.toLowerCase() });
+            } while (!ctx.dependencies?.contains(documentPath));
+            parentChain.reverse();
+            for (const idx in parentChain) {
+                await ctx.treeView?.reveal(parentChain[idx], { select: true, expand: true });


do you mind adding a comment explaining why this bit is necessary? is it to show a dependency of a dependency?

I'll add it.
Its because the TreeView is Lazy, so at first it only has the root dependencies: Example:

core

alloc

std

if I want to reveal alloc/src/str.rs, I have to:

reveal every children of alloc

core

alloc
|-beches
|-src
|- ...

std

reveal every children of src:

core

alloc
|-beches
|-src
| - lib.rs
| - str.rs <------- FOUND IT!
| - ...
|- ...

std

So basically, i have to calculate the parents of the file that I want to reveal until i find the root crate path.

davidbarsky · 2023-04-04T17:41:29Z

crates/ide/src/fetch_crates.rs

+fn crate_path(db: &RootDatabase, data: &ide_db::base_db::CrateData, crate_name: &str) -> String {
+    let source_root_id = db.file_source_root(data.root_file_id);
+    let source_root = db.source_root(source_root_id);
+    let source_root_path = source_root.path_for_file(&data.root_file_id);
+    match source_root_path.cloned() {
+        Some(mut root_path) => {
+            let mut crate_path = "".to_string();
+            while let Some(vfs_path) = root_path.parent() {
+                match vfs_path.name_and_extension() {
+                    Some((name, _)) => {
+                        if name.starts_with(crate_name) {
+                            crate_path = vfs_path.to_string();
+                            break;
+                        }
+                    }
+                    None => break,
+                }
+                root_path = vfs_path;
+            }
+            crate_path
+        }
+        None => "".to_owned(),
+    }


I think that your instinct that this is a bit hacky is correct, but I'm struggling to fully understand this: are you trying to convert a URI to a path?

crates/ide/src/fetch_crates.rs

bruno-ortiz · 2023-04-05T04:06:44Z

I converted this PR to draft because I detected some problems working with multi-workspaces.
I'll try to fix it ASAP.
Marking this PR as ready to review again.

bruno-ortiz · 2023-05-02T14:49:32Z

@bors r+

bors · 2023-05-02T14:49:34Z

📌 Commit ecfe7c0 has been approved by bruno-ortiz

It is now in the queue for this repository.

bors · 2023-05-02T14:49:41Z

⌛ Testing commit ecfe7c0 with merge a48e0e1...

bors · 2023-05-02T15:03:17Z

☀️ Test successful - checks-actions
Approved by: bruno-ortiz
Pushing a48e0e1 to master...

bruno-ortiz force-pushed the rust-dependencies branch from 700af65 to 01b0fd8 Compare February 26, 2022 14:57

Veykril self-assigned this Mar 5, 2022

Veykril reviewed Mar 10, 2022

View reviewed changes

lnicola reviewed Mar 10, 2022

View reviewed changes

bitgaoshu mentioned this pull request Apr 5, 2022

nothing #11903

Closed

Veykril force-pushed the rust-dependencies branch from 01b0fd8 to c6d3337 Compare July 17, 2022 15:39

Veykril removed their assignment Aug 31, 2022

Veykril marked this pull request as draft September 12, 2022 14:05

Veykril self-assigned this Nov 2, 2022

Veykril added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 3, 2022

Veykril removed their assignment Jan 24, 2023

bruno-ortiz mentioned this pull request Apr 3, 2023

Announcements: Client Changes #4604

Open

bruno-ortiz marked this pull request as ready for review April 3, 2023 02:23

bruno-ortiz commented Apr 3, 2023

View reviewed changes

nemethf mentioned this pull request Apr 3, 2023

Implement rust-analyzer/fetchDependencyGraph nemethf/eglot-x#2

Open

nemethf reviewed Apr 4, 2023

View reviewed changes

davidbarsky reviewed Apr 4, 2023

View reviewed changes

crates/ide/src/fetch_crates.rs Outdated Show resolved Hide resolved

bruno-ortiz marked this pull request as draft April 5, 2023 04:05

bruno-ortiz and others added 20 commits May 2, 2023 10:56

removing unused function

d1721b1

Reformat VSCode client code

af999f1

Remove unnecessary openFile command

9533644

WIP: Add lsp-ext scaffold

299382d

removing unused code

fc57339

fixing main_loop.rs

440889e

running prettier

061940d

Fixing tests

e253592

Fixing naming from graph to list

1b8288f

improving code to work with multi-workspaces

8e687f7

Adding crate_root_path to crate_graph

a3081a6

reveal only when tree is visible

fe7874a

fixing tests for windows

c372fb3

accepting review suggestions

66fe84d

final rabasing fixes

bd2160f

fixing ts linting and rust test

072f69e

adding doc and simplifying function

800b3b6

Accepting review suggestions

bcb2131

fixing TS linting, removing import

0aed507

last fixes after rebase

ecfe7c0

bruno-ortiz force-pushed the rust-dependencies branch from 4b7914d to ecfe7c0 Compare May 2, 2023 14:24

bors merged commit a48e0e1 into rust-lang:master May 2, 2023

This was referenced May 2, 2023

Provide links to locally built documentation for experimental/externalDocs #14662

Merged

internal: Use triomphe::Arc #14718

Merged

Veykril mentioned this pull request May 3, 2023

Sort dependencies in vscode dependencies view #14729

Closed

lnicola changed the title ~~Creating rust dependencies tree explorer~~ feat: Creating rust dependencies tree explorer May 7, 2023

feat: Creating rust dependencies tree explorer #11557

feat: Creating rust dependencies tree explorer #11557

Conversation

bruno-ortiz commented Feb 26, 2022 • edited Loading

bruno-ortiz commented Mar 1, 2022

Veykril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented Mar 10, 2022

bruno-ortiz commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnicola Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented Mar 10, 2022

lnicola commented Mar 10, 2022

Veykril commented Mar 10, 2022

bors commented May 17, 2022

Veykril commented Jul 17, 2022

bors commented Aug 2, 2022

bruno-ortiz commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-ortiz Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbarsky Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-ortiz commented Apr 5, 2023 • edited Loading

bruno-ortiz commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

bors commented May 2, 2023

bruno-ortiz commented Feb 26, 2022 •

edited

Loading

lnicola Mar 16, 2022 •

edited

Loading

bruno-ortiz Apr 4, 2023 •

edited

Loading

davidbarsky Apr 5, 2023 •

edited

Loading

bruno-ortiz commented Apr 5, 2023 •

edited

Loading