Skip to content

failed to parse if_chain macro invocation #7845

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

Closed
digama0 opened this issue Mar 2, 2021 · 7 comments · Fixed by #8044
Closed

failed to parse if_chain macro invocation #7845

digama0 opened this issue Mar 2, 2021 · 7 comments · Fixed by #8044
Labels
A-macro macro expansion E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now

Comments

@digama0
Copy link
Contributor

digama0 commented Mar 2, 2021

Here's a simple version of the problem using the if_chain macro:

#[macro_use] extern crate if_chain;

fn main() {
  if_chain! {
    then { let _ = drop(()); }
  }
}

In rustc this works fine, but rust-analyzer marks the (()) as failed to parse macro invocation. Note that if_chain! is a macro_rules macro, so this is an issue in macro expansion. The contents of the then { ... } branch is matched using the matcher then { $($then:tt)* } so even though this portion of the code is mostly untouched by the macro (it's supposed to expand to just let _ = drop(());) it is matched as a list of token trees, of which (()) is one of them. The let _ = is also part of the error, so it may be getting confused with the other matches in the macro.

@edwin0cheng edwin0cheng added the A-macro macro expansion label Mar 2, 2021
@digama0
Copy link
Contributor Author

digama0 commented Mar 2, 2021

Here's a MWE without the use of if_chain:

fn main() {
  macro_rules! identity { ($($es:tt)*) => { $($es)* } }
  identity! { let _ = (); }
}

Adding any tokens after the call to identity, or a semicolon, fixes the error, so I think it has something to do with the fact that this expands directly to a sequence of statements and is used in expression context.

@edwin0cheng
Copy link
Member

edwin0cheng commented Mar 2, 2021

[deleted]

@digama0
Copy link
Contributor Author

digama0 commented Mar 2, 2021

Are you sure? In the simplified version there is no alternate case to match.

@edwin0cheng
Copy link
Member

Yeah, you are right, it is another issue.

@edwin0cheng edwin0cheng added the S-actionable Someone could pick this issue up and work on it right now label Mar 2, 2021
@edwin0cheng
Copy link
Member

edwin0cheng commented Mar 2, 2021

Diagnosis

The syntax tree of that MWE (without semicolon) :

  BLOCK_EXPR
     MACRO_CALL

But with semicolon:

BLOCK_EXPR
   EXPR_STMT
       MACRO_CALL

And in to_fragment_kind: (The function we check what kind of macro should expand)

https://github.com/rust-analyzer/rust-analyzer/blob/b7fa6dfabc7a5326c78aa19807fd6c30ca5d1b4b/crates/hir_expand/src/db.rs#L407-L409

So that's why the one without semi colon we will treat it as expr and such that it failed to expand.

On the other hand, in body lowering code:

https://github.com/rust-analyzer/rust-analyzer/blob/b7fa6dfabc7a5326c78aa19807fd6c30ca5d1b4b/crates/hir_def/src/body/lower.rs#L543

We only handle expr macro expansion in expression collecting code.

Fixes

So, to fix this bug, we have to handle MacroStmts in lowering AND change to_fragment_kind function I mentioned above.

@edwin0cheng edwin0cheng added the E-has-instructions Issue has some instructions and pointers to code to get started label Mar 2, 2021
@digama0
Copy link
Contributor Author

digama0 commented Mar 2, 2021

Note that adding a semicolon is not necessarily an option, for example in

fn foo() -> bool {
  macro_rules! identity { ($($es:tt)*) => { $($es)* } }
  identity! { let _ = (); true }
}

adding a semicolon would cause a typechecking error. It would be an option to introduce an extra block scope here, i.e. assume that identity! yields an EXPR and then after macro expansion use the expression { let _ = (); true } even though the macro doesn't ask for braces. This might not always work though, since braces sometimes cause a change to drop order and whether things are moved vs reborrowed.

@lambda-fairy
Copy link

For reference, this is how rustc expands @digama0's snippet:

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std;
fn foo() -> bool {
    macro_rules! identity { ($ ($ es : tt) *) => { $ ($ es) * } }
    let _ = ();
    true
}

So the tokens are substituted right in, with no extra braces or semicolons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macro macro expansion E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants