Skip to content

Commit

Permalink
Rollup merge of #111068 - Urgau:check-cfg-improvements, r=petrochenkov
Browse files Browse the repository at this point in the history 
Improve check-cfg implementation

This PR makes multiple improvements into the implementation of check-cfg, it is a prerequisite to a follow-up PR that will introduce a simpler and more explicit syntax.

The 2 main area of improvements are:
 1. Internal representation of expected values:
    - now uses `FxHashSet<Option<Symbol>>` instead of `FxHashSet<Symbol>`, it made the no value expected case only possible when no values where in the `HashSet` which is now represented as `None` (same as cfg represent-it).
    - a enum with `Some` and `Any` makes it now clear if some values are expected or not, necessary for `feature` and `target_feature`.
 2. Diagnostics: Improve the diagnostics in multiple case and fix case where a missing value could have had a new name suggestion instead of the value diagnostic; and some drive by improvements

I highly recommend reviewing commit by commit.

r? `@petrochenkov`
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored May 5, 2023
2 parents 65702bf + 5364784 commit ded0a9e
Show file tree
Hide file tree
Showing 19 changed files with 420 additions and 236 deletions.
39 changes: 20 additions & 19 deletions compiler/rustc_attr/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use rustc_ast::{Attribute, LitKind, MetaItem, MetaItemKind, MetaItemLit, NestedM
use rustc_ast_pretty::pprust;
use rustc_feature::{find_gated_cfg, is_builtin_attr_name, Features, GatedCfg};
use rustc_macros::HashStable_Generic;
use rustc_session::config::ExpectedValues;
use rustc_session::lint::builtin::UNEXPECTED_CFGS;
use rustc_session::lint::BuiltinLintDiagnostics;
use rustc_session::parse::{feature_err, ParseSess};
Expand Down Expand Up @@ -581,32 +582,32 @@ pub fn cfg_matches(
) -> bool {
eval_condition(cfg, sess, features, &mut |cfg| {
try_gate_cfg(cfg.name, cfg.span, sess, features);
if let Some(names_valid) = &sess.check_config.names_valid {
if !names_valid.contains(&cfg.name) {
match sess.check_config.expecteds.get(&cfg.name) {
Some(ExpectedValues::Some(values)) if !values.contains(&cfg.value) => {
sess.buffer_lint_with_diagnostic(
UNEXPECTED_CFGS,
cfg.span,
lint_node_id,
"unexpected `cfg` condition name",
BuiltinLintDiagnostics::UnexpectedCfg((cfg.name, cfg.name_span), None),
"unexpected `cfg` condition value",
BuiltinLintDiagnostics::UnexpectedCfgValue(
(cfg.name, cfg.name_span),
cfg.value.map(|v| (v, cfg.value_span.unwrap())),
),
);
}
}
if let Some(value) = cfg.value {
if let Some(values) = &sess.check_config.values_valid.get(&cfg.name) {
if !values.contains(&value) {
sess.buffer_lint_with_diagnostic(
UNEXPECTED_CFGS,
cfg.span,
lint_node_id,
"unexpected `cfg` condition value",
BuiltinLintDiagnostics::UnexpectedCfg(
(cfg.name, cfg.name_span),
cfg.value_span.map(|vs| (value, vs)),
),
);
}
None if sess.check_config.exhaustive_names => {
sess.buffer_lint_with_diagnostic(
UNEXPECTED_CFGS,
cfg.span,
lint_node_id,
"unexpected `cfg` condition name",
BuiltinLintDiagnostics::UnexpectedCfgName(
(cfg.name, cfg.name_span),
cfg.value.map(|v| (v, cfg.value_span.unwrap())),
),
);
}
_ => { /* not unexpected */ }
}
sess.config.contains(&(cfg.name, cfg.value))
})
Expand Down
76 changes: 48 additions & 28 deletions compiler/rustc_interface/src/interface.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ use rustc_data_structures::OnDrop;
use rustc_errors::registry::Registry;
use rustc_errors::{ErrorGuaranteed, Handler};
use rustc_lint::LintStore;
use rustc_middle::ty;
use rustc_middle::{bug, ty};
use rustc_parse::maybe_new_parser_from_source_str;
use rustc_query_impl::QueryCtxt;
use rustc_query_system::query::print_query_stack;
use rustc_session::config::{self, CheckCfg, ErrorOutputType, Input, OutputFilenames};
use rustc_session::config::{self, ErrorOutputType, Input, OutputFilenames};
use rustc_session::config::{CheckCfg, ExpectedValues};
use rustc_session::lint;
use rustc_session::parse::{CrateConfig, ParseSess};
use rustc_session::Session;
Expand Down Expand Up @@ -121,9 +122,9 @@ pub fn parse_cfgspecs(cfgspecs: Vec<String>) -> FxHashSet<(String, Option<String
/// Converts strings provided as `--check-cfg [specs]` into a `CheckCfg`.
pub fn parse_check_cfg(specs: Vec<String>) -> CheckCfg {
rustc_span::create_default_session_if_not_set_then(move |_| {
let mut cfg = CheckCfg::default();
let mut check_cfg = CheckCfg::default();

'specs: for s in specs {
for s in specs {
let sess = ParseSess::with_silent_emitter(Some(format!(
"this error occurred on the command line: `--check-cfg={s}`"
)));
Expand All @@ -137,76 +138,95 @@ pub fn parse_check_cfg(specs: Vec<String>) -> CheckCfg {
concat!("invalid `--check-cfg` argument: `{}` (", $reason, ")"),
s
),
);
)
};
}

let expected_error = || {
error!(
"expected `names(name1, name2, ... nameN)` or \
`values(name, \"value1\", \"value2\", ... \"valueN\")`"
)
};

match maybe_new_parser_from_source_str(&sess, filename, s.to_string()) {
Ok(mut parser) => match parser.parse_meta_item() {
Ok(meta_item) if parser.token == token::Eof => {
if let Some(args) = meta_item.meta_item_list() {
if meta_item.has_name(sym::names) {
let names_valid =
cfg.names_valid.get_or_insert_with(|| FxHashSet::default());
check_cfg.exhaustive_names = true;
for arg in args {
if arg.is_word() && arg.ident().is_some() {
let ident = arg.ident().expect("multi-segment cfg key");
names_valid.insert(ident.name.to_string());
check_cfg
.expecteds
.entry(ident.name.to_string())
.or_insert(ExpectedValues::Any);
} else {
error!("`names()` arguments must be simple identifiers");
}
}
continue 'specs;
} 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 ident = name.ident().expect("multi-segment cfg key");
let ident_values = cfg
.values_valid
let expected_values = check_cfg
.expecteds
.entry(ident.name.to_string())
.or_insert_with(|| FxHashSet::default());
.or_insert_with(|| {
ExpectedValues::Some(FxHashSet::default())
});

let ExpectedValues::Some(expected_values) = expected_values else {
bug!("shoudn't be possible")
};

for val in values {
if let Some(LitKind::Str(s, _)) =
val.lit().map(|lit| &lit.kind)
{
ident_values.insert(s.to_string());
expected_values.insert(Some(s.to_string()));
} else {
error!(
"`values()` arguments must be string literals"
);
}
}

continue 'specs;
if values.is_empty() {
expected_values.insert(None);
}
} else {
error!(
"`values()` first argument must be a simple identifier"
);
}
} else if args.is_empty() {
cfg.well_known_values = true;
continue 'specs;
check_cfg.exhaustive_values = true;
} else {
expected_error();
}
} else {
expected_error();
}
} else {
expected_error();
}
}
Ok(..) => {}
Err(err) => err.cancel(),
Ok(..) => expected_error(),
Err(err) => {
err.cancel();
expected_error();
}
},
Err(errs) => drop(errs),
Err(errs) => {
drop(errs);
expected_error();
}
}

error!(
"expected `names(name1, name2, ... nameN)` or \
`values(name, \"value1\", \"value2\", ... \"valueN\")`"
);
}

if let Some(names_valid) = &mut cfg.names_valid {
names_valid.extend(cfg.values_valid.keys().cloned());
}
cfg
check_cfg
})
}

