Skip to content

Incorrect fix for to_string_in_format_args #9540

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

Closed
nyurik opened this issue Sep 26, 2022 · 5 comments · Fixed by #9590
Closed

Incorrect fix for to_string_in_format_args #9540

nyurik opened this issue Sep 26, 2022 · 5 comments · Fixed by #9590
Assignees
Labels
I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@nyurik
Copy link
Contributor

nyurik commented Sep 26, 2022

I just discovered another bug in the format args parser FormatArgsExpn::parse while creating more tests for #9233.

Add this test case to the tests/ui/format_args.rs main() to see it fail:

print!("error: something failed at {}", (Location::caller().to_string()));
//                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^  (does not include parens)

The resulting format_args.args[0].param.value has a span does NOT include the surrounding parenthesis, which means that it suggest this:

Auto-fix is incorrectly suggesting this:

error: `to_string` applied to a type that implements `Display` in `print!` args
  --> $DIR/format_args.rs:86:64
   |
LL |     print!("error: something failed at {}", (Location::caller().to_string()));
   |                                                                ^^^^^^^^^^^^^ help: remove this

which results in an incompilabre

print!("error: something failed at {}", (Location::caller());  // <-- missing closing parenthesis

cc: @Alexendoo

@Alexendoo Alexendoo added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Sep 27, 2022
@Alexendoo Alexendoo self-assigned this Sep 27, 2022
@Alexendoo
Copy link
Member

This is unrelated to format_args parsing, the expression does include the parenthesis

@nyurik
Copy link
Contributor Author

nyurik commented Oct 3, 2022

@Alexendoo why do you think that? In these examples of uninlined-format-args test that also fails, just like the format-args lint, the captured arg is clearly not including the surrounding parens - and that's why when it uses expand_past_previous_comma(), it is dis-balanced - on the left side it includes all the non-captured parens plus the comma, whereas on the right side it stays where the parsing captured it, without the closing paren.

See the build results in https://github.com/rust-lang/rust-clippy/pull/9548/checks

@Alexendoo
Copy link
Member

Haven't looked into that one but looks like a different issue. For this it's because the parenthesis are part of the expression's span, when it does

value.span.with_lo(receiver.span.hi()),
it's including the closing parenthesis in the thing it's suggesting to remove

@nyurik
Copy link
Contributor Author

nyurik commented Oct 4, 2022

Thanks @Alexendoo, you are right uninlined_format_args issue was due to a bug that I now have a PR for: #9588

As for the current issue in the to_string_in_format_args:

For the test case, I think I figured it out thanks to your help! See #9590. Sorry to step on your toes on this one.

@nyurik nyurik changed the title Incorrect param parsing in FormatArgsExpn::parse Incorrect fix for to_string_in_format_args Oct 4, 2022
@nyurik
Copy link
Contributor Author

nyurik commented Oct 5, 2022

@rustbot claim

@bors bors closed this as completed in 42bdfa2 Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants