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

Fix expanding to short versions of constructs #8

Merged
merged 2 commits into from
Jan 29, 2019

Conversation

hcpl
Copy link
Contributor

@hcpl hcpl commented Dec 22, 2018

This PR fixes 2 issues:

  • The documentation shows that if_chain expands let and if let to equivalent Rust expressions, but that's not the case on latest master --- both expand to match expressions. This can be surprising when using macro expansion tools (e.g. cargo expand) and the result doesn't look like what one would expect after reading docs.
    Special handling for some cases should make the result clearer for users and potentially reduce burden placed on the optimizer, even if by a very small margin.
  • Block matchers (or any AST node matchers in general) and verbatim code interact poorly: after {} is matched with a block matcher, it can't be matched with literal {} anymore. When that happens in a certain recursion step, for all next recursion steps the macro match arm with {} becomes unreachable and the code gets expanded to if { ... } else { {} }.
    The fix is to not use block matchers in presence of verbatim matching code. Generic tt matchers should be used in this case.

Example that exhibits both problems:

if_chain! {
   if let Some(y) = x;
   if y.len() == 2;
   if let Some(z) = y.first();
   then {
       do_stuff_with(*z);
   }
}

Expansion before this PR:

match x {
    Some(y) => {
        if y.len() == 2 {
            match y.first() {
                Some(z) => {
                    do_stuff_with(*z);
                }
                _ => {}
            }
        } else {
            {}
        }
    }
    _ => {}
}

Expansion after this PR:

if let Some(y) = x {
    if y.len() == 2 {
        if let Some(z) = y.first() {
            do_stuff_with(*z);
        }
    }
}

I think if_chain should have macro expansion tests, but I don't know how to set up something like that (compiletest_rs seems to not support that case).

To do so, this commit replaces `$other:block` with `{ $($other:tt)* }`
and `{ $($other:tt)+ }`. This is because block matchers behave poorly
with literal empty blocks `{}`: after it is matched with the block
matcher it can't match literal `{}` anymore.

Also adds separate match arms for:
* `let` with a single pattern;
* `if let` with a single pattern;
* `if let` with a single pattern and a fallback.
@hcpl
Copy link
Contributor Author

hcpl commented Dec 22, 2018

Travis fails on Rust 1.11 because of the old behavior for tt matchers that has been fixed in Rust 1.12 (confirmed that locally).

Is it OK to bump to Rust 1.12?

@lambda-fairy
Copy link
Owner

Is it OK to bump to Rust 1.12?

Sure, go for it! (And Merry Christmas 🙂)

Rust 1.11 had an old behaviour for `tt` matchers that has been fixed in
Rust 1.12 (rust-lang/rust#34908).
@hcpl
Copy link
Contributor Author

hcpl commented Jan 27, 2019

Sure, go for it!

Done.

(And Merry Christmas 🙂)

Whoops, Merry Super Belated Christmas (sorry for not noticing earlier, hope you don't mind :-) )!

I like the way you've commented each branch -- thanks for doing this!

No problem!

@lambda-fairy lambda-fairy merged commit 0a3e156 into lambda-fairy:master Jan 29, 2019
@lambda-fairy
Copy link
Owner

Thanks!

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 this pull request may close these issues.

2 participants