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.

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

named_arguments_used_positionally lint drops some formatting options #100992

Closed
m-ou-se opened this issue Aug 25, 2022 · 6 comments
Closed

named_arguments_used_positionally lint drops some formatting options #100992

m-ou-se opened this issue Aug 25, 2022 · 6 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Aug 25, 2022

fn main() {
    format!("{:x}", a=1);
}
warning: named argument `a` is not used by name
 --> src/main.rs:2:21
  |
2 |     format!("{:x}", a=1);
  |              ----   ^ this named argument is referred to by position in formatting string
  |              |
  |              this formatting argument uses named argument `a` by position
  |
  = note: `#[warn(named_arguments_used_positionally)]` on by default
help: use the named argument by name to avoid ambiguity
  |
2 |     format!("{a}", a=1);
  |               ~

The :x disappears.

@m-ou-se m-ou-se added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Aug 25, 2022
@m-ou-se
Copy link
Member Author

m-ou-se commented Aug 25, 2022

I found this while refactoring compiler/rustc_builtin_macros/src/format.rs. Assigning to myself to fix as part of the refactor.

@m-ou-se m-ou-se self-assigned this Aug 25, 2022
@TaKO8Ki
Copy link
Member

TaKO8Ki commented Aug 25, 2022

I think this problem has already been solved in #100826.

@m-ou-se
Copy link
Member Author

m-ou-se commented Aug 25, 2022

Ah, thanks. Yes, that fixes it.

It seems like it could've been fixed by only removing some 'special' cases in the original code, without span_to_snippet, but I'll do that as part of my refactor.

@m-ou-se m-ou-se closed this as completed Aug 25, 2022
@TaKO8Ki
Copy link
Member

TaKO8Ki commented Aug 25, 2022

@m-ou-se

It seems like it could've been fixed by only removing some 'special' cases in the original code, without span_to_snippet, but I'll do that as part of my refactor.

Yes. Ideally, we should fix PositionalNamedArg::get_positional_arg_spans as noted in #100826 (comment).

ref: #100891

fn get_positional_arg_spans(&self, cx: &Context<'_, '_>) -> (Option<Span>, Option<Span>) {
if let Some(inner_span) = &self.inner_span_to_replace {
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, 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)
}
}
}

@m-ou-se
Copy link
Member Author

m-ou-se commented Aug 25, 2022

That struct and that method are entirely gone after my refector, and the entire has_formatting will be unnecessary. :)

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Aug 25, 2022

That's great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants