Properly allow macro expanded format_args invocations to uses captures

[Noratrieb]

Originally, this was kinda half-allowed. There were some primitive checks in place that looked at the span to see whether the input was likely a literal. These "source literal" checks are needed because the spans created during format_args parsing only make sense when it is indeed a literal that was written in the source code directly.

This is orthogonal to the restriction that the first argument must be a "direct literal", not being exanpanded from macros. This restriction was imposed by RFC 2795 on the basis of being too confusing. But this was only concerned with the argument of the invocation being a literal, not whether it was a source literal (maybe in spirit it meant it being a source literal, this is not clear to me).

Since the original check only really cared about source literals (which is good enough to deny the format_args!(concat!()) example), macros expanding to format_args invocations were able to use implicit captures if they spanned the string in a way that lead back to a source string.

The "source literal" checks were not strict enough and caused ICEs in certain cases (see #106191). So I tightened it up in #106195 to really only work if it's a direct source literal.

This caused the indoc crate to break. indoc transformed the source literal by removing whitespace, which made it not a "source literal" anymore (which is required to fix the ICE). But since indoc spanned the literal in ways that made the old check think that it's a literal, it was able to use implicit captures (which is useful and nice for the users of indoc).

This commit properly seperates the previously introduced concepts of "source literal" and "direct literal" and therefore allows indoc invocations, which don't create "source literals" to use implicit captures again.

Fixes #106191

[Noratrieb]

Since this changes what kind of code is allowed to compile and settles a question not fully answered by the RFC, I'll lang nominate this.

The PR description isn't really relevant here, TLDR: The check for "the first argument must be a string literal" was implemented badly and a recent PR of mine made it worse, forcing me to properly fix it here.

Question for the lang team

Should format_args! invocations that were generated by a proc-macro be able to use implicit argument captures?

This used to be kind of half-supported on stable (depending on the spans of the string literal) and is now (accidentally) mostly unsupported on nightly.

A concrete example that does not compile before this PR (neither on stable nor nightly) but does compile with it:
pm.rs

extern crate proc_macro;

#[proc_macro]
pub fn foo(_: proc_macro::TokenStream) -> proc_macro::TokenStream {
    r#"{ let x = 5; format!("{x}") }"#.parse().unwrap()
}

rustc pm.rs --crate-type proc-macro

fn main() {
    dbg!(pm::foo!());
}

rustc main.rs --extern pm=libpm.so

Arguments for it

The indoc crate is an example of a crate that makes usage of this. Note that format args captures indoc compiles on stable but not nightly because of the old broken and inconsistent semantics. It's in my opinion unreasonable to support the indoc usage but not my example above (since only the spans of the string literal are different).

Arguments against it

This could lead to confusing behavior in contrived situations. indoc (or other crates like it) could do their own parsing of the implicit captures and expand them out into separate bindings before forwarding them to format_args, but this can of course be said for the whole format_args machinery.

[tmandry]

Discussed in yesterday's lang team meeting (notes).

Should format_args! invocations that were generated by a proc-macro be able to use implicit argument captures?

Several of us had the instinct that the answer should be "yes, if the span attached to the {foo} has hygiene that can name foo". It's possible that this answer misses some nuance, but if it does we're missing that context.

It's in my opinion unreasonable to support the indoc usage but not my example above (since only the spans of the string literal are different).

I think I agree, but I don't have context on what the spans of the string literal produced by indoc are. I would guess that it uses Span::call_site()?

indoc (or other crates like it) could do their own parsing of the implicit captures and expand them out into separate bindings before forwarding them to format_args, but this can of course be said for the whole format_args machinery.

I don't follow. Can you please give an example?

Generally, we'd like to see an explainer that compares the behavior in the RFC (to the extent that it's specified) to the old behavior, current behavior, and behavior after landing this PR.

In the short term, we think it makes sense to revert #106195 (and backport that revert if needed) so that we don't break existing code in the next release. Fixing an ICE is lower priority than keeping existing code working. Then we can discuss what the right behavior is and implement that without the time pressure.

Please re-nominate for discussion when our input is needed again. Thanks!

[Noratrieb]

I reverted it in the meantime which makes the issue about indoc work again.
My last comment was a little confusing, I'll try to make it clearer now^^
FWIW I think that this is a clear bug that should be fixed like in this PR, but I want to make sure that you agree with this new behavior since it makes more code compile which we won't be able to undo in the future.

