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

Compiling a Rocket application on the 2015 edition doesn't emit errors #89699

Closed
ferrohd opened this issue Oct 9, 2021 · 10 comments · Fixed by #117973
Closed

Compiling a Rocket application on the 2015 edition doesn't emit errors #89699

ferrohd opened this issue Oct 9, 2021 · 10 comments · Fixed by #117973
Assignees
Labels
A-proc-macros Area: Procedural macros C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-edition Diagnostics: An error or lint that should account for edition differences. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ferrohd
Copy link

ferrohd commented Oct 9, 2021

When trying to compile any Rocket application in the 2015 edition the compiler says an error occured without showing any actual error:

$ RUST_BACKTRACE=1 cargo build
   Compiling repro v0.1.0
error: could not compile `repro`
$

Switching to the 2018 edition fixes the problem, by either showing errors or properly compiling the application. Minimal reproduction:

// src/main.rs

#[rocket::launch]
fn foo() -> _ {
    rocket::build()
}
# Cargo.toml

[package]
name = "repro"
version = "0.1.0"
edition = "2015"

[dependencies]
rocket = "0.5.0-rc.1"

Meta

rustc --version --verbose:

rustc 1.55.0 (c8dfcfe04 2021-09-06)
binary: rustc
commit-hash: c8dfcfe046a7680554bf4eb612bad840e7631c4b
commit-date: 2021-09-06
host: x86_64-unknown-linux-gnu
release: 1.55.0
LLVM version: 12.0.1
@ferrohd ferrohd added the C-bug Category: This is a bug. label Oct 9, 2021
@estebank estebank added A-proc-macros Area: Procedural macros D-confusing Diagnostics: Confusing error or lint that should be reworked. D-edition Diagnostics: An error or lint that should account for edition differences. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 9, 2021
@hkratz
Copy link
Contributor

hkratz commented Oct 9, 2021

This leads to FatalError::raise() in the parser, which apparently does not produce any output nor tracing.

@rustbot label +I-ICE

@rustbot rustbot added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Oct 9, 2021
@hkratz
Copy link
Contributor

hkratz commented Oct 9, 2021

The fatal error is raised here:

FatalError.raise();

@ehuss
Copy link
Contributor

ehuss commented Oct 9, 2021

Here's an overview of what is happening.

The proc-macro creates something along the lines of:

fn main() { ::rocket::async_main(async move { /* stuff */ }) }

In 2015, when it goes to parse the arguments, it sees async as a normal identifier. It then sees move which is unexpected, which creates an unexpected error in the parser. However, here it attempts to recover by assuming the user forgot a ,. The parser then sees move and assumes that must be the start of a closure. However, it then sees { when it expected a | to start the closure arguments. This emits another unexpected error. However, this hits the FatalError here because it believes it has not made any progress. The original error has not yet been emitted, and the compiler exits without a message.

Normally this sequence of events won't happen because the last_unepxected_token_span would be different, and it would just emit a normal error. However, rocket is using quote_spanned! which causes all of the tokens to end up with the same span.

I think the use of FatalError.raise in expect_one_of is questionable, and not really explained in the code why it is there. Maybe this is to prevent the parser from getting stuck somehow? That could really use some comments explaining its purpose. Without understanding the purpose there, it is hard to guess at a possible fix.

Here is a minimal repro:

// src/lib.rs
#[pm::foo]
fn foo() {}

And a proc-macro named pm:

extern crate proc_macro;
use proc_macro::*;

#[proc_macro_attribute]
pub fn foo(_attr: TokenStream, item: TokenStream) -> TokenStream {
    let tt = item.into_iter().next().unwrap();
    let sp = tt.span();
    let mut arg = TokenStream::new();
    let mut g = Group::new(Delimiter::Brace, TokenStream::new());
    g.set_span(sp);
    arg.extend([
        TokenTree::Ident(Ident::new("async", sp)),
        TokenTree::Ident(Ident::new("move", sp)),
        TokenTree::Group(g),
    ]);
    let mut body = TokenStream::new();
    body.extend([
        TokenTree::Ident(Ident::new("async_main", sp)),
        TokenTree::Group(Group::new(Delimiter::Parenthesis, arg)),
    ]);

    let mut ret = TokenStream::new();
    ret.extend([
        TokenTree::Ident(Ident::new("fn", sp)),
        TokenTree::Ident(Ident::new("main", sp)),
        TokenTree::Group(Group::new(Delimiter::Parenthesis, TokenStream::new())),
        TokenTree::Group(Group::new(Delimiter::Brace, body)),
    ]);
    eprintln!("{:?}", ret);
    ret
}

@ehuss
Copy link
Contributor

ehuss commented Oct 9, 2021

That check was added in #58903, but unfortunately there isn't much of a description explaining why it was added.

@estebank
Copy link
Contributor

IIRC I believe that it was to deduplicate some redundant errors that occurred otherwise. This whole ordeal reminds me that we should be trying to parse async blocks in 2015 when encountering the kw in idents and then encountering parse errors to emit clear diagnostics about setting the edition (I thought we were doing that already). Macros can also set the edition metadata in the Span, IIRC (it might be an unstable API), which can let 2018 proc-macros work even in 2015 crates (do we even want to open that can of worms, I don't think so). I'm glad to see this is a parser error, it means that we can fix it "easily".

@Enselic
Copy link
Member

Enselic commented Sep 10, 2023

Triage: Can someone please give a concrete example of a minimal "Rocket application" that reproduces this?

@Enselic Enselic added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Sep 10, 2023
@ehuss
Copy link
Contributor

ehuss commented Sep 11, 2023

@Enselic Is there any particular reason you'd like a Rocket application specifically? There is a minimal reproduction without Rocket listed above at #89699 (comment).

I believe this is mostly addressed via #114237. The reproduction now prints the error:

error: `async move` blocks are only allowed in Rust 2018 or later

However, I would suggest someone adding that reproduction to the UI testsuite, since it is a relatively subtle construction of tokens, and the test in #114237 doesn't test anything like what that is doing.

I'd also defer to @estebank if they think there is anything more fundamental that should be done, particularly around the use of last_unepxected_token_span and FatalError in expect_one_of. I'm a bit uneasy about the use of FatalError there, since it is capable of stopping the compiler even if no errors have been emitted. Similarly, comparing spans against last_unepxected_token_span may not always work if something like a proc-macro ends up using the same span across multiple tokens. I would imagine someone could cook up a different example that has the same problem where the compiler exits with no error.

@ehuss ehuss removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Sep 11, 2023
@Enselic
Copy link
Member

Enselic commented Sep 11, 2023

My bad, sorry about that. I'm trying to keep a high pace when triaging ICE-issues since there are so many of them. In this case the pace was too high, so I missed that comment of yours.

@vincenzopalazzo
Copy link
Member

We discuss this in the wg-macros triage meeting and @CohenArthur will work on it 🛩️

If you want to join the discussion, please join our zulip stream

@CohenArthur
Copy link
Contributor

@rustbot claim

CohenArthur added a commit to CohenArthur/rust that referenced this issue Nov 20, 2023
Add a test to ensure issue rust-lang#89699 does not show up again. This test
emits an `async move` closure in a proc macro, which is used in a
test program compiled with edition 2015. We make sure the error message
is nice and shows up properly.
@bors bors closed this as completed in 7fd7dad Nov 21, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 21, 2023
Rollup merge of rust-lang#117973 - CohenArthur:fix-89699, r=lqd

test: Add test for async-move in 2015 Rust proc macro

Fixes rust-lang#89699

Ran cargo bisect-rustc to find when this was fixed exactly, which is in 474709a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macros Area: Procedural macros C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-edition Diagnostics: An error or lint that should account for edition differences. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants