From 2bc4c1ff31b6ef0ca1848a7ce12f981b93dcded0 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 21 Oct 2020 13:57:12 +0200 Subject: Simplify cfg representation --- crates/cfg/src/cfg_expr.rs | 44 ++++++++++++++++++--------- crates/cfg/src/lib.rs | 22 +++++--------- crates/rust-analyzer/src/cargo_target_spec.rs | 6 ++-- 3 files changed, 41 insertions(+), 31 deletions(-) diff --git a/crates/cfg/src/cfg_expr.rs b/crates/cfg/src/cfg_expr.rs index 336fe25bc..db3655b74 100644 --- a/crates/cfg/src/cfg_expr.rs +++ b/crates/cfg/src/cfg_expr.rs @@ -6,26 +6,42 @@ use std::slice::Iter as SliceIter; use tt::SmolStr; +/// A simple configuration value passed in from the outside. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum CfgAtom { + /// eg. `#[cfg(test)]` + Flag(SmolStr), + /// eg. `#[cfg(target_os = "linux")]` + /// + /// Note that a key can have multiple values that are all considered "active" at the same time. + /// For example, `#[cfg(target_feature = "sse")]` and `#[cfg(target_feature = "sse2")]`. + KeyValue { key: SmolStr, value: SmolStr }, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum CfgExpr { Invalid, - Atom(SmolStr), - KeyValue { key: SmolStr, value: SmolStr }, + Atom(CfgAtom), All(Vec), Any(Vec), Not(Box), } +impl From for CfgExpr { + fn from(atom: CfgAtom) -> Self { + CfgExpr::Atom(atom) + } +} + impl CfgExpr { pub fn parse(tt: &tt::Subtree) -> CfgExpr { next_cfg_expr(&mut tt.token_trees.iter()).unwrap_or(CfgExpr::Invalid) } /// Fold the cfg by querying all basic `Atom` and `KeyValue` predicates. - pub fn fold(&self, query: &dyn Fn(&SmolStr, Option<&SmolStr>) -> bool) -> Option { + pub fn fold(&self, query: &dyn Fn(&CfgAtom) -> bool) -> Option { match self { CfgExpr::Invalid => None, - CfgExpr::Atom(name) => Some(query(name, None)), - CfgExpr::KeyValue { key, value } => Some(query(key, Some(value))), + CfgExpr::Atom(atom) => Some(query(atom)), CfgExpr::All(preds) => { preds.iter().try_fold(true, |s, pred| Some(s && pred.fold(query)?)) } @@ -54,7 +70,7 @@ fn next_cfg_expr(it: &mut SliceIter) -> Option { // FIXME: escape? raw string? let value = SmolStr::new(literal.text.trim_start_matches('"').trim_end_matches('"')); - CfgExpr::KeyValue { key: name, value } + CfgAtom::KeyValue { key: name, value }.into() } _ => return Some(CfgExpr::Invalid), } @@ -70,7 +86,7 @@ fn next_cfg_expr(it: &mut SliceIter) -> Option { _ => CfgExpr::Invalid, } } - _ => CfgExpr::Atom(name), + _ => CfgAtom::Flag(name).into(), }; // Eat comma separator @@ -101,22 +117,22 @@ mod tests { #[test] fn test_cfg_expr_parser() { - assert_parse_result("#![cfg(foo)]", CfgExpr::Atom("foo".into())); - assert_parse_result("#![cfg(foo,)]", CfgExpr::Atom("foo".into())); + assert_parse_result("#![cfg(foo)]", CfgAtom::Flag("foo".into()).into()); + assert_parse_result("#![cfg(foo,)]", CfgAtom::Flag("foo".into()).into()); assert_parse_result( "#![cfg(not(foo))]", - CfgExpr::Not(Box::new(CfgExpr::Atom("foo".into()))), + CfgExpr::Not(Box::new(CfgAtom::Flag("foo".into()).into())), ); assert_parse_result("#![cfg(foo(bar))]", CfgExpr::Invalid); // Only take the first - assert_parse_result(r#"#![cfg(foo, bar = "baz")]"#, CfgExpr::Atom("foo".into())); + assert_parse_result(r#"#![cfg(foo, bar = "baz")]"#, CfgAtom::Flag("foo".into()).into()); assert_parse_result( r#"#![cfg(all(foo, bar = "baz"))]"#, CfgExpr::All(vec![ - CfgExpr::Atom("foo".into()), - CfgExpr::KeyValue { key: "bar".into(), value: "baz".into() }, + CfgAtom::Flag("foo".into()).into(), + CfgAtom::KeyValue { key: "bar".into(), value: "baz".into() }.into(), ]), ); @@ -126,7 +142,7 @@ mod tests { CfgExpr::Not(Box::new(CfgExpr::Invalid)), CfgExpr::All(vec![]), CfgExpr::Invalid, - CfgExpr::KeyValue { key: "bar".into(), value: "baz".into() }, + CfgAtom::KeyValue { key: "bar".into(), value: "baz".into() }.into(), ]), ); } diff --git a/crates/cfg/src/lib.rs b/crates/cfg/src/lib.rs index a9d50e698..35f540ac3 100644 --- a/crates/cfg/src/lib.rs +++ b/crates/cfg/src/lib.rs @@ -5,7 +5,7 @@ mod cfg_expr; use rustc_hash::FxHashSet; use tt::SmolStr; -pub use cfg_expr::CfgExpr; +pub use cfg_expr::{CfgAtom, CfgExpr}; /// Configuration options used for conditional compilition on items with `cfg` attributes. /// We have two kind of options in different namespaces: atomic options like `unix`, and @@ -19,33 +19,25 @@ pub use cfg_expr::CfgExpr; /// See: https://doc.rust-lang.org/reference/conditional-compilation.html#set-configuration-options #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct CfgOptions { - atoms: FxHashSet, - key_values: FxHashSet<(SmolStr, SmolStr)>, + enabled: FxHashSet, } impl CfgOptions { pub fn check(&self, cfg: &CfgExpr) -> Option { - cfg.fold(&|key, value| match value { - None => self.atoms.contains(key), - Some(value) => self.key_values.contains(&(key.clone(), value.clone())), - }) + cfg.fold(&|atom| self.enabled.contains(atom)) } pub fn insert_atom(&mut self, key: SmolStr) { - self.atoms.insert(key); + self.enabled.insert(CfgAtom::Flag(key)); } pub fn insert_key_value(&mut self, key: SmolStr, value: SmolStr) { - self.key_values.insert((key, value)); + self.enabled.insert(CfgAtom::KeyValue { key, value }); } pub fn append(&mut self, other: &CfgOptions) { - for atom in &other.atoms { - self.atoms.insert(atom.clone()); - } - - for (key, value) in &other.key_values { - self.key_values.insert((key.clone(), value.clone())); + for atom in &other.enabled { + self.enabled.insert(atom.clone()); } } } diff --git a/crates/rust-analyzer/src/cargo_target_spec.rs b/crates/rust-analyzer/src/cargo_target_spec.rs index ddc028148..7da935464 100644 --- a/crates/rust-analyzer/src/cargo_target_spec.rs +++ b/crates/rust-analyzer/src/cargo_target_spec.rs @@ -1,6 +1,6 @@ //! See `CargoTargetSpec` -use cfg::CfgExpr; +use cfg::{CfgAtom, CfgExpr}; use ide::{FileId, RunnableKind, TestId}; use project_model::{self, TargetKind}; use vfs::AbsPathBuf; @@ -160,7 +160,9 @@ impl CargoTargetSpec { /// Fill minimal features needed fn required_features(cfg_expr: &CfgExpr, features: &mut Vec) { match cfg_expr { - CfgExpr::KeyValue { key, value } if key == "feature" => features.push(value.to_string()), + CfgExpr::Atom(CfgAtom::KeyValue { key, value }) if key == "feature" => { + features.push(value.to_string()) + } CfgExpr::All(preds) => { preds.iter().for_each(|cfg| required_features(cfg, features)); } -- cgit v1.2.3 From 68b17986c7c272d9be8df9a7abb9b162329b9d65 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 21 Oct 2020 19:54:04 +0200 Subject: Implement DNF-based `#[cfg]` introspection --- Cargo.lock | 1 + crates/cfg/Cargo.toml | 1 + crates/cfg/src/cfg_expr.rs | 35 +++- crates/cfg/src/dnf.rs | 477 +++++++++++++++++++++++++++++++++++++++++++++ crates/cfg/src/lib.rs | 110 +++++++++++ 5 files changed, 622 insertions(+), 2 deletions(-) create mode 100644 crates/cfg/src/dnf.rs diff --git a/Cargo.lock b/Cargo.lock index 65c8de719..d435ec2ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -162,6 +162,7 @@ checksum = "ed67cbde08356238e75fc4656be4749481eeffb09e19f320a25237d5221c985d" name = "cfg" version = "0.0.0" dependencies = [ + "expect-test", "mbe", "rustc-hash", "syntax", diff --git a/crates/cfg/Cargo.toml b/crates/cfg/Cargo.toml index a6785ee8e..c68e391c1 100644 --- a/crates/cfg/Cargo.toml +++ b/crates/cfg/Cargo.toml @@ -17,3 +17,4 @@ tt = { path = "../tt", version = "0.0.0" } [dev-dependencies] mbe = { path = "../mbe" } syntax = { path = "../syntax" } +expect-test = "1.0" diff --git a/crates/cfg/src/cfg_expr.rs b/crates/cfg/src/cfg_expr.rs index db3655b74..3f12a3fa8 100644 --- a/crates/cfg/src/cfg_expr.rs +++ b/crates/cfg/src/cfg_expr.rs @@ -2,12 +2,12 @@ //! //! See: https://doc.rust-lang.org/reference/conditional-compilation.html#conditional-compilation -use std::slice::Iter as SliceIter; +use std::{fmt, slice::Iter as SliceIter}; use tt::SmolStr; /// A simple configuration value passed in from the outside. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)] pub enum CfgAtom { /// eg. `#[cfg(test)]` Flag(SmolStr), @@ -18,6 +18,37 @@ pub enum CfgAtom { KeyValue { key: SmolStr, value: SmolStr }, } +impl CfgAtom { + /// Returns `true` when the atom comes from the target specification. + /// + /// If this returns `true`, then changing this atom requires changing the compilation target. If + /// it returns `false`, the atom might come from a build script or the build system. + pub fn is_target_defined(&self) -> bool { + match self { + CfgAtom::Flag(flag) => matches!(&**flag, "unix" | "windows"), + CfgAtom::KeyValue { key, value: _ } => matches!( + &**key, + "target_arch" + | "target_os" + | "target_env" + | "target_family" + | "target_endian" + | "target_pointer_width" + | "target_vendor" // NOTE: `target_feature` is left out since it can be configured via `-Ctarget-feature` + ), + } + } +} + +impl fmt::Display for CfgAtom { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + CfgAtom::Flag(name) => write!(f, "{}", name), + CfgAtom::KeyValue { key, value } => write!(f, "{} = \"{}\"", key, value), + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum CfgExpr { Invalid, diff --git a/crates/cfg/src/dnf.rs b/crates/cfg/src/dnf.rs new file mode 100644 index 000000000..35f946e6f --- /dev/null +++ b/crates/cfg/src/dnf.rs @@ -0,0 +1,477 @@ +//! Disjunctive Normal Form construction. +//! +//! Algorithm from , +//! which would have been much easier to read if it used pattern matching. It's also missing the +//! entire "distribute ANDs over ORs" part, which is not trivial. Oh well. +//! +//! This is currently both messy and inefficient. Feel free to improve, there are unit tests. + +use std::fmt; + +use rustc_hash::FxHashSet; + +use crate::{CfgAtom, CfgDiff, CfgExpr, CfgOptions, InactiveReason}; + +/// A `#[cfg]` directive in Disjunctive Normal Form (DNF). +pub struct DnfExpr { + conjunctions: Vec, +} + +impl DnfExpr { + pub fn new(expr: CfgExpr) -> Self { + let builder = Builder { expr: DnfExpr { conjunctions: Vec::new() } }; + + builder.lower(expr.clone()) + } + + /// Computes a list of present or absent atoms in `opts` that cause this expression to evaluate + /// to `false`. + /// + /// Note that flipping a subset of these atoms might be sufficient to make the whole expression + /// evaluate to `true`. For that, see `compute_enable_hints`. + /// + /// Returns `None` when `self` is already true, or contains errors. + pub fn why_inactive(&self, opts: &CfgOptions) -> Option { + let mut res = InactiveReason { enabled: Vec::new(), disabled: Vec::new() }; + + for conj in &self.conjunctions { + let mut conj_is_true = true; + for lit in &conj.literals { + let atom = lit.var.as_ref()?; + let enabled = opts.enabled.contains(atom); + if lit.negate == enabled { + // Literal is false, but needs to be true for this conjunction. + conj_is_true = false; + + if enabled { + res.enabled.push(atom.clone()); + } else { + res.disabled.push(atom.clone()); + } + } + } + + if conj_is_true { + // This expression is not actually inactive. + return None; + } + } + + res.enabled.sort_unstable(); + res.enabled.dedup(); + res.disabled.sort_unstable(); + res.disabled.dedup(); + Some(res) + } + + /// Returns `CfgDiff` objects that would enable this directive if applied to `opts`. + pub fn compute_enable_hints<'a>( + &'a self, + opts: &'a CfgOptions, + ) -> impl Iterator + 'a { + // A cfg is enabled if any of `self.conjunctions` evaluate to `true`. + + self.conjunctions.iter().filter_map(move |conj| { + let mut enable = FxHashSet::default(); + let mut disable = FxHashSet::default(); + for lit in &conj.literals { + let atom = lit.var.as_ref()?; + let enabled = opts.enabled.contains(atom); + if lit.negate && enabled { + disable.insert(atom.clone()); + } + if !lit.negate && !enabled { + enable.insert(atom.clone()); + } + } + + // Check that this actually makes `conj` true. + for lit in &conj.literals { + let atom = lit.var.as_ref()?; + let enabled = enable.contains(atom) + || (opts.enabled.contains(atom) && !disable.contains(atom)); + if enabled == lit.negate { + return None; + } + } + + if enable.is_empty() && disable.is_empty() { + return None; + } + + let mut diff = CfgDiff { + enable: enable.into_iter().collect(), + disable: disable.into_iter().collect(), + }; + + // Undo the FxHashMap randomization for consistent output. + diff.enable.sort_unstable(); + diff.disable.sort_unstable(); + + Some(diff) + }) + } +} + +impl fmt::Display for DnfExpr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.conjunctions.len() != 1 { + write!(f, "any(")?; + } + for (i, conj) in self.conjunctions.iter().enumerate() { + if i != 0 { + f.write_str(", ")?; + } + + write!(f, "{}", conj)?; + } + if self.conjunctions.len() != 1 { + write!(f, ")")?; + } + + Ok(()) + } +} + +struct Conjunction { + literals: Vec, +} + +impl Conjunction { + fn new(parts: Vec) -> Self { + let mut literals = Vec::new(); + for part in parts { + match part { + CfgExpr::Invalid | CfgExpr::Atom(_) | CfgExpr::Not(_) => { + literals.push(Literal::new(part)); + } + CfgExpr::All(conj) => { + // Flatten. + literals.extend(Conjunction::new(conj).literals); + } + CfgExpr::Any(_) => unreachable!("disjunction in conjunction"), + } + } + + Self { literals } + } +} + +impl fmt::Display for Conjunction { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.literals.len() != 1 { + write!(f, "all(")?; + } + for (i, lit) in self.literals.iter().enumerate() { + if i != 0 { + f.write_str(", ")?; + } + + write!(f, "{}", lit)?; + } + if self.literals.len() != 1 { + write!(f, ")")?; + } + + Ok(()) + } +} + +struct Literal { + negate: bool, + var: Option, // None = Invalid +} + +impl Literal { + fn new(expr: CfgExpr) -> Self { + match expr { + CfgExpr::Invalid => Self { negate: false, var: None }, + CfgExpr::Atom(atom) => Self { negate: false, var: Some(atom) }, + CfgExpr::Not(expr) => match *expr { + CfgExpr::Invalid => Self { negate: true, var: None }, + CfgExpr::Atom(atom) => Self { negate: true, var: Some(atom) }, + _ => unreachable!("non-atom {:?}", expr), + }, + CfgExpr::Any(_) | CfgExpr::All(_) => unreachable!("non-literal {:?}", expr), + } + } +} + +impl fmt::Display for Literal { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.negate { + write!(f, "not(")?; + } + + match &self.var { + Some(var) => write!(f, "{}", var)?, + None => f.write_str("")?, + } + + if self.negate { + write!(f, ")")?; + } + + Ok(()) + } +} + +struct Builder { + expr: DnfExpr, +} + +impl Builder { + fn lower(mut self, expr: CfgExpr) -> DnfExpr { + let expr = make_nnf(expr); + let expr = make_dnf(expr); + + match expr { + CfgExpr::Invalid | CfgExpr::Atom(_) | CfgExpr::Not(_) => { + self.expr.conjunctions.push(Conjunction::new(vec![expr])); + } + CfgExpr::All(conj) => { + self.expr.conjunctions.push(Conjunction::new(conj)); + } + CfgExpr::Any(mut disj) => { + disj.reverse(); + while let Some(conj) = disj.pop() { + match conj { + CfgExpr::Invalid | CfgExpr::Atom(_) | CfgExpr::All(_) | CfgExpr::Not(_) => { + self.expr.conjunctions.push(Conjunction::new(vec![conj])); + } + CfgExpr::Any(inner_disj) => { + // Flatten. + disj.extend(inner_disj.into_iter().rev()); + } + } + } + } + } + + self.expr + } +} + +fn make_dnf(expr: CfgExpr) -> CfgExpr { + match expr { + CfgExpr::Invalid | CfgExpr::Atom(_) | CfgExpr::Not(_) => expr, + CfgExpr::Any(e) => CfgExpr::Any(e.into_iter().map(|expr| make_dnf(expr)).collect()), + CfgExpr::All(e) => { + let e = e.into_iter().map(|expr| make_nnf(expr)).collect::>(); + + CfgExpr::Any(distribute_conj(&e)) + } + } +} + +/// Turns a conjunction of expressions into a disjunction of expressions. +fn distribute_conj(conj: &[CfgExpr]) -> Vec { + fn go(out: &mut Vec, with: &mut Vec, rest: &[CfgExpr]) { + match rest { + [head, tail @ ..] => match head { + CfgExpr::Any(disj) => { + for part in disj { + with.push(part.clone()); + go(out, with, tail); + with.pop(); + } + } + _ => { + with.push(head.clone()); + go(out, with, tail); + with.pop(); + } + }, + _ => { + // Turn accumulated parts into a new conjunction. + out.push(CfgExpr::All(with.clone())); + } + } + } + + let mut out = Vec::new(); + let mut with = Vec::new(); + + go(&mut out, &mut with, conj); + + out +} + +fn make_nnf(expr: CfgExpr) -> CfgExpr { + match expr { + CfgExpr::Invalid | CfgExpr::Atom(_) => expr, + CfgExpr::Any(expr) => CfgExpr::Any(expr.into_iter().map(|expr| make_nnf(expr)).collect()), + CfgExpr::All(expr) => CfgExpr::All(expr.into_iter().map(|expr| make_nnf(expr)).collect()), + CfgExpr::Not(operand) => match *operand { + CfgExpr::Invalid | CfgExpr::Atom(_) => CfgExpr::Not(operand.clone()), // Original negated expr + CfgExpr::Not(expr) => { + // Remove double negation. + make_nnf(*expr) + } + // Convert negated conjunction/disjunction using DeMorgan's Law. + CfgExpr::Any(inner) => CfgExpr::All( + inner.into_iter().map(|expr| make_nnf(CfgExpr::Not(Box::new(expr)))).collect(), + ), + CfgExpr::All(inner) => CfgExpr::Any( + inner.into_iter().map(|expr| make_nnf(CfgExpr::Not(Box::new(expr)))).collect(), + ), + }, + } +} + +#[cfg(test)] +mod test { + use expect_test::{expect, Expect}; + use mbe::ast_to_token_tree; + use syntax::{ast, AstNode}; + + use super::*; + + fn check_dnf(input: &str, expect: Expect) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + let actual = format!("#![cfg({})]", DnfExpr::new(cfg)); + expect.assert_eq(&actual); + } + + fn check_why_inactive(input: &str, opts: &CfgOptions, expect: Expect) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + let dnf = DnfExpr::new(cfg); + let why_inactive = dnf.why_inactive(opts).unwrap().to_string(); + expect.assert_eq(&why_inactive); + } + + #[track_caller] + fn check_enable_hints(input: &str, opts: &CfgOptions, expected_hints: &[&str]) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + let dnf = DnfExpr::new(cfg); + let hints = dnf.compute_enable_hints(opts).map(|diff| diff.to_string()).collect::>(); + assert_eq!(hints, expected_hints); + } + + #[test] + fn smoke() { + check_dnf("#![cfg(test)]", expect![[r#"#![cfg(test)]"#]]); + check_dnf("#![cfg(not(test))]", expect![[r#"#![cfg(not(test))]"#]]); + check_dnf("#![cfg(not(not(test)))]", expect![[r#"#![cfg(test)]"#]]); + + check_dnf("#![cfg(all(a, b))]", expect![[r#"#![cfg(all(a, b))]"#]]); + check_dnf("#![cfg(any(a, b))]", expect![[r#"#![cfg(any(a, b))]"#]]); + + check_dnf("#![cfg(not(a))]", expect![[r#"#![cfg(not(a))]"#]]); + } + + #[test] + fn distribute() { + check_dnf("#![cfg(all(any(a, b), c))]", expect![[r#"#![cfg(any(all(a, c), all(b, c)))]"#]]); + check_dnf("#![cfg(all(c, any(a, b)))]", expect![[r#"#![cfg(any(all(c, a), all(c, b)))]"#]]); + check_dnf( + "#![cfg(all(any(a, b), any(c, d)))]", + expect![[r#"#![cfg(any(all(a, c), all(a, d), all(b, c), all(b, d)))]"#]], + ); + + check_dnf( + "#![cfg(all(any(a, b, c), any(d, e, f), g))]", + expect![[ + r#"#![cfg(any(all(a, d, g), all(a, e, g), all(a, f, g), all(b, d, g), all(b, e, g), all(b, f, g), all(c, d, g), all(c, e, g), all(c, f, g)))]"# + ]], + ); + } + + #[test] + fn demorgan() { + check_dnf("#![cfg(not(all(a, b)))]", expect![[r#"#![cfg(any(not(a), not(b)))]"#]]); + check_dnf("#![cfg(not(any(a, b)))]", expect![[r#"#![cfg(all(not(a), not(b)))]"#]]); + + check_dnf("#![cfg(not(all(not(a), b)))]", expect![[r#"#![cfg(any(a, not(b)))]"#]]); + check_dnf("#![cfg(not(any(a, not(b))))]", expect![[r#"#![cfg(all(not(a), b))]"#]]); + } + + #[test] + fn nested() { + check_dnf( + "#![cfg(all(any(a), not(all(any(b)))))]", + expect![[r#"#![cfg(all(a, not(b)))]"#]], + ); + + check_dnf("#![cfg(any(any(a, b)))]", expect![[r#"#![cfg(any(a, b))]"#]]); + check_dnf("#![cfg(not(any(any(a, b))))]", expect![[r#"#![cfg(all(not(a), not(b)))]"#]]); + check_dnf("#![cfg(all(all(a, b)))]", expect![[r#"#![cfg(all(a, b))]"#]]); + check_dnf("#![cfg(not(all(all(a, b))))]", expect![[r#"#![cfg(any(not(a), not(b)))]"#]]); + } + + #[test] + fn hints() { + let mut opts = CfgOptions::default(); + + check_enable_hints("#![cfg(test)]", &opts, &["enable test"]); + check_enable_hints("#![cfg(not(test))]", &opts, &[]); + + check_enable_hints("#![cfg(any(a, b))]", &opts, &["enable a", "enable b"]); + check_enable_hints("#![cfg(any(b, a))]", &opts, &["enable b", "enable a"]); + + check_enable_hints("#![cfg(all(a, b))]", &opts, &["enable a and b"]); + + opts.insert_atom("test".into()); + + check_enable_hints("#![cfg(test)]", &opts, &[]); + check_enable_hints("#![cfg(not(test))]", &opts, &["disable test"]); + } + + /// Tests that we don't suggest hints for cfgs that express an inconsistent formula. + #[test] + fn hints_impossible() { + let mut opts = CfgOptions::default(); + + check_enable_hints("#![cfg(all(test, not(test)))]", &opts, &[]); + + opts.insert_atom("test".into()); + + check_enable_hints("#![cfg(all(test, not(test)))]", &opts, &[]); + } + + #[test] + fn why_inactive() { + let mut opts = CfgOptions::default(); + opts.insert_atom("test".into()); + opts.insert_atom("test2".into()); + + check_why_inactive("#![cfg(a)]", &opts, expect![["a is disabled"]]); + check_why_inactive("#![cfg(not(test))]", &opts, expect![["test is enabled"]]); + + check_why_inactive( + "#![cfg(all(not(test), not(test2)))]", + &opts, + expect![["test and test2 are enabled"]], + ); + check_why_inactive( + "#![cfg(all(not(test), a))]", + &opts, + expect![["test is enabled and a is disabled"]], + ); + check_why_inactive( + "#![cfg(all(not(test), test2, a))]", + &opts, + expect![["test is enabled and a is disabled"]], + ); + check_why_inactive( + "#![cfg(all(not(test), not(test2), a))]", + &opts, + expect![["test and test2 are enabled and a is disabled"]], + ); + } +} diff --git a/crates/cfg/src/lib.rs b/crates/cfg/src/lib.rs index 35f540ac3..0b0734213 100644 --- a/crates/cfg/src/lib.rs +++ b/crates/cfg/src/lib.rs @@ -1,11 +1,15 @@ //! cfg defines conditional compiling options, `cfg` attibute parser and evaluator mod cfg_expr; +mod dnf; + +use std::fmt; use rustc_hash::FxHashSet; use tt::SmolStr; pub use cfg_expr::{CfgAtom, CfgExpr}; +pub use dnf::DnfExpr; /// Configuration options used for conditional compilition on items with `cfg` attributes. /// We have two kind of options in different namespaces: atomic options like `unix`, and @@ -40,4 +44,110 @@ impl CfgOptions { self.enabled.insert(atom.clone()); } } + + pub fn apply_diff(&mut self, diff: CfgDiff) { + for atom in diff.enable { + self.enabled.insert(atom); + } + + for atom in diff.disable { + self.enabled.remove(&atom); + } + } +} + +pub struct CfgDiff { + // Invariants: No duplicates, no atom that's both in `enable` and `disable`. + enable: Vec, + disable: Vec, +} + +impl CfgDiff { + /// Returns the total number of atoms changed by this diff. + pub fn len(&self) -> usize { + self.enable.len() + self.disable.len() + } +} + +impl fmt::Display for CfgDiff { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if !self.enable.is_empty() { + f.write_str("enable ")?; + for (i, atom) in self.enable.iter().enumerate() { + let sep = match i { + 0 => "", + _ if i == self.enable.len() - 1 => " and ", + _ => ", ", + }; + f.write_str(sep)?; + + write!(f, "{}", atom)?; + } + + if !self.disable.is_empty() { + f.write_str("; ")?; + } + } + + if !self.disable.is_empty() { + f.write_str("disable ")?; + for (i, atom) in self.disable.iter().enumerate() { + let sep = match i { + 0 => "", + _ if i == self.enable.len() - 1 => " and ", + _ => ", ", + }; + f.write_str(sep)?; + + write!(f, "{}", atom)?; + } + } + + Ok(()) + } +} + +pub struct InactiveReason { + enabled: Vec, + disabled: Vec, +} + +impl fmt::Display for InactiveReason { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if !self.enabled.is_empty() { + for (i, atom) in self.enabled.iter().enumerate() { + let sep = match i { + 0 => "", + _ if i == self.enabled.len() - 1 => " and ", + _ => ", ", + }; + f.write_str(sep)?; + + write!(f, "{}", atom)?; + } + let is_are = if self.enabled.len() == 1 { "is" } else { "are" }; + write!(f, " {} enabled", is_are)?; + + if !self.disabled.is_empty() { + f.write_str(" and ")?; + } + } + + if !self.disabled.is_empty() { + for (i, atom) in self.disabled.iter().enumerate() { + let sep = match i { + 0 => "", + _ if i == self.enabled.len() - 1 => " and ", + _ => ", ", + }; + f.write_str(sep)?; + + write!(f, "{}", atom)?; + } + let is_are = if self.disabled.len() == 1 { "is" } else { "are" }; + write!(f, " {} disabled", is_are)?; + } + + Ok(()) + } } -- cgit v1.2.3 From 978cc936491d23bd38ef18aa98ddcc7472ef5f54 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 22 Oct 2020 19:19:05 +0200 Subject: Fix typo --- crates/cfg/src/dnf.rs | 1 + crates/cfg/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/cfg/src/dnf.rs b/crates/cfg/src/dnf.rs index 35f946e6f..a08c307da 100644 --- a/crates/cfg/src/dnf.rs +++ b/crates/cfg/src/dnf.rs @@ -458,6 +458,7 @@ mod test { &opts, expect![["test and test2 are enabled"]], ); + check_why_inactive("#![cfg(all(a, b))]", &opts, expect![["a and b are disabled"]]); check_why_inactive( "#![cfg(all(not(test), a))]", &opts, diff --git a/crates/cfg/src/lib.rs b/crates/cfg/src/lib.rs index 0b0734213..28a40a082 100644 --- a/crates/cfg/src/lib.rs +++ b/crates/cfg/src/lib.rs @@ -137,7 +137,7 @@ impl fmt::Display for InactiveReason { for (i, atom) in self.disabled.iter().enumerate() { let sep = match i { 0 => "", - _ if i == self.enabled.len() - 1 => " and ", + _ if i == self.disabled.len() - 1 => " and ", _ => ", ", }; f.write_str(sep)?; -- cgit v1.2.3 From 3421b645e6f7d15ddad0e8e526d8a7db09b72516 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 22 Oct 2020 19:19:18 +0200 Subject: Emit better #[cfg] diagnostics --- crates/hir_def/src/attr.rs | 14 ++++- crates/hir_def/src/diagnostics.rs | 14 ++++- crates/hir_def/src/nameres.rs | 16 +++-- crates/hir_def/src/nameres/collector.rs | 29 +++++---- crates/hir_def/src/nameres/tests/diagnostics.rs | 22 +++++++ crates/ide/src/hover.rs | 4 +- crates/ide/src/runnables.rs | 84 +++++++++++++------------ crates/rust-analyzer/src/cargo_target_spec.rs | 4 +- crates/rust-analyzer/src/to_proto.rs | 2 +- 9 files changed, 124 insertions(+), 65 deletions(-) diff --git a/crates/hir_def/src/attr.rs b/crates/hir_def/src/attr.rs index dea552a60..b2ce7ca3c 100644 --- a/crates/hir_def/src/attr.rs +++ b/crates/hir_def/src/attr.rs @@ -125,12 +125,20 @@ impl Attrs { AttrQuery { attrs: self, key } } - pub fn cfg(&self) -> impl Iterator + '_ { + pub fn cfg(&self) -> Option { // FIXME: handle cfg_attr :-) - self.by_key("cfg").tt_values().map(CfgExpr::parse) + let mut cfgs = self.by_key("cfg").tt_values().map(CfgExpr::parse).collect::>(); + match cfgs.len() { + 0 => None, + 1 => Some(cfgs.pop().unwrap()), + _ => Some(CfgExpr::All(cfgs)), + } } pub(crate) fn is_cfg_enabled(&self, cfg_options: &CfgOptions) -> bool { - self.cfg().all(|cfg| cfg_options.check(&cfg) != Some(false)) + match self.cfg() { + None => true, + Some(cfg) => cfg_options.check(&cfg) != Some(false), + } } } diff --git a/crates/hir_def/src/diagnostics.rs b/crates/hir_def/src/diagnostics.rs index c9c08b01f..34a6a8d4b 100644 --- a/crates/hir_def/src/diagnostics.rs +++ b/crates/hir_def/src/diagnostics.rs @@ -1,7 +1,9 @@ //! Diagnostics produced by `hir_def`. use std::any::Any; +use std::fmt::Write; +use cfg::{CfgExpr, CfgOptions, DnfExpr}; use hir_expand::diagnostics::{Diagnostic, DiagnosticCode}; use syntax::{ast, AstPtr, SyntaxNodePtr}; @@ -94,6 +96,8 @@ impl Diagnostic for UnresolvedImport { pub struct InactiveCode { pub file: HirFileId, pub node: SyntaxNodePtr, + pub cfg: CfgExpr, + pub opts: CfgOptions, } impl Diagnostic for InactiveCode { @@ -101,8 +105,14 @@ impl Diagnostic for InactiveCode { DiagnosticCode("inactive-code") } fn message(&self) -> String { - // FIXME: say *why* it is configured out - "code is inactive due to #[cfg] directives".to_string() + let inactive = DnfExpr::new(self.cfg.clone()).why_inactive(&self.opts); + let mut buf = "code is inactive due to #[cfg] directives".to_string(); + + if let Some(inactive) = inactive { + write!(buf, ": {}", inactive).unwrap(); + } + + buf } fn display_source(&self) -> InFile { InFile::new(self.file, self.node.clone()) diff --git a/crates/hir_def/src/nameres.rs b/crates/hir_def/src/nameres.rs index 01a28aeeb..eb41d324e 100644 --- a/crates/hir_def/src/nameres.rs +++ b/crates/hir_def/src/nameres.rs @@ -283,6 +283,7 @@ pub enum ModuleSource { } mod diagnostics { + use cfg::{CfgExpr, CfgOptions}; use hir_expand::diagnostics::DiagnosticSink; use hir_expand::hygiene::Hygiene; use hir_expand::InFile; @@ -299,7 +300,7 @@ mod diagnostics { UnresolvedImport { ast: AstId, index: usize }, - UnconfiguredCode { ast: InFile }, + UnconfiguredCode { ast: InFile, cfg: CfgExpr, opts: CfgOptions }, } #[derive(Debug, PartialEq, Eq)] @@ -341,8 +342,10 @@ mod diagnostics { pub(super) fn unconfigured_code( container: LocalModuleId, ast: InFile, + cfg: CfgExpr, + opts: CfgOptions, ) -> Self { - Self { in_module: container, kind: DiagnosticKind::UnconfiguredCode { ast } } + Self { in_module: container, kind: DiagnosticKind::UnconfiguredCode { ast, cfg, opts } } } pub(super) fn add_to( @@ -395,8 +398,13 @@ mod diagnostics { } } - DiagnosticKind::UnconfiguredCode { ast } => { - sink.push(InactiveCode { file: ast.file_id, node: ast.value.clone() }); + DiagnosticKind::UnconfiguredCode { ast, cfg, opts } => { + sink.push(InactiveCode { + file: ast.file_id, + node: ast.value.clone(), + cfg: cfg.clone(), + opts: opts.clone(), + }); } } } diff --git a/crates/hir_def/src/nameres/collector.rs b/crates/hir_def/src/nameres/collector.rs index bff8edb62..f30172d90 100644 --- a/crates/hir_def/src/nameres/collector.rs +++ b/crates/hir_def/src/nameres/collector.rs @@ -6,7 +6,7 @@ use std::iter; use base_db::{CrateId, FileId, ProcMacroId}; -use cfg::CfgOptions; +use cfg::{CfgExpr, CfgOptions}; use hir_expand::InFile; use hir_expand::{ ast_id_map::FileAstId, @@ -900,7 +900,8 @@ impl ModCollector<'_, '_> { // `#[macro_use] extern crate` is hoisted to imports macros before collecting // any other items. for item in items { - if self.is_cfg_enabled(self.item_tree.attrs((*item).into())) { + let attrs = self.item_tree.attrs((*item).into()); + if attrs.cfg().map_or(true, |cfg| self.is_cfg_enabled(&cfg)) { if let ModItem::ExternCrate(id) = item { let import = self.item_tree[*id].clone(); if import.is_macro_use { @@ -912,9 +913,11 @@ impl ModCollector<'_, '_> { for &item in items { let attrs = self.item_tree.attrs(item.into()); - if !self.is_cfg_enabled(attrs) { - self.emit_unconfigured_diagnostic(item); - continue; + if let Some(cfg) = attrs.cfg() { + if !self.is_cfg_enabled(&cfg) { + self.emit_unconfigured_diagnostic(item, &cfg); + continue; + } } let module = ModuleId { krate: self.def_collector.def_map.krate, local_id: self.module_id }; @@ -1321,20 +1324,22 @@ impl ModCollector<'_, '_> { } } - fn is_cfg_enabled(&self, attrs: &Attrs) -> bool { - attrs.is_cfg_enabled(self.def_collector.cfg_options) + fn is_cfg_enabled(&self, cfg: &CfgExpr) -> bool { + self.def_collector.cfg_options.check(cfg) != Some(false) } - fn emit_unconfigured_diagnostic(&mut self, item: ModItem) { + fn emit_unconfigured_diagnostic(&mut self, item: ModItem, cfg: &CfgExpr) { let ast_id = item.ast_id(self.item_tree); let id_map = self.def_collector.db.ast_id_map(self.file_id); let syntax_ptr = id_map.get(ast_id).syntax_node_ptr(); let ast_node = InFile::new(self.file_id, syntax_ptr); - self.def_collector - .def_map - .diagnostics - .push(DefDiagnostic::unconfigured_code(self.module_id, ast_node)); + self.def_collector.def_map.diagnostics.push(DefDiagnostic::unconfigured_code( + self.module_id, + ast_node, + cfg.clone(), + self.def_collector.cfg_options.clone(), + )); } } diff --git a/crates/hir_def/src/nameres/tests/diagnostics.rs b/crates/hir_def/src/nameres/tests/diagnostics.rs index 576b813d2..5972248de 100644 --- a/crates/hir_def/src/nameres/tests/diagnostics.rs +++ b/crates/hir_def/src/nameres/tests/diagnostics.rs @@ -129,3 +129,25 @@ fn unresolved_module() { ", ); } + +#[test] +fn inactive_item() { + // Additional tests in `cfg` crate. This only tests disabled cfgs. + + check_diagnostics( + r#" + //- /lib.rs + #[cfg(no)] pub fn f() {} + //^^^^^^^^^^^^^^^^^^^^^^^^ code is inactive due to #[cfg] directives: no is disabled + + #[cfg(no)] #[cfg(no2)] mod m; + //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ code is inactive due to #[cfg] directives: no and no2 are disabled + + #[cfg(all(not(a), b))] enum E {} + //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ code is inactive due to #[cfg] directives: b is disabled + + #[cfg(feature = "std")] use std; + //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ code is inactive due to #[cfg] directives: feature = "std" is disabled + "#, + ); +} diff --git a/crates/ide/src/hover.rs b/crates/ide/src/hover.rs index 6466422c5..0332c7be0 100644 --- a/crates/ide/src/hover.rs +++ b/crates/ide/src/hover.rs @@ -2128,7 +2128,7 @@ fn foo_<|>test() {} ignore: false, }, }, - cfg_exprs: [], + cfg: None, }, ), ] @@ -2166,7 +2166,7 @@ mod tests<|> { kind: TestMod { path: "tests", }, - cfg_exprs: [], + cfg: None, }, ), ] diff --git a/crates/ide/src/runnables.rs b/crates/ide/src/runnables.rs index 752ef2f21..eb82456ad 100644 --- a/crates/ide/src/runnables.rs +++ b/crates/ide/src/runnables.rs @@ -15,7 +15,7 @@ use crate::{display::ToNav, FileId, NavigationTarget}; pub struct Runnable { pub nav: NavigationTarget, pub kind: RunnableKind, - pub cfg_exprs: Vec, + pub cfg: Option, } #[derive(Debug, Clone)] @@ -168,7 +168,7 @@ fn runnable_fn( }; let attrs = Attrs::from_attrs_owner(sema.db, InFile::new(HirFileId::from(file_id), &fn_def)); - let cfg_exprs = attrs.cfg().collect(); + let cfg = attrs.cfg(); let nav = if let RunnableKind::DocTest { .. } = kind { NavigationTarget::from_doc_commented( @@ -179,7 +179,7 @@ fn runnable_fn( } else { NavigationTarget::from_named(sema.db, InFile::new(file_id.into(), &fn_def)) }; - Some(Runnable { nav, kind, cfg_exprs }) + Some(Runnable { nav, kind, cfg }) } #[derive(Debug, Copy, Clone)] @@ -255,9 +255,9 @@ fn runnable_mod( .join("::"); let attrs = Attrs::from_attrs_owner(sema.db, InFile::new(HirFileId::from(file_id), &module)); - let cfg_exprs = attrs.cfg().collect(); + let cfg = attrs.cfg(); let nav = module_def.to_nav(sema.db); - Some(Runnable { nav, kind: RunnableKind::TestMod { path }, cfg_exprs }) + Some(Runnable { nav, kind: RunnableKind::TestMod { path }, cfg }) } // We could create runnables for modules with number_of_test_submodules > 0, @@ -348,7 +348,7 @@ fn bench() {} docs: None, }, kind: Bin, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -373,7 +373,7 @@ fn bench() {} ignore: false, }, }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -398,7 +398,7 @@ fn bench() {} ignore: true, }, }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -420,7 +420,7 @@ fn bench() {} "bench", ), }, - cfg_exprs: [], + cfg: None, }, ] "#]], @@ -507,7 +507,7 @@ fn should_have_no_runnable_6() {} docs: None, }, kind: Bin, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -527,7 +527,7 @@ fn should_have_no_runnable_6() {} "should_have_runnable", ), }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -547,7 +547,7 @@ fn should_have_no_runnable_6() {} "should_have_runnable_1", ), }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -567,7 +567,7 @@ fn should_have_no_runnable_6() {} "should_have_runnable_2", ), }, - cfg_exprs: [], + cfg: None, }, ] "#]], @@ -609,7 +609,7 @@ impl Data { docs: None, }, kind: Bin, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -629,7 +629,7 @@ impl Data { "Data::foo", ), }, - cfg_exprs: [], + cfg: None, }, ] "#]], @@ -668,7 +668,7 @@ mod test_mod { kind: TestMod { path: "test_mod", }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -693,7 +693,7 @@ mod test_mod { ignore: false, }, }, - cfg_exprs: [], + cfg: None, }, ] "#]], @@ -748,7 +748,7 @@ mod root_tests { kind: TestMod { path: "root_tests::nested_tests_0", }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -768,7 +768,7 @@ mod root_tests { kind: TestMod { path: "root_tests::nested_tests_0::nested_tests_1", }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -793,7 +793,7 @@ mod root_tests { ignore: false, }, }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -818,7 +818,7 @@ mod root_tests { ignore: false, }, }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -838,7 +838,7 @@ mod root_tests { kind: TestMod { path: "root_tests::nested_tests_0::nested_tests_2", }, - cfg_exprs: [], + cfg: None, }, Runnable { nav: NavigationTarget { @@ -863,7 +863,7 @@ mod root_tests { ignore: false, }, }, - cfg_exprs: [], + cfg: None, }, ] "#]], @@ -906,12 +906,14 @@ fn test_foo1() {} ignore: false, }, }, - cfg_exprs: [ - KeyValue { - key: "feature", - value: "foo", - }, - ], + cfg: Some( + Atom( + KeyValue { + key: "feature", + value: "foo", + }, + ), + ), }, ] "#]], @@ -954,20 +956,24 @@ fn test_foo1() {} ignore: false, }, }, - cfg_exprs: [ + cfg: Some( All( [ - KeyValue { - key: "feature", - value: "foo", - }, - KeyValue { - key: "feature", - value: "bar", - }, + Atom( + KeyValue { + key: "feature", + value: "foo", + }, + ), + Atom( + KeyValue { + key: "feature", + value: "bar", + }, + ), ], ), - ], + ), }, ] "#]], diff --git a/crates/rust-analyzer/src/cargo_target_spec.rs b/crates/rust-analyzer/src/cargo_target_spec.rs index 7da935464..1ab72bd91 100644 --- a/crates/rust-analyzer/src/cargo_target_spec.rs +++ b/crates/rust-analyzer/src/cargo_target_spec.rs @@ -24,7 +24,7 @@ impl CargoTargetSpec { snap: &GlobalStateSnapshot, spec: Option, kind: &RunnableKind, - cfgs: &[CfgExpr], + cfg: &Option, ) -> Result<(Vec, Vec)> { let mut args = Vec::new(); let mut extra_args = Vec::new(); @@ -87,7 +87,7 @@ impl CargoTargetSpec { args.push("--all-features".to_string()); } else { let mut features = Vec::new(); - for cfg in cfgs { + if let Some(cfg) = cfg.as_ref() { required_features(cfg, &mut features); } for feature in &snap.config.cargo.features { diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index aeacde0f7..121357a5a 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -745,7 +745,7 @@ pub(crate) fn runnable( let workspace_root = spec.as_ref().map(|it| it.workspace_root.clone()); let target = spec.as_ref().map(|s| s.target.clone()); let (cargo_args, executable_args) = - CargoTargetSpec::runnable_args(snap, spec, &runnable.kind, &runnable.cfg_exprs)?; + CargoTargetSpec::runnable_args(snap, spec, &runnable.kind, &runnable.cfg)?; let label = runnable.label(target); let location = location_link(snap, None, runnable.nav)?; -- cgit v1.2.3 From dbd6266bc91c3934176e251eedb7de381fcc1006 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 22 Oct 2020 20:08:27 +0200 Subject: Update crates/cfg/src/cfg_expr.rs Co-authored-by: Aleksey Kladov --- crates/cfg/src/cfg_expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cfg/src/cfg_expr.rs b/crates/cfg/src/cfg_expr.rs index 3f12a3fa8..283ff968f 100644 --- a/crates/cfg/src/cfg_expr.rs +++ b/crates/cfg/src/cfg_expr.rs @@ -44,7 +44,7 @@ impl fmt::Display for CfgAtom { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { CfgAtom::Flag(name) => write!(f, "{}", name), - CfgAtom::KeyValue { key, value } => write!(f, "{} = \"{}\"", key, value), + CfgAtom::KeyValue { key, value } => write!(f, "{} = {:?}", key, value), } } } -- cgit v1.2.3 From bfe1efca26a9ae07d44ca3cc79ffdc4342f72b44 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 23 Oct 2020 12:08:55 +0200 Subject: Use format_to Co-authored-by: Aleksey Kladov --- crates/hir_def/src/diagnostics.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/hir_def/src/diagnostics.rs b/crates/hir_def/src/diagnostics.rs index 34a6a8d4b..532496b62 100644 --- a/crates/hir_def/src/diagnostics.rs +++ b/crates/hir_def/src/diagnostics.rs @@ -1,13 +1,12 @@ //! Diagnostics produced by `hir_def`. use std::any::Any; -use std::fmt::Write; +use stdx::format_to; use cfg::{CfgExpr, CfgOptions, DnfExpr}; use hir_expand::diagnostics::{Diagnostic, DiagnosticCode}; -use syntax::{ast, AstPtr, SyntaxNodePtr}; - use hir_expand::{HirFileId, InFile}; +use syntax::{ast, AstPtr, SyntaxNodePtr}; // Diagnostic: unresolved-module // @@ -109,7 +108,7 @@ impl Diagnostic for InactiveCode { let mut buf = "code is inactive due to #[cfg] directives".to_string(); if let Some(inactive) = inactive { - write!(buf, ": {}", inactive).unwrap(); + format_to!(buf, ": {}", inactive); } buf -- cgit v1.2.3 From dab8870f5c6d90ea1a327037825dc87a46778045 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 23 Oct 2020 12:10:22 +0200 Subject: Reorder items --- crates/cfg/src/dnf.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/cfg/src/dnf.rs b/crates/cfg/src/dnf.rs index a08c307da..7ee7b0062 100644 --- a/crates/cfg/src/dnf.rs +++ b/crates/cfg/src/dnf.rs @@ -17,6 +17,15 @@ pub struct DnfExpr { conjunctions: Vec, } +struct Conjunction { + literals: Vec, +} + +struct Literal { + negate: bool, + var: Option, // None = Invalid +} + impl DnfExpr { pub fn new(expr: CfgExpr) -> Self { let builder = Builder { expr: DnfExpr { conjunctions: Vec::new() } }; @@ -133,10 +142,6 @@ impl fmt::Display for DnfExpr { } } -struct Conjunction { - literals: Vec, -} - impl Conjunction { fn new(parts: Vec) -> Self { let mut literals = Vec::new(); @@ -177,11 +182,6 @@ impl fmt::Display for Conjunction { } } -struct Literal { - negate: bool, - var: Option, // None = Invalid -} - impl Literal { fn new(expr: CfgExpr) -> Self { match expr { -- cgit v1.2.3 From a246d4f599dcf2612fa99720fb4ca98101ed93fb Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Fri, 23 Oct 2020 12:14:58 +0200 Subject: cfg: move tests to separate file that way we don't have to re-check the entire project when a test is changed --- crates/cfg/src/cfg_expr.rs | 50 ------------ crates/cfg/src/dnf.rs | 158 ------------------------------------- crates/cfg/src/lib.rs | 2 + crates/cfg/src/tests.rs | 193 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 195 insertions(+), 208 deletions(-) create mode 100644 crates/cfg/src/tests.rs diff --git a/crates/cfg/src/cfg_expr.rs b/crates/cfg/src/cfg_expr.rs index 283ff968f..42327f1e1 100644 --- a/crates/cfg/src/cfg_expr.rs +++ b/crates/cfg/src/cfg_expr.rs @@ -128,53 +128,3 @@ fn next_cfg_expr(it: &mut SliceIter) -> Option { } Some(ret) } - -#[cfg(test)] -mod tests { - use super::*; - - use mbe::ast_to_token_tree; - use syntax::ast::{self, AstNode}; - - fn assert_parse_result(input: &str, expected: CfgExpr) { - let (tt, _) = { - let source_file = ast::SourceFile::parse(input).ok().unwrap(); - let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt).unwrap() - }; - let cfg = CfgExpr::parse(&tt); - assert_eq!(cfg, expected); - } - - #[test] - fn test_cfg_expr_parser() { - assert_parse_result("#![cfg(foo)]", CfgAtom::Flag("foo".into()).into()); - assert_parse_result("#![cfg(foo,)]", CfgAtom::Flag("foo".into()).into()); - assert_parse_result( - "#![cfg(not(foo))]", - CfgExpr::Not(Box::new(CfgAtom::Flag("foo".into()).into())), - ); - assert_parse_result("#![cfg(foo(bar))]", CfgExpr::Invalid); - - // Only take the first - assert_parse_result(r#"#![cfg(foo, bar = "baz")]"#, CfgAtom::Flag("foo".into()).into()); - - assert_parse_result( - r#"#![cfg(all(foo, bar = "baz"))]"#, - CfgExpr::All(vec![ - CfgAtom::Flag("foo".into()).into(), - CfgAtom::KeyValue { key: "bar".into(), value: "baz".into() }.into(), - ]), - ); - - assert_parse_result( - r#"#![cfg(any(not(), all(), , bar = "baz",))]"#, - CfgExpr::Any(vec![ - CfgExpr::Not(Box::new(CfgExpr::Invalid)), - CfgExpr::All(vec![]), - CfgExpr::Invalid, - CfgAtom::KeyValue { key: "bar".into(), value: "baz".into() }.into(), - ]), - ); - } -} diff --git a/crates/cfg/src/dnf.rs b/crates/cfg/src/dnf.rs index 7ee7b0062..580c9a9a2 100644 --- a/crates/cfg/src/dnf.rs +++ b/crates/cfg/src/dnf.rs @@ -318,161 +318,3 @@ fn make_nnf(expr: CfgExpr) -> CfgExpr { }, } } - -#[cfg(test)] -mod test { - use expect_test::{expect, Expect}; - use mbe::ast_to_token_tree; - use syntax::{ast, AstNode}; - - use super::*; - - fn check_dnf(input: &str, expect: Expect) { - let (tt, _) = { - let source_file = ast::SourceFile::parse(input).ok().unwrap(); - let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt).unwrap() - }; - let cfg = CfgExpr::parse(&tt); - let actual = format!("#![cfg({})]", DnfExpr::new(cfg)); - expect.assert_eq(&actual); - } - - fn check_why_inactive(input: &str, opts: &CfgOptions, expect: Expect) { - let (tt, _) = { - let source_file = ast::SourceFile::parse(input).ok().unwrap(); - let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt).unwrap() - }; - let cfg = CfgExpr::parse(&tt); - let dnf = DnfExpr::new(cfg); - let why_inactive = dnf.why_inactive(opts).unwrap().to_string(); - expect.assert_eq(&why_inactive); - } - - #[track_caller] - fn check_enable_hints(input: &str, opts: &CfgOptions, expected_hints: &[&str]) { - let (tt, _) = { - let source_file = ast::SourceFile::parse(input).ok().unwrap(); - let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); - ast_to_token_tree(&tt).unwrap() - }; - let cfg = CfgExpr::parse(&tt); - let dnf = DnfExpr::new(cfg); - let hints = dnf.compute_enable_hints(opts).map(|diff| diff.to_string()).collect::>(); - assert_eq!(hints, expected_hints); - } - - #[test] - fn smoke() { - check_dnf("#![cfg(test)]", expect![[r#"#![cfg(test)]"#]]); - check_dnf("#![cfg(not(test))]", expect![[r#"#![cfg(not(test))]"#]]); - check_dnf("#![cfg(not(not(test)))]", expect![[r#"#![cfg(test)]"#]]); - - check_dnf("#![cfg(all(a, b))]", expect![[r#"#![cfg(all(a, b))]"#]]); - check_dnf("#![cfg(any(a, b))]", expect![[r#"#![cfg(any(a, b))]"#]]); - - check_dnf("#![cfg(not(a))]", expect![[r#"#![cfg(not(a))]"#]]); - } - - #[test] - fn distribute() { - check_dnf("#![cfg(all(any(a, b), c))]", expect![[r#"#![cfg(any(all(a, c), all(b, c)))]"#]]); - check_dnf("#![cfg(all(c, any(a, b)))]", expect![[r#"#![cfg(any(all(c, a), all(c, b)))]"#]]); - check_dnf( - "#![cfg(all(any(a, b), any(c, d)))]", - expect![[r#"#![cfg(any(all(a, c), all(a, d), all(b, c), all(b, d)))]"#]], - ); - - check_dnf( - "#![cfg(all(any(a, b, c), any(d, e, f), g))]", - expect![[ - r#"#![cfg(any(all(a, d, g), all(a, e, g), all(a, f, g), all(b, d, g), all(b, e, g), all(b, f, g), all(c, d, g), all(c, e, g), all(c, f, g)))]"# - ]], - ); - } - - #[test] - fn demorgan() { - check_dnf("#![cfg(not(all(a, b)))]", expect![[r#"#![cfg(any(not(a), not(b)))]"#]]); - check_dnf("#![cfg(not(any(a, b)))]", expect![[r#"#![cfg(all(not(a), not(b)))]"#]]); - - check_dnf("#![cfg(not(all(not(a), b)))]", expect![[r#"#![cfg(any(a, not(b)))]"#]]); - check_dnf("#![cfg(not(any(a, not(b))))]", expect![[r#"#![cfg(all(not(a), b))]"#]]); - } - - #[test] - fn nested() { - check_dnf( - "#![cfg(all(any(a), not(all(any(b)))))]", - expect![[r#"#![cfg(all(a, not(b)))]"#]], - ); - - check_dnf("#![cfg(any(any(a, b)))]", expect![[r#"#![cfg(any(a, b))]"#]]); - check_dnf("#![cfg(not(any(any(a, b))))]", expect![[r#"#![cfg(all(not(a), not(b)))]"#]]); - check_dnf("#![cfg(all(all(a, b)))]", expect![[r#"#![cfg(all(a, b))]"#]]); - check_dnf("#![cfg(not(all(all(a, b))))]", expect![[r#"#![cfg(any(not(a), not(b)))]"#]]); - } - - #[test] - fn hints() { - let mut opts = CfgOptions::default(); - - check_enable_hints("#![cfg(test)]", &opts, &["enable test"]); - check_enable_hints("#![cfg(not(test))]", &opts, &[]); - - check_enable_hints("#![cfg(any(a, b))]", &opts, &["enable a", "enable b"]); - check_enable_hints("#![cfg(any(b, a))]", &opts, &["enable b", "enable a"]); - - check_enable_hints("#![cfg(all(a, b))]", &opts, &["enable a and b"]); - - opts.insert_atom("test".into()); - - check_enable_hints("#![cfg(test)]", &opts, &[]); - check_enable_hints("#![cfg(not(test))]", &opts, &["disable test"]); - } - - /// Tests that we don't suggest hints for cfgs that express an inconsistent formula. - #[test] - fn hints_impossible() { - let mut opts = CfgOptions::default(); - - check_enable_hints("#![cfg(all(test, not(test)))]", &opts, &[]); - - opts.insert_atom("test".into()); - - check_enable_hints("#![cfg(all(test, not(test)))]", &opts, &[]); - } - - #[test] - fn why_inactive() { - let mut opts = CfgOptions::default(); - opts.insert_atom("test".into()); - opts.insert_atom("test2".into()); - - check_why_inactive("#![cfg(a)]", &opts, expect![["a is disabled"]]); - check_why_inactive("#![cfg(not(test))]", &opts, expect![["test is enabled"]]); - - check_why_inactive( - "#![cfg(all(not(test), not(test2)))]", - &opts, - expect![["test and test2 are enabled"]], - ); - check_why_inactive("#![cfg(all(a, b))]", &opts, expect![["a and b are disabled"]]); - check_why_inactive( - "#![cfg(all(not(test), a))]", - &opts, - expect![["test is enabled and a is disabled"]], - ); - check_why_inactive( - "#![cfg(all(not(test), test2, a))]", - &opts, - expect![["test is enabled and a is disabled"]], - ); - check_why_inactive( - "#![cfg(all(not(test), not(test2), a))]", - &opts, - expect![["test and test2 are enabled and a is disabled"]], - ); - } -} diff --git a/crates/cfg/src/lib.rs b/crates/cfg/src/lib.rs index 28a40a082..d0e08cf5f 100644 --- a/crates/cfg/src/lib.rs +++ b/crates/cfg/src/lib.rs @@ -2,6 +2,8 @@ mod cfg_expr; mod dnf; +#[cfg(test)] +mod tests; use std::fmt; diff --git a/crates/cfg/src/tests.rs b/crates/cfg/src/tests.rs new file mode 100644 index 000000000..bd0f9ec48 --- /dev/null +++ b/crates/cfg/src/tests.rs @@ -0,0 +1,193 @@ +use expect_test::{expect, Expect}; +use mbe::ast_to_token_tree; +use syntax::{ast, AstNode}; + +use crate::{CfgAtom, CfgExpr, CfgOptions, DnfExpr}; + +fn assert_parse_result(input: &str, expected: CfgExpr) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + assert_eq!(cfg, expected); +} + +fn check_dnf(input: &str, expect: Expect) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + let actual = format!("#![cfg({})]", DnfExpr::new(cfg)); + expect.assert_eq(&actual); +} + +fn check_why_inactive(input: &str, opts: &CfgOptions, expect: Expect) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + let dnf = DnfExpr::new(cfg); + let why_inactive = dnf.why_inactive(opts).unwrap().to_string(); + expect.assert_eq(&why_inactive); +} + +#[track_caller] +fn check_enable_hints(input: &str, opts: &CfgOptions, expected_hints: &[&str]) { + let (tt, _) = { + let source_file = ast::SourceFile::parse(input).ok().unwrap(); + let tt = source_file.syntax().descendants().find_map(ast::TokenTree::cast).unwrap(); + ast_to_token_tree(&tt).unwrap() + }; + let cfg = CfgExpr::parse(&tt); + let dnf = DnfExpr::new(cfg); + let hints = dnf.compute_enable_hints(opts).map(|diff| diff.to_string()).collect::>(); + assert_eq!(hints, expected_hints); +} + +#[test] +fn test_cfg_expr_parser() { + assert_parse_result("#![cfg(foo)]", CfgAtom::Flag("foo".into()).into()); + assert_parse_result("#![cfg(foo,)]", CfgAtom::Flag("foo".into()).into()); + assert_parse_result( + "#![cfg(not(foo))]", + CfgExpr::Not(Box::new(CfgAtom::Flag("foo".into()).into())), + ); + assert_parse_result("#![cfg(foo(bar))]", CfgExpr::Invalid); + + // Only take the first + assert_parse_result(r#"#![cfg(foo, bar = "baz")]"#, CfgAtom::Flag("foo".into()).into()); + + assert_parse_result( + r#"#![cfg(all(foo, bar = "baz"))]"#, + CfgExpr::All(vec![ + CfgAtom::Flag("foo".into()).into(), + CfgAtom::KeyValue { key: "bar".into(), value: "baz".into() }.into(), + ]), + ); + + assert_parse_result( + r#"#![cfg(any(not(), all(), , bar = "baz",))]"#, + CfgExpr::Any(vec![ + CfgExpr::Not(Box::new(CfgExpr::Invalid)), + CfgExpr::All(vec![]), + CfgExpr::Invalid, + CfgAtom::KeyValue { key: "bar".into(), value: "baz".into() }.into(), + ]), + ); +} + +#[test] +fn smoke() { + check_dnf("#![cfg(test)]", expect![[r#"#![cfg(test)]"#]]); + check_dnf("#![cfg(not(test))]", expect![[r#"#![cfg(not(test))]"#]]); + check_dnf("#![cfg(not(not(test)))]", expect![[r#"#![cfg(test)]"#]]); + + check_dnf("#![cfg(all(a, b))]", expect![[r#"#![cfg(all(a, b))]"#]]); + check_dnf("#![cfg(any(a, b))]", expect![[r#"#![cfg(any(a, b))]"#]]); + + check_dnf("#![cfg(not(a))]", expect![[r#"#![cfg(not(a))]"#]]); +} + +#[test] +fn distribute() { + check_dnf("#![cfg(all(any(a, b), c))]", expect![[r#"#![cfg(any(all(a, c), all(b, c)))]"#]]); + check_dnf("#![cfg(all(c, any(a, b)))]", expect![[r#"#![cfg(any(all(c, a), all(c, b)))]"#]]); + check_dnf( + "#![cfg(all(any(a, b), any(c, d)))]", + expect![[r#"#![cfg(any(all(a, c), all(a, d), all(b, c), all(b, d)))]"#]], + ); + + check_dnf( + "#![cfg(all(any(a, b, c), any(d, e, f), g))]", + expect![[ + r#"#![cfg(any(all(a, d, g), all(a, e, g), all(a, f, g), all(b, d, g), all(b, e, g), all(b, f, g), all(c, d, g), all(c, e, g), all(c, f, g)))]"# + ]], + ); +} + +#[test] +fn demorgan() { + check_dnf("#![cfg(not(all(a, b)))]", expect![[r#"#![cfg(any(not(a), not(b)))]"#]]); + check_dnf("#![cfg(not(any(a, b)))]", expect![[r#"#![cfg(all(not(a), not(b)))]"#]]); + + check_dnf("#![cfg(not(all(not(a), b)))]", expect![[r#"#![cfg(any(a, not(b)))]"#]]); + check_dnf("#![cfg(not(any(a, not(b))))]", expect![[r#"#![cfg(all(not(a), b))]"#]]); +} + +#[test] +fn nested() { + check_dnf("#![cfg(all(any(a), not(all(any(b)))))]", expect![[r#"#![cfg(all(a, not(b)))]"#]]); + + check_dnf("#![cfg(any(any(a, b)))]", expect![[r#"#![cfg(any(a, b))]"#]]); + check_dnf("#![cfg(not(any(any(a, b))))]", expect![[r#"#![cfg(all(not(a), not(b)))]"#]]); + check_dnf("#![cfg(all(all(a, b)))]", expect![[r#"#![cfg(all(a, b))]"#]]); + check_dnf("#![cfg(not(all(all(a, b))))]", expect![[r#"#![cfg(any(not(a), not(b)))]"#]]); +} + +#[test] +fn hints() { + let mut opts = CfgOptions::default(); + + check_enable_hints("#![cfg(test)]", &opts, &["enable test"]); + check_enable_hints("#![cfg(not(test))]", &opts, &[]); + + check_enable_hints("#![cfg(any(a, b))]", &opts, &["enable a", "enable b"]); + check_enable_hints("#![cfg(any(b, a))]", &opts, &["enable b", "enable a"]); + + check_enable_hints("#![cfg(all(a, b))]", &opts, &["enable a and b"]); + + opts.insert_atom("test".into()); + + check_enable_hints("#![cfg(test)]", &opts, &[]); + check_enable_hints("#![cfg(not(test))]", &opts, &["disable test"]); +} + +/// Tests that we don't suggest hints for cfgs that express an inconsistent formula. +#[test] +fn hints_impossible() { + let mut opts = CfgOptions::default(); + + check_enable_hints("#![cfg(all(test, not(test)))]", &opts, &[]); + + opts.insert_atom("test".into()); + + check_enable_hints("#![cfg(all(test, not(test)))]", &opts, &[]); +} + +#[test] +fn why_inactive() { + let mut opts = CfgOptions::default(); + opts.insert_atom("test".into()); + opts.insert_atom("test2".into()); + + check_why_inactive("#![cfg(a)]", &opts, expect![["a is disabled"]]); + check_why_inactive("#![cfg(not(test))]", &opts, expect![["test is enabled"]]); + + check_why_inactive( + "#![cfg(all(not(test), not(test2)))]", + &opts, + expect![["test and test2 are enabled"]], + ); + check_why_inactive("#![cfg(all(a, b))]", &opts, expect![["a and b are disabled"]]); + check_why_inactive( + "#![cfg(all(not(test), a))]", + &opts, + expect![["test is enabled and a is disabled"]], + ); + check_why_inactive( + "#![cfg(all(not(test), test2, a))]", + &opts, + expect![["test is enabled and a is disabled"]], + ); + check_why_inactive( + "#![cfg(all(not(test), not(test2), a))]", + &opts, + expect![["test and test2 are enabled and a is disabled"]], + ); +} -- cgit v1.2.3