Skip to content

Commit 977d09a

Browse files
paldepindCopilot
andcommitted
Avoid eager dependency semantics when selecting Rust configurations
Rank unambiguous dependency roots using crate graph metadata, preserving semantic ownership checks for roots with differing configurations. Retry inactive modules in lower-priority configurations and bypass selection entirely for single-workspace extractions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d675ac38-ec3b-47ee-8b48-268c63fc7777
1 parent b57a67c commit 977d09a

2 files changed

Lines changed: 246 additions & 62 deletions

File tree

‎rust/extractor/src/configuration.rs‎

Lines changed: 196 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
use crate::util::cmp_option_by;
22
use itertools::Itertools;
3-
use ra_ap_base_db::SourceDatabase;
3+
use ra_ap_base_db::{SourceDatabase, SourceRootId, all_crates};
44
use ra_ap_cfg::{CfgAtom, CfgOptions};
55
use ra_ap_hir::Semantics;
66
use ra_ap_ide_db::RootDatabase;
77
use ra_ap_project_model::ProjectManifest;
8-
use ra_ap_vfs::{FileId, VfsPath};
8+
use ra_ap_vfs::{FileId, Vfs};
99
use std::cmp::Ordering;
1010
use std::collections::{BTreeMap, HashMap, HashSet, btree_map::Entry};
1111
use std::path::{Path, PathBuf};
@@ -15,6 +15,65 @@ pub fn manifest_parent(manifest: &ProjectManifest) -> &Path {
1515
manifest.manifest_path().parent().as_ref()
1616
}
1717

18+
/// Reads the file's source-root ID in `db`.
19+
///
20+
/// A source root is rust-analyzer's group of files within which relative paths
21+
/// are resolved. It need not correspond to one crate: a package's library and
22+
/// build script can share a source root.
23+
fn source_root_id(db: &RootDatabase, file_id: FileId) -> SourceRootId {
24+
db.file_source_root(file_id).source_root_id(db)
25+
}
26+
27+
/// Maps source root IDs to cfg options shared by all their crates, using only
28+
/// metadata.
29+
///
30+
/// In order for a file to be included in a crate the crate's root file and the
31+
/// file must share the same source root. However, this condition is not
32+
/// sufficient. The construction here relies on this to identify the cases where
33+
/// this requirement _is_ sufficient to determine the cfg options of any crate
34+
/// that could contain the file. Either because only one crate can contain it or
35+
/// because all such crates share the same cfg options.
36+
///
37+
/// Roots whose crates disagree map to `None` and need per-file semantic lookup.
38+
fn root_configurations(db: &RootDatabase) -> HashMap<SourceRootId, Option<&CfgOptions>> {
39+
let mut configurations = HashMap::new();
40+
for &krate in all_crates(db).iter() {
41+
let root = source_root_id(db, krate.data(db).root_file_id);
42+
configurations
43+
.entry(root)
44+
.and_modify(|configuration: &mut Option<&CfgOptions>| {
45+
// We're in the case where multiple crates share a source root,
46+
// for example a library and its build script. If their cfg
47+
// options differ, we mark the configuration as `None`.
48+
if configuration.is_some_and(|cfg| cfg != krate.cfg_options(db)) {
49+
*configuration = None;
50+
}
51+
})
52+
.or_insert_with(|| Some(krate.cfg_options(db)));
53+
}
54+
configurations
55+
}
56+
57+
/// Returns an iterator over the dependency files in `vfs`.
58+
///
59+
/// These are files that correspond to libraries and that are not in `source_files`.
60+
pub fn dependency_files<'a>(
61+
db: &'a RootDatabase,
62+
vfs: &'a Vfs,
63+
source_files: &'a HashSet<PathBuf>,
64+
) -> impl Iterator<Item = (FileId, &'a Path)> {
65+
vfs.iter().filter_map(|(file_id, file)| {
66+
let file = file.as_path().map(<_ as AsRef<Path>>::as_ref)?;
67+
(file.extension().is_some_and(|ext| ext == "rs")
68+
&& !source_files.contains(file)
69+
&& db
70+
.source_root(source_root_id(db, file_id))
71+
.source_root(db)
72+
.is_library)
73+
.then_some((file_id, file))
74+
})
75+
}
76+
1877
/// Compare two `CfgOptions`. Feature count wins, sorted cfg options break ties.
1978
fn compare_cfg_options(left: &CfgOptions, right: &CfgOptions) -> Ordering {
2079
let feature_count = |cfg_options: &CfgOptions| {
@@ -49,38 +108,48 @@ impl PartialOrd for DependencySource<'_> {
49108
}
50109
}
51110

52-
/// Map dependency file paths to their highest-priority workspace context.
111+
/// Tracks extraction candidates for each dependency file. Keeps one preferred
112+
/// manifest per distinct set of cfg options, allowing lower-priority
113+
/// configurations to be tried if the preferred one disables a file.
53114
#[derive(Default)]
54115
pub struct DependencyFiles<'a> {
55-
files: BTreeMap<PathBuf, DependencySource<'a>>,
116+
files: BTreeMap<PathBuf, Vec<DependencySource<'a>>>,
56117
}
57118

58119
impl<'a> DependencyFiles<'a> {
59-
/// Considers this workspace as an extraction context for a dependency file,
60-
/// retaining only the highest-priority context seen so far. The lexicographically
61-
/// largest workspace path breaks identical-priority ties.
62-
pub fn add_file(
120+
/// Considers dependency files loaded from this manifest as extraction
121+
/// candidates. The lexicographically largest manifest directory wins for
122+
/// each configuration.
123+
pub fn add_manifest(
63124
&mut self,
64125
manifest: &'a ProjectManifest,
65-
file_id: FileId,
66-
file: &VfsPath,
67126
semantics: &Semantics<'_, RootDatabase>,
127+
vfs: &Vfs,
68128
source_files: &HashSet<PathBuf>,
69129
) {
70130
let db = semantics.db;
71-
if let Some(file) = file.as_path().map(<_ as AsRef<Path>>::as_ref)
72-
&& file.extension().is_some_and(|ext| ext == "rs")
73-
&& !source_files.contains(file)
74-
&& let root = db.file_source_root(file_id).source_root_id(db)
75-
&& db.source_root(root).source_root(db).is_library
76-
{
77-
let cfg_options = semantics
78-
.file_to_module_def(file_id)
79-
.map(|module| module.krate(db).cfg(db).clone());
131+
let root_configurations = root_configurations(db);
132+
for (file_id, file) in dependency_files(db, vfs, source_files) {
133+
let root = source_root_id(db, file_id);
134+
let cfg_options = match root_configurations.get(&root) {
135+
// There's a unique cfg configuration for this file.
136+
Some(Some(configuration)) => Some((*configuration).clone()),
137+
// There's no unique cfg configuration for this file. In this
138+
// case we fall back to the expensive `file_to_module_def`
139+
// approach which requires rust_analyzer to calculate semantic
140+
// information.
141+
Some(None) => semantics
142+
.file_to_module_def(file_id)
143+
.map(|module| module.krate(db).cfg(db).clone()),
144+
None => None,
145+
};
80146
self.add_candidate(file, cfg_options, manifest);
81147
}
82148
}
83149

150+
/// Adds `cfg_options` and `manifest` as an extraction candidate for the
151+
/// given file unless a higher-priority candidate with the same cfg options
152+
/// already exists.
84153
fn add_candidate(
85154
&mut self,
86155
file: &Path,
@@ -93,38 +162,132 @@ impl<'a> DependencyFiles<'a> {
93162
};
94163
match self.files.entry(file.to_owned()) {
95164
Entry::Occupied(mut entry) => {
96-
if candidate > *entry.get() {
97-
entry.insert(candidate);
165+
let sources = entry.get_mut();
166+
if let Some(source) = sources
167+
.iter_mut()
168+
.find(|source| source.cfg_options == candidate.cfg_options)
169+
{
170+
if manifest_parent(manifest) > manifest_parent(source.manifest) {
171+
source.manifest = manifest;
172+
}
173+
} else {
174+
sources.push(candidate);
98175
}
99176
}
100177
Entry::Vacant(entry) => {
101-
entry.insert(candidate);
178+
entry.insert(vec![candidate]);
102179
}
103180
}
104181
}
105182

106-
/// Groups the selected dependency files by manifest for the dependency extraction pass.
107-
pub fn by_manifests(self) -> HashMap<&'a ProjectManifest, Vec<PathBuf>> {
183+
/// Takes the highest-priority remaining context for each file, grouped by
184+
/// manifest. Lower-priority configurations remain available if the selected
185+
/// one disables the file.
186+
pub fn by_manifests(&mut self) -> HashMap<&'a ProjectManifest, Vec<PathBuf>> {
108187
self.files
109-
.into_iter()
110-
.map(|(file, source)| (source.manifest, file))
188+
.iter_mut()
189+
.filter_map(|(file, sources)| {
190+
sources.sort_unstable();
191+
sources.pop().map(|source| (source.manifest, file.clone()))
192+
})
111193
.into_group_map()
112194
}
195+
196+
/// Returns `true` if there are alternative configurations left for the
197+
/// given file.
198+
pub fn has_alternative(&self, file: &Path) -> bool {
199+
self.files
200+
.get(file)
201+
.is_some_and(|sources| !sources.is_empty())
202+
}
203+
204+
/// Marks a file as extracted, removing it from the map of files.
205+
pub fn extracted(&mut self, file: &Path) {
206+
self.files.remove(file);
207+
}
113208
}
114209

115210
#[cfg(test)]
116211
mod tests {
117212
use super::*;
213+
use ra_ap_base_db::{
214+
CrateGraphBuilder, CrateOrigin, CrateWorkspaceData, FileChange, FileSet, SourceRoot,
215+
};
118216
use ra_ap_intern::Symbol;
217+
use ra_ap_span::Edition;
218+
use ra_ap_vfs::VfsPath;
219+
use triomphe::Arc;
220+
221+
fn cfg(features: &[&str]) -> CfgOptions {
222+
let mut cfg = CfgOptions::default();
223+
for feature in features {
224+
cfg.insert_key_value(Symbol::intern("feature"), Symbol::intern(feature));
225+
}
226+
cfg
227+
}
228+
229+
fn workspace(files: &[&str], crates: &[(&str, &[&str])]) -> (RootDatabase, Vfs) {
230+
let root = crate::cwd().unwrap();
231+
let mut vfs = Vfs::default();
232+
let mut file_set = FileSet::default();
233+
let mut change = FileChange::default();
234+
for &name in files {
235+
let path = VfsPath::from(root.join(name));
236+
vfs.set_file_contents(path.clone(), Some(Vec::new()));
237+
let (id, _) = vfs.file_id(&path).unwrap();
238+
file_set.insert(id, path);
239+
}
240+
let mut graph = CrateGraphBuilder::default();
241+
for &(name, features) in crates {
242+
let (id, _) = vfs.file_id(&VfsPath::from(root.join(name))).unwrap();
243+
graph.add_crate_root(
244+
id,
245+
Edition::Edition2024,
246+
None,
247+
None,
248+
cfg(features),
249+
None,
250+
Default::default(),
251+
CrateOrigin::Library {
252+
repo: None,
253+
name: Symbol::intern(name),
254+
},
255+
Vec::new(),
256+
false,
257+
Arc::new(root.clone()),
258+
Arc::new(CrateWorkspaceData {
259+
target: Err("unused in these tests".into()),
260+
toolchain: None,
261+
}),
262+
);
263+
}
264+
change.set_roots(vec![SourceRoot::new_library(file_set)]);
265+
change.set_crate_graph(graph);
266+
let mut db = RootDatabase::default();
267+
change.apply(&mut db);
268+
(db, vfs)
269+
}
270+
119271
#[test]
120-
fn selects_highest_priority_in_either_order() {
121-
let cfg = |features: &[&str]| {
122-
let mut cfg = CfgOptions::default();
123-
for feature in features {
124-
cfg.insert_key_value(Symbol::intern("feature"), Symbol::intern(feature));
272+
fn selects_configuration_without_reading_dependency_sources() {
273+
for crates in [
274+
vec![("lib.rs", &["a"][..])],
275+
vec![("lib.rs", &["a"][..]), ("build.rs", &["a"][..])],
276+
] {
277+
let (db, vfs) = workspace(&["lib.rs", "build.rs", "module.rs"], &crates);
278+
let root = crate::cwd().unwrap();
279+
let manifest = ProjectManifest::from_manifest_file(root.join("Cargo.toml")).unwrap();
280+
let mut plan = DependencyFiles::default();
281+
plan.add_manifest(&manifest, &Semantics::new(&db), &vfs, &HashSet::new());
282+
assert_eq!(plan.files.len(), 3);
283+
for sources in plan.files.values() {
284+
assert_eq!(sources[0].cfg_options, Some(cfg(&["a"])));
125285
}
126-
cfg
127-
};
286+
}
287+
}
288+
289+
#[test]
290+
fn selects_highest_priority_in_either_order() {
128291
let root = crate::cwd().unwrap();
129292
let manifest = |path| ProjectManifest::from_manifest_file(root.join(path)).unwrap();
130293
let candidates = [

0 commit comments

Comments
 (0)