Stream: t-compiler/rust-analyzer

Topic: Support for #[cfg(test)]


Thiébaud Weksteen (Aug 07 2020 at 07:26, on Zulip):

I've noticed that r-a was not resolving names when inside a tests mod (that is, preceded with the attribute #[cfg(test)]). This is for a rust-project.json type project. This can be fixed by adding "test" to the cfg attribute but I've noticed that many projects uses this as a fake/mocking mechanism (with #[cfg(not(test))] for the real module). In this case, we would lose the analysis of the real module.
Is there anything I'm missing here? Is there any solution to have both parsed?

Thiébaud Weksteen (Aug 10 2020 at 09:04, on Zulip):

@Paul Faria , you've submitted https://github.com/rust-analyzer/rust-analyzer/pull/4784/commits/9c35f135b9c872e904ee1e838cfa69fc5745c45f#diff-1c8e6b45624e6ce512961af52e9a1aa3R396 which explicitly adds "test" cfg. Any input on my question?

Paul Faria (Aug 10 2020 at 11:33, on Zulip):

As I understand it, this isn't currently possible. It would break goto def functionality (which item do you route to?). It would also not be clear what functionality is available. What happens when the types, functions or fields conflict with each other?

I had proposed configuring cfg items with some kind of UI, and @matklad had brought up an example of a checkbox UI that could appear on Cargo.toml files. Basically having the ability to toggle the features on and off.

I've definitely run into your issue before in my own crates and at work, so I understand the frustration. I could also be wrong, and the above might be possible, but I don't have enough experience with all the details to say for sure.

Florian Diebold (Aug 10 2020 at 12:15, on Zulip):

it is kind of possible, functionality like goto def is already built with this in mind. We can have multiple configurations of a crate in the crate graph, and a file can belong to any number of them. Actually implementing that would probably not be completely trivial anyway though, and would have a performance impact since we'd have to do a lot of analysis twice.

Florian Diebold (Aug 10 2020 at 12:18, on Zulip):

it would be interesting to experiment with though

Florian Diebold (Aug 10 2020 at 12:24, on Zulip):

(basically, the way it works for e.g. goto def all goes back to this, where it tries to find a crate that actually contains the file. I'm not 100% sure it'll deal well with the case where the crate contains the file, but the actual code we're interested in is cfg'd out though)

Paul Faria (Aug 10 2020 at 13:06, on Zulip):

TIL. I assume this means we'd need to keep track of two trees? One with test on and one off? If not, how would it interact with chalk (I assume that's what's used to do the type analysis, I've never delved that deep into the RA codebase before).

Florian Diebold (Aug 10 2020 at 13:19, on Zulip):

it's really just like two different crates in the crate graph, that happen to share files

Florian Diebold (Aug 10 2020 at 13:19, on Zulip):

all the HIR is completely separate and duplicated

Paul Faria (Aug 10 2020 at 13:51, on Zulip):

Assuming we get this working, what if, as a way of mitigating performance issues, this is a feature that's defaulted off and only turned on through settings?

Florian Diebold (Aug 10 2020 at 13:54, on Zulip):

as a first step at least, probably

Thiébaud Weksteen (Aug 11 2020 at 07:35, on Zulip):

Thanks, I'll add this info to an issue on github so this can be tracked.

woody77 (Aug 12 2020 at 23:12, on Zulip):

Florian Diebold said:

it's really just like two different crates in the crate graph, that happen to share files

This is actually how we're currently generating our rust-project.json, because I haven't worked on deduplication, yet. And for now "first one wins" seems to be the behavior.

woody77 (Aug 12 2020 at 23:15, on Zulip):

Paul Faria said:

As I understand it, this isn't currently possible. It would break goto def functionality (which item do you route to?). It would also not be clear what functionality is available. What happens when the types, functions or fields conflict with each other?

I had proposed configuring cfg items with some kind of UI, and matklad had brought up an example of a checkbox UI that could appear on Cargo.toml files. Basically having the ability to toggle the features on and off.

I've definitely run into your issue before in my own crates and at work, so I understand the frustration. I could also be wrong, and the above might be possible, but I don't have enough experience with all the details to say for sure.

I wonder how difficult it would be to wire up a "config-switcher" that just reloaded rust-analyzer after switching cfgs? (so you could switch between cfgs, and then just kick off a re-start of the language server for the workspace). Seems like it could be implemented in the VSCode extension itself, maybe?

