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

[xizheyin]

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

[xizheyin]

Use maybe to imply that maybe x is in the format string, and add a comment here to clarify.

[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.