ra_project_model::json_project::Crate.key_value_cfg doesn't match ra_config::CfgOptions.key_values

[woody77]

In debugging some #[cfg(<feature>)] issues, I discovered that there's a mismatch in how features are handled by the json_project module vs. the CfgOptions struct:

in ra_config/src/lib.rs:23

pub struct CfgOptions {
    atoms: FxHashSet<SmolStr>,
    key_values: FxHashSet<(SmolStr, SmolStr)>,
}

and in ra_project/src/json_project:19

pub struct Crate {
    pub(crate) root_module: PathBuf,
    pub(crate) edition: Edition,
    pub(crate) deps: Vec<Dep>,
    pub(crate) atom_cfgs: FxHashSet<String>,
    pub(crate) key_value_cfgs: FxHashMap<String, String>,
    pub(crate) out_dir: Option<PathBuf>,
    pub(crate) proc_macro_dylib_path: Option<PathBuf>,
}

CfgOptions.key_values is a HashSet<(String, String)>, while Crate.key_value_cfg is a HashMap<String, String>.

The result being that the json_project configuration path only excepts a single feature (or a single value for any named kind of item in the cfg).

I have a quick fix together that switches Crate.key_value_cfg over to a HashSet<(String, String>) to match the CfgOptions, but it needs a different json structure:

Instead of:

"key_value_cfgs": {
    "feature": "alloc",
    "feature": "default",
    "feature": "i128_support",
},

(which when parsed results in a ("feature", "i128_support") tuple as the only "feature" that was loaded),

it becomes:

"key_value_cfgs": [
    ["feature" , "alloc"],
    ["feature" , "default"],
    ["feature" , "i128_support"],
],

But that's a breaking change with respect to anyone currently using the json config method, the old json fails to be parsed entirely. Anyone using it will need to update their json files to match, at the same time, or I can write a custom Deserialize impl that will support both (and can later be removed in favor of the

I have a couple questions:

1. Is making key_value_cfgs a hashset of (String, String) the right change?
2. Is a custom Deserialize impl that bridges the gap for users welcome?
3. Where can I put the unit-tests for this? (in the same file?)

[matklad]

Excellent catch! Yeah, CfgOptions is right here!

This functionality is tested by a (single) integration test here: https://github.com/rust-analyzer/rust-analyzer/blob/a76d392fe9cb3c8adc67ba3700645e1bc3713f11/crates/rust-analyzer/tests/heavy_tests/main.rs#L372

I think we should extend this test, rather than add a second one alongside -- heavy tests are slow, so, long term, I think it's important to prioritize execution speed over ortogonal factoring here.

In terms of backwards compatability, the current format is not considered stable, so its should be fine to change it without fallbacks (but cc @benbrittain).

Not sure what would be the best json format here. I guess maybe we need to merge atom and keyvalue into simply:

"cfg": [
  "unix",
  "feature=foo",
  "feature=bar",
]

ie, split by = just like rustc does?

[benbrittain]

I'm fine with dropping it into a single list. It felt more structured the other way but it's not a big deal.

I told @woody77 that I'm fine with a hard transition or soft, whatever is easiest for him and you. We'll need to time our next toolchain role with the rust-analyzer release pretty tightly if we do a hard transition though. I'm looking forward to having rust-analyzer distributed with rustc so that we can vendor a prebuilt.

[matklad]

Ah, haven't realized that you are already aware of this, feel free to unsubscribe from the issue then, Benjamin :)

@woody77, so, if you control both sides of the transition here, than do whatever you find more convenient, with the constraint that, on rust-analyzer's side, we would prefer to remove the fallback in the short/medium term.

I don't have strong preference about the json format, but I still think I am +0 on the single cfg with splitting on the first =.

[woody77]

I like the approach of a single cfg. It makes for an easy transition, as it doesn't require any custom deserialization, and just a new field, and then atom_cfgs and key_value_cfgs can be removed separately, after a toolchain role.

[woody77]

A question on the integration test at:
https://github.com/rust-analyzer/rust-analyzer/blob/a76d392fe9cb3c8adc67ba3700645e1bc3713f11/crates/rust-analyzer/tests/heavy_tests/main.rs#L372

This doesn't verify the cfgs, just that they they don't cause any issues. Were you thinking of extending that to include some #[cfg(<feature>)] items that would then be validated in the output?

[matklad]

For the test, I think it's best to verify that goto definition takes cfgs into account, like is done in this PR for cargo-based builds:

https://github.com/rust-analyzer/rust-analyzer/pull/4296/files#diff-6c022a6d0a252a09b52cea16ebcaf287R578

we probably should rename the test itself to something like test_project_json.

[woody77]

Thanks for the pointer to that. That definitely makes for a better test. For my prototyping, I've added a few very light unit-tests to json_project.rs and was thinking of adding one to ra_project_model/lib.rs to make sure that CfgOptions is properly set from the json file.

But it looks like you have a preference for integration tests over unit tests in this code?

[matklad]

Unit tests indeed would not harm, but, for those areas where perfectly pure and reproducible rust-analyzer internals are glued to the messy external world, we indeed favor (few) integration tests.

[woody77]

I've been using unit-tests to validate my code changes (using both the previous and the new values). I'm trying to get a PR up. It's been a while since I did any PRs with GitHub, so it's taking me time to get that sorted.

[woody77]

I've hit a snag, however. What's this env var?
https://github.com/rust-analyzer/rust-analyzer/blob/6f2f9049dad24681a5e785555f975bfed8138d45/crates/rust-analyzer/tests/heavy_tests/main.rs#L794

Well, I guess the right question is, why would I not be getting it set, now that I've found what it's supposed to be..?

error: environment variable `CARGO_BIN_EXE_rust-analyzer` not defined
   --> crates/rust-analyzer/tests/heavy_tests/main.rs:691:44
    |
691 |         let macro_srv_path = PathBuf::from(env!("CARGO_BIN_EXE_rust-analyzer"));
    |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: could not compile `rust-analyzer`.

[edwin0cheng]

This is a new env introduced in cargo 1.43 :

rust-lang/cargo#7697

[woody77]

That worked, thanks much.

[woody77]

#4382 is a draft PR for this. I haven't expanded the integration test yet, and I'm still sorting out how to use a locally-build rust-analyzer, but wanted to get feedback on approach and the tests.