Skip to content
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

Rust nightly broke formatdoc! #106408

Closed
VorfeedCanal opened this issue Jan 3, 2023 · 7 comments
Closed

Rust nightly broke formatdoc! #106408

VorfeedCanal opened this issue Jan 3, 2023 · 7 comments
Assignees
Labels
P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@VorfeedCanal
Copy link

I guess good deeds don't go unpubished.

When I filed issue about indoc crashing the compiler with safe code I was, kinda, expecting that fix for the issue would lead to successful compilation, not regression.

Note that this code:

use indoc::formatdoc;

fn main() {
    let trait_name = "Trait";
    let fn_name = "fun";
    println!("{}", formatdoc!("
                          pub trait {trait_name};
                          fn {fn_name};}}"));
}

works on all version of Rust starting from 1.58 till 1.67.

Now it no longer works (after fix for #106191).

One may argue that code was never supposed to work, but since it was supported for whole year… it's probably good to note it somewhere (preferable in release notes, I guess).

@Mark-Simulacrum Mark-Simulacrum added this to the 1.68.0 milestone Jan 3, 2023
@Mark-Simulacrum Mark-Simulacrum added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Jan 3, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jan 3, 2023
@Noratrieb
Copy link
Member

Uh, this is pretty bad. I did not intend to break anything, but I think I see what's going on. I'll take a look but I'm not sure whether it will be easy, as I don't think this code should technically compile according to our rule for format_args captures (but in my opinion this should work).

If we don't find a conclusion quickly enough it might be worth it to revert e6c02aa.

@Noratrieb Noratrieb self-assigned this Jan 3, 2023
@VorfeedCanal
Copy link
Author

The documentation clearly says that it shouldn't work.

But it was accepted for a year. The question now, obviously, is: how many crates actually managed to depend on that misfeature.

@VorfeedCanal
Copy link
Author

Note that you accidentally fixed another bug in the indoc crate… although maybe not in a way the guy who filed that bug expected.

@Nemo157
Copy link
Member

Nemo157 commented Jan 3, 2023

The documentation clearly says that it shouldn't work.

Where exactly does it say that it shouldn't work? Note that indoc explicitly sets the span of the output literal to inherit the hygiene from the input, the macro examples I could find in the RFC only talk about when the input to format! is an expression, but indoc is generating it as a string literal (just with content that does not match the source code its span points to).

@Nemo157
Copy link
Member

Nemo157 commented Jan 4, 2023

It does look like the RFC's rejection of format!(<expr>) with an expression containing a macro expanding to a literal was also used to reject proc-macros outputting format!(<literal>) directly. indoc was just happening to hit a bug in this rejection that allowed it to sneak a literal in by applying a span that had a literal in the source code too.

#[proc_macro]
pub fn foo(_: proc_macro::TokenStream) -> proc_macro::TokenStream {
  r#"{ let x = 5; format!("{x}") }"#.parse().unwrap()
}
fn main() {
  dbg!(procmacro::foo!());
}
error: there is no argument named `x`
 --> src/main.rs:2:8
  |
2 |   dbg!(procmacro::foo!());
  |        ^^^^^^^^^^^^^^^^^
  |
  = note: did you intend to capture a variable `x` from the surrounding scope?
  = note: to avoid ambiguity, `format_args!` cannot capture variables when the format string is expanded from a macro
  = note: this error originates in the macro `procmacro::foo` (in Nightly builds, run with -Z macro-backtrace for more info)

I don't really see any reason to outright reject this, it's still possible to implement the same thing but it forces the proc-macro to parse the format string and extract all the parameters and individually span them (format_with_prefix!("{x}") would expand via let arg = Ident::new("x").span(input.span()); quote! { format!("prefix {x}", x=arg) }).

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 10, 2023
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 18, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 18, 2023
…ther-it-really-is-a-literal, r=compiler-errors

Revert "Improve heuristics whether `format_args` string is a source literal"

This reverts commit e6c02aa (from rust-lang#106195).

Keeps the code improvements from the PR and the test (as a known-bug).

Works around rust-lang#106408 while a proper fix is discussed more thoroughly in rust-lang#106505, as proposed by `@tmandry.`

Reopens rust-lang#106191

r? compiler-errors
@Noratrieb
Copy link
Member

Closing this as the revert in #107041 fixed this and also added a test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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

6 participants