Skip to content

Commit 68e9161

Browse files
alibektasalibektas
committed
Introduce a new CrateOrigin variant Member
CrateGraph has been suffering from ignoring a unified definition of a Workspace : Cargo rightly assumes that each project has its own workspace but in RA this assumption does not hold and this has caused problems for renaming symbols ( rust-lang#15656 ). What I propose is to add a new variant to `CrateOrigin` called `Member` ( the name is open to discussion :D ). Member will be assigned to workspace members and this is easy to find out as cargo provides this information directly. `CrateOrigin::Local` will now be those crates that workspaces depend on and these crates need to be in the local filesystem and open to editing ( current state of things doesn't really reflect what I describe but anyway... ) and `CrateOrigin::Library` will be the *vendor* crates. By doing this we can now express things like "a symbol is renameable if it originates from a user defined crate ( meaning if `CrateOrigin` is `Local` or `Member`)". This follows in part @ogapo's findings so let's thank them too!
1 parent 457b966 commit 68e9161

17 files changed

+1678
-41
lines changed

‎crates/base-db/src/fixture.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,7 @@ impl ChangeFixture {
225225
Some(default_cfg),
226226
default_env,
227227
false,
228-
CrateOrigin::Local { repo: None, name: None },
228+
CrateOrigin::Member { repo: None, name: None },
229229
default_target_data_layout
230230
.map(|it| it.into())
231231
.ok_or_else(|| "target_data_layout unset".into()),
@@ -319,7 +319,7 @@ impl ChangeFixture {
319319
Default::default(),
320320
Env::new_for_test_fixture(),
321321
true,
322-
CrateOrigin::Local { repo: None, name: None },
322+
CrateOrigin::Member { repo: None, name: None },
323323
target_layout,
324324
Some(toolchain),
325325
);
@@ -524,7 +524,7 @@ fn parse_crate(
524524
if non_workspace_member {
525525
CrateOrigin::Library { repo, name }
526526
} else {
527-
CrateOrigin::Local { repo, name: Some(name) }
527+
CrateOrigin::Member { repo, name: Some(name) }
528528
}
529529
}
530530
origin => CrateOrigin::Lang(origin),

‎crates/base-db/src/input.rs

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -145,14 +145,24 @@ pub enum CrateOrigin {
145145
/// Crates that are from the rustc workspace.
146146
Rustc { name: String },
147147
/// Crates that are workspace members.
148+
Member { repo: Option<String>, name: Option<String> },
149+
/// Crates that are non member user libraries.
148150
Local { repo: Option<String>, name: Option<String> },
149-
/// Crates that are non member libraries.
151+
/// Crates that are non member external libraries.
150152
Library { repo: Option<String>, name: String },
151153
/// Crates that are provided by the language, like std, core, proc-macro, ...
152154
Lang(LangCrateOrigin),
153155
}
154156

155157
impl CrateOrigin {
158+
pub fn is_user(&self) -> bool {
159+
matches!(self, CrateOrigin::Member { .. } | CrateOrigin::Local { .. })
160+
}
161+
162+
pub fn is_member(&self) -> bool {
163+
matches!(self, CrateOrigin::Member { .. })
164+
}
165+
156166
pub fn is_local(&self) -> bool {
157167
matches!(self, CrateOrigin::Local { .. })
158168
}
@@ -672,16 +682,20 @@ impl CrateGraph {
672682
return Some((id, false));
673683
}
674684
}
675-
(a @ CrateOrigin::Local { .. }, CrateOrigin::Library { .. })
676-
| (a @ CrateOrigin::Library { .. }, CrateOrigin::Local { .. }) => {
685+
(a, b) if a.is_user() && b.is_lib() || a.is_member() && b.is_local() => {
677686
// If the origins differ, check if the two crates are equal without
678687
// considering the dev dependencies, if they are, they most likely are in
679688
// different loaded workspaces which may cause issues. We keep the local
680689
// version and discard the library one as the local version may have
681690
// dev-dependencies that we want to keep resolving. See #15656 for more
682691
// information.
683692
if data.eq_ignoring_origin_and_deps(&crate_data, true) {
684-
return Some((id, if a.is_local() { false } else { true }));
693+
return Some((id, false));
694+
}
695+
}
696+
(b, a) if a.is_user() && b.is_lib() || a.is_member() && b.is_local() => {
697+
if data.eq_ignoring_origin_and_deps(&crate_data, true) {
698+
return Some((id, true));
685699
}
686700
}
687701
(_, _) => return None,
@@ -693,8 +707,7 @@ impl CrateGraph {
693707
if let Some((res, should_update_lib_to_local)) = res {
694708
id_map.insert(topo, res);
695709
if should_update_lib_to_local {
696-
assert!(self.arena[res].origin.is_lib());
697-
assert!(crate_data.origin.is_local());
710+
assert!(crate_data.origin.is_user());
698711
self.arena[res].origin = crate_data.origin.clone();
699712

700713
// Move local's dev dependencies into the newly-local-formerly-lib crate.
@@ -888,7 +901,7 @@ mod tests {
888901
Default::default(),
889902
Env::default(),
890903
false,
891-
CrateOrigin::Local { repo: None, name: None },
904+
CrateOrigin::Member { repo: None, name: None },
892905
Err("".into()),
893906
None,
894907
);
@@ -901,7 +914,7 @@ mod tests {
901914
Default::default(),
902915
Env::default(),
903916
false,
904-
CrateOrigin::Local { repo: None, name: None },
917+
CrateOrigin::Member { repo: None, name: None },
905918
Err("".into()),
906919
None,
907920
);
@@ -914,7 +927,7 @@ mod tests {
914927
Default::default(),
915928
Env::default(),
916929
false,
917-
CrateOrigin::Local { repo: None, name: None },
930+
CrateOrigin::Member { repo: None, name: None },
918931
Err("".into()),
919932
None,
920933
);
@@ -950,7 +963,7 @@ mod tests {
950963
Default::default(),
951964
Env::default(),
952965
false,
953-
CrateOrigin::Local { repo: None, name: None },
966+
CrateOrigin::Member { repo: None, name: None },
954967
Err("".into()),
955968
None,
956969
);
@@ -963,7 +976,7 @@ mod tests {
963976
Default::default(),
964977
Env::default(),
965978
false,
966-
CrateOrigin::Local { repo: None, name: None },
979+
CrateOrigin::Member { repo: None, name: None },
967980
Err("".into()),
968981
None,
969982
);
@@ -993,7 +1006,7 @@ mod tests {
9931006
Default::default(),
9941007
Env::default(),
9951008
false,
996-
CrateOrigin::Local { repo: None, name: None },
1009+
CrateOrigin::Member { repo: None, name: None },
9971010
Err("".into()),
9981011
None,
9991012
);
@@ -1006,7 +1019,7 @@ mod tests {
10061019
Default::default(),
10071020
Env::default(),
10081021
false,
1009-
CrateOrigin::Local { repo: None, name: None },
1022+
CrateOrigin::Member { repo: None, name: None },
10101023
Err("".into()),
10111024
None,
10121025
);
@@ -1019,7 +1032,7 @@ mod tests {
10191032
Default::default(),
10201033
Env::default(),
10211034
false,
1022-
CrateOrigin::Local { repo: None, name: None },
1035+
CrateOrigin::Member { repo: None, name: None },
10231036
Err("".into()),
10241037
None,
10251038
);
@@ -1049,7 +1062,7 @@ mod tests {
10491062
Default::default(),
10501063
Env::default(),
10511064
false,
1052-
CrateOrigin::Local { repo: None, name: None },
1065+
CrateOrigin::Member { repo: None, name: None },
10531066
Err("".into()),
10541067
None,
10551068
);
@@ -1062,7 +1075,7 @@ mod tests {
10621075
Default::default(),
10631076
Env::default(),
10641077
false,
1065-
CrateOrigin::Local { repo: None, name: None },
1078+
CrateOrigin::Member { repo: None, name: None },
10661079
Err("".into()),
10671080
None,
10681081
);

‎crates/ide-assists/src/handlers/add_missing_impl_members.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ fn add_missing_impl_members_inner(
126126
if let IgnoreAssocItems::DocHiddenAttrPresent = ignore_items {
127127
// Relax condition for local crates.
128128
let db = ctx.db();
129-
if trait_.module(db).krate().origin(db).is_local() {
129+
if trait_.module(db).krate().origin(db).is_user() {
130130
ign_item = IgnoreAssocItems::No;
131131
}
132132
}

‎crates/ide-db/src/rename.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ impl Definition {
7676
// it is not allowed for these defs to be renamed.
7777
// cases where self.krate() is None is handled below.
7878
if let Some(krate) = self.krate(sema.db) {
79-
if !krate.origin(sema.db).is_local() {
79+
if !krate.origin(sema.db).is_user() {
8080
bail!("Cannot rename a non-local definition.")
8181
}
8282
}

‎crates/ide/src/doc_links.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -512,7 +512,7 @@ fn get_doc_base_urls(
512512
CrateOrigin::Rustc { name: _ } => {
513513
(Some(format!("https://doc.rust-lang.org/{channel}/nightly-rustc/")), None)
514514
}
515-
CrateOrigin::Local { repo: _, name: _ } => {
515+
CrateOrigin::Local { repo: _, name: _ } | CrateOrigin::Member { .. } => {
516516
// FIXME: These should not attempt to link to docs.rs!
517517
let weblink = krate.get_html_root_url(db).or_else(|| {
518518
let version = krate.version(db);

‎crates/ide/src/fetch_crates.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ pub(crate) fn fetch_crates(db: &RootDatabase) -> FxIndexSet<CrateInfo> {
2626
crate_graph
2727
.iter()
2828
.map(|crate_id| &crate_graph[crate_id])
29-
.filter(|&data| !matches!(data.origin, CrateOrigin::Local { .. }))
29+
.filter(|&data| !matches!(data.origin, CrateOrigin::Member { .. }))
3030
.map(|data| crate_info(data))
3131
.collect()
3232
}

‎crates/ide/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ impl Analysis {
251251
None,
252252
Env::default(),
253253
false,
254-
CrateOrigin::Local { repo: None, name: None },
254+
CrateOrigin::Member { repo: None, name: None },
255255
Err("Analysis::from_single_file has no target layout".into()),
256256
None,
257257
);

‎crates/ide/src/moniker.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ pub(crate) fn def_to_moniker(
275275
package_information: {
276276
let (name, repo, version) = match krate.origin(db) {
277277
CrateOrigin::Library { repo, name } => (name, repo, krate.version(db)),
278-
CrateOrigin::Local { repo, name } => (
278+
CrateOrigin::Local { repo, name } | CrateOrigin::Member { repo, name } => (
279279
name.unwrap_or(krate.display_name(db)?.canonical_name().to_string()),
280280
repo,
281281
krate.version(db),

‎crates/project-model/src/tests.rs

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use std::{
33
path::{Path, PathBuf},
44
};
55

6-
use base_db::{CrateGraph, FileId, ProcMacroPaths};
6+
use base_db::{CrateGraph, CrateOrigin, FileId, ProcMacroPaths};
77
use cfg::{CfgAtom, CfgDiff};
88
use expect_test::{expect_file, ExpectFile};
99
use paths::{AbsPath, AbsPathBuf};
@@ -273,7 +273,7 @@ fn test_deduplicate_origin_dev() {
273273

274274
assert!(crates_named_p2.len() == 1);
275275
let p2 = crates_named_p2[0];
276-
assert!(p2.origin.is_local());
276+
assert!(p2.origin.is_member());
277277
}
278278

279279
#[test]
@@ -299,5 +299,58 @@ fn test_deduplicate_origin_dev_rev() {
299299

300300
assert!(crates_named_p2.len() == 1);
301301
let p2 = crates_named_p2[0];
302-
assert!(p2.origin.is_local());
302+
assert!(p2.origin.is_member());
303+
}
304+
305+
#[test]
306+
fn test_local_origin_for_complex_workspaces() {
307+
let path_map = &mut Default::default();
308+
309+
let (mut crate_graph, _proc_macros) = load_cargo_with_sysroot(path_map, "proj1.json");
310+
311+
let (mut crate_graph_1, mut _proc_macros_1) = load_cargo_with_sysroot(path_map, "proj2.json");
312+
crate_graph_1.sort_deps();
313+
crate_graph.extend(crate_graph_1, &mut _proc_macros_1);
314+
315+
let (mut crate_graph_2, mut _proc_macros_2) = load_cargo_with_sysroot(path_map, "proj3.json");
316+
crate_graph_2.sort_deps();
317+
crate_graph.extend(crate_graph_2, &mut _proc_macros_2);
318+
319+
let (mut crate_graph_3, mut _proc_macros_3) = load_cargo_with_sysroot(path_map, "proj4.json");
320+
crate_graph_3.sort_deps();
321+
crate_graph.extend(crate_graph_3, &mut _proc_macros_3);
322+
323+
// Each crate named following the pattern "example.*" gets to have exactly one edge on the crate graph
324+
// whose origin is `CrateOrigin::Member`
325+
let mut example_1 = false;
326+
let mut example_1_macro = false;
327+
let mut example_2 = false;
328+
let mut example_2_macro = false;
329+
330+
for idx in crate_graph.crates_in_topological_order() {
331+
let cr = &crate_graph[idx];
332+
if let Some(name) = &cr.display_name {
333+
if cr.origin.is_member() {
334+
match name.canonical_name() {
335+
"example1" => {
336+
assert!(!example_1);
337+
example_1 = true;
338+
}
339+
"example1-macros" => {
340+
assert!(!example_1_macro);
341+
example_1_macro = true;
342+
}
343+
"example2" => {
344+
assert!(!example_2);
345+
example_2 = true;
346+
}
347+
"example2-macros" => {
348+
assert!(!example_2_macro);
349+
example_2_macro = true;
350+
}
351+
_ => (),
352+
}
353+
}
354+
}
355+
}
303356
}

‎crates/project-model/src/workspace.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1299,7 +1299,9 @@ fn add_target_crate_root(
12991299
if rustc_crate {
13001300
CrateOrigin::Rustc { name: pkg.name.clone() }
13011301
} else if pkg.is_member {
1302-
CrateOrigin::Local { repo: pkg.repository.clone(), name: Some(pkg.name.clone()) }
1302+
CrateOrigin::Member { repo: None, name: Some(pkg.name.clone()) }
1303+
} else if pkg.id.contains("path+file") {
1304+
CrateOrigin::Local { repo: None, name: Some(pkg.name.clone()) }
13031305
} else {
13041306
CrateOrigin::Library { repo: pkg.repository.clone(), name: pkg.name.clone() }
13051307
},

‎crates/project-model/test_data/output/cargo_hello_world_project_model.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
prelude: true,
5353
},
5454
],
55-
origin: Local {
55+
origin: Member {
5656
repo: None,
5757
name: Some(
5858
"hello-world",
@@ -125,7 +125,7 @@
125125
prelude: true,
126126
},
127127
],
128-
origin: Local {
128+
origin: Member {
129129
repo: None,
130130
name: Some(
131131
"hello-world",
@@ -198,7 +198,7 @@
198198
prelude: true,
199199
},
200200
],
201-
origin: Local {
201+
origin: Member {
202202
repo: None,
203203
name: Some(
204204
"hello-world",
@@ -271,7 +271,7 @@
271271
prelude: true,
272272
},
273273
],
274-
origin: Local {
274+
origin: Member {
275275
repo: None,
276276
name: Some(
277277
"hello-world",

‎crates/project-model/test_data/output/cargo_hello_world_project_model_with_selective_overrides.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
prelude: true,
5353
},
5454
],
55-
origin: Local {
55+
origin: Member {
5656
repo: None,
5757
name: Some(
5858
"hello-world",
@@ -125,7 +125,7 @@
125125
prelude: true,
126126
},
127127
],
128-
origin: Local {
128+
origin: Member {
129129
repo: None,
130130
name: Some(
131131
"hello-world",
@@ -198,7 +198,7 @@
198198
prelude: true,
199199
},
200200
],
201-
origin: Local {
201+
origin: Member {
202202
repo: None,
203203
name: Some(
204204
"hello-world",
@@ -271,7 +271,7 @@
271271
prelude: true,
272272
},
273273
],
274-
origin: Local {
274+
origin: Member {
275275
repo: None,
276276
name: Some(
277277
"hello-world",

0 commit comments

Comments
 (0)