Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve check-cfg implementation #111068

Merged
merged 4 commits into from
May 5, 2023
Merged

Improve check-cfg implementation #111068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad6f4b7
Use explicit instead of implicit control-flow for check-cfg parsing
Urgau Apr 30, 2023
d327d5b
Improve internal representation of check-cfg
Urgau Apr 30, 2023
a5f8dba
Improve check-cfg diagnostics (part 1)
Urgau Apr 30, 2023
5364784
Improve check-cfg diagnostics (part 2)
Urgau Apr 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 20 additions & 19 deletions compiler/rustc_attr/src/builtin.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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 {
Urgau marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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