Skip to content

Improve position named arguments lint underline and formatting names #99958

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

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Improve position named arguments lint underline and formatting names #99958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0ea440
Improve position named arguments lint underline and formatting names
PrestonFrom Jul 30, 2022
298acef
Move if-block into closure to reduce duplicate code
PrestonFrom Aug 2, 2022
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
84 changes: 59 additions & 25 deletions compiler/rustc_builtin_macros/src/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use smallvec::SmallVec;

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

Expand Down Expand Up @@ -57,26 +58,45 @@ struct PositionalNamedArg {
replacement: Symbol,
/// The span for the positional named argument (so the lint can point a message to it)
positional_named_arg_span: Span,
has_formatting: bool,
}

impl PositionalNamedArg {
/// Determines what span to replace with the name of the named argument
fn get_span_to_replace(&self, cx: &Context<'_, '_>) -> Option<Span> {
/// Determines:
/// 1) span to be replaced with the name of the named argument and
/// 2) span to be underlined for error messages
fn get_positional_arg_spans(&self, cx: &Context<'_, '_>) -> (Option<Span>, Option<Span>) {
if let Some(inner_span) = &self.inner_span_to_replace {
return Some(
cx.fmtsp.from_inner(InnerSpan { start: inner_span.start, end: inner_span.end }),
);
let span =
cx.fmtsp.from_inner(InnerSpan { start: inner_span.start, end: inner_span.end });
(Some(span), Some(span))
} else if self.ty == PositionalNamedArgType::Arg {
// In the case of a named argument whose position is implicit, there will not be a span
// to replace. Instead, we insert the name after the `{`, which is the first character
// of arg_span.
return cx
.arg_spans
.get(self.cur_piece)
.map(|arg_span| arg_span.with_lo(arg_span.lo() + BytePos(1)).shrink_to_lo());
// In the case of a named argument whose position is implicit, if the argument *has*
// formatting, there will not be a span to replace. Instead, we insert the name after
// the `{`, which will be the first character of arg_span. If the argument does *not*
// have formatting, there may or may not be a span to replace. This is because
// whitespace is allowed in arguments without formatting (such as `format!("{ }", 1);`)
// but is not allowed in arguments with formatting (an error will be generated in cases
// like `format!("{ :1.1}", 1.0f32);`.
// For the message span, if there is formatting, we want to use the opening `{` and the
// next character, which will the `:` indicating the start of formatting. If there is
// not any formatting, we want to underline the entire span.
cx.arg_spans.get(self.cur_piece).map_or((None, None), |arg_span| {
if self.has_formatting {
(
Some(arg_span.with_lo(arg_span.lo() + BytePos(1)).shrink_to_lo()),
Some(arg_span.with_hi(arg_span.lo() + BytePos(2))),
)
} else {
let replace_start = arg_span.lo() + BytePos(1);
let replace_end = arg_span.hi() - BytePos(1);
let to_replace = arg_span.with_lo(replace_start).with_hi(replace_end);
(Some(to_replace), Some(*arg_span))
}
})
} else {
(None, None)
}

None
}
}

Expand Down Expand Up @@ -117,10 +137,18 @@ impl PositionalNamedArgsLint {
cur_piece: usize,
inner_span_to_replace: Option<rustc_parse_format::InnerSpan>,
names: &FxHashMap<Symbol, (usize, Span)>,
has_formatting: bool,
) {
let start_of_named_args = total_args_length - names.len();
if current_positional_arg >= start_of_named_args {
self.maybe_push(format_argument_index, ty, cur_piece, inner_span_to_replace, names)
self.maybe_push(
format_argument_index,
ty,
cur_piece,
inner_span_to_replace,
names,
has_formatting,
)
}
}

Expand All @@ -134,6 +162,7 @@ impl PositionalNamedArgsLint {
cur_piece: usize,
inner_span_to_replace: Option<rustc_parse_format::InnerSpan>,
names: &FxHashMap<Symbol, (usize, Span)>,
has_formatting: bool,
) {
let named_arg = names
.iter()
Expand All @@ -156,6 +185,7 @@ impl PositionalNamedArgsLint {
inner_span_to_replace,
replacement,
positional_named_arg_span,
has_formatting,
});
}
}
Expand Down Expand Up @@ -414,6 +444,9 @@ impl<'a, 'b> Context<'a, 'b> {
PositionalNamedArgType::Precision,
);

let has_precision = arg.format.precision != Count::CountImplied;
let has_width = arg.format.width != Count::CountImplied;

// argument second, if it's an implicit positional parameter
// it's written second, so it should come after width/precision.
let pos = match arg.position {
Expand All @@ -426,6 +459,7 @@ impl<'a, 'b> Context<'a, 'b> {
self.curpiece,
arg_end,
&self.names,
has_precision || has_width,
);

Exact(i)
Expand All @@ -439,6 +473,7 @@ impl<'a, 'b> Context<'a, 'b> {
self.curpiece,
None,
&self.names,
has_precision || has_width,
);
Exact(i)
}
Expand Down Expand Up @@ -529,6 +564,7 @@ impl<'a, 'b> Context<'a, 'b> {
self.curpiece,
*inner_span,
&self.names,
true,
);
self.verify_arg_type(Exact(i), Count);
}
Expand Down Expand Up @@ -1150,24 +1186,22 @@ pub fn expand_format_args_nl<'cx>(

fn create_lints_for_named_arguments_used_positionally(cx: &mut Context<'_, '_>) {
for named_arg in &cx.unused_names_lint.positional_named_args {
let arg_span = named_arg.get_span_to_replace(cx);
let (position_sp_to_replace, position_sp_for_msg) = named_arg.get_positional_arg_spans(cx);

let msg = format!("named argument `{}` is not used by name", named_arg.replacement);
let replacement = match named_arg.ty {
PositionalNamedArgType::Arg => named_arg.replacement.to_string(),
_ => named_arg.replacement.to_string() + "$",
};

cx.ecx.buffered_early_lint.push(BufferedEarlyLint {
span: MultiSpan::from_span(named_arg.positional_named_arg_span),
msg: msg.clone(),
node_id: ast::CRATE_NODE_ID,
lint_id: LintId::of(&NAMED_ARGUMENTS_USED_POSITIONALLY),
diagnostic: BuiltinLintDiagnostics::NamedArgumentUsedPositionally(
arg_span,
named_arg.positional_named_arg_span,
replacement,
),
diagnostic: BuiltinLintDiagnostics::NamedArgumentUsedPositionally {
position_sp_to_replace,
position_sp_for_msg,
named_arg_sp: named_arg.positional_named_arg_span,
named_arg_name: named_arg.replacement.to_string(),
is_formatting_arg: named_arg.ty != PositionalNamedArgType::Arg,
},
});
}
}
Expand Down
17 changes: 11 additions & 6 deletions compiler/rustc_lint/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,13 +856,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");
if let Some(positional_arg) = positional_arg {
let msg = format!("this formatting argument uses named argument `{}` by position", name);
db.span_label(positional_arg, msg);
BuiltinLintDiagnostics::NamedArgumentUsedPositionally{ position_sp_to_replace, position_sp_for_msg, named_arg_sp, named_arg_name, is_formatting_arg} => {
db.span_label(named_arg_sp, "this named argument is referred to by position in formatting string");
if let Some(positional_arg_for_msg) = position_sp_for_msg {
let msg = format!("this formatting argument uses named argument `{}` by position", named_arg_name);
db.span_label(positional_arg_for_msg, msg);
}

if let Some(positional_arg_to_replace) = position_sp_to_replace {
let name = if is_formatting_arg { named_arg_name + "$" } else { named_arg_name };

db.span_suggestion_verbose(
positional_arg,
positional_arg_to_replace,
"use the named argument by name to avoid ambiguity",
name,
Applicability::MaybeIncorrect,
Expand Down
14 changes: 13 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 @@ -467,7 +467,19 @@ pub enum BuiltinLintDiagnostics {
/// If true, the lifetime will be fully elided.
use_span: Option<(Span, bool)>,
},
NamedArgumentUsedPositionally(Option<Span>, Span, String),
NamedArgumentUsedPositionally {
/// Span where the named argument is used by position and will be replaced with the named
/// argument name
position_sp_to_replace: Option<Span>,
/// Span where the named argument is used by position and is used for lint messages
position_sp_for_msg: Option<Span>,
/// Span where the named argument's name is (so we know where to put the warning message)
named_arg_sp: Span,
/// String containing the named arguments name
named_arg_name: String,
/// Indicates if the named argument is used as a width/precision for formatting
is_formatting_arg: bool,
},
}

/// Lints that are buffered up early on in the `Session` before the
Expand Down
36 changes: 18 additions & 18 deletions src/test/ui/macros/issue-98466.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ warning: named argument `_x` is not used by name
--> $DIR/issue-98466.rs:7:26
|
LL | println!("_x is {}", _x = 5);
| - ^^ this named argument is only referred to by position in formatting string
| |
| this formatting argument uses named argument `_x` by position
| -- ^^ this named argument is referred to by position in formatting string
| |
| this formatting argument uses named argument `_x` by position
|
= note: `#[warn(named_arguments_used_positionally)]` on by default
help: use the named argument by name to avoid ambiguity
Expand All @@ -16,9 +16,9 @@ warning: named argument `y` is not used by name
--> $DIR/issue-98466.rs:10:26
|
LL | println!("_x is {}", y = _x);
| - ^ this named argument is only referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
| -- ^ this named argument is referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
|
help: use the named argument by name to avoid ambiguity
|
Expand All @@ -29,9 +29,9 @@ warning: named argument `y` is not used by name
--> $DIR/issue-98466.rs:13:83
|
LL | println!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x);
| - ^ this named argument is only referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
| -- ^ this named argument is referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
|
help: use the named argument by name to avoid ambiguity
|
Expand All @@ -42,9 +42,9 @@ warning: named argument `_x` is not used by name
--> $DIR/issue-98466.rs:19:34
|
LL | let _f = format!("_x is {}", _x = 5);
| - ^^ this named argument is only referred to by position in formatting string
| |
| this formatting argument uses named argument `_x` by position
| -- ^^ this named argument is referred to by position in formatting string
| |
| this formatting argument uses named argument `_x` by position
|
help: use the named argument by name to avoid ambiguity
|
Expand All @@ -55,9 +55,9 @@ warning: named argument `y` is not used by name
--> $DIR/issue-98466.rs:22:34
|
LL | let _f = format!("_x is {}", y = _x);
| - ^ this named argument is only referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
| -- ^ this named argument is referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
|
help: use the named argument by name to avoid ambiguity
|
Expand All @@ -68,9 +68,9 @@ warning: named argument `y` is not used by name
--> $DIR/issue-98466.rs:25:91
|
LL | let _f = format!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x);
| - ^ this named argument is only referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
| -- ^ this named argument is referred to by position in formatting string
| |
| this formatting argument uses named argument `y` by position
|
help: use the named argument by name to avoid ambiguity
|
Expand Down
Loading