Expand Down
20 changes: 10 additions & 10 deletions compiler/rustc_lint/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ use rustc_middle::ty::layout::{LayoutError, LayoutOf};
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::{self, Instance, Ty, TyCtxt, VariantDef};
use rustc_session::config::ExpectedValues;
use rustc_session::lint::{BuiltinLintDiagnostics, FutureIncompatibilityReason};
use rustc_span::edition::Edition;
use rustc_span::source_map::Spanned;
Expand Down Expand Up @@ -3306,16 +3307,15 @@ impl EarlyLintPass for UnexpectedCfgs {
let cfg = &cx.sess().parse_sess.config;
let check_cfg = &cx.sess().parse_sess.check_config;
for &(name, value) in cfg {
if let Some(names_valid) = &check_cfg.names_valid && !names_valid.contains(&name){
cx.emit_lint(UNEXPECTED_CFGS, BuiltinUnexpectedCliConfigName {
name,
});
}
if let Some(value) = value && let Some(values) = check_cfg.values_valid.get(&name) && !values.contains(&value) {
cx.emit_lint(
UNEXPECTED_CFGS,
BuiltinUnexpectedCliConfigValue { name, value },
);
match check_cfg.expecteds.get(&name) {
Some(ExpectedValues::Some(values)) if !values.contains(&value) => {
let value = value.unwrap_or(kw::Empty);
cx.emit_lint(UNEXPECTED_CFGS, BuiltinUnexpectedCliConfigValue { name, value });
}
None if check_cfg.exhaustive_names => {
cx.emit_lint(UNEXPECTED_CFGS, BuiltinUnexpectedCliConfigName { name });
}
_ => { /* expected */ }
}
}
}
Expand Down
70 changes: 54 additions & 16 deletions compiler/rustc_lint/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use rustc_middle::middle::stability;
use rustc_middle::ty::layout::{LayoutError, LayoutOfHelpers, TyAndLayout};
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::{self, print::Printer, subst::GenericArg, RegisteredTools, Ty, TyCtxt};
use rustc_session::config::ExpectedValues;
use rustc_session::lint::{BuiltinLintDiagnostics, LintExpectationId};
use rustc_session::lint::{FutureIncompatibleInfo, Level, Lint, LintBuffer, LintId};
use rustc_session::Session;
Expand Down Expand Up @@ -768,22 +769,52 @@ pub trait LintContext: Sized {
db.help(help);
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((name, name_span), None) => {
let Some(names_valid) = &sess.parse_sess.check_config.names_valid else {
bug!("it shouldn't be possible to have a diagnostic on a name if name checking is not enabled");
};
let possibilities: Vec<Symbol> = names_valid.iter().map(|s| *s).collect();
BuiltinLintDiagnostics::UnexpectedCfgName((name, name_span), value) => {
let possibilities: Vec<Symbol> = sess.parse_sess.check_config.expecteds.keys().map(|s| *s).collect();

// Suggest the most probable if we found one
if let Some(best_match) = find_best_match_for_name(&possibilities, name, None) {
db.span_suggestion(name_span, "did you mean", best_match, Applicability::MaybeIncorrect);
if let Some(ExpectedValues::Some(best_match_values)) =
sess.parse_sess.check_config.expecteds.get(&best_match) {
let mut possibilities = best_match_values.iter()
.flatten()
.map(Symbol::as_str)
.collect::<Vec<_>>();
possibilities.sort();

if let Some((value, value_span)) = value {
if best_match_values.contains(&Some(value)) {
db.span_suggestion(name_span, "there is a config with a similar name and value", best_match, Applicability::MaybeIncorrect);
} else if best_match_values.contains(&None) {
db.span_suggestion(name_span.to(value_span), "there is a config with a similar name and no value", best_match, Applicability::MaybeIncorrect);
} else if let Some(first_value) = possibilities.first() {
db.span_suggestion(name_span.to(value_span), "there is a config with a similar name and different values", format!("{best_match} = \"{first_value}\""), Applicability::MaybeIncorrect);
} else {
db.span_suggestion(name_span.to(value_span), "there is a config with a similar name and different values", best_match, Applicability::MaybeIncorrect);
};
} else {
db.span_suggestion(name_span, "there is a config with a similar name", best_match, Applicability::MaybeIncorrect);
}

if !possibilities.is_empty() {
let possibilities = possibilities.join("`, `");
db.help(format!("expected values for `{best_match}` are: `{possibilities}`"));
}
} else {
db.span_suggestion(name_span, "there is a config with a similar name", best_match, Applicability::MaybeIncorrect);
}
}
},
BuiltinLintDiagnostics::UnexpectedCfg((name, name_span), Some((value, value_span))) => {
let Some(values) = &sess.parse_sess.check_config.values_valid.get(&name) else {
BuiltinLintDiagnostics::UnexpectedCfgValue((name, name_span), value) => {
let Some(ExpectedValues::Some(values)) = &sess.parse_sess.check_config.expecteds.get(&name) else {
bug!("it shouldn't be possible to have a diagnostic on a value whose name is not in values");
};
let possibilities: Vec<Symbol> = values.iter().map(|&s| s).collect();
let mut have_none_possibility = false;
let possibilities: Vec<Symbol> = values.iter()
.inspect(|a| have_none_possibility |= a.is_none())
.copied()
.flatten()
.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
Expand All @@ -792,17 +823,24 @@ pub trait LintContext: Sized {
let mut possibilities = possibilities.iter().map(Symbol::as_str).collect::<Vec<_>>();
possibilities.sort();

let possibilities = possibilities.join(", ");
db.note(format!("expected values for `{name}` are: {possibilities}"));
let possibilities = possibilities.join("`, `");
let none = if have_none_possibility { "(none), " } else { "" };

db.note(format!("expected values for `{name}` are: {none}`{possibilities}`"));
}

// Suggest the most probable if we found one
if let Some(best_match) = find_best_match_for_name(&possibilities, value, None) {
db.span_suggestion(value_span, "did you mean", format!("\"{best_match}\""), Applicability::MaybeIncorrect);
if let Some((value, value_span)) = value {
// Suggest the most probable if we found one
if let Some(best_match) = find_best_match_for_name(&possibilities, value, None) {
db.span_suggestion(value_span, "there is a expected value with a similar name", format!("\"{best_match}\""), Applicability::MaybeIncorrect);

}
} else if let &[first_possibility] = &possibilities[..] {
db.span_suggestion(name_span.shrink_to_hi(), "specify a config value", format!(" = \"{first_possibility}\""), Applicability::MaybeIncorrect);
}
} else {
} else if have_none_possibility {
db.note(format!("no expected value for `{name}`"));
if name != sym::feature {
if let Some((_value, value_span)) = value {
db.span_suggestion(name_span.shrink_to_hi().to(value_span), "remove the value", "", Applicability::MaybeIncorrect);
}
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_lint_defs/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,8 @@ pub enum BuiltinLintDiagnostics {
BreakWithLabelAndLoop(Span),
NamedAsmLabel(String),
UnicodeTextFlow(Span, String),
UnexpectedCfg((Symbol, Span), Option<(Symbol, Span)>),
UnexpectedCfgName((Symbol, Span), Option<(Symbol, Span)>),
UnexpectedCfgValue((Symbol, Span), Option<(Symbol, Span)>),
DeprecatedWhereclauseLocation(Span, String),
SingleUseLifetime {
/// Span of the parameter which declares this lifetime.
Expand Down
Loading

0 comments on commit ded0a9e

Please sign in to comment.