-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ra_project_model::json_project::Crate.key_value_cfg
doesn't match ra_config::CfgOptions.key_values
#4310
Comments
Excellent catch! Yeah, 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:
ie, split by |
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. |
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 |
I like the approach of a single |
A question on the integration test at: This doesn't verify the cfgs, just that they they don't cause any issues. Were you thinking of extending that to include some |
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: we probably should rename the test itself to something like |
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 But it looks like you have a preference for integration tests over unit tests in this code? |
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. |
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. |
I've hit a snag, however. What's this env var? 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..?
|
This is a new env introduced in cargo 1.43 : |
That worked, thanks much. |
#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 |
In debugging some
#[cfg(<feature>)]
issues, I discovered that there's a mismatch in how features are handled by thejson_project
module vs. theCfgOptions
struct:in ra_config/src/lib.rs:23
and in ra_project/src/json_project:19
CfgOptions.key_values
is aHashSet<(String, String)>
, whileCrate.key_value_cfg
is aHashMap<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 aHashSet<(String, String>)
to match the CfgOptions, but it needs a different json structure:Instead of:
(which when parsed results in a
("feature", "i128_support")
tuple as the only "feature" that was loaded),it becomes:
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 theI have a couple questions:
key_value_cfgs
a hashset of(String, String)
the right change?Deserialize
impl that bridges the gap for users welcome?The text was updated successfully, but these errors were encountered: