diff --git a/rust/extractor/src/configuration.rs b/rust/extractor/src/configuration.rs new file mode 100644 index 000000000000..cbf94ee068d3 --- /dev/null +++ b/rust/extractor/src/configuration.rs @@ -0,0 +1,307 @@ +use crate::util::cmp_option_by; +use itertools::Itertools; +use ra_ap_base_db::{SourceDatabase, SourceRootId, all_crates}; +use ra_ap_cfg::{CfgAtom, CfgOptions}; +use ra_ap_hir::Semantics; +use ra_ap_ide_db::RootDatabase; +use ra_ap_project_model::ProjectManifest; +use ra_ap_vfs::{FileId, Vfs}; +use std::cmp::Ordering; +use std::collections::{BTreeMap, HashMap, HashSet, btree_map::Entry}; +use std::path::{Path, PathBuf}; + +/// Returns the enclosing directory of the manifest file. +pub fn manifest_parent(manifest: &ProjectManifest) -> &Path { + manifest.manifest_path().parent().as_ref() +} + +/// Reads the file's source-root ID in `db`. +/// +/// A source root is rust-analyzer's group of files within which relative paths +/// are resolved. It need not correspond to one crate: a package's library and +/// build script can share a source root. +fn source_root_id(db: &RootDatabase, file_id: FileId) -> SourceRootId { + db.file_source_root(file_id).source_root_id(db) +} + +/// Maps source root IDs to cfg options shared by all their crates, using only +/// metadata. +/// +/// In order for a file to be included in a crate the crate's root file and the +/// file must share the same source root. However, this condition is not +/// sufficient. The construction here relies on this to identify the cases where +/// this requirement _is_ sufficient to determine the cfg options of any crate +/// that could contain the file. Either because only one crate can contain it or +/// because all such crates share the same cfg options. +/// +/// Roots whose crates disagree map to `None` and need per-file semantic lookup. +fn root_configurations(db: &RootDatabase) -> HashMap> { + let mut configurations = HashMap::new(); + for &krate in all_crates(db).iter() { + let root = source_root_id(db, krate.data(db).root_file_id); + configurations + .entry(root) + .and_modify(|configuration: &mut Option<&CfgOptions>| { + // We're in the case where multiple crates share a source root, + // for example a library and its build script. If their cfg + // options differ, we mark the configuration as `None`. + if configuration.is_some_and(|cfg| cfg != krate.cfg_options(db)) { + *configuration = None; + } + }) + .or_insert_with(|| Some(krate.cfg_options(db))); + } + configurations +} + +/// Returns an iterator over the dependency files in `vfs`. +/// +/// These are files that correspond to libraries and that are not in `source_files`. +pub fn dependency_files<'a>( + db: &'a RootDatabase, + vfs: &'a Vfs, + source_files: &'a HashSet, +) -> impl Iterator { + vfs.iter().filter_map(|(file_id, file)| { + let file = file.as_path().map(<_ as AsRef>::as_ref)?; + (file.extension().is_some_and(|ext| ext == "rs") + && !source_files.contains(file) + && db + .source_root(source_root_id(db, file_id)) + .source_root(db) + .is_library) + .then_some((file_id, file)) + }) +} + +/// Compare two `CfgOptions`. Feature count wins, sorted cfg options break ties. +fn compare_cfg_options(left: &CfgOptions, right: &CfgOptions) -> Ordering { + let feature_count = |cfg_options: &CfgOptions| { + cfg_options + .into_iter() + .filter(|cfg| matches!(cfg, CfgAtom::KeyValue { key, .. } if key.as_str() == "feature")) + .count() + }; + feature_count(left) + .cmp(&feature_count(right)) + .then_with(|| left.into_iter().sorted().cmp(right.into_iter().sorted())) +} + +#[derive(PartialEq, Eq)] +struct DependencySource<'a> { + cfg_options: Option, + manifest: &'a ProjectManifest, +} + +impl Ord for DependencySource<'_> { + fn cmp(&self, other: &Self) -> Ordering { + // Compare by cfg options first, then by manifest parent directory + // (arbitrary but deterministic). + cmp_option_by(&self.cfg_options, &other.cfg_options, compare_cfg_options) + .then_with(|| manifest_parent(self.manifest).cmp(manifest_parent(other.manifest))) + } +} + +impl PartialOrd for DependencySource<'_> { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +/// Tracks extraction candidates for each dependency file. Keeps one preferred +/// manifest per distinct set of cfg options, allowing lower-priority +/// configurations to be tried if the preferred one disables a file. +#[derive(Default)] +pub struct DependencyFiles<'a> { + files: BTreeMap>>, +} + +impl<'a> DependencyFiles<'a> { + /// Considers dependency files loaded from this manifest as extraction + /// candidates. The lexicographically largest manifest directory wins for + /// each configuration. + pub fn add_manifest( + &mut self, + manifest: &'a ProjectManifest, + semantics: &Semantics<'_, RootDatabase>, + vfs: &Vfs, + source_files: &HashSet, + ) { + let db = semantics.db; + let root_configurations = root_configurations(db); + for (file_id, file) in dependency_files(db, vfs, source_files) { + let root = source_root_id(db, file_id); + let cfg_options = match root_configurations.get(&root) { + // There's a unique cfg configuration for this file. + Some(Some(configuration)) => Some((*configuration).clone()), + // There's no unique cfg configuration for this file. In this + // case we fall back to the expensive `file_to_module_def` + // approach which requires rust_analyzer to calculate semantic + // information. + Some(None) => semantics + .file_to_module_def(file_id) + .map(|module| module.krate(db).cfg(db).clone()), + None => None, + }; + self.add_candidate(file, cfg_options, manifest); + } + } + + /// Adds `cfg_options` and `manifest` as an extraction candidate for the + /// given file unless a higher-priority candidate with the same cfg options + /// already exists. + fn add_candidate( + &mut self, + file: &Path, + cfg_options: Option, + manifest: &'a ProjectManifest, + ) { + let candidate = DependencySource { + cfg_options, + manifest, + }; + match self.files.entry(file.to_owned()) { + Entry::Occupied(mut entry) => { + let sources = entry.get_mut(); + if let Some(source) = sources + .iter_mut() + .find(|source| source.cfg_options == candidate.cfg_options) + { + if manifest_parent(manifest) > manifest_parent(source.manifest) { + source.manifest = manifest; + } + } else { + sources.push(candidate); + } + } + Entry::Vacant(entry) => { + entry.insert(vec![candidate]); + } + } + } + + /// Takes the highest-priority remaining context for each file, grouped by + /// manifest. Lower-priority configurations remain available if the selected + /// one disables the file. + pub fn by_manifests(&mut self) -> HashMap<&'a ProjectManifest, Vec> { + self.files + .iter_mut() + .filter_map(|(file, sources)| { + sources.sort_unstable(); + sources.pop().map(|source| (source.manifest, file.clone())) + }) + .into_group_map() + } + + /// Returns `true` if there are alternative configurations left for the + /// given file. + pub fn has_alternative(&self, file: &Path) -> bool { + self.files + .get(file) + .is_some_and(|sources| !sources.is_empty()) + } + + /// Marks a file as extracted, removing it from the map of files. + pub fn extracted(&mut self, file: &Path) { + self.files.remove(file); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use ra_ap_base_db::{ + CrateGraphBuilder, CrateOrigin, CrateWorkspaceData, FileChange, FileSet, SourceRoot, + }; + use ra_ap_intern::Symbol; + use ra_ap_span::Edition; + use ra_ap_vfs::VfsPath; + use triomphe::Arc; + + fn cfg(features: &[&str]) -> CfgOptions { + let mut cfg = CfgOptions::default(); + for feature in features { + cfg.insert_key_value(Symbol::intern("feature"), Symbol::intern(feature)); + } + cfg + } + + fn workspace(files: &[&str], crates: &[(&str, &[&str])]) -> (RootDatabase, Vfs) { + let root = crate::cwd().unwrap(); + let mut vfs = Vfs::default(); + let mut file_set = FileSet::default(); + let mut change = FileChange::default(); + for &name in files { + let path = VfsPath::from(root.join(name)); + vfs.set_file_contents(path.clone(), Some(Vec::new())); + let (id, _) = vfs.file_id(&path).unwrap(); + file_set.insert(id, path); + } + let mut graph = CrateGraphBuilder::default(); + for &(name, features) in crates { + let (id, _) = vfs.file_id(&VfsPath::from(root.join(name))).unwrap(); + graph.add_crate_root( + id, + Edition::Edition2024, + None, + None, + cfg(features), + None, + Default::default(), + CrateOrigin::Library { + repo: None, + name: Symbol::intern(name), + }, + Vec::new(), + false, + Arc::new(root.clone()), + Arc::new(CrateWorkspaceData { + target: Err("unused in these tests".into()), + toolchain: None, + }), + ); + } + change.set_roots(vec![SourceRoot::new_library(file_set)]); + change.set_crate_graph(graph); + let mut db = RootDatabase::default(); + change.apply(&mut db); + (db, vfs) + } + + #[test] + fn selects_configuration_without_reading_dependency_sources() { + for crates in [ + vec![("lib.rs", &["a"][..])], + vec![("lib.rs", &["a"][..]), ("build.rs", &["a"][..])], + ] { + let (db, vfs) = workspace(&["lib.rs", "build.rs", "module.rs"], &crates); + let root = crate::cwd().unwrap(); + let manifest = ProjectManifest::from_manifest_file(root.join("Cargo.toml")).unwrap(); + let mut plan = DependencyFiles::default(); + plan.add_manifest(&manifest, &Semantics::new(&db), &vfs, &HashSet::new()); + assert_eq!(plan.files.len(), 3); + for sources in plan.files.values() { + assert_eq!(sources[0].cfg_options, Some(cfg(&["a"]))); + } + } + } + + #[test] + fn selects_highest_priority_in_either_order() { + let root = crate::cwd().unwrap(); + let manifest = |path| ProjectManifest::from_manifest_file(root.join(path)).unwrap(); + let candidates = [ + (manifest("z/Cargo.toml"), cfg(&["z"])), + (manifest("a/Cargo.toml"), cfg(&["a", "b"])), + (manifest("b/Cargo.toml"), cfg(&["a", "b"])), + ]; + for order in [[0, 1, 2], [2, 1, 0]] { + let mut plan = DependencyFiles::default(); + for (manifest, cfg) in order.map(|index| &candidates[index]) { + plan.add_candidate(Path::new("lib.rs"), Some(cfg.clone()), manifest); + } + let expected = HashMap::from([(&candidates[2].0, vec![PathBuf::from("lib.rs")])]); + assert_eq!(plan.by_manifests(), expected); + } + } +} diff --git a/rust/extractor/src/diagnostics.rs b/rust/extractor/src/diagnostics.rs index 0db3358d558a..ab88192c5d3e 100644 --- a/rust/extractor/src/diagnostics.rs +++ b/rust/extractor/src/diagnostics.rs @@ -5,7 +5,7 @@ use chrono::{DateTime, Utc}; use ra_ap_project_model::ProjectManifest; use serde::Serialize; use serde::ser::SerializeMap; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt::Display; use std::fs::File; use std::path::{Path, PathBuf}; @@ -220,13 +220,15 @@ struct ExtractionSummary { type ExtractionDiagnostics = Diagnostics; fn summary(start: Instant, steps: &[ExtractionStep]) -> ExtractionSummary { - let mut number_of_manifests = 0; + // The manifests loaded during extraction. We might load the same manifest + // multiple times, so we use a set to keep track of unique manifests. + let mut manifests = HashSet::new(); let mut number_of_files = 0; let mut durations = HashMap::new(); for step in steps { match &step.action { ExtractionStepKind::LoadManifest => { - number_of_manifests += 1; + manifests.extend(step.file.as_ref()); } ExtractionStepKind::Parse => { number_of_files += 1; @@ -244,7 +246,7 @@ fn summary(start: Instant, steps: &[ExtractionStep]) -> ExtractionSummary { } info!("total duration: {total}"); ExtractionSummary { - number_of_manifests, + number_of_manifests: manifests.len(), number_of_files, durations: DurationsSummary { durations, total }, } @@ -284,3 +286,22 @@ pub fn emit_extraction_diagnostics( .with_context(|| format!("writing to diagnostics file {}", target.display()))?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn counts_unique_manifests_but_includes_reload_duration() { + let steps: Vec<_> = ["a/Cargo.toml", "b/Cargo.toml", "a/Cargo.toml"] + .into_iter() + .map(|path| ExtractionStep { + action: ExtractionStepKind::LoadManifest, + file: Some(PathBuf::from(path)), + ms: 1, + }) + .collect(); + let summary = summary(Instant::now(), &steps); + assert_eq!(summary.number_of_manifests, 2); + } +} diff --git a/rust/extractor/src/main.rs b/rust/extractor/src/main.rs index 72b6f20c9fb0..ac7b74656588 100644 --- a/rust/extractor/src/main.rs +++ b/rust/extractor/src/main.rs @@ -5,7 +5,7 @@ use crate::translate::SourceKind; use crate::trap::TrapId; use anyhow::Context; use archive::Archiver; -use ra_ap_base_db::SourceDatabase; +use configuration::{DependencyFiles, dependency_files, manifest_parent}; use ra_ap_hir::Semantics; use ra_ap_ide_db::RootDatabase; use ra_ap_ide_db::line_index::{LineCol, LineIndex}; @@ -14,13 +14,9 @@ use ra_ap_paths::{AbsPathBuf, Utf8PathBuf}; use ra_ap_project_model::{CargoConfig, ProjectManifest}; use ra_ap_vfs::Vfs; use rust_analyzer::{ParseResult, RustAnalyzer}; -use std::collections::HashSet; -use std::hash::RandomState; +use std::collections::{BTreeMap, HashSet}; +use std::path::{Path, PathBuf}; use std::time::Instant; -use std::{ - collections::HashMap, - path::{Path, PathBuf}, -}; use std::{env, fs}; use tracing::{error, info, warn}; use tracing_subscriber::layer::SubscriberExt; @@ -28,6 +24,7 @@ use tracing_subscriber::util::SubscriberInitExt; mod archive; mod config; +mod configuration; mod crate_graph; mod diagnostics; pub mod generated; @@ -36,6 +33,7 @@ mod rust_analyzer; mod toolchain; mod translate; pub mod trap; +mod util; struct Extractor<'a> { archiver: &'a Archiver, @@ -200,6 +198,34 @@ impl<'a> Extractor<'a> { self.steps.push(ExtractionStep::find_manifests(before)); ret } + + fn group_files<'b>( + &mut self, + manifests: &'b [ProjectManifest], + files: &'b [PathBuf], + ) -> BTreeMap<&'b Path, (&'b ProjectManifest, Vec<&'b Path>)> { + let mut workspaces: BTreeMap<&Path, (&ProjectManifest, Vec<&Path>)> = manifests + .iter() + .map(|manifest| (manifest_parent(manifest), (manifest, Vec::new()))) + .collect(); + + 'outer: for file in files { + for ancestor in file.as_path().ancestors() { + if let Some((_, files)) = workspaces.get_mut(ancestor) { + files.push(file); + continue 'outer; + } + } + self.extract_without_semantics( + file, + SourceKind::Source, + RustAnalyzerNoSemantics::warning("no manifest found"), + ); + } + + workspaces.retain(|_, (_, files)| !files.is_empty()); + workspaces + } } fn cwd() -> anyhow::Result { @@ -212,12 +238,12 @@ fn cwd() -> anyhow::Result { } fn main() -> anyhow::Result<()> { - let mut cfg = config::Config::extract().context("failed to load configuration")?; - if cfg.qltest { - qltest::prepare(&mut cfg)?; + let mut config = config::Config::extract().context("failed to load configuration")?; + if config.qltest { + qltest::prepare(&mut config)?; } let start = Instant::now(); - let (flame_layer, _flush_guard) = if let Some(path) = &cfg.logging_flamegraph { + let (flame_layer, _flush_guard) = if let Some(path) = &config.logging_flamegraph { tracing_flame::FlameLayer::with_file(path) .ok() .map(|(a, b)| (Some(a), Some(b))) @@ -229,18 +255,18 @@ fn main() -> anyhow::Result<()> { tracing_subscriber::registry() .with(codeql_extractor::extractor::default_subscriber_with_level( "single_arch", - &cfg.logging_verbosity, + &config.logging_verbosity, )) .with(flame_layer) .init(); - info!("{cfg:#?}\n"); + info!("{config:#?}\n"); - let traps = trap::TrapFileProvider::new(&cfg).context("failed to set up trap files")?; + let traps = trap::TrapFileProvider::new(&config).context("failed to set up trap files")?; let archiver = archive::Archiver { - root: cfg.source_archive_dir.clone(), + root: config.source_archive_dir.clone(), }; let mut extractor = Extractor::new(&archiver, &traps); - let files: Vec = cfg + let files = config .inputs .iter() .map(|file| { @@ -250,69 +276,53 @@ fn main() -> anyhow::Result<()> { // [1]: https://github.com/rust-lang/rust-analyzer/issues/18894#issuecomment-2580014730 dunce::canonicalize(&file).unwrap_or(file) }) - .collect(); + .collect::>(); let manifests = extractor.find_manifests(&files)?; - let mut map: HashMap<&Path, (&ProjectManifest, Vec<&Path>)> = manifests - .iter() - .map(|x| (x.manifest_path().parent().as_ref(), (x, Vec::new()))) - .collect(); - - 'outer: for file in &files { - for ancestor in file.as_path().ancestors() { - if let Some((_, files)) = map.get_mut(ancestor) { - files.push(file); - continue 'outer; - } - } - extractor.extract_without_semantics( - file, - SourceKind::Source, - RustAnalyzerNoSemantics::warning("no manifest found"), - ); - } + let workspaces = extractor.group_files(&manifests, &files); let cwd = cwd()?; log_project_toolchain(); - let (cargo_config, load_cargo_config) = cfg.to_cargo_config(&cwd); - let library_mode = if cfg.extract_dependencies_as_source { + let (cargo_config, load_cargo_config) = config.to_cargo_config(&cwd); + let library_mode = if config.extract_dependencies_as_source { SourceKind::Source } else { SourceKind::Library }; - let source_mode = if cfg.force_library_mode { + let source_mode = if config.force_library_mode { library_mode } else { SourceKind::Source }; - let mut processed_files: HashSet = - HashSet::from_iter(files.iter().cloned()); - for (manifest, files) in map.values().filter(|(_, files)| !files.is_empty()) { - if let Some((ref db, ref vfs)) = + let source_files = HashSet::from_iter(files.iter().cloned()); + + let mut dependencies = DependencyFiles::default(); + + // When there more than one workspace, it's not a priori clear in which + // workspace a dependency file should be extracted. + let dependency_extraction_ambiguity = workspaces.len() == 1; + + for (manifest, files) in workspaces.values() { + if let Some((db, vfs)) = extractor.load_manifest(manifest, &cargo_config, &load_cargo_config) { let before_crate_graph = Instant::now(); - crate_graph::extract_crate_graph(extractor.traps, db, vfs); + crate_graph::extract_crate_graph(extractor.traps, &db, &vfs); extractor .steps .push(ExtractionStep::crate_graph(before_crate_graph)); - let semantics = Semantics::new(db); + let semantics = Semantics::new(&db); for file in files { - match extractor.load_source(file, &semantics, vfs) { - Ok(()) => extractor.extract_with_semantics(file, &semantics, vfs, source_mode), + match extractor.load_source(file, &semantics, &vfs) { + Ok(()) => extractor.extract_with_semantics(file, &semantics, &vfs, source_mode), Err(e) => extractor.extract_without_semantics(file, source_mode, e), }; } - for (file_id, file) in vfs.iter() { - if let Some(file) = file.as_path().map(<_ as AsRef>::as_ref) - && file.extension().is_some_and(|ext| ext == "rs") - && processed_files.insert(file.to_owned()) - && db - .source_root(db.file_source_root(file_id).source_root_id(db)) - .source_root(db) - .is_library - { - extractor.extract_with_semantics(file, &semantics, vfs, library_mode); - extractor.archiver.archive(file); + // Fast path when there is no ambiguity in dependency extraction. + if dependency_extraction_ambiguity { + for (_, file) in dependency_files(&db, &vfs, &source_files) { + extractor.extract_with_semantics(file, &semantics, &vfs, library_mode); } + } else { + dependencies.add_manifest(manifest, &semantics, &vfs, &source_files); } } else { for file in files { @@ -324,6 +334,49 @@ fn main() -> anyhow::Result<()> { } } } + + // For each manifest, extract the dependency files for which it is + // preferred. Note that when `dependency_extraction_ambiguity` is true then + // `selected` below is empty. + loop { + let selected = dependencies.by_manifests(); + if selected.is_empty() { + break; + } + for (manifest, files) in selected { + if let Some((db, vfs)) = + extractor.load_manifest(manifest, &cargo_config, &load_cargo_config) + { + let semantics = Semantics::new(&db); + for file in files { + // A richer configuration can disable a file when using cfg + // options like `#[cfg(not(feature = "foo"))]`. When this is + // the case `file_to_module_def` will return `None` for the + // file. When this is the case and there are alternatives we + // do not extract the file yet. Instead the file will be + // considered again in the next iteration of the loop. + if !dependencies.has_alternative(&file) + || path_to_file_id(&file, &vfs) + .and_then(|id| semantics.file_to_module_def(id)) + .is_some() + { + extractor.extract_with_semantics(&file, &semantics, &vfs, library_mode); + dependencies.extracted(&file); + } + } + } else { + for file in files { + extractor.extract_without_semantics( + &file, + library_mode, + RustAnalyzerNoSemantics::warning("unable to reload manifest"), + ); + dependencies.extracted(&file); + } + } + } + } + let builtins_dir = env::var("CODEQL_EXTRACTOR_RUST_ROOT") .map(|path| Path::new(&path).join("tools").join("builtins"))?; let builtins = fs::read_dir(builtins_dir).context("failed to read builtins directory")?; @@ -335,5 +388,5 @@ fn main() -> anyhow::Result<()> { } } - extractor.emit_extraction_diagnostics(start, &cfg) + extractor.emit_extraction_diagnostics(start, &config) } diff --git a/rust/extractor/src/rust_analyzer.rs b/rust/extractor/src/rust_analyzer.rs index 25d88ec5ce6b..d3129af12796 100644 --- a/rust/extractor/src/rust_analyzer.rs +++ b/rust/extractor/src/rust_analyzer.rs @@ -217,6 +217,12 @@ fn workspace_members_match(workspace_dir: &AbsPath, members: &[String], target: }) } +/// Finds an enclosing Cargo workspace that includes `manifest`. That is, its +/// `members` patterns include the manifest's directory and its `exclude` +/// patterns do not. +/// +/// Returns `None` if the manifest itself defines a workspace, is not a Cargo +/// manifest, cannot be loaded, or has no matching workspace. fn find_workspace(reader: &mut TomlReader, manifest: &ProjectManifest) -> Option { let ProjectManifest::CargoToml(cargo) = manifest else { return None; @@ -224,12 +230,13 @@ fn find_workspace(reader: &mut TomlReader, manifest: &ProjectManifest) -> Option let parsed_cargo = reader.read(cargo).ok()?; if parsed_cargo.workspace.is_some() { debug!("{cargo} is a workspace"); - return Some(manifest.clone()); + return None; } let Some(parent_dir) = cargo.parent().parent() else { warn!("no parent dir for {cargo}"); return None; }; + // Search upward from `parent_dir` for manifest files. let discovered = ProjectManifest::discover(parent_dir) .map_err(|e| { error!( @@ -240,25 +247,22 @@ fn find_workspace(reader: &mut TomlReader, manifest: &ProjectManifest) -> Option }) .ok()?; discovered - .iter() - .find_map(|it| match it { - ProjectManifest::CargoToml(other) - if cargo.starts_with(other.parent()) - && reader.read(other).is_ok_and(|it| { - it.workspace.as_ref().is_some_and(|w| { - workspace_members_match(other.parent(), &w.members, cargo.parent()) - && !workspace_members_match( - other.parent(), - &w.exclude, - cargo.parent(), - ) - }) - }) => + .into_iter() + .find(|it| { + if let ProjectManifest::CargoToml(other) = it + && cargo.starts_with(other.parent()) + && reader.read(other).is_ok_and(|it| { + it.workspace.as_ref().is_some_and(|w| { + workspace_members_match(other.parent(), &w.members, cargo.parent()) + && !workspace_members_match(other.parent(), &w.exclude, cargo.parent()) + }) + }) { debug!("found workspace {other} containing {cargo}"); - Some(it.clone()) + true + } else { + false } - _ => None, }) .or_else(|| { debug!("no workspace found for {cargo}"); diff --git a/rust/extractor/src/util.rs b/rust/extractor/src/util.rs new file mode 100644 index 000000000000..a12ca6de74a1 --- /dev/null +++ b/rust/extractor/src/util.rs @@ -0,0 +1,15 @@ +use std::cmp::Ordering; + +/// Compares options using a custom comparator, with `None` ordered before `Some`. +pub(crate) fn cmp_option_by( + left: &Option, + right: &Option, + compare: impl FnOnce(&T, &T) -> Ordering, +) -> Ordering { + match (left, right) { + (Some(left), Some(right)) => compare(left, right), + (Some(_), None) => Ordering::Greater, + (None, Some(_)) => Ordering::Less, + (None, None) => Ordering::Equal, + } +} diff --git a/rust/ql/integration-tests/dependency-features/.cargo/config.toml b/rust/ql/integration-tests/dependency-features/.cargo/config.toml new file mode 100644 index 000000000000..6f5d1edd7231 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/.cargo/config.toml @@ -0,0 +1,7 @@ +# Add a source definition backed by the directories in "dependencies" +[source.test-dependencies] +directory = "dependencies" + +# Map crates.io to our local source definition +[source.crates-io] +replace-with = "test-dependencies" diff --git a/rust/ql/integration-tests/dependency-features/dependencies/alpha/.cargo-checksum.json b/rust/ql/integration-tests/dependency-features/dependencies/alpha/.cargo-checksum.json new file mode 100644 index 000000000000..09bf61c6fa41 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/alpha/.cargo-checksum.json @@ -0,0 +1 @@ +{"files": {}, "package": null} diff --git a/rust/ql/integration-tests/dependency-features/dependencies/alpha/Cargo.toml b/rust/ql/integration-tests/dependency-features/dependencies/alpha/Cargo.toml new file mode 100644 index 000000000000..151c6e5949f1 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/alpha/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "alpha" +version = "0.1.0" +edition = "2024" + +[features] +a = [] +b = [] diff --git a/rust/ql/integration-tests/dependency-features/dependencies/alpha/src/lib.rs b/rust/ql/integration-tests/dependency-features/dependencies/alpha/src/lib.rs new file mode 100644 index 000000000000..70d16610593c --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/alpha/src/lib.rs @@ -0,0 +1,27 @@ +#[cfg(all(feature = "a", feature = "b"))] +pub fn a() {} + +#[cfg(all(feature = "a", not(feature = "b")))] +mod only_a; + +#[cfg(all(feature = "a", not(feature = "b")))] +pub use only_a::a; + +#[cfg(feature = "b")] +pub fn b() {} + +#[cfg(feature = "b")] +#[macro_export] +macro_rules! invoke { + () => { + $crate::b() + }; +} + +#[cfg(not(feature = "b"))] +#[macro_export] +macro_rules! invoke { + () => { + $crate::a() + }; +} diff --git a/rust/ql/integration-tests/dependency-features/dependencies/alpha/src/only_a.rs b/rust/ql/integration-tests/dependency-features/dependencies/alpha/src/only_a.rs new file mode 100644 index 000000000000..6dcbbb1ccfe8 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/alpha/src/only_a.rs @@ -0,0 +1,7 @@ +macro_rules! define_a { + () => { + pub fn a() {} + }; +} + +define_a!(); diff --git a/rust/ql/integration-tests/dependency-features/dependencies/beta/.cargo-checksum.json b/rust/ql/integration-tests/dependency-features/dependencies/beta/.cargo-checksum.json new file mode 100644 index 000000000000..09bf61c6fa41 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/beta/.cargo-checksum.json @@ -0,0 +1 @@ +{"files": {}, "package": null} diff --git a/rust/ql/integration-tests/dependency-features/dependencies/beta/Cargo.toml b/rust/ql/integration-tests/dependency-features/dependencies/beta/Cargo.toml new file mode 100644 index 000000000000..b9c6e4f70445 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/beta/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "beta" +version = "0.1.0" +edition = "2024" + +[features] +a = [] +b = [] diff --git a/rust/ql/integration-tests/dependency-features/dependencies/beta/src/lib.rs b/rust/ql/integration-tests/dependency-features/dependencies/beta/src/lib.rs new file mode 100644 index 000000000000..c2aafb3b633e --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/dependencies/beta/src/lib.rs @@ -0,0 +1,21 @@ +#[cfg(feature = "a")] +pub fn a() {} + +#[cfg(feature = "b")] +pub fn b() {} + +#[cfg(feature = "b")] +#[macro_export] +macro_rules! invoke { + () => { + $crate::b() + }; +} + +#[cfg(not(feature = "b"))] +#[macro_export] +macro_rules! invoke { + () => { + $crate::a() + }; +} diff --git a/rust/ql/integration-tests/dependency-features/functions.ql b/rust/ql/integration-tests/dependency-features/functions.ql new file mode 100644 index 000000000000..a475c9258072 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/functions.ql @@ -0,0 +1,13 @@ +import rust + +from Call c, string target +where + c.getFile().getRelativePath() in [ + "first/src/lib.rs", "second/src/lib.rs", "src/lib.rs", "build.rs" + ] and + ( + target = c.getStaticTarget().getCanonicalPath() + or + not exists(c.getStaticTarget()) and target = "" + ) +select c, target diff --git a/rust/ql/integration-tests/dependency-features/functions.rich.expected b/rust/ql/integration-tests/dependency-features/functions.rich.expected new file mode 100644 index 000000000000..796c4e1aae87 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/functions.rich.expected @@ -0,0 +1,4 @@ +| source/first/src/lib.rs:2:5:2:18 | ...::b(...) | alpha::b | +| source/first/src/lib.rs:3:5:3:17 | ...::a(...) | beta::a | +| source/second/src/lib.rs:2:5:2:18 | ...::a(...) | alpha::a | +| source/second/src/lib.rs:3:5:3:17 | ...::b(...) | beta::b | diff --git a/rust/ql/integration-tests/dependency-features/functions.rich.reversed.expected b/rust/ql/integration-tests/dependency-features/functions.rich.reversed.expected new file mode 100644 index 000000000000..f352f77ba179 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/functions.rich.reversed.expected @@ -0,0 +1,4 @@ +| source/first/src/lib.rs:2:5:2:18 | ...::a(...) | alpha::a | +| source/first/src/lib.rs:3:5:3:17 | ...::b(...) | beta::b | +| source/second/src/lib.rs:2:5:2:18 | ...::b(...) | alpha::b | +| source/second/src/lib.rs:3:5:3:17 | ...::a(...) | beta::a | diff --git a/rust/ql/integration-tests/dependency-features/functions.tie.expected b/rust/ql/integration-tests/dependency-features/functions.tie.expected new file mode 100644 index 000000000000..31cbfba529fa --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/functions.tie.expected @@ -0,0 +1,4 @@ +| source/first/src/lib.rs:2:5:2:18 | ...::a(...) | | +| source/first/src/lib.rs:3:5:3:17 | ...::b(...) | beta::b | +| source/second/src/lib.rs:2:5:2:18 | ...::b(...) | alpha::b | +| source/second/src/lib.rs:3:5:3:17 | ...::a(...) | | diff --git a/rust/ql/integration-tests/dependency-features/functions.tie.reversed.expected b/rust/ql/integration-tests/dependency-features/functions.tie.reversed.expected new file mode 100644 index 000000000000..a4475eb87c83 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/functions.tie.reversed.expected @@ -0,0 +1,4 @@ +| source/first/src/lib.rs:2:5:2:18 | ...::b(...) | alpha::b | +| source/first/src/lib.rs:3:5:3:17 | ...::a(...) | | +| source/second/src/lib.rs:2:5:2:18 | ...::a(...) | | +| source/second/src/lib.rs:3:5:3:17 | ...::b(...) | beta::b | diff --git a/rust/ql/integration-tests/dependency-features/inactive-module.ql b/rust/ql/integration-tests/dependency-features/inactive-module.ql new file mode 100644 index 000000000000..9096a1ac43c6 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/inactive-module.ql @@ -0,0 +1,5 @@ +import rust + +from Function f +where f.getFile().getBaseName() = "only_a.rs" +select f, f.getName() diff --git a/rust/ql/integration-tests/dependency-features/inactive-module.rich.expected b/rust/ql/integration-tests/dependency-features/inactive-module.rich.expected new file mode 100644 index 000000000000..377f38232a12 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/inactive-module.rich.expected @@ -0,0 +1 @@ +| dependencies/alpha/src/only_a.rs:7:1:7:9 | fn a | dependencies/alpha/src/only_a.rs:7:1:7:9 | a | diff --git a/rust/ql/integration-tests/dependency-features/inactive-module.rich.reversed.expected b/rust/ql/integration-tests/dependency-features/inactive-module.rich.reversed.expected new file mode 100644 index 000000000000..377f38232a12 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/inactive-module.rich.reversed.expected @@ -0,0 +1 @@ +| dependencies/alpha/src/only_a.rs:7:1:7:9 | fn a | dependencies/alpha/src/only_a.rs:7:1:7:9 | a | diff --git a/rust/ql/integration-tests/dependency-features/inactive-module.tie.expected b/rust/ql/integration-tests/dependency-features/inactive-module.tie.expected new file mode 100644 index 000000000000..377f38232a12 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/inactive-module.tie.expected @@ -0,0 +1 @@ +| dependencies/alpha/src/only_a.rs:7:1:7:9 | fn a | dependencies/alpha/src/only_a.rs:7:1:7:9 | a | diff --git a/rust/ql/integration-tests/dependency-features/inactive-module.tie.reversed.expected b/rust/ql/integration-tests/dependency-features/inactive-module.tie.reversed.expected new file mode 100644 index 000000000000..377f38232a12 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/inactive-module.tie.reversed.expected @@ -0,0 +1 @@ +| dependencies/alpha/src/only_a.rs:7:1:7:9 | fn a | dependencies/alpha/src/only_a.rs:7:1:7:9 | a | diff --git a/rust/ql/integration-tests/dependency-features/source/first/src/lib.rs b/rust/ql/integration-tests/dependency-features/source/first/src/lib.rs new file mode 100644 index 000000000000..a4a49f20b4ce --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/source/first/src/lib.rs @@ -0,0 +1,4 @@ +pub fn emit() { + alpha::invoke!(); + beta::invoke!(); +} diff --git a/rust/ql/integration-tests/dependency-features/source/second/src/lib.rs b/rust/ql/integration-tests/dependency-features/source/second/src/lib.rs new file mode 100644 index 000000000000..a4a49f20b4ce --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/source/second/src/lib.rs @@ -0,0 +1,4 @@ +pub fn emit() { + alpha::invoke!(); + beta::invoke!(); +} diff --git a/rust/ql/integration-tests/dependency-features/test_dependency_features.py b/rust/ql/integration-tests/dependency-features/test_dependency_features.py new file mode 100644 index 000000000000..a7da594e6908 --- /dev/null +++ b/rust/ql/integration-tests/dependency-features/test_dependency_features.py @@ -0,0 +1,82 @@ +import json +from textwrap import dedent + +import commands +import pytest + + +@pytest.mark.parametrize( + ("configurations", "reverse"), + [ + pytest.param( + configurations, + reverse, + marks=pytest.mark.ql_test( + expected=f".{expected}{'.reversed' if reverse else ''}.expected" + ), + id=f"{name}{'-reversed' if reverse else ''}", + ) + for name, expected, configurations in [ + ("more-features", "rich", [(["a", "b"], ["a"]), (["a"], ["a", "b"])]), + ("equal-features", "tie", [(["a"], ["b"]), (["b"], ["a"])]), + ] + for reverse in [False, True] + ], +) +def test_dependency_features(codeql, rust, cwd, configurations, reverse): + """ + Two independent workspaces, `source/first` and `source/second`, share the + local dependencies `alpha` and `beta`, but enable different Cargo features + (their `Cargo.toml` are created in this test). + + Below is an overview of the files in the test setup. + ``` + dependency-features/ + |-- dependencies/ + | |-- alpha/ + | | |-- Cargo.toml + | | `-- src/lib.rs # feature-gated functions and invoke! macro + | `-- beta/ + | |-- Cargo.toml + | `-- src/lib.rs # feature-gated functions and invoke! macro + |-- source/ + | |-- first/ + | | |-- Cargo.toml # generated by this test for each configuration + | | `-- src/lib.rs # calls alpha::invoke! and beta::invoke! + | `-- second/ + | |-- Cargo.toml # generated by this test for each configuration + | `-- src/lib.rs # calls alpha::invoke! and beta::invoke! + ``` + + Each `src/lib.rs` file calls a macro from each dependency. The macros expand + to different calls depending on the enabled features. The `functions.ql` + query checks which calls and targets are extracted for the `src/lib.rs` + files. + + Alpha's `only_a` module is disabled by feature `b`. Its item macro must still + be expanded using a configuration in which the module is enabled, even when + a richer configuration wins for alpha's other files. + """ + if reverse: + configurations = configurations[::-1] + for workspace, features in zip(["first", "second"], configurations): + manifest = cwd / "source" / workspace / "Cargo.toml" + manifest.write_text( + dedent(f"""\ + [package] + name = "{workspace}" + version = "0.1.0" + edition = "2024" + + [workspace] + + [dependencies] + """) + + "\n".join( + f'{name} = {{ version = "=0.1.0", features = {json.dumps(enabled)} }}' + for name, enabled in zip(["alpha", "beta"], features) + ) + + "\n" + ) + commands.run(f'cargo check --quiet --manifest-path "{manifest}"') + codeql.database.create(source_root="source") diff --git a/rust/ql/integration-tests/workspace-with-glob/steps.expected b/rust/ql/integration-tests/workspace-with-glob/steps.expected index 4b0e6ed828b6..3ea9ee61bed3 100644 --- a/rust/ql/integration-tests/workspace-with-glob/steps.expected +++ b/rust/ql/integration-tests/workspace-with-glob/steps.expected @@ -6,6 +6,7 @@ | lib/src/lib.rs:0:0:0:0 | Extract(lib/src/lib.rs) | | lib/src/lib.rs:0:0:0:0 | Parse(lib/src/lib.rs) | | other/Cargo.toml:0:0:0:0 | LoadManifest(other/Cargo.toml) | +| other/Cargo.toml:0:0:0:0 | LoadManifest(other/Cargo.toml) | | other/src/lib.rs:0:0:0:0 | Extract(other/src/lib.rs) | | other/src/lib.rs:0:0:0:0 | LoadSource(other/src/lib.rs) | | other/src/lib.rs:0:0:0:0 | Parse(other/src/lib.rs) |