Paul Faria (Aug 13 2020 at 13:17, on Zulip):

I was actually wondering if we could tie it into the lookup logic Florian shared above. Rather than just passing in a FileId, we could pass in a FileId and a filtering callback of some kind. Like, if there are multiple options, then use the callback to filter down and then if there are still options, fallback to a single value. I'm not sure how complicated it would be to thread that callback through to that code though. And Florian did mention this would cause perf issues. I think I'll experiment with something on the side there.

Florian Diebold (Aug 13 2020 at 13:20, on Zulip):

I think it's more that the lookup logic needs to be able to deal with the possibility that the container exists in a certain crate, but the child that we actually want doesn't, and then we need to try the next crate

Florian Diebold (Aug 13 2020 at 13:20, on Zulip):

but the config switcher thing has also been talked about and is probably the easier short-term solution

Paul Faria (Aug 13 2020 at 13:21, on Zulip):

ok, I'll focus on that then

Thiébaud Weksteen (Oct 02 2020 at 07:52, on Zulip):

Apologies for the delay in creating that bug. Here it is: https://github.com/rust-analyzer/rust-analyzer/issues/6117

Paul Faria (Oct 20 2020 at 13:29, on Zulip):

Now that I'm back from vacation, I've had time to work on this a bit more. I'm currently able to keep track of the configurations on the initialization with cargo.toml projects (rust-project.json will be done soon too), and I've found the spot to properly load the configurations at https://github.com/rust-analyzer/rust-analyzer/blob/be762ccccd5a86632e60351518528d078785a3e2/crates/hir/src/semantics/source_to_def.rs#L32

My idea to connect the two is to add a method available on the db such that I can filter like so:

            .filter(|&&crate_id| self.db.config_active_for(crate_id))

In between the iter and the find_map linked above.

My thought was that this makes the most sense on the FileLoader trait: https://github.com/rust-analyzer/rust-analyzer/blob/be762ccccd5a86632e60351518528d078785a3e2/crates/base_db/src/lib.rs#L91, but it wasn't clear to me how I would keep this up to date or populate this from CrateData (where I'm currently storing the available features from cargo.toml). @matklad any ideas on how I should approach this, or am I possibly starting at the wrong place?

matklad (Oct 20 2020 at 15:39, on Zulip):

I don't think we should add callbacks for configuation

matklad (Oct 20 2020 at 15:40, on Zulip):

We need to expose the API that iterates all possible candidates, instead of returning the first one, and select the approriate candidate higher in the stack, in the ide layer

Paul Faria (Oct 20 2020 at 17:45, on Zulip):

Ok, I'll give that a shot after work today.

Paul Faria (Oct 23 2020 at 14:37, on Zulip):

So I feel like I may have been taking the wrong approach again. I modified the approach to return a vec of matching modules, and have started bubbling that up (I tried returning impl iterator but the usage of self within the functions causes issues due to borrowing self mutably and immutably at the same time. There might be workarounds available). The issue is there's a lot of code within the various layers that assume there's only 0 or 1 modules, and then maps that data to other types. I've gotten to the point where now even multiple source analyzers are being generated since it's not clear which ids to pass to those. I feel like bubbling that complexity up might make sense if there was a way to filter which analysis to perform, but with vecs, this is causing a ton of unused information to be processed, on top of making the code higher up the stack harder to follow.

Paul Faria (Oct 24 2020 at 17:39, on Zulip):

@matklad I wonder if I misunderstood your original suggestion. Were you implying that I should modify RootDabase (in the ide_db crate) so that when its <impl FileLoader for RootDabase>::relevant_crates is called, the filtering is done within that fn? I've tried to continue with the exposing the Vec<Module> up the call stack (rather than returning the first result from source_to_def.rs:SourceToDefCtx::file_to_def, but it just touches so much that it really feels like the wrong approach. Let me know if you have time this weekend or early in the morning (I'm in Eastern US) to go over things.

matklad (Oct 26 2020 at 10:49, on Zulip):

No, I do feel that "just return a list of things and kick the can up to the caller to select one" is a fundamentally better approach than "has some global config for selection".

But I think some engineering should be applied to minimize the syntactic overhead of threading this information. Infecting everything with a new viral monad (list) is indeed problematic. Generally, when you want to thread something somewhere, it's better to piggyback on some existing piece of context, which already is threaded everywhere.

I think for rust-analyzer the appropriate context is the Semantics. Currently it is created with pub fn new(db: &DB) -> Semantics<DB>, and just from this signature it is obvious that it is agnostic about current set of CFG (well, unless we put some global current context in DB, which is the reason why we should avoid doing that)

I think we should add pub fn new_for_crate(db: &DB, krate: CrateId) -> Semantics<DB> to allow creating semantics scoped to the current crate.
Then, somewhere in the IDE, we should have a function that returns a list of applicable CrateId and the one that matches best. I am not sure what that would look like exactly, in theory it should be able to do tricks like (pick the crate with test if cursor is inside tests). After that, we can incrementally start substitutingSemantics::new_for_crate for Semantics::new. When there are no crate-less usages left, we can delete thouse.

Paul Faria (Oct 26 2020 at 13:58, on Zulip):

I played around with your idea of storing the CrateId inside of Semantics and it really simplifies a lot of the changes. I think in the ide layer I'll just need a way to call db.relevant_crates(file_id) and use that to filter by configs before creating a Semantics. This was incredibly helpful! I'll try some more things out after work.

Paul Faria (Nov 02 2020 at 14:30, on Zulip):

@matklad where is the best place to store the currently active crate for the current file? For example, with rust-project.json, we can have two crates, one with test enabled, and one with it disabled, that both include some_file.rs in its list of files. Where's the best place to store that crate id so that when I do something like goto_definition, I can refer to that crate id so I know which definitions to analyze? Does GlobalState make sense, or does the lsp not necessarily notify the server of when the active file has changed, so there might be a risk of the context being out of sync with the UI?

woody77 (Nov 04 2020 at 00:34, on Zulip):

Paul Faria said:

matklad where is the best place to store the currently active crate for the current file? For example, with rust-project.json, we can have two crates, one with test enabled, and one with it disabled, that both include some_file.rs in its list of files.

Should those really be separate crates? Or just separate instances of the same crate in the crate graph? (or does that really mean the same thing, as I haven't dug deeply into the crate graph after it's created).

Paul Faria (Nov 04 2020 at 15:36, on Zulip):

Maybe we could add a field to rust-project.json to say whether test is configurable or not and collapse the two?
Though there still might be cases where one file is included in multiple crates. There's currently nothing that prevents that with the rust-project.json format.

matklad (Nov 04 2020 at 17:05, on Zulip):

where is the best place to store the currently active crate for the current file? F

Well, the best choice is not to store this at all, but rather pass in as an argument with every request

matklad (Nov 04 2020 at 17:05, on Zulip):

Noteice how things like CompletionConfig are arguments, rather then some global state

matklad (Nov 04 2020 at 17:06, on Zulip):

So I don't think we should add any state

matklad (Nov 04 2020 at 17:06, on Zulip):

other than that, stuffing the required info in the Config struct (in rust-analyzer), and materializing the low-level config during requests seems like a good approach

woody77 (Nov 05 2020 at 02:08, on Zulip):

I'm actually debugging something related to this at the moment: If multiple crates in a rust-project.json contain the same file, what would change _which_ of those crates is found to be the right one? (we've determined that adding/removing a _different_ crate from the file will cause the test vs. rlib crate to be used for a given source file, and are very confused).

matklad (Nov 06 2020 at 11:15, on Zulip):

what would change _which_ of those crates is found to be the right one?

The relative ordering. I think we are not using any hashsets on the path there, so probably "first one in projec.json wins" works, though I am not entirely sure

Paul Faria (Nov 06 2020 at 17:11, on Zulip):

Part of the mapping from file id to module id goes through fn relevant_crates(&self, file_id: FileId) -> Arc<FxHashSet<CrateId>>. I think this is where the ordering won't be guaranteed.

matklad (Nov 06 2020 at 17:12, on Zulip):

Right!

matklad (Nov 06 2020 at 17:13, on Zulip):

Argh, I think we actually should change that back to a Vec

Paul Faria (Nov 06 2020 at 17:14, on Zulip):

Also, I should be on top of this more next week. This week has been a bit distracting to say the least.

woody77 (Nov 10 2020 at 23:40, on Zulip):

Paul Faria said:

Part of the mapping from file id to module id goes through fn relevant_crates(&self, file_id: FileId) -> Arc<FxHashSet<CrateId>>. I think this is where the ordering won't be guaranteed.

