Remove the unhelpful let binding diag comes from FormatArguments #114511
Conversation
|
r? @fee1-dead (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Not entirely sure if this is what should happen. r? compiler |
| if self.span == expr.span.source_callsite() { | ||
| self.found = self.current; | ||
| if self.prop_expr.is_none() { |
There was a problem hiding this comment.
What if we have nested format_args expressions?
Do we only care about the case where we assign the result of format_args!? If so, why can't we just check for stmt being an assignment where the rhs expr is_format_args_item?
There was a problem hiding this comment.
the if self.span == expr.span.source_callsite() guarantee we find out the expr we're intrested in, the code goes here means a borrow(mostly binding) happens, I think use span to find expr is mostly enough for suggestions.
There was a problem hiding this comment.
What does your branch output on something like this: let x = format_args!("a {} {} {}.", 1, format_args!("b{}!", 2), 3)? Can you maybe add this to your test?
There was a problem hiding this comment.
The output is:
error[E0716]: temporary value dropped while borrowed
--> tests/ui/borrowck/issue-114374-invalid-help-fmt-args.rs:5:13
|
5 | let x = format_args!("a {} {} {}.", 1, format_args!("b{}!", 2), 3);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
| |
| creates a temporary value which is freed while still in use
6 | bar(x);
| - borrow later used here
|
= note: the result of `format_args!` can only be assigned directly if no placeholders in it's arguments are used
= note: to learn more, visit <https://doc.rust-lang.org/std/macro.format_args.html>
= note: this error originates in the macro `format_args` (in Nightly builds, run with -Z macro-backtrace for more info)I added it to the test case.
| if visitor.found == 0 | ||
| && !is_format_arguments_item |
There was a problem hiding this comment.
If we already special-case for FormatArguments, can't we also provide a more helpful diagnostic, which explains why this fails, instead of just getting rid of the binding suggestion? Note that we also still have this note: = note: consider using a let binding to create a longer lived value, so it might make sense to special case this.
There was a problem hiding this comment.
Good point!
I add a special note for format_args!, I'm not sure whether the words are accurate enough, seems it's hard to explain 😂 , so I added a link to the document.
chenyukang
force-pushed
the
yukang-fix-114374-fmt-args
branch
2 times, most recently
from
397b91a
to
116ef45
Compare
chenyukang
force-pushed
the
yukang-fix-114374-fmt-args
branch
from
116ef45
to
340fea4
Compare
chenyukang
force-pushed
the
yukang-fix-114374-fmt-args
branch
from
340fea4
to
1f107b1
Compare
|
I'm sorry that I didn't respond. I missed the notifications for your replies, so thought you hadn't replied. @bors r+ rollup |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#114511 (Remove the unhelpful let binding diag comes from FormatArguments) - rust-lang#115473 (Add explanatory note to 'expected item' error) - rust-lang#115574 (Replace `rustc_data_structures` dependency with `rustc_index` in `rustc_parse_format`) - rust-lang#115578 (Clarify cryptic comments) - rust-lang#115587 (fix rust-lang#115348) - rust-lang#115596 (A small change) - rust-lang#115598 (Fix log formatting in bootstrap) - rust-lang#115605 (Better Debug for `Ty` in smir) - rust-lang#115614 (Fix minor grammar typo) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#114511 - chenyukang:yukang-fix-114374-fmt-args, r=b-naber Remove the unhelpful let binding diag comes from FormatArguments Fixes rust-lang#114374
Fixes #114374