Suggest to bind self.x to x when field x may be in format string

[xizheyin]

View all comments

Fixes #141350

I added the new test in the first commit, and committed the changes in the second one.

r? @fmease
cc @mejrs

[rustbot]

fmease is not available for reviewing at the moment.

Please choose another assignee.

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[davidtwco]

I don't think this is an improvement, the help message being changed is just as correct as what you are suggesting, but feels more appropriate as it adds to the format macro that is where the error originates rather than adding another line entirely. You could attempt to improve this with a structured suggestion that the user add a , foo = self.foo argument to the format macro.

[xizheyin]

For structured suggestions, I tried, but I don't currently have a better way to get the HIR or ast of the format macro, in this diagnostic I can only get the span of x. There is so little information given here. Do you have a good idea?

[xizheyin]

The original help message is wrong for the following example

struct Type { field: i32 }

impl Type {
    fn method(&self) { {field} }
}

So, a line of binding had to be added to ensure that the formatted string and this case in the example were satisfied.

[mejrs]

I don't think this is an improvement, the help message being changed is just as correct as what you are suggesting, but feels more appropriate as it adds to the format macro that is where the error originates rather than adding another line entirely. You could attempt to improve this with a structured suggestion that the user add a , foo = self.foo argument to the format macro.

The problem is that there's not a good way to detect that we're in the format (or write, print etc) macro to begin with.

But format string parsing has a specialized error path for format!("{thing.field}") that suggests what you say. So I guess if we just revert #141213 then they'll eventually get there:

With that PR reverted, users will go from
format!("{x}") to format!("{self.x}") to format!("{}", self.x).

Not that elegant, but it works.

[mejrs]

But if we suggest creating a new variable, the suggestion is always correct;

let x = &self.x;
format!("{x}");

[xizheyin]

With that PR reverted, users will go from format!("{x}") to format!("{self.x}") to format!("{}", self.x).

I'm more curious if this is considered proper advice if it's suggested through a two-paragraph application?

But if we suggest creating a new variable, the suggestion is always correct;

let x = &self.x;
format!("{x}");

Yes, this prompts the user once and avoids having to apply the suggestion twice.

[mejrs]

I'm more curious if this is considered proper advice if it's suggested through a two-paragraph application?

It's not, but you also have to weigh that against how accurate the suggestion is and how likely someone is to run into it to begin with. Going through code snippets and looking for braces is a hacky thing that risks being inaccurate.

[davidtwco]

For structured suggestions, I tried, but I don't currently have a better way to get the HIR or ast of the format macro, in this diagnostic I can only get the span of x. There is so little information given here. Do you have a good idea?

Does Span::from_expansion help?

[xizheyin]

I tried this and it was false. Probably because it's not what the macro expands out to, but is simply treated as a variable.

[xizheyin]

This PR seems to be on hold for a while, maybe David doesn't have time on this PR for a while.
may r? @nnethercote

[rustbot]

nnethercote is not on the review rotation at the moment.
They may take a while to respond.

[apiraino]

I don't have context here but looking at the linked comment, work here can now move forward due to #142594 being merged. Provided the design here is deemed ok by reviewers.

@rustbot author

[xizheyin]

I'll re-examine this PR these days. I recall that #142594 couldn't help because of #142594 (comment).

[bors]

☔ The latest upstream changes (presumably #149836) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@xizheyin any updates on this? thanks

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[xizheyin]

Sorry for late response. This version may resolve the issue.
waiting for green

View changes since this review

[xizheyin]

Here is a small fix, because the start may be 0 in some cases.

[xizheyin]

Here we check FormatLiteral.

[xizheyin]

@rustbot ready

[nnethercote]

@bors r+

[rust-bors]

📌 Commit c4f14ad has been approved by nnethercote

It is now in the queue for this repository.