Skip to content

Commit

Permalink
Gate #[test] expansion under cfg(test).
Browse files Browse the repository at this point in the history
This will mean users opting to not activate `cfg(test)` will lose IDE experience on them, which is quite unfortunate, but this is unavoidable if we want to avoid false positives on e.g. diagnostics. The real fix is to provide IDE experience even for cfg'ed out code, but this is out of scope for this PR.
  • Loading branch information
ChayimFriedman2 committed Sep 29, 2024
1 parent 923cb99 commit 91834ec
Show file tree
Hide file tree
Showing 17 changed files with 78 additions and 45 deletions.
4 changes: 4 additions & 0 deletions src/tools/rust-analyzer/crates/cfg/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ impl CfgOptions {
cfg.fold(&|atom| self.enabled.contains(atom))
}

pub fn check_atom(&self, cfg: &CfgAtom) -> bool {
self.enabled.contains(cfg)
}

pub fn insert_atom(&mut self, key: Symbol) {
self.enabled.insert(CfgAtom::Flag(key));
}
Expand Down
14 changes: 11 additions & 3 deletions src/tools/rust-analyzer/crates/hir-def/src/nameres/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use std::{cmp::Ordering, iter, mem, ops::Not};

use base_db::{CrateId, CrateOrigin, Dependency, LangCrateOrigin};
use cfg::{CfgExpr, CfgOptions};
use cfg::{CfgAtom, CfgExpr, CfgOptions};
use either::Either;
use hir_expand::{
attrs::{Attr, AttrId},
Expand Down Expand Up @@ -1324,13 +1324,21 @@ impl DefCollector<'_> {
};

// Skip #[test]/#[bench] expansion, which would merely result in more memory usage
// due to duplicating functions into macro expansions
// due to duplicating functions into macro expansions, but only if `cfg(test)` is active,
// otherwise they are expanded to nothing and this can impact e.g. diagnostics (due to things
// being cfg'ed out).
// Ideally we will just expand them to nothing here. But we are only collecting macro calls,
// not expanding them, so we have no way to do that.
if matches!(
def.kind,
MacroDefKind::BuiltInAttr(_, expander)
if expander.is_test() || expander.is_bench()
) {
return recollect_without(self);
let test_is_active =
self.cfg_options.check_atom(&CfgAtom::Flag(sym::test.clone()));
if test_is_active {
return recollect_without(self);
}
}

let call_id = || {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use span::{MacroCallId, Span};

use crate::{db::ExpandDatabase, name, tt, ExpandResult, MacroCallKind};

use super::quote;

macro_rules! register_builtin {
($(($name:ident, $variant:ident) => $expand:ident),* ) => {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -52,15 +54,15 @@ impl BuiltinAttrExpander {
}

register_builtin! {
(bench, Bench) => dummy_attr_expand,
(bench, Bench) => dummy_gate_test_expand,
(cfg_accessible, CfgAccessible) => dummy_attr_expand,
(cfg_eval, CfgEval) => dummy_attr_expand,
(derive, Derive) => derive_expand,
// derive const is equivalent to derive for our proposes.
(derive_const, DeriveConst) => derive_expand,
(global_allocator, GlobalAllocator) => dummy_attr_expand,
(test, Test) => dummy_attr_expand,
(test_case, TestCase) => dummy_attr_expand
(test, Test) => dummy_gate_test_expand,
(test_case, TestCase) => dummy_gate_test_expand
}

pub fn find_builtin_attr(ident: &name::Name) -> Option<BuiltinAttrExpander> {
Expand All @@ -76,6 +78,19 @@ fn dummy_attr_expand(
ExpandResult::ok(tt.clone())
}

fn dummy_gate_test_expand(
_db: &dyn ExpandDatabase,
_id: MacroCallId,
tt: &tt::Subtree,
span: Span,
) -> ExpandResult<tt::Subtree> {
let result = quote::quote! { span=>
#[cfg(test)]
#tt
};
ExpandResult::ok(result)
}

/// We generate a very specific expansion here, as we do not actually expand the `#[derive]` attribute
/// itself in name res, but we do want to expand it to something for the IDE layer, so that the input
/// derive attributes can be downmapped, and resolved as proper paths.
Expand Down
5 changes: 1 addition & 4 deletions src/tools/rust-analyzer/crates/load-cargo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ pub struct LoadCargoConfig {
pub load_out_dirs_from_check: bool,
pub with_proc_macro_server: ProcMacroServerChoice,
pub prefill_caches: bool,
pub set_test: bool,
}

#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -100,7 +99,6 @@ pub fn load_workspace(
vfs.file_id(&path)
},
extra_env,
load_config.set_test,
);
let proc_macros = {
let proc_macro_server = match &proc_macro_server {
Expand Down Expand Up @@ -528,12 +526,11 @@ mod tests {
#[test]
fn test_loading_rust_analyzer() {
let path = Path::new(env!("CARGO_MANIFEST_DIR")).parent().unwrap().parent().unwrap();
let cargo_config = CargoConfig::default();
let cargo_config = CargoConfig { set_test: true, ..CargoConfig::default() };
let load_cargo_config = LoadCargoConfig {
load_out_dirs_from_check: false,
with_proc_macro_server: ProcMacroServerChoice::None,
prefill_caches: false,
set_test: true,
};
let (db, _vfs, _proc_macro) =
load_workspace_at(path, &cargo_config, &load_cargo_config, &|_| {}).unwrap();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ pub struct CargoConfig {
pub invocation_strategy: InvocationStrategy,
/// Optional path to use instead of `target` when building
pub target_dir: Option<Utf8PathBuf>,
pub set_test: bool,
}

pub type Package = Idx<PackageData>;
Expand Down
4 changes: 2 additions & 2 deletions src/tools/rust-analyzer/crates/project-model/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ fn load_cargo_with_overrides(
rustc: Err(None),
cargo_config_extra_env: Default::default(),
error: None,
set_test: true,
},
cfg_overrides,
sysroot: Sysroot::empty(),
Expand Down Expand Up @@ -136,7 +137,6 @@ fn to_crate_graph(project_workspace: ProjectWorkspace) -> (CrateGraph, ProcMacro
}
},
&Default::default(),
true,
)
}

Expand Down Expand Up @@ -243,6 +243,7 @@ fn smoke_test_real_sysroot_cargo() {
rustc: Err(None),
cargo_config_extra_env: Default::default(),
error: None,
set_test: true,
},
sysroot,
rustc_cfg: Vec::new(),
Expand All @@ -258,6 +259,5 @@ fn smoke_test_real_sysroot_cargo() {
}
},
&Default::default(),
true,
);
}
35 changes: 23 additions & 12 deletions src/tools/rust-analyzer/crates/project-model/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ pub enum ProjectWorkspaceKind {
rustc: Result<Box<(CargoWorkspace, WorkspaceBuildScripts)>, Option<String>>,
/// Environment variables set in the `.cargo/config` file.
cargo_config_extra_env: FxHashMap<String, String>,
set_test: bool,
},
/// Project workspace was specified using a `rust-project.json` file.
Json(ProjectJson),
Expand All @@ -98,6 +99,7 @@ pub enum ProjectWorkspaceKind {
cargo: Option<(CargoWorkspace, WorkspaceBuildScripts, Option<Arc<anyhow::Error>>)>,
/// Environment variables set in the `.cargo/config` file.
cargo_config_extra_env: FxHashMap<String, String>,
set_test: bool,
},
}

Expand All @@ -112,6 +114,7 @@ impl fmt::Debug for ProjectWorkspace {
build_scripts,
rustc,
cargo_config_extra_env,
set_test,
} => f
.debug_struct("Cargo")
.field("root", &cargo.workspace_root().file_name())
Expand All @@ -126,6 +129,7 @@ impl fmt::Debug for ProjectWorkspace {
.field("toolchain", &toolchain)
.field("data_layout", &target_layout)
.field("cargo_config_extra_env", &cargo_config_extra_env)
.field("set_test", set_test)
.field("build_scripts", &build_scripts.error().unwrap_or("ok"))
.finish(),
ProjectWorkspaceKind::Json(project) => {
Expand All @@ -137,12 +141,14 @@ impl fmt::Debug for ProjectWorkspace {
.field("toolchain", &toolchain)
.field("data_layout", &target_layout)
.field("n_cfg_overrides", &cfg_overrides.len());

debug_struct.finish()
}
ProjectWorkspaceKind::DetachedFile {
file,
cargo: cargo_script,
cargo_config_extra_env,
set_test,
} => f
.debug_struct("DetachedFiles")
.field("file", &file)
Expand All @@ -154,6 +160,7 @@ impl fmt::Debug for ProjectWorkspace {
.field("data_layout", &target_layout)
.field("n_cfg_overrides", &cfg_overrides.len())
.field("cargo_config_extra_env", &cargo_config_extra_env)
.field("set_test", set_test)
.finish(),
}
}
Expand Down Expand Up @@ -329,6 +336,7 @@ impl ProjectWorkspace {
rustc,
cargo_config_extra_env,
error: error.map(Arc::new),
set_test: config.set_test,
},
sysroot,
rustc_cfg,
Expand Down Expand Up @@ -423,6 +431,7 @@ impl ProjectWorkspace {
file: detached_file.to_owned(),
cargo: cargo_script,
cargo_config_extra_env,
set_test: config.set_test,
},
sysroot,
rustc_cfg,
Expand Down Expand Up @@ -609,6 +618,7 @@ impl ProjectWorkspace {
build_scripts,
cargo_config_extra_env: _,
error: _,
set_test: _,
} => {
cargo
.packages()
Expand Down Expand Up @@ -728,7 +738,6 @@ impl ProjectWorkspace {
&self,
load: FileLoader<'_>,
extra_env: &FxHashMap<String, String>,
set_test: bool,
) -> (CrateGraph, ProcMacroPaths) {
let _p = tracing::info_span!("ProjectWorkspace::to_crate_graph").entered();

Expand All @@ -742,7 +751,6 @@ impl ProjectWorkspace {
sysroot,
extra_env,
cfg_overrides,
set_test,
),
sysroot,
),
Expand All @@ -752,6 +760,7 @@ impl ProjectWorkspace {
build_scripts,
cargo_config_extra_env: _,
error: _,
set_test,
} => (
cargo_to_crate_graph(
load,
Expand All @@ -761,11 +770,11 @@ impl ProjectWorkspace {
rustc_cfg.clone(),
cfg_overrides,
build_scripts,
set_test,
*set_test,
),
sysroot,
),
ProjectWorkspaceKind::DetachedFile { file, cargo: cargo_script, .. } => (
ProjectWorkspaceKind::DetachedFile { file, cargo: cargo_script, set_test, .. } => (
if let Some((cargo, build_scripts, _)) = cargo_script {
cargo_to_crate_graph(
&mut |path| load(path),
Expand All @@ -775,7 +784,7 @@ impl ProjectWorkspace {
rustc_cfg.clone(),
cfg_overrides,
build_scripts,
set_test,
*set_test,
)
} else {
detached_file_to_crate_graph(
Expand All @@ -784,7 +793,7 @@ impl ProjectWorkspace {
file,
sysroot,
cfg_overrides,
set_test,
*set_test,
)
},
sysroot,
Expand Down Expand Up @@ -818,13 +827,15 @@ impl ProjectWorkspace {
cargo_config_extra_env,
build_scripts: _,
error: _,
set_test: _,
},
ProjectWorkspaceKind::Cargo {
cargo: o_cargo,
rustc: o_rustc,
cargo_config_extra_env: o_cargo_config_extra_env,
build_scripts: _,
error: _,
set_test: _,
},
) => {
cargo == o_cargo
Expand All @@ -839,11 +850,13 @@ impl ProjectWorkspace {
file,
cargo: Some((cargo_script, _, _)),
cargo_config_extra_env,
set_test: _,
},
ProjectWorkspaceKind::DetachedFile {
file: o_file,
cargo: Some((o_cargo_script, _, _)),
cargo_config_extra_env: o_cargo_config_extra_env,
set_test: _,
},
) => {
file == o_file
Expand Down Expand Up @@ -875,12 +888,11 @@ fn project_json_to_crate_graph(
sysroot: &Sysroot,
extra_env: &FxHashMap<String, String>,
override_cfg: &CfgOverrides,
set_test: bool,
) -> (CrateGraph, ProcMacroPaths) {
let mut res = (CrateGraph::default(), ProcMacroPaths::default());
let (crate_graph, proc_macros) = &mut res;
let (public_deps, libproc_macro) =
sysroot_to_crate_graph(crate_graph, sysroot, rustc_cfg.clone(), load, set_test);
sysroot_to_crate_graph(crate_graph, sysroot, rustc_cfg.clone(), load);

let r_a_cfg_flag = CfgAtom::Flag(sym::rust_analyzer.clone());
let mut cfg_cache: FxHashMap<&str, Vec<CfgAtom>> = FxHashMap::default();
Expand Down Expand Up @@ -1000,7 +1012,7 @@ fn cargo_to_crate_graph(
let crate_graph = &mut res.0;
let proc_macros = &mut res.1;
let (public_deps, libproc_macro) =
sysroot_to_crate_graph(crate_graph, sysroot, rustc_cfg.clone(), load, set_test);
sysroot_to_crate_graph(crate_graph, sysroot, rustc_cfg.clone(), load);

let cfg_options = CfgOptions::from_iter(rustc_cfg);

Expand Down Expand Up @@ -1187,7 +1199,7 @@ fn detached_file_to_crate_graph(
let _p = tracing::info_span!("detached_file_to_crate_graph").entered();
let mut crate_graph = CrateGraph::default();
let (public_deps, _libproc_macro) =
sysroot_to_crate_graph(&mut crate_graph, sysroot, rustc_cfg.clone(), load, set_test);
sysroot_to_crate_graph(&mut crate_graph, sysroot, rustc_cfg.clone(), load);

let mut cfg_options = CfgOptions::from_iter(rustc_cfg);
if set_test {
Expand Down Expand Up @@ -1416,7 +1428,6 @@ fn sysroot_to_crate_graph(
sysroot: &Sysroot,
rustc_cfg: Vec<CfgAtom>,
load: FileLoader<'_>,
set_test: bool,
) -> (SysrootPublicDeps, Option<CrateId>) {
let _p = tracing::info_span!("sysroot_to_crate_graph").entered();
match sysroot.mode() {
Expand All @@ -1439,7 +1450,7 @@ fn sysroot_to_crate_graph(
..Default::default()
},
&WorkspaceBuildScripts::default(),
set_test,
false,
);

let mut pub_deps = vec![];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ impl flags::AnalysisStats {
false => Some(RustLibSource::Discover),
},
all_targets: true,
set_test: true,
..Default::default()
};
let no_progress = &|_| ();
Expand All @@ -84,7 +85,6 @@ impl flags::AnalysisStats {
ProcMacroServerChoice::Sysroot
},
prefill_caches: false,
set_test: true,
};

let build_scripts_time = if self.disable_build_scripts {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ impl flags::Diagnostics {
load_out_dirs_from_check: !self.disable_build_scripts,
with_proc_macro_server,
prefill_caches: false,
// We don't pass `--all-targets` so we also set `cfg(test)` to false.
set_test: false,
};
let (db, _vfs, _proc_macro) =
load_workspace_at(&self.path, &cargo_config, &load_cargo_config, &|_| {})?;
Expand Down
Loading

0 comments on commit 91834ec

Please sign in to comment.