Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper support for standalone/detached files #14318

Open
Veykril opened this issue Mar 10, 2023 · 14 comments
Open

Proper support for standalone/detached files #14318

Veykril opened this issue Mar 10, 2023 · 14 comments
Labels
A-project-model project model and workspace related issues C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) )

Comments

@Veykril
Copy link
Member

Veykril commented Mar 10, 2023

// FIXME: The primary limitation of this approach is that the set of detached files needs to be fixed at the beginning.
// That's not the end user experience we should strive for.
// Ideally, you should be able to just open a random detached file in existing cargo projects, and get the basic features working.
// That needs some changes on the salsa-level though.
// In particular, we should split the unified CrateGraph (which currently has maximal durability) into proper crate graph, and a set of ad hoc roots (with minimal durability).
// Then, we need to hide the graph behind the queries such that most queries look only at the proper crate graph, and fall back to ad hoc roots only if there's no results.
// After this, we should be able to tweak the logic in reload.rs to add newly opened files, which don't belong to any existing crates, to the set of the detached files.
// //
/// Project with a set of disjoint files, not belonging to any particular workspace.
/// Backed by basic sysroot crates for basic completion and highlighting.
DetachedFiles { files: Vec<AbsPathBuf>, sysroot: Option<Sysroot>, rustc_cfg: Vec<CfgFlag> },

Ideally, you should be able to just open a random detached file in existing cargo projects, and get the basic features working. That needs some changes on the salsa-level though. In particular, we should split the unified CrateGraph (which currently has maximal durability) into proper crate graph, and a set of ad hoc roots (with minimal durability). Then, we need to hide the graph behind the queries such that most queries look only at the proper crate graph, and fall back to ad hoc roots only if there's no results. After this, we should be able to tweak the logic in reload.rs to add newly opened files, which don't belong to any existing crates, to the set of the detached files.

Fixing this would be beneficial to the rustlings project, removing the need for the rust-project.json generation step they currently employ.

@Veykril Veykril added the C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) label Mar 10, 2023
@HKalbasi
Copy link
Member

It might also make sense to adding a syntax for declaring dependencies inline via comments in detached files. It will also helps for #9343 as evcxr currently handles dependencies in jupyter notebooks via :deps dep = "x.y.z" commands.

@Veykril
Copy link
Member Author

Veykril commented Mar 10, 2023

We can't really support that, to resolve dependencies we need cargo which in turns needs a proper cargo workspace

@HKalbasi
Copy link
Member

We probably need to have a mirror of the project in a temp directory with a "normal" project structure at that point, which is what evcxr is doing if I understand correctly.

@Veykril
Copy link
Member Author

Veykril commented May 4, 2023

rust-lang/rfcs#3424 will be interesting to us here. Unsure how we can support this but we probably would need to handle it similar to build scripts, that is we analyze the file without dep info first/invoke it initially with cargo metadata once, and afterwards keep track of the dependencies comment. If it changes re-run metadata.

@victor-timofei
Copy link

Is anyone working on this? If not, I'd love to pick it up.

@Veykril
Copy link
Member Author

Veykril commented Aug 4, 2023

No one is working on this right now, though it's also not quite clear how to tackle this. I personally don't really understand the idea described in the comment tbh, I took a look at this at some point and the comment didn't really make sense to me in terms of how things are working in r-a right now.

@victor-timofei
Copy link

victor-timofei commented Aug 7, 2023

I did a little bit of experimentation and the way I found that this currently works (tried it on Neovim with lspconfig) is like:

local lsp_conf = require('lspconfig')

lsp_conf.rust_analyzer.setup {
    -- Other setup stuff
    -- ...

    -- To be able to start rust-analyzer at all a root_dir is needed,
    -- I added a star match for testing purposes to match on any file
    -- This could be an lspconfig/Neovim only issue
    root_dir = lsp_conf.util.root_pattern("*"),

    -- LSP Client needs to know the detached files ahead of time on
    -- initialization and pass them to the r-a on startup
    init_options = { detachedFiles = { "/tmp/file.rs" } }
}

r-a Client for VSCode does something similar here.

And the PR for the initial support is #8955

