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

syn::parse_str<Ident>() panics inside a proc-macro on "'1" #749

Closed
abonander opened this issue Jan 17, 2020 · 4 comments · Fixed by dtolnay/proc-macro2#211
Closed

syn::parse_str<Ident>() panics inside a proc-macro on "'1" #749

abonander opened this issue Jan 17, 2020 · 4 comments · Fixed by dtolnay/proc-macro2#211

Comments

@abonander
Copy link

abonander commented Jan 17, 2020

This is pretty peculiar.

If you run the following code snippet in a regular program, it prints an error as expected:

fn main() {
    println!("{:?}", syn::parse_str::<syn::Ident>("'1")) // Err(Error("LexError"))
}

However, if you write it in a proc-macro, the proc-macro panics:

// definition
#[proc_macro]
pub fn parse_as_ident(input: TokenStream) -> TokenStream {
    let _ = syn::parse_str::<syn::Ident>("'1");
    TokenStream::new()
}

// usage (I discovered this in `sqlx` so it was easiest to just add this to `sqlx_macros` to demo)
sqlx::sqlx_macros::parse_as_ident!();

you get the following:

error: lifetimes cannot start with a number
   --> tests/postgres-macros.rs:141:1
    |
141 | sqlx::sqlx_macros::parse_as_ident!();
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ in this macro invocation

error: proc macro panicked
   --> tests/postgres-macros.rs:141:1
    |
141 | sqlx::sqlx_macros::parse_as_ident!();
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: message: `"1"` is not a valid identifier

error: aborting due to 2 previous errors

I've captured the panic backtrace in a gist (parse_str() call is frame 18): https://gist.github.com/abonander/02fa0e207002f447ae7c5f77f687c8d6

The actual error is in constructing the ParseStream which suggests a behavior discrepancy in proc_macro2 between running in a proc-macro vs not, however I expected parse_str() to emit an error instead of panic (as documented on Ident::new(): https://docs.rs/syn/1.0.13/syn/struct.Ident.html#panics) so I leave it up to you to decide where it belongs.

cc launchbadge/sqlx#53

@dtolnay
Copy link
Owner

dtolnay commented Jan 17, 2020

This is a compiler bug. rust-lang/rust#58736

@abonander
Copy link
Author

A simple fix in our case is to check for invalid characters ourselves:

    let is_valid_ident = name.chars()
        .all(|c| c.is_alphanumeric() || c == '_');

    if is_valid_ident {
        if let Ok(ident) = syn::parse_str(name) {
            return Ok(ident)
        }
    }

    // emit our own error

@dtolnay
Copy link
Owner

dtolnay commented Jan 17, 2020

I added a catch_unwind in proc-macro2 1.0.8, hopefully that helps! I think rustc will still write its own diagnostic out to the console from inside proc_macro::TokenStream::from_str which is not always desirable...

Be aware there is a separate issue that proc_macro::TokenStream::from_str will sometimes return Ok on invalid inputs: rust-lang/rust#58772.

@abonander
Copy link
Author

abonander commented Jan 17, 2020

Thanks for clarifying, and the quick fix!

I imagine the workaround I came up with will avoid emitting the extra diagnostics noise anyway so it's probably worth keeping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants