Skip to content

Rollup of 6 pull requests #99242

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.

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

Merged
merged 19 commits into from
Jul 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4514,6 +4514,7 @@ dependencies = [
"rustc_data_structures",
"rustc_errors",
"rustc_hir",
"rustc_index",
"rustc_infer",
"rustc_middle",
"rustc_session",
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_builtin_macros/src/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,8 +534,8 @@ fn expand_preparsed_asm(ecx: &mut ExtCtxt<'_>, args: AsmArgs) -> Option<ast::Inl

let mut template_strs = Vec::with_capacity(args.templates.len());

for template_expr in args.templates.into_iter() {
if !template.is_empty() {
for (i, template_expr) in args.templates.into_iter().enumerate() {
if i != 0 {
template.push(ast::InlineAsmTemplatePiece::String("\n".to_string()));
}

Expand Down
85 changes: 72 additions & 13 deletions compiler/rustc_builtin_macros/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::{InnerSpan, Span};
use smallvec::SmallVec;

use rustc_lint_defs::builtin::NAMED_ARGUMENTS_USED_POSITIONALLY;
use rustc_lint_defs::{BufferedEarlyLint, BuiltinLintDiagnostics, LintId};
use rustc_parse_format::{Count, FormatSpec};
use std::borrow::Cow;
use std::collections::hash_map::Entry;

Expand Down Expand Up @@ -57,7 +60,7 @@ struct Context<'a, 'b> {
/// Unique format specs seen for each argument.
arg_unique_types: Vec<Vec<ArgumentType>>,
/// Map from named arguments to their resolved indices.
names: FxHashMap<Symbol, usize>,
names: FxHashMap<Symbol, (usize, Span)>,

/// The latest consecutive literal strings, or empty if there weren't any.
literal: String,
Expand Down Expand Up @@ -130,9 +133,9 @@ fn parse_args<'a>(
ecx: &mut ExtCtxt<'a>,
sp: Span,
tts: TokenStream,
) -> PResult<'a, (P<ast::Expr>, Vec<P<ast::Expr>>, FxHashMap<Symbol, usize>)> {
) -> PResult<'a, (P<ast::Expr>, Vec<P<ast::Expr>>, FxHashMap<Symbol, (usize, Span)>)> {
let mut args = Vec::<P<ast::Expr>>::new();
let mut names = FxHashMap::<Symbol, usize>::default();
let mut names = FxHashMap::<Symbol, (usize, Span)>::default();

let mut p = ecx.new_parser_from_tts(tts);

Expand Down Expand Up @@ -197,7 +200,7 @@ fn parse_args<'a>(
p.bump();
p.expect(&token::Eq)?;
let e = p.parse_expr()?;
if let Some(prev) = names.get(&ident.name) {
if let Some((prev, _)) = names.get(&ident.name) {
ecx.struct_span_err(e.span, &format!("duplicate argument named `{}`", ident))
.span_label(args[*prev].span, "previously here")
.span_label(e.span, "duplicate argument")
Expand All @@ -210,7 +213,7 @@ fn parse_args<'a>(
// if the input is valid, we can simply append to the positional
// args. And remember the names.
let slot = args.len();
names.insert(ident.name, slot);
names.insert(ident.name, (slot, ident.span));
args.push(e);
}
_ => {
Expand All @@ -222,7 +225,7 @@ fn parse_args<'a>(
);
err.span_label(e.span, "positional arguments must be before named arguments");
for pos in names.values() {
err.span_label(args[*pos].span, "named argument");
err.span_label(args[pos.0].span, "named argument");
}
err.emit();
}
Expand All @@ -242,7 +245,8 @@ impl<'a, 'b> Context<'a, 'b> {
fn resolve_name_inplace(&self, p: &mut parse::Piece<'_>) {
// NOTE: the `unwrap_or` branch is needed in case of invalid format
// arguments, e.g., `format_args!("{foo}")`.
let lookup = |s: &str| *self.names.get(&Symbol::intern(s)).unwrap_or(&0);
let lookup =
|s: &str| self.names.get(&Symbol::intern(s)).unwrap_or(&(0, Span::default())).0;

match *p {
parse::String(_) => {}
Expand Down Expand Up @@ -548,7 +552,7 @@ impl<'a, 'b> Context<'a, 'b> {
match self.names.get(&name) {
Some(&idx) => {
// Treat as positional arg.
self.verify_arg_type(Capture(idx), ty)
self.verify_arg_type(Capture(idx.0), ty)
}
None => {
// For the moment capturing variables from format strings expanded from macros is
Expand All @@ -565,7 +569,7 @@ impl<'a, 'b> Context<'a, 'b> {
};
self.num_captured_args += 1;
self.args.push(self.ecx.expr_ident(span, Ident::new(name, span)));
self.names.insert(name, idx);
self.names.insert(name, (idx, span));
self.verify_arg_type(Capture(idx), ty)
} else {
let msg = format!("there is no argument named `{}`", name);
Expand Down Expand Up @@ -967,14 +971,57 @@ pub fn expand_format_args_nl<'cx>(
expand_format_args_impl(ecx, sp, tts, true)
}

fn lint_named_arguments_used_positionally(
names: FxHashMap<Symbol, (usize, Span)>,
cx: &mut Context<'_, '_>,
unverified_pieces: Vec<parse::Piece<'_>>,
) {
let mut used_argument_names = FxHashSet::<&str>::default();
for piece in unverified_pieces {
if let rustc_parse_format::Piece::NextArgument(a) = piece {
match a.position {
rustc_parse_format::Position::ArgumentNamed(arg_name, _) => {
used_argument_names.insert(arg_name);
}
_ => {}
};
match a.format {
FormatSpec { width: Count::CountIsName(s, _), .. }
| FormatSpec { precision: Count::CountIsName(s, _), .. } => {
used_argument_names.insert(s);
}
_ => {}
};
}
}

for (symbol, (index, span)) in names {
if !used_argument_names.contains(symbol.as_str()) {
let msg = format!("named argument `{}` is not used by name", symbol.as_str());
let arg_span = cx.arg_spans[index];
cx.ecx.buffered_early_lint.push(BufferedEarlyLint {
span: MultiSpan::from_span(span),
msg: msg.clone(),
node_id: ast::CRATE_NODE_ID,
lint_id: LintId::of(&NAMED_ARGUMENTS_USED_POSITIONALLY),
diagnostic: BuiltinLintDiagnostics::NamedArgumentUsedPositionally(
arg_span,
span,
symbol.to_string(),
),
});
}
}
}

/// Take the various parts of `format_args!(efmt, args..., name=names...)`
/// and construct the appropriate formatting expression.
pub fn expand_preparsed_format_args(
ecx: &mut ExtCtxt<'_>,
sp: Span,
efmt: P<ast::Expr>,
args: Vec<P<ast::Expr>>,
names: FxHashMap<Symbol, usize>,
names: FxHashMap<Symbol, (usize, Span)>,
append_newline: bool,
) -> P<ast::Expr> {
// NOTE: this verbose way of initializing `Vec<Vec<ArgumentType>>` is because
Expand Down Expand Up @@ -1073,7 +1120,12 @@ pub fn expand_preparsed_format_args(
.map(|span| fmt_span.from_inner(InnerSpan::new(span.start, span.end)))
.collect();

let named_pos: FxHashSet<usize> = names.values().cloned().collect();
let named_pos: FxHashSet<usize> = names.values().cloned().map(|(i, _)| i).collect();

// Clone `names` because `names` in Context get updated by verify_piece, which includes usages
// of the names of named arguments, resulting in incorrect errors if a name argument is used
// but not declared, such as: `println!("x = {x}");`
let named_arguments = names.clone();

let mut cx = Context {
ecx,
Expand Down Expand Up @@ -1101,9 +1153,11 @@ pub fn expand_preparsed_format_args(
is_literal: parser.is_literal,
};

// This needs to happen *after* the Parser has consumed all pieces to create all the spans
// This needs to happen *after* the Parser has consumed all pieces to create all the spans.
// unverified_pieces is used later to check named argument names are used, so clone each piece.
let pieces = unverified_pieces
.into_iter()
.iter()
.cloned()
.map(|mut piece| {
cx.verify_piece(&piece);
cx.resolve_name_inplace(&mut piece);
Expand Down Expand Up @@ -1265,6 +1319,11 @@ pub fn expand_preparsed_format_args(
}

diag.emit();
} else if cx.invalid_refs.is_empty() && !named_arguments.is_empty() {
// Only check for unused named argument names if there are no other errors to avoid causing
// too much noise in output errors, such as when a named argument is entirely unused.
// We also only need to perform this check if there are actually named arguments.
lint_named_arguments_used_positionally(named_arguments, &mut cx, unverified_pieces);
}

cx.into_expr()
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_expand/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::sync::{self, Lrc};
use rustc_errors::{Applicability, DiagnosticBuilder, ErrorGuaranteed, MultiSpan, PResult};
use rustc_lint_defs::builtin::PROC_MACRO_BACK_COMPAT;
use rustc_lint_defs::BuiltinLintDiagnostics;
use rustc_lint_defs::{BufferedEarlyLint, BuiltinLintDiagnostics};
use rustc_parse::{self, parser, MACRO_ARGUMENTS};
use rustc_session::{parse::ParseSess, Limit, Session, SessionDiagnostic};
use rustc_span::def_id::{CrateNum, DefId, LocalDefId};
Expand Down Expand Up @@ -988,6 +988,8 @@ pub struct ExtCtxt<'a> {
pub expansions: FxHashMap<Span, Vec<String>>,
/// Used for running pre-expansion lints on freshly loaded modules.
pub(super) lint_store: LintStoreExpandDyn<'a>,
/// Used for storing lints generated during expansion, like `NAMED_ARGUMENTS_USED_POSITIONALLY`
pub buffered_early_lint: Vec<BufferedEarlyLint>,
/// When we 'expand' an inert attribute, we leave it
/// in the AST, but insert it here so that we know
/// not to expand it again.
Expand Down Expand Up @@ -1020,6 +1022,7 @@ impl<'a> ExtCtxt<'a> {
force_mode: false,
expansions: FxHashMap::default(),
expanded_inert_attrs: MarkedAttrs::new(),
buffered_early_lint: vec![],
}
}

Expand Down
11 changes: 8 additions & 3 deletions compiler/rustc_infer/src/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use rustc_middle::infer::unify_key::{ConstVarValue, ConstVariableValue};
use rustc_middle::infer::unify_key::{ConstVariableOrigin, ConstVariableOriginKind, ToType};
use rustc_middle::mir::interpret::{ErrorHandled, EvalToValTreeResult};
use rustc_middle::traits::select;
use rustc_middle::ty::abstract_const::AbstractConst;
use rustc_middle::ty::error::{ExpectedFound, TypeError};
use rustc_middle::ty::fold::{TypeFoldable, TypeFolder, TypeSuperFoldable};
use rustc_middle::ty::relate::RelateResult;
Expand Down Expand Up @@ -1651,14 +1652,18 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
unevaluated: ty::Unevaluated<'tcx>,
span: Option<Span>,
) -> EvalToValTreeResult<'tcx> {
let substs = self.resolve_vars_if_possible(unevaluated.substs);
let mut substs = self.resolve_vars_if_possible(unevaluated.substs);
debug!(?substs);

// Postpone the evaluation of constants whose substs depend on inference
// variables
if substs.has_infer_types_or_consts() {
debug!("substs have infer types or consts: {:?}", substs);
return Err(ErrorHandled::TooGeneric);
let ac = AbstractConst::new(self.tcx, unevaluated.shrink());
if let Ok(None) = ac {
substs = InternalSubsts::identity_for_item(self.tcx, unevaluated.def.did);
} else {
return Err(ErrorHandled::TooGeneric);
}
}

let param_env_erased = self.tcx.erase_regions(param_env);
Expand Down
7 changes: 5 additions & 2 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_errors::{Applicability, ErrorGuaranteed, MultiSpan, PResult};
use rustc_expand::base::{ExtCtxt, LintStoreExpand, ResolverExpand};
use rustc_hir::def_id::{StableCrateId, LOCAL_CRATE};
use rustc_hir::definitions::Definitions;
use rustc_lint::{EarlyCheckNode, LintStore};
use rustc_lint::{BufferedEarlyLint, EarlyCheckNode, LintStore};
use rustc_metadata::creader::CStore;
use rustc_metadata::{encode_metadata, EncodedMetadata};
use rustc_middle::arena::Arena;
Expand Down Expand Up @@ -336,12 +336,15 @@ pub fn configure_and_expand(

let lint_store = LintStoreExpandImpl(lint_store);
let mut ecx = ExtCtxt::new(sess, cfg, resolver, Some(&lint_store));

// Expand macros now!
let krate = sess.time("expand_crate", || ecx.monotonic_expander().expand_crate(krate));

// The rest is error reporting

sess.parse_sess.buffered_lints.with_lock(|buffered_lints: &mut Vec<BufferedEarlyLint>| {
buffered_lints.append(&mut ecx.buffered_early_lint);
});

sess.time("check_unused_macros", || {
ecx.check_unused_macros();
});
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_lint/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,18 @@ pub trait LintContext: Sized {
Applicability::MachineApplicable,
);
},
BuiltinLintDiagnostics::NamedArgumentUsedPositionally(positional_arg, named_arg, name) => {
db.span_label(named_arg, "this named argument is only referred to by position in formatting string");
let msg = format!("this formatting argument uses named argument `{}` by position", name);
db.span_label(positional_arg, msg);
db.span_suggestion_verbose(
positional_arg,
"use the named argument by name to avoid ambiguity",
format!("{{{}}}", name),
Applicability::MaybeIncorrect,
);

}
}
// Rewrap `db`, and pass control to the user.
decorate(LintDiagnosticBuilder::new(db));
Expand Down
31 changes: 31 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3292,6 +3292,7 @@ declare_lint_pass! {
TEST_UNSTABLE_LINT,
FFI_UNWIND_CALLS,
REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS,
NAMED_ARGUMENTS_USED_POSITIONALLY,
]
}

Expand Down Expand Up @@ -3996,3 +3997,33 @@ declare_lint! {
"call to foreign functions or function pointers with FFI-unwind ABI",
@feature_gate = sym::c_unwind;
}

declare_lint! {
/// The `named_arguments_used_positionally` lint detects cases where named arguments are only
/// used positionally in format strings. This usage is valid but potentially very confusing.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(named_arguments_used_positionally)]
/// fn main() {
/// let _x = 5;
/// println!("{}", _x = 1); // Prints 1, will trigger lint
///
/// println!("{}", _x); // Prints 5, no lint emitted
/// println!("{_x}", _x = _x); // Prints 5, no lint emitted
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Rust formatting strings can refer to named arguments by their position, but this usage is
/// potentially confusing. In particular, readers can incorrectly assume that the declaration
/// of named arguments is an assignment (which would produce the unit type).
/// For backwards compatibility, this is not a hard error.
pub NAMED_ARGUMENTS_USED_POSITIONALLY,
Warn,
"named arguments in format used positionally"
}
1 change: 1 addition & 0 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ pub enum BuiltinLintDiagnostics {
/// If true, the lifetime will be fully elided.
use_span: Option<(Span, bool)>,
},
NamedArgumentUsedPositionally(Span, Span, String),
}

/// Lints that are buffered up early on in the `Session` before the
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_metadata/src/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use rustc_index::vec::{Idx, IndexVec};
use rustc_middle::metadata::ModChild;
use rustc_middle::middle::exported_symbols::{ExportedSymbol, SymbolExportInfo};
use rustc_middle::mir::interpret::{AllocDecodingSession, AllocDecodingState};
use rustc_middle::thir;
use rustc_middle::ty::codec::TyDecoder;
use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::GeneratorDiagnosticData;
Expand Down Expand Up @@ -638,7 +637,7 @@ impl<'a, 'tcx> Decodable<DecodeContext<'a, 'tcx>> for Span {
}
}

impl<'a, 'tcx> Decodable<DecodeContext<'a, 'tcx>> for &'tcx [thir::abstract_const::Node<'tcx>] {
impl<'a, 'tcx> Decodable<DecodeContext<'a, 'tcx>> for &'tcx [ty::abstract_const::Node<'tcx>] {
fn decode(d: &mut DecodeContext<'a, 'tcx>) -> Self {
ty::codec::RefDecodable::decode(d)
}
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_metadata/src/rmeta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use rustc_middle::metadata::ModChild;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
use rustc_middle::middle::exported_symbols::{ExportedSymbol, SymbolExportInfo};
use rustc_middle::mir;
use rustc_middle::thir;
use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{self, ReprOptions, Ty};
Expand Down Expand Up @@ -361,7 +360,7 @@ define_tables! {
mir_for_ctfe: Table<DefIndex, LazyValue<mir::Body<'static>>>,
promoted_mir: Table<DefIndex, LazyValue<IndexVec<mir::Promoted, mir::Body<'static>>>>,
// FIXME(compiler-errors): Why isn't this a LazyArray?
thir_abstract_const: Table<DefIndex, LazyValue<&'static [thir::abstract_const::Node<'static>]>>,
thir_abstract_const: Table<DefIndex, LazyValue<&'static [ty::abstract_const::Node<'static>]>>,
impl_parent: Table<DefIndex, RawDefId>,
impl_polarity: Table<DefIndex, ty::ImplPolarity>,
constness: Table<DefIndex, hir::Constness>,
Expand Down
Loading