@victor-timofei
Copy link

What I currently do not understand yet why would this require a change on the salsa-level(does this mean on the upstream salsa crate?).

The way I understand this it would require to hide the CrateGraph behind an API that would query by default the current CrateGraph and on a failure to fallback to querying the detached files I guess.

I would guess at the time of writing reload.rs was the one responsible for discovering the files and adding the to the CrateGraph?

@Veykril
Copy link
Member Author

Veykril commented Aug 8, 2023

Maybe @matklad can explain what they meant with the comment there since it was written by them

@matklad
Copy link
Member

matklad commented Aug 8, 2023

This has to do with durabilities :

https://rust-analyzer.github.io/blog/2023/07/24/durable-incrementality.html

if we add an ability to open files in an ad-how way, we should make sure that opening such a file does not require to revalidate the entire graph of queries.

This is in contrast to assign a new dependency via cargo add —- in that case, it’s OK to revalidate everything, because that’s a rare explicit operation, that could be slow.

right now, each detached file is treated as a separate crate, and all crates are a part of a singleton crate graph query.s thwt means that:

  • opening a new detached file
  • Changes the crate graph
  • which sets the corresponding salsa query
  • With “Durable” durability as that’s what we use for crate graph
  • Which results in all queries being invalidated
  • So we spend the next half a second or so veryfying that nothing really changed in the stdlib

To fix this, we need to go from a single crate graph query to something more fine grained, so that we can add new detached files without disturbing Durable parts of the query graph.

@HKalbasi
Copy link
Member

Opening a detached file (in case of cargo scripts) is going to change the crate graph anyway. One thing we can do is separating the query graphs of workspaces. Currently GlobalState can track multiple ProjectWorkspaces, but it merges all of their crate graphs. I guess it is possible to split those crate graphs, so that each ProjectWorkspace get its own crate graph (It wastes a huge amount of memory on loading multiple stds, and it makes some challenges on ide features for files inside multiple workspaces, but I think it is doable) so opening a new detached file won't invalidate analysis on the old files and workspaces. But does it matter? After opening a file, the user probably needs analysis in that file, not somewhere else, and that is going to take same amount of time, if not more.

Opening a detached file as a cargo script is as slow as opening a new cargo project, and I don't see how preventing things being invalidated will make it fast. By opening a new cargo script, we will discover a whole new cargo toml which some of its dependencies might not be even downloaded yet, I don't think any change in salsa can make that fast.

That said, we may still be able to do something. We can try to detect which files are going to be opened in the detached mode (for example by looking at the shebang) and constructing the correct project workspace and crate graph from the beginning. It won't help in opening random files from random places (and I don't think we can do anything for that), but will help in projects that are multiple detached files in a folder, similar to the rustling.

@Veykril
Copy link
Member Author

Veykril commented Apr 21, 2024

While the crate graph issues is problematic in terms of perf, I think it should be fine for us to skip that optimization for now (at least in favor of cargo script workspaces). The main issue with these detached files / cargo script workspaces are that we cannot eagerly support them, as we would need to built the full module graph for the project to figure out if a file is detached or not which is too much up front work. So at best we can support these lazily, which while not perfect is still decent I think. That is, once such a file is opened we can permanently add it to the known workspaces (I wonder if we should do the same for undiscovered workspaces).

@mo8it
Copy link
Contributor

mo8it commented Jul 7, 2024

Small note about the issue description: Rustlings wouldn't benefit from this anymore after the release of version 6. It uses an automatically updated Cargo.toml file that contains all exercises and solutions as binaries.

@davidbarsky
Copy link
Contributor

Small note about the issue description: Rustlings wouldn't benefit from this anymore after the release of version 6. It uses an automatically updated Cargo.toml file that contains all exercises and solutions as binaries.

@mo8it Congratulations on releasing Rustlings 6.0! Depending on your interest/desire to maintain that Cargo.toml file, #17246 might be helpful here, as that'd make finding/loading rust-project.json files much more Cargo-like.

(rust-analyzer still very much needs better standalone file support for the reasons I described in #17537.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-project-model project model and workspace related issues C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) )
Projects
None yet
Development

No branches or pull requests

7 participants