Skip to content

Diagnostic assumes that braced unresolved identifiers are formatting arguments #141350

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

Open
fmease opened this issue May 21, 2025 · 7 comments
Open
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-resolve Area: Name/path resolution done by `rustc_resolve` specifically D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fmease
Copy link
Member

fmease commented May 21, 2025

Uplifted from #141213 (comment). Given

struct Type { field: i32 }

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

the compiler outputs

error[E0425]: cannot find value `field` in this scope
 --> file.rs:4:25
  |
4 |     fn method(&self) { {field} }
  |                         ^^^^^
  |
  = help: you might have meant to use the available field in a format string: `"{}", self.field`

warning: unnecessary braces around block return value
 --> file.rs:4:24
  |
4 |     fn method(&self) { {field} }
  |                        ^     ^
  |
  = note: `#[warn(unused_braces)]` on by default
help: remove these braces
  |
4 -     fn method(&self) { {field} }
4 +     fn method(&self) { field }
  |

error: aborting due to 1 previous error; 1 warning emitted

Notice the (unstructured) suggestion you might have meant to use the available field in a format string: `"{}", self.field` which is obviously incorrect, there's no format string in sight and fn method(&self) { "{}", self.field; } is butchered.

For context, this regressed in PR #141213.

Version Information

rustc 1.89.0-nightly (c43786c9b 2025-05-21)
binary: rustc
commit-hash: c43786c9b7b8d8dcc3f9c604e0e3074c16ed69d3
commit-date: 2025-05-21
host: x86_64-unknown-linux-gnu
release: 1.89.0-nightly
LLVM version: 20.1.5
@fmease fmease added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-papercut Diagnostics: An error or lint that needs small tweaks. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels May 21, 2025
@fmease
Copy link
Member Author

fmease commented May 21, 2025

We might be able to fix this by marking the span with DesugaringKind::FormatArgs (yet to be defined) by using mark_with_reason during the lowering of ast::FormatArgs (no surface syntax) (not to be confused with the expansion of format_args!(…)) and querying it at the relevant site.

However, even then we'd need to be wary of cases like format!("{}", {field}) (we don't want to (essentially) suggest format!("{}", "{}", self.field)).

@fmease fmease added the A-resolve Area: Name/path resolution done by `rustc_resolve` specifically label May 21, 2025
@fmease fmease changed the title Diagnostic assumes that braced unresolved identifiers are formatting argument Diagnostic assumes that braced unresolved identifiers are formatting arguments May 21, 2025
@xizheyin
Copy link
Contributor

Great case, I'll look at adding DesugaringKind::FormatArgs in the near future if you're not going to do that.

@xizheyin
Copy link
Contributor

@rustbot claim

@fmease
Copy link
Member Author

fmease commented May 21, 2025

Also CC @mejrs to whom I recently also suggested to create DesugaringKind::FormatArgs & to mark_with_reason during the lowering of ast::FormatArgs for an unrelated diagnostic improvement. Pinging them to avoid duplicate efforts.

@mejrs
Copy link
Contributor

mejrs commented May 21, 2025

I did not get that far with the implementation of the desugaring aspect of it, that turned out to be more complicated than I thought. Feel free to take it if you want.

Just some thought:

  • For example the format specifiers do not get a span when the literal is first parsed, so changes need to happen there as well.
  • I found it more useful to just have the desugaring pointing to the literal itself, not the positional or indexed arguments so I called it DesugaringKind::FormatLiteral. Open to ideas.
  • the span coming from the expansion also allows some internal features, so needs additional care
  • this should be its own PR imo.

@xizheyin
Copy link
Contributor

Here are some notes as I'm going along, and feel free to make some suggestions.

I've found that marking span at ast_lowering doesn't solve the problem because the diagnostic occurs at rustc_resolve, which comes before ast_lowering.

I also tried to mark FormatArgs as soon as the span is generated in rustc_builtin_macros, but found that tcx is missing, and I may have to add a rustc_middle dependency, which I'm worried will interfere with compilation efficiency.

I need to do some more investigation to see if I can mark elsewhere.

@mejrs
Copy link
Contributor

mejrs commented May 25, 2025

In rustc_resolve we should at least be able to tell if we're in an expansion of some particular macros, using the outer* methods on Span and that the format macros are diagnostic items.

Another way would be to revert #141213 and instead just tell the user to write let x = self.x; or let x = &self.x; in the surrounding scope. This also gets around issues like "what if the macro already has positional arguments".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-resolve Area: Name/path resolution done by `rustc_resolve` specifically D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants