Skip to content

Commit d0ea440

Browse files
PrestonFromPrestonFrom
committed
Improve position named arguments lint underline and formatting names
For named arguments used as implicit position arguments, underline both the opening curly brace and either: * if there is formatting, the next character (which will either be the closing curl brace or the `:` denoting the start of formatting args) * if there is no formatting, the entire arg span (important if there is whitespace like `{ }`) This should make it more obvious where the named argument should be. Additionally, in the lint message, emit the formatting argument names without a dollar sign to avoid potentially confusion. Fixes #99907
1 parent 9d5cd21 commit d0ea440

File tree

8 files changed

+305
-136
lines changed

8 files changed

+305
-136
lines changed

‎compiler/rustc_builtin_macros/src/format.rs‎

Lines changed: 61 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use smallvec::SmallVec;
1616

1717
use rustc_lint_defs::builtin::NAMED_ARGUMENTS_USED_POSITIONALLY;
1818
use rustc_lint_defs::{BufferedEarlyLint, BuiltinLintDiagnostics, LintId};
19+
use rustc_parse_format::Count;
1920
use std::borrow::Cow;
2021
use std::collections::hash_map::Entry;
2122

@@ -57,26 +58,47 @@ struct PositionalNamedArg {
5758
replacement: Symbol,
5859
/// The span for the positional named argument (so the lint can point a message to it)
5960
positional_named_arg_span: Span,
61+
has_formatting: bool,
6062
}
6163

6264
impl PositionalNamedArg {
63-
/// Determines what span to replace with the name of the named argument
64-
fn get_span_to_replace(&self, cx: &Context<'_, '_>) -> Option<Span> {
65+
/// Determines:
66+
/// 1) span to be replaced with the name of the named argument and
67+
/// 2) span to be underlined for error messages
68+
fn get_positional_arg_spans(&self, cx: &Context<'_, '_>) -> (Option<Span>, Option<Span>) {
6569
if let Some(inner_span) = &self.inner_span_to_replace {
66-
return Some(
67-
cx.fmtsp.from_inner(InnerSpan { start: inner_span.start, end: inner_span.end }),
68-
);
70+
let span =
71+
cx.fmtsp.from_inner(InnerSpan { start: inner_span.start, end: inner_span.end });
72+
(Some(span), Some(span))
6973
} else if self.ty == PositionalNamedArgType::Arg {
70-
// In the case of a named argument whose position is implicit, there will not be a span
71-
// to replace. Instead, we insert the name after the `{`, which is the first character
72-
// of arg_span.
73-
return cx
74-
.arg_spans
75-
.get(self.cur_piece)
76-
.map(|arg_span| arg_span.with_lo(arg_span.lo() + BytePos(1)).shrink_to_lo());
74+
// In the case of a named argument whose position is implicit, if the argument *has*
75+
// formatting, there will not be a span to replace. Instead, we insert the name after
76+
// the `{`, which will be the first character of arg_span. If the argument does *not*
77+
// have formatting, there may or may not be a span to replace. This is because
78+
// whitespace is allowed in arguments without formatting (such as `format!("{ }", 1);`)
79+
// but is not allowed in arguments with formatting (an error will be generated in cases
80+
// like `format!("{ :1.1}", 1.0f32);`.
81+
// For the message span, if there is formatting, we want to use the opening `{` and the
82+
// next character, which will the `:` indicating the start of formatting. If there is
83+
// not any formatting, we want to underline the entire span.
84+
if self.has_formatting {
85+
cx.arg_spans.get(self.cur_piece).map_or((None, None), |arg_span| {
86+
(
87+
Some(arg_span.with_lo(arg_span.lo() + BytePos(1)).shrink_to_lo()),
88+
Some(arg_span.with_hi(arg_span.lo() + BytePos(2))),
89+
)
90+
})
91+
} else {
92+
cx.arg_spans.get(self.cur_piece).map_or((None, None), |arg_span| {
93+
let replace_start = arg_span.lo() + BytePos(1);
94+
let replace_end = arg_span.hi() - BytePos(1);
95+
let to_replace = arg_span.with_lo(replace_start).with_hi(replace_end);
96+
(Some(to_replace), Some(*arg_span))
97+
})
98+
}
99+
} else {
100+
(None, None)
77101
}
78-
79-
None
80102
}
81103
}
82104

@@ -117,10 +139,18 @@ impl PositionalNamedArgsLint {
117139
cur_piece: usize,
118140
inner_span_to_replace: Option<rustc_parse_format::InnerSpan>,
119141
names: &FxHashMap<Symbol, (usize, Span)>,
142+
has_formatting: bool,
120143
) {
121144
let start_of_named_args = total_args_length - names.len();
122145
if current_positional_arg >= start_of_named_args {
123-
self.maybe_push(format_argument_index, ty, cur_piece, inner_span_to_replace, names)
146+
self.maybe_push(
147+
format_argument_index,
148+
ty,
149+
cur_piece,
150+
inner_span_to_replace,
151+
names,
152+
has_formatting,
153+
)
124154
}
125155
}
126156

@@ -134,6 +164,7 @@ impl PositionalNamedArgsLint {
134164
cur_piece: usize,
135165
inner_span_to_replace: Option<rustc_parse_format::InnerSpan>,
136166
names: &FxHashMap<Symbol, (usize, Span)>,
167+
has_formatting: bool,
137168
) {
138169
let named_arg = names
139170
.iter()
@@ -156,6 +187,7 @@ impl PositionalNamedArgsLint {
156187
inner_span_to_replace,
157188
replacement,
158189
positional_named_arg_span,
190+
has_formatting,
159191
});
160192
}
161193
}
@@ -414,6 +446,9 @@ impl<'a, 'b> Context<'a, 'b> {
414446
PositionalNamedArgType::Precision,
415447
);
416448

449+
let has_precision = arg.format.precision != Count::CountImplied;
450+
let has_width = arg.format.width != Count::CountImplied;
451+
417452
// argument second, if it's an implicit positional parameter
418453
// it's written second, so it should come after width/precision.
419454
let pos = match arg.position {
@@ -426,6 +461,7 @@ impl<'a, 'b> Context<'a, 'b> {
426461
self.curpiece,
427462
arg_end,
428463
&self.names,
464+
has_precision || has_width,
429465
);
430466

431467
Exact(i)
@@ -439,6 +475,7 @@ impl<'a, 'b> Context<'a, 'b> {
439475
self.curpiece,
440476
None,
441477
&self.names,
478+
has_precision || has_width,
442479
);
443480
Exact(i)
444481
}
@@ -529,6 +566,7 @@ impl<'a, 'b> Context<'a, 'b> {
529566
self.curpiece,
530567
*inner_span,
531568
&self.names,
569+
true,
532570
);
533571
self.verify_arg_type(Exact(i), Count);
534572
}
@@ -1150,24 +1188,22 @@ pub fn expand_format_args_nl<'cx>(
11501188

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

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

11611195
cx.ecx.buffered_early_lint.push(BufferedEarlyLint {
11621196
span: MultiSpan::from_span(named_arg.positional_named_arg_span),
11631197
msg: msg.clone(),
11641198
node_id: ast::CRATE_NODE_ID,
11651199
lint_id: LintId::of(&NAMED_ARGUMENTS_USED_POSITIONALLY),
1166-
diagnostic: BuiltinLintDiagnostics::NamedArgumentUsedPositionally(
1167-
arg_span,
1168-
named_arg.positional_named_arg_span,
1169-
replacement,
1170-
),
1200+
diagnostic: BuiltinLintDiagnostics::NamedArgumentUsedPositionally {
1201+
position_sp_to_replace,
1202+
position_sp_for_msg,
1203+
named_arg_sp: named_arg.positional_named_arg_span,
1204+
named_arg_name: named_arg.replacement.to_string(),
1205+
is_formatting_arg: named_arg.ty != PositionalNamedArgType::Arg,
1206+
},
11711207
});
11721208
}
11731209
}

‎compiler/rustc_lint/src/context.rs‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -856,13 +856,18 @@ pub trait LintContext: Sized {
856856
Applicability::MachineApplicable,
857857
);
858858
},
859-
BuiltinLintDiagnostics::NamedArgumentUsedPositionally(positional_arg, named_arg, name) => {
860-
db.span_label(named_arg, "this named argument is only referred to by position in formatting string");
861-
if let Some(positional_arg) = positional_arg {
862-
let msg = format!("this formatting argument uses named argument `{}` by position", name);
863-
db.span_label(positional_arg, msg);
859+
BuiltinLintDiagnostics::NamedArgumentUsedPositionally{ position_sp_to_replace, position_sp_for_msg, named_arg_sp, named_arg_name, is_formatting_arg} => {
860+
db.span_label(named_arg_sp, "this named argument is referred to by position in formatting string");
861+
if let Some(positional_arg_for_msg) = position_sp_for_msg {
862+
let msg = format!("this formatting argument uses named argument `{}` by position", named_arg_name);
863+
db.span_label(positional_arg_for_msg, msg);
864+
}
865+
866+
if let Some(positional_arg_to_replace) = position_sp_to_replace {
867+
let name = if is_formatting_arg { named_arg_name + "$" } else { named_arg_name };
868+
864869
db.span_suggestion_verbose(
865-
positional_arg,
870+
positional_arg_to_replace,
866871
"use the named argument by name to avoid ambiguity",
867872
name,
868873
Applicability::MaybeIncorrect,

‎compiler/rustc_lint_defs/src/lib.rs‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,19 @@ pub enum BuiltinLintDiagnostics {
467467
/// If true, the lifetime will be fully elided.
468468
use_span: Option<(Span, bool)>,
469469
},
470-
NamedArgumentUsedPositionally(Option<Span>, Span, String),
470+
NamedArgumentUsedPositionally {
471+
/// Span where the named argument is used by position and will be replaced with the named
472+
/// argument name
473+
position_sp_to_replace: Option<Span>,
474+
/// Span where the named argument is used by position and is used for lint messages
475+
position_sp_for_msg: Option<Span>,
476+
/// Span where the named argument's name is (so we know where to put the warning message)
477+
named_arg_sp: Span,
478+
/// String containing the named arguments name
479+
named_arg_name: String,
480+
/// Indicates if the named argument is used as a width/precision for formatting
481+
is_formatting_arg: bool,
482+
},
471483
}
472484

473485
/// Lints that are buffered up early on in the `Session` before the

‎src/test/ui/macros/issue-98466.stderr‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ warning: named argument `_x` is not used by name
22
--> $DIR/issue-98466.rs:7:26
33
|
44
LL | println!("_x is {}", _x = 5);
5-
| - ^^ this named argument is only referred to by position in formatting string
6-
| |
7-
| this formatting argument uses named argument `_x` by position
5+
| -- ^^ this named argument is referred to by position in formatting string
6+
| |
7+
| this formatting argument uses named argument `_x` by position
88
|
99
= note: `#[warn(named_arguments_used_positionally)]` on by default
1010
help: use the named argument by name to avoid ambiguity
@@ -16,9 +16,9 @@ warning: named argument `y` is not used by name
1616
--> $DIR/issue-98466.rs:10:26
1717
|
1818
LL | println!("_x is {}", y = _x);
19-
| - ^ this named argument is only referred to by position in formatting string
20-
| |
21-
| this formatting argument uses named argument `y` by position
19+
| -- ^ this named argument is referred to by position in formatting string
20+
| |
21+
| this formatting argument uses named argument `y` by position
2222
|
2323
help: use the named argument by name to avoid ambiguity
2424
|
@@ -29,9 +29,9 @@ warning: named argument `y` is not used by name
2929
--> $DIR/issue-98466.rs:13:83
3030
|
3131
LL | println!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x);
32-
| - ^ this named argument is only referred to by position in formatting string
33-
| |
34-
| this formatting argument uses named argument `y` by position
32+
| -- ^ this named argument is referred to by position in formatting string
33+
| |
34+
| this formatting argument uses named argument `y` by position
3535
|
3636
help: use the named argument by name to avoid ambiguity
3737
|
@@ -42,9 +42,9 @@ warning: named argument `_x` is not used by name
4242
--> $DIR/issue-98466.rs:19:34
4343
|
4444
LL | let _f = format!("_x is {}", _x = 5);
45-
| - ^^ this named argument is only referred to by position in formatting string
46-
| |
47-
| this formatting argument uses named argument `_x` by position
45+
| -- ^^ this named argument is referred to by position in formatting string
46+
| |
47+
| this formatting argument uses named argument `_x` by position
4848
|
4949
help: use the named argument by name to avoid ambiguity
5050
|
@@ -55,9 +55,9 @@ warning: named argument `y` is not used by name
5555
--> $DIR/issue-98466.rs:22:34
5656
|
5757
LL | let _f = format!("_x is {}", y = _x);
58-
| - ^ this named argument is only referred to by position in formatting string
59-
| |
60-
| this formatting argument uses named argument `y` by position
58+
| -- ^ this named argument is referred to by position in formatting string
59+
| |
60+
| this formatting argument uses named argument `y` by position
6161
|
6262
help: use the named argument by name to avoid ambiguity
6363
|
@@ -68,9 +68,9 @@ warning: named argument `y` is not used by name
6868
--> $DIR/issue-98466.rs:25:91
6969
|
7070
LL | let _f = format!("first positional arg {}, second positional arg {}, _x is {}", 1, 2, y = _x);
71-
| - ^ this named argument is only referred to by position in formatting string
72-
| |
73-
| this formatting argument uses named argument `y` by position
71+
| -- ^ this named argument is referred to by position in formatting string
72+
| |
73+
| this formatting argument uses named argument `y` by position
7474
|
7575
help: use the named argument by name to avoid ambiguity
7676
|

0 commit comments

Comments
 (0)