Fix the wrong suggestion to add a format literal `"{}"` directly where a proc macro is invoked. #116298

cafce25 · 2023-09-30T20:42:33Z

rustbot · 2023-09-30T20:42:40Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

cafce25 · 2023-09-30T20:48:51Z

I'm not done yet, sorry for the troubles.
@rustbot author

cafce25 · 2023-10-01T14:07:17Z

@rustbot review

petrochenkov · 2023-10-11T15:23:57Z

compiler/rustc_span/src/lib.rs

@@ -587,6 +587,12 @@ impl Span {
        matches!(outer_expn.kind, ExpnKind::Macro(..)) && outer_expn.collapse_debuginfo
    }

+    /// Returns `true` if `span` originates in a macros 2.0 expansion


Why macros 2.0 specifically, why is it relevant to this error?

Maybe I'm confused on nomenclature, the wrong help is emitted for proc macros only, with declarative macros the compiler puts the suggestion in the correct place. Aren't proc marcos and macros 2.0 the same thing essentially? If not this should probably just read proc macro expansion instead of macros 2.0 expansion

Aren't proc marcos and macros 2.0 the same thing essentially?

No, not at all.
Both declarative and procedural macros can use both old and macro 2.0 hygiene.

This situation is far from being unique, I suggest checking what other diagnostics do (fn in_external_macro or something like that).

petrochenkov · 2023-10-11T15:26:11Z

compiler/rustc_builtin_macros/src/format.rs

@@ -186,6 +186,8 @@ fn make_format_args(
                            ],
                             Applicability::MaybeIncorrect,
                        );
+                    } else if unexpanded_fmt_span.in_proc_macro_expansion() {
+                        err.help("you might be missing a string literal to format with in the definition of this macro");


A lot of compiler suggestions are skipped for code from macros, but suggestions to look into the macro like this are typically not emitted in such cases.

petrochenkov · 2023-10-11T15:27:56Z

tests/ui/fmt/format_in_macro.stderr

+   |          ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: you might be missing a string literal to format with in the definition of this macro
+   = note: this error originates in the derive macro `format_in_macro::derive` (in Nightly builds, run with -Z macro-backtrace for more info)


I don't see the issue from #116190 (#[derive("{}", macros::bare_println)]) in the test outputs.
Could you add all the tests and make them pass in one commit, and then apply compiler fixes and corresponding test updates in the second commit?

JohnCSimon · 2023-11-12T18:42:51Z

@cafce25

ping from triage - can you post your status on this PR? There hasn't been an update in a few weeks. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

cafce25 · 2023-11-14T16:07:51Z

Hi @JohnCSimon,

I've started to work on the feedback from petrochenkov, unfortunately I didn't have too much time in the past few weeks.
Fortunately the big project at work which partially took my hours should be finished this week and I will have some time for further work this weekend / next week so I expect this to be ready for another round of review by end of next week.

rustbot · 2023-11-23T07:57:29Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

5b9af2d

Dylan-DPC · 2024-02-15T18:08:12Z

@cafce25 any updates on this? if this ready for review you can comment with @rustbot ready

JohnCSimon · 2024-04-14T17:59:41Z

@cafce25

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned petrochenkov Sep 30, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 30, 2023

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2023

This comment has been minimized.

Sign in to view

cafce25 marked this pull request as ready for review October 1, 2023 14:06

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2023

petrochenkov reviewed Oct 11, 2023

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2023

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Nov 23, 2023

test current behaviour of proc macros without format literal

9b2a36e

cafce25 force-pushed the issue-116190-fix branch from 5b9af2d to 9b2a36e Compare November 23, 2023 18:46

dtolnay removed the has-merge-commits PR has merge commits, merge with caution. label Jan 21, 2024

JohnCSimon closed this Apr 14, 2024

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Apr 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the wrong suggestion to add a format literal `"{}"` directly where a proc macro is invoked. #116298

Fix the wrong suggestion to add a format literal `"{}"` directly where a proc macro is invoked. #116298

cafce25 commented Sep 30, 2023

rustbot commented Sep 30, 2023

cafce25 commented Sep 30, 2023

This comment has been minimized.

cafce25 commented Oct 1, 2023

petrochenkov Oct 11, 2023

cafce25 Oct 12, 2023

petrochenkov Oct 12, 2023

petrochenkov Oct 11, 2023

petrochenkov Oct 11, 2023

JohnCSimon commented Nov 12, 2023

cafce25 commented Nov 14, 2023

rustbot commented Nov 23, 2023

Dylan-DPC commented Feb 15, 2024

JohnCSimon commented Apr 14, 2024

Fix the wrong suggestion to add a format literal "{}" directly where a proc macro is invoked. #116298

Fix the wrong suggestion to add a format literal "{}" directly where a proc macro is invoked. #116298

Conversation

cafce25 commented Sep 30, 2023

rustbot commented Sep 30, 2023

cafce25 commented Sep 30, 2023

This comment has been minimized.

cafce25 commented Oct 1, 2023

petrochenkov Oct 11, 2023

Choose a reason for hiding this comment

cafce25 Oct 12, 2023

Choose a reason for hiding this comment

petrochenkov Oct 12, 2023

Choose a reason for hiding this comment

petrochenkov Oct 11, 2023

Choose a reason for hiding this comment

petrochenkov Oct 11, 2023

Choose a reason for hiding this comment

JohnCSimon commented Nov 12, 2023

cafce25 commented Nov 14, 2023

rustbot commented Nov 23, 2023

Dylan-DPC commented Feb 15, 2024

JohnCSimon commented Apr 14, 2024

Fix the wrong suggestion to add a format literal `"{}"` directly where a proc macro is invoked. #116298

Fix the wrong suggestion to add a format literal `"{}"` directly where a proc macro is invoked. #116298