I found that, tracing through code, but wasn't sure how one other crate being added/removed would cause a hashset to change iteration order, unless it _also_ hit a threshold which caused more/less buckets to be used, thereby reshuffling across a larger/smaller set of buckets than before.

Paul Faria (Nov 12 2020 at 15:59, on Zulip):

matklad said:

Noteice how things like CompletionConfig are arguments, rather then some global state

@matklad I just got some time this week to jump back. I think I was originally confused by this because CompletionConfig and Config are kept in a struct called GlobalState. My intention was to keep track of the last known active crate for a file somwhere in GlobalState, but doesn't keeping in Config mean that I can't actively update the currently active crate for a file? For example, looking at how https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/rust-analyzer/src/handlers.rs#L399 works, the current design doesn't really have a way to notify GlobalState or Config the crate context of the file that will be navigated to. Or maybe I'm misreading your original intent. Would your idea be to store the crate and file associations on initialization and not allow any changes without a full refresh?

matklad (Nov 13 2020 at 14:34, on Zulip):

Why do you need to track last active thing? Why this can't be stateless?

matklad (Nov 13 2020 at 14:36, on Zulip):

SO, for goto_definition, we should change snap.analysis.goto_definition(position) to snap.analysis.goto_definition(position, goto_definition_config), where goto_definition_config is a struct contain information about how to bias crates which contain the current file

matklad (Nov 13 2020 at 14:38, on Zulip):

It could probably look like this:

struct GotoDefConfig {
    // If gotodef request can be resolved in the context of several crates, bias towards the crates from this set
    priority_crates: FxHashSet<CrateId>,
}
Paul Faria (Nov 13 2020 at 15:50, on Zulip):

In the process of trying to come up with an example, I found that (at least in the small example) rust-analyzer was already doing what I was trying to make it do. I'm very clearly missing something in my mental model. I had a crate (let's call it Z) with two features, a and b. I had two other crates (use_a and use_b) the each depended on Z with a different feature enabled. They are in different directories, but I included them in the same vs code workspace. When navigating from use_a to Z, once I'm in Z, it will always act as if only feature a is enabled. When navigating from use_b to Z, once I'm in Z, it will alwasy act as if only feature b is enabled. How/where is the information about the current feature being stored? From what I can see, only the file name is being passed through lsp, so I can't understand how the context is being managed.

matklad (Nov 13 2020 at 15:53, on Zulip):

are you opening the whole workspace, or only the respective use crates? If the letter, then the difference is in what cargo metadataoutput looks liek

Paul Faria (Nov 13 2020 at 15:57, on Zulip):

I don't have a cargo workspace, but a vs code workspace (where both crates are opened as individual directories within VS Code, done through the File -> Add Folder to Workspace menu option)

matklad (Nov 13 2020 at 16:03, on Zulip):

yeah, i think in this case we"ll call metadata twice, with two different current_dirs, so cargo would enable two sets of flags for us

Paul Faria (Nov 13 2020 at 16:08, on Zulip):

Though what level is tracking which feature set is currently active for some given file path? I think that's what I've been struggling to figure out. At least, the impression I get is that going from one of the use_* crates to Z maintains the feature set while navigating around Z. Does manually viewing one of the files in use_a or use_b switch the overall context somehow?

Paul Faria (Nov 13 2020 at 16:09, on Zulip):

And by manually viewing I mean selecting the file from the file list in VS Code.

matklad (Nov 13 2020 at 16:17, on Zulip):

Hm, there shouldnt the the "maintinig view" effect. Maybe there are separate instances of rust-analyzer involved somehow?

Paul Faria (Nov 13 2020 at 16:17, on Zulip):

Maybe it's actually two separate analyses and switching the active file changes which analysis is active?

Paul Faria (Nov 13 2020 at 16:18, on Zulip):

I'll take some time to review how that works with the workspaces setup. I'm guessing this is not the same as setting a root project when opening rust-project.json files, right? I.e. those all share the same analysis, it's not separate analyses?

matklad (Nov 13 2020 at 16:22, on Zulip):

Well, there should be separate analysis anywhere

matklad (Nov 13 2020 at 16:22, on Zulip):

If that happens, that is a bug

matklad (Nov 13 2020 at 16:22, on Zulip):

But it looks like we get those...

Last update: Jul 29 2021 at 07:45UTC