rust icon indicating copy to clipboard operation
rust copied to clipboard

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

Open xizheyin opened this issue 6 months ago • 12 comments

Fixes rust-lang/rust#141350

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

r? @fmease
cc @mejrs

xizheyin avatar May 27 '25 06:05 xizheyin

fmease is not available for reviewing at the moment.

Please choose another assignee.

rustbot avatar May 27 '25 06:05 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

rustbot avatar May 27 '25 06:05 rustbot

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 avatar May 27 '25 09:05 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.

xizheyin avatar May 27 '25 09:05 xizheyin

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 https://github.com/rust-lang/rust/pull/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 avatar May 27 '25 10:05 mejrs

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

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

mejrs avatar May 27 '25 10:05 mejrs

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.

xizheyin avatar May 27 '25 14:05 xizheyin

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.

mejrs avatar May 27 '25 15:05 mejrs

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?

davidtwco avatar May 29 '25 11:05 davidtwco

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 avatar May 29 '25 15:05 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

xizheyin avatar Jun 10 '25 06:06 xizheyin

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

rustbot avatar Jun 10 '25 06:06 rustbot

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

apiraino avatar Oct 09 '25 13:10 apiraino

I'll re-examine this PR these days. I recall that #142594 couldn't help because of https://github.com/rust-lang/rust/pull/142594#issuecomment-3016506144.

xizheyin avatar Oct 10 '25 09:10 xizheyin