I made a small table of things macros were able to expand to before, after my PR that fixed an ICE and caused a regression and was then reverted, and this PR which fixes it all.

Macro expanding to	Originally	After my first PR	With this PR
format!("{x}", x = 0)	✅	✅	✅
format!("{x}"), where "{x}" is the macro input and there is an x ident with the same hygiene as the literal and the literal span is the same span as in the source	✅	✅	✅
{ let x = 5; format!("{x}") } (no macro input, any hygiene for x)	❌	❌	✅
format!("A{x}"), where "{x}" is the macro input and there is an x ident with the same hygiene as the literal. The literal has been slightly modified by the macro so its span does not point at the exact same characters in the source.	✅	❌#106408	✅
A macro that expands to a format invocation and modifies a non-ascii string literal in just the right way	🧊 (ICE)	✅	✅

[scottmcm]

We discussed this in the lang team meeting today. Broadly we thought that proc macros being able to emit tokens that use captures makes logical sense. Basically, a proc macro emitting something as a whole that one could write directly, as in https://github.com/rust-lang/rust/pull/106505/files#diff-868206cf54a88d81468a76d45e639cea40c537f2031d19f06c114e3cb329a426R44, seems entirely fine, though mixing things like format!(concat!(…)) might still be best to reject.

So I'll start a

@rfcbot fcp merge

but we had some requests to confirm, so...

[rfcbot]

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

Concerns:

• please-link-tests resolved by Properly allow macro expanded format_args invocations to uses captures #106505 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

@rfcbot concern please-link-tests

We did discuss a bunch of situations in the meeting where we'd like to get them listed out to affirmatively confirm those cases. It sounds like a bunch of them already work as expected, but we'd like to have tests (or pointers to existing tests) for things like how hygiene works in macro-generated format strings with captures, which things don't work, how it relates to when things are expanded, etc. (Like is format!(macro_be!()) different from format!(p_macro!())?)

[petrochenkov]

a proc macro emitting something as a whole that one could write directly ... seems entirely fine, though mixing things like format!(concat!(…)) might still be best to reject

This was always my understanding as well.
We parse the expression at the start of TOKENS in format_args!(TOKENS), if it's a literal - good, if it's not a literal - bad.
It's not clear why the implementation considered spans at all.

[petrochenkov]

The current implementation seems overcomplicated too - we don't need is_string_literal_including_nt, we only need to parse an expression and then do a post factum check on it.

[tmandry]

{ let x = 5; format!("{x}") } (no macro input, any hygiene for x)

This PR still requires the hygiene for x in the binding and format string to match, right?

[Noratrieb]

Yes. Well, it doesn't require them to match for macro expansion to succeed, but x will not be found later if it doesn't match.

[Noratrieb]

I have collected and added a few tests:

tests/ui/fmt/indoc-issue-106408.rs tests that hygiene info is preserved correctly through a proc macro invocation
tests/ui/fmt/format-args-capture-first-literal-is-macro.rs tests that MBEs, proc macros and builtin macros in argument position block captures
tests/ui/fmt/format-args-capture-from-pm-first-arg-macro.rs tests that we still forbid format!(concat!("{a}")) even if it comes from a proc macro

[joshtriplett]

@scottmcm Do the latest changes by @Nilstrieb address your concern?

[scottmcm]

Thanks, @Nilstrieb ! That's very helpful
@rfcbot resolve please-link-tests

The only other thing that comes to mind is something like this

macro_rules! format_mbe {
    ($tt:tt) => {
        {
            let a = 123;
            format!($tt)
        }
    };
}

fn main() {
    let a = 0;
    assert_eq!(format_mbe!("{a}"), "0");
}

but if that's not actually different because of how resolution works, then feel free to omit it.

[petrochenkov]

r=me after addressing the remaining comments.
@rustbot author

[Noratrieb]

@bors r=petrochenkov

[bors]

📌 Commit 427aceb has been approved by petrochenkov

It is now in the queue for this repository.

[bors]

⌛ Testing commit 427aceb with merge 2e7034e...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 2e7034e to master...

[rust-timer]

Finished benchmarking commit (2e7034e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.4%	[0.4%, 0.5%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[3.5%, 3.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.5%	[-3.5%, -3.5%]	1
All ❌✅ (primary)	3.5%	[3.5%, 3.5%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.9%	[4.1%, 5.5%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.1%]	2
All ❌✅ (primary)	-	-	0