Continue improvements on the --check-cfg implementation

Urgau · Urgau · commit 8d3de56da1eb · 2022-02-23T13:22:23.000+01:00
- Test the combinations of --check-cfg with partial values() and --cfg
- Test that we detect unexpected value when none are expected
diff --git a/compiler/rustc_attr/src/builtin.rs b/compiler/rustc_attr/src/builtin.rs
@@ -462,7 +462,8 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat
                 true
             }
             MetaItemKind::NameValue(..) | MetaItemKind::Word => {
-                let name = cfg.ident().expect("multi-segment cfg predicate").name;
+                let ident = cfg.ident().expect("multi-segment cfg predicate");
+                let name = ident.name;
                 let value = cfg.value_str();
                 if let Some(names_valid) = &sess.check_config.names_valid {
                     if !names_valid.contains(&name) {
@@ -471,30 +472,24 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat
                             cfg.span,
                             CRATE_NODE_ID,
                             "unexpected `cfg` condition name",
-                            BuiltinLintDiagnostics::UnexpectedCfg(
-                                cfg.ident().unwrap().span,
-                                name,
-                                None,
-                            ),
+                            BuiltinLintDiagnostics::UnexpectedCfg(ident.span, name, None),
                         );
                     }
                 }
-                if let Some(val) = value {
-                    if let Some(values_valid) = &sess.check_config.values_valid {
-                        if let Some(values) = values_valid.get(&name) {
-                            if !values.contains(&val) {
-                                sess.buffer_lint_with_diagnostic(
-                                    UNEXPECTED_CFGS,
-                                    cfg.span,
-                                    CRATE_NODE_ID,
-                                    "unexpected `cfg` condition value",
-                                    BuiltinLintDiagnostics::UnexpectedCfg(
-                                        cfg.name_value_literal_span().unwrap(),
-                                        name,
-                                        Some(val),
-                                    ),
-                                );
-                            }
+                if let Some(value) = value {
+                    if let Some(values) = &sess.check_config.values_valid.get(&name) {
+                        if !values.contains(&value) {
+                            sess.buffer_lint_with_diagnostic(
+                                UNEXPECTED_CFGS,
+                                cfg.span,
+                                CRATE_NODE_ID,
+                                "unexpected `cfg` condition value",
+                                BuiltinLintDiagnostics::UnexpectedCfg(
+                                    cfg.name_value_literal_span().unwrap(),
+                                    name,
+                                    Some(value),
+                                ),
+                            );
                         }
                     }
                 }
diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs
@@ -183,12 +183,10 @@ pub fn parse_check_cfg(specs: Vec<String>) -> CheckCfg {
                             } else if meta_item.has_name(sym::values) {
                                 if let Some((name, values)) = args.split_first() {
                                     if name.is_word() && name.ident().is_some() {
-                                        let values_valid = cfg
-                                            .values_valid
-                                            .get_or_insert_with(|| FxHashMap::default());
                                         let ident = name.ident().expect("multi-segment cfg key");
-                                        let ident_values = values_valid
-                                            .entry(ident.to_string())
+                                        let ident_values = cfg
+                                            .values_valid
+                                            .entry(ident.name.to_string())
                                             .or_insert_with(|| FxHashSet::default());
 
                                         for val in values {
@@ -225,10 +223,8 @@ pub fn parse_check_cfg(specs: Vec<String>) -> CheckCfg {
             );
         }
 
-        if let Some(values_valid) = &cfg.values_valid {
-            if let Some(names_valid) = &mut cfg.names_valid {
-                names_valid.extend(values_valid.keys().cloned());
-            }
+        if let Some(names_valid) = &mut cfg.names_valid {
+            names_valid.extend(cfg.values_valid.keys().cloned());
         }
         cfg
     })
diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs
@@ -768,35 +768,36 @@ pub trait LintContext: Sized {
                     db.note("see the asm section of Rust By Example <https://doc.rust-lang.org/nightly/rust-by-example/unsafe/asm.html#labels> for more information");
                 },
                 BuiltinLintDiagnostics::UnexpectedCfg(span, name, value) => {
-                    let mut possibilities: Vec<Symbol> = if value.is_some() {
-                        let Some(values_valid) = &sess.parse_sess.check_config.values_valid else {
-                            bug!("it shouldn't be possible to have a diagnostic on a value if values checking is not enable");
-                        };
-                        let Some(values) = values_valid.get(&name) else {
+                    let possibilities: Vec<Symbol> = if value.is_some() {
+                        let Some(values) = &sess.parse_sess.check_config.values_valid.get(&name) else {
                             bug!("it shouldn't be possible to have a diagnostic on a value whose name is not in values");
                         };
                         values.iter().map(|&s| s).collect()
                     } else {
                         let Some(names_valid) = &sess.parse_sess.check_config.names_valid else {
-                            bug!("it shouldn't be possible to have a diagnostic on a value if values checking is not enable");
+                            bug!("it shouldn't be possible to have a diagnostic on a name if name checking is not enabled");
                         };
                         names_valid.iter().map(|s| *s).collect()
                     };
 
                     // Show the full list if all possible values for a given name, but don't do it
                     // for names as the possibilities could be very long
                     if value.is_some() {
-                        // Sorting can take some time, so we only do it if required
-                        possibilities.sort();
+                        if !possibilities.is_empty() {
+                            let mut possibilities = possibilities.iter().map(Symbol::as_str).collect::<Vec<_>>();
+                            possibilities.sort();
 
-                        let possibilities = possibilities.iter().map(Symbol::as_str).intersperse(", ").collect::<String>();
-                        db.note(&format!("possible values for `{name}` are: {possibilities}"));
+                            let possibilities = possibilities.join(", ");
+                            db.note(&format!("expected values for `{name}` are: {possibilities}"));
+                        } else {
+                            db.note(&format!("no expected value for `{name}`"));
+                        }
                     }
 
                     // Suggest the most probable if we found one
                     if let Some(best_match) = find_best_match_for_name(&possibilities, value.unwrap_or(name), None) {
-                        let ponctuation = if value.is_some() { "\"" } else { "" };
-                        db.span_suggestion(span, "did you mean", format!("{ponctuation}{best_match}{ponctuation}"), Applicability::MaybeIncorrect);
+                        let punctuation = if value.is_some() { "\"" } else { "" };
+                        db.span_suggestion(span, "did you mean", format!("{punctuation}{best_match}{punctuation}"), Applicability::MaybeIncorrect);
                     }
                 },
             }
diff --git a/compiler/rustc_session/src/config.rs b/compiler/rustc_session/src/config.rs
@@ -1023,10 +1023,10 @@ pub fn to_crate_config(cfg: FxHashSet<(String, Option<String>)>) -> CrateConfig
 
 /// The parsed `--check-cfg` options
 pub struct CheckCfg<T = String> {
-    /// The set of all `names()`, if none no names checking is performed
+    /// The set of all `names()`, if None no name checking is performed
     pub names_valid: Option<FxHashSet<T>>,
-    /// The set of all `values()`, if none no values chcking is performed
-    pub values_valid: Option<FxHashMap<T, FxHashSet<T>>>,
+    /// The set of all `values()`
+    pub values_valid: FxHashMap<T, FxHashSet<T>>,
 }
 
 impl<T> Default for CheckCfg<T> {
@@ -1042,9 +1042,11 @@ impl<T> CheckCfg<T> {
                 .names_valid
                 .as_ref()
                 .map(|names_valid| names_valid.iter().map(|a| f(a)).collect()),
-            values_valid: self.values_valid.as_ref().map(|values_valid| {
-                values_valid.iter().map(|(a, b)| (f(a), b.iter().map(|b| f(b)).collect())).collect()
-            }),
+            values_valid: self
+                .values_valid
+                .iter()
+                .map(|(a, b)| (f(a), b.iter().map(|b| f(b)).collect()))
+                .collect(),
         }
     }
 }
@@ -1098,11 +1100,9 @@ impl CrateCheckConfig {
                 names_valid.insert(k);
             }
             if let Some(v) = v {
-                if let Some(values_valid) = &mut self.values_valid {
-                    values_valid.entry(k).and_modify(|values| {
-                        values.insert(v);
-                    });
-                }
+                self.values_valid.entry(k).and_modify(|values| {
+                    values.insert(v);
+                });
             }
         }
     }
diff --git a/src/test/ui/check-cfg/invalid-cfg-value.stderr b/src/test/ui/check-cfg/invalid-cfg-value.stderr
@@ -5,7 +5,7 @@ LL | #[cfg(feature = "sedre")]
    |       ^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unexpected_cfgs)]` on by default
-   = note: possible values for `feature` are: rand, serde, full
+   = note: expected values for `feature` are: full, rand, serde
 
 warning: 1 warning emitted
 
diff --git a/src/test/ui/check-cfg/mix.rs b/src/test/ui/check-cfg/mix.rs
@@ -0,0 +1,50 @@
+// This test checks the combination of well known names, their activation via names(), the usage of
+// partial values() with a --cfg and test that we also correctly lint on the `cfg!` macro and
+// `cfg_attr` attribute.
+//
+// check-pass
+// compile-flags: --check-cfg=names() --check-cfg=values(feature,"foo") --cfg feature="bar" -Z unstable-options
+
+#[cfg(windows)]
+fn do_windows_stuff() {}
+
+#[cfg(widnows)]
+//~^ WARNING unexpected `cfg` condition name
+fn do_windows_stuff() {}
+
+#[cfg(feature = "foo")]
+fn use_foo() {}
+
+#[cfg(feature = "bar")]
+fn use_bar() {}
+
+#[cfg(feature = "zebra")]
+//~^ WARNING unexpected `cfg` condition value
+fn use_zebra() {}
+
+#[cfg_attr(uu, test)]
+//~^ WARNING unexpected `cfg` condition name
+fn do_test() {}
+
+#[cfg_attr(feature = "foo", no_mangle)]
+fn do_test_foo() {}
+
+fn test_cfg_macro() {
+    cfg!(windows);
+    cfg!(widnows);
+    //~^ WARNING unexpected `cfg` condition name
+    cfg!(feature = "foo");
+    cfg!(feature = "bar");
+    cfg!(feature = "zebra");
+    //~^ WARNING unexpected `cfg` condition value
+    cfg!(xxx = "foo");
+    //~^ WARNING unexpected `cfg` condition name
+    cfg!(xxx);
+    //~^ WARNING unexpected `cfg` condition name
+    cfg!(any(xxx, windows));
+    //~^ WARNING unexpected `cfg` condition name
+    cfg!(any(feature = "bad", windows));
+    //~^ WARNING unexpected `cfg` condition value
+}
+
+fn main() {}
diff --git a/src/test/ui/check-cfg/mix.stderr b/src/test/ui/check-cfg/mix.stderr
@@ -0,0 +1,66 @@
+warning: unexpected `cfg` condition name
+  --> $DIR/mix.rs:11:7
+   |
+LL | #[cfg(widnows)]
+   |       ^^^^^^^ help: did you mean: `windows`
+   |
+   = note: `#[warn(unexpected_cfgs)]` on by default
+
+warning: unexpected `cfg` condition value
+  --> $DIR/mix.rs:21:7
+   |
+LL | #[cfg(feature = "zebra")]
+   |       ^^^^^^^^^^^^^^^^^
+   |
+   = note: expected values for `feature` are: bar, foo
+
+warning: unexpected `cfg` condition name
+  --> $DIR/mix.rs:25:12
+   |
+LL | #[cfg_attr(uu, test)]
+   |            ^^
+
+warning: unexpected `cfg` condition name
+  --> $DIR/mix.rs:34:10
+   |
+LL |     cfg!(widnows);
+   |          ^^^^^^^ help: did you mean: `windows`
+
+warning: unexpected `cfg` condition value
+  --> $DIR/mix.rs:38:10
+   |
+LL |     cfg!(feature = "zebra");
+   |          ^^^^^^^^^^^^^^^^^
+   |
+   = note: expected values for `feature` are: bar, foo
+
+warning: unexpected `cfg` condition name
+  --> $DIR/mix.rs:40:10
+   |
+LL |     cfg!(xxx = "foo");
+   |          ^^^^^^^^^^^
+
+warning: unexpected `cfg` condition name
+  --> $DIR/mix.rs:42:10
+   |
+LL |     cfg!(xxx);
+   |          ^^^
+
+warning: unexpected `cfg` condition name
+  --> $DIR/mix.rs:44:14
+   |
+LL |     cfg!(any(xxx, windows));
+   |              ^^^
+
+warning: unexpected `cfg` condition value
+  --> $DIR/mix.rs:46:14
+   |
+LL |     cfg!(any(feature = "bad", windows));
+   |              ^^^^^^^^^^-----
+   |                        |
+   |                        help: did you mean: `"bar"`
+   |
+   = note: expected values for `feature` are: bar, foo
+
+warning: 9 warnings emitted
+
diff --git a/src/test/ui/check-cfg/no-values.rs b/src/test/ui/check-cfg/no-values.rs
@@ -0,0 +1,10 @@
+// Check that we detect unexpected value when none are allowed
+//
+// check-pass
+// compile-flags: --check-cfg=values(feature) -Z unstable-options
+
+#[cfg(feature = "foo")]
+//~^ WARNING unexpected `cfg` condition value
+fn do_foo() {}
+
+fn main() {}
diff --git a/src/test/ui/check-cfg/no-values.stderr b/src/test/ui/check-cfg/no-values.stderr
@@ -0,0 +1,11 @@
+warning: unexpected `cfg` condition value
+  --> $DIR/no-values.rs:6:7
+   |
+LL | #[cfg(feature = "foo")]
+   |       ^^^^^^^^^^^^^^^
+   |
+   = note: `#[warn(unexpected_cfgs)]` on by default
+   = note: no expected value for `feature`
+
+warning: 1 warning emitted
+

Original file line number	Diff line number	Diff line change
`@@ -1023,10 +1023,10 @@ pub fn to_crate_config(cfg: FxHashSet<(String, Option<String>)>) -> CrateConfig`
`1023`	`1023`
`1024`	`1024`	/// The parsed `--check-cfg` options
`1025`	`1025`	`pub struct CheckCfg<T = String> {`
`1026`		- /// The set of all `names()`, if none no names checking is performed
	`1026`	+ /// The set of all `names()`, if None no name checking is performed
`1027`	`1027`	`pub names_valid: Option<FxHashSet<T>>,`
`1028`		- /// The set of all `values()`, if none no values chcking is performed
`1029`		`- pub values_valid: Option<FxHashMap<T, FxHashSet<T>>>,`
	`1028`	+ /// The set of all `values()`
	`1029`	`+ pub values_valid: FxHashMap<T, FxHashSet<T>>,`
`1030`	`1030`	`}`
`1031`	`1031`
`1032`	`1032`	`impl<T> Default for CheckCfg<T> {`
`@@ -1042,9 +1042,11 @@ impl<T> CheckCfg<T> {`
`1042`	`1042`	`.names_valid`
`1043`	`1043`	`.as_ref()`
`1044`	`1044`	`.map(\|names_valid\| names_valid.iter().map(\|a\| f(a)).collect()),`
`1045`		`- values_valid: self.values_valid.as_ref().map(\|values_valid\| {`
`1046`		`- values_valid.iter().map(\|(a, b)\| (f(a), b.iter().map(\|b\| f(b)).collect())).collect()`
`1047`		`- }),`
	`1045`	`+ values_valid: self`
	`1046`	`+ .values_valid`
	`1047`	`+ .iter()`
	`1048`	`+ .map(\|(a, b)\| (f(a), b.iter().map(\|b\| f(b)).collect()))`
	`1049`	`+ .collect(),`
`1048`	`1050`	`}`
`1049`	`1051`	`}`
`1050`	`1052`	`}`
`@@ -1098,11 +1100,9 @@ impl CrateCheckConfig {`
`1098`	`1100`	`names_valid.insert(k);`
`1099`	`1101`	`}`
`1100`	`1102`	`if let Some(v) = v {`
`1101`		`- if let Some(values_valid) = &mut self.values_valid {`
`1102`		`- values_valid.entry(k).and_modify(\|values\| {`
`1103`		`- values.insert(v);`
`1104`		`- });`
`1105`		`- }`
	`1103`	`+ self.values_valid.entry(k).and_modify(\|values\| {`
	`1104`	`+ values.insert(v);`
	`1105`	`+ });`
`1106`	`1106`	`}`
`1107`	`1107`	`}`
`1108`	`1108`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ LL \| #[cfg(feature = "sedre")]`
`5`	`5`	`\| ^^^^^^^^^^^^^^^^^`
`6`	`6`	`\|`
`7`	`7`	= note: `#[warn(unexpected_cfgs)]` on by default
`8`		- = note: possible values for `feature` are: rand, serde, full
	`8`	+ = note: expected values for `feature` are: full, rand, serde
`9`	`9`
`10`	`10`	`warning: 1 warning emitted`
`11`	`11`