macros sometimes allow expr followed by ident

4848ce8
Opened by Alex Burka at 2018-10-26 15:15:22

You can use a repetition to get around the future proofer.

macro_rules! bad {
    ($e:expr $i:ident) => {} //~ERROR
}

macro_rules! sneaky {
    ($($i:ident $e:expr)*) => {} // no error
}

fn main() {
    sneaky!(a b c d);
}

I feel that both of these should be accepted, or neither. BTW, I am using the equivalent of sneaky! in brainmunch (found this while preparing my RustFest talk).

Fixing this by disallowing both macros would require cratering.

cc @jseyfried @LeoTestard

  1. cc @pnkfelix

    Aaron Turon at 2017-10-19 19:45:57

  2. The danger here is that contextual keywords become impossible. In particular, consider if we added await f as an expression, which would work with bad! today but fail tomorrow.

    Niko Matsakis at 2017-10-19 19:48:11

  3. One possibility would be to leave this alone and fix it in macros 2.0. The other would be a crater run and evaluation for a fix.

    Josh Triplett at 2017-10-19 19:50:27

  4. Some possible routes:

    • I don't think we should accept bad!, for the reasons given above.
    • We could leave this as is -- we have said in the past that when you break the forwards compatibility rules "in spirit" you may get broken.
    • Or, better really, we would do a fix and try to do a crater run.

    Niko Matsakis at 2017-10-19 19:51:43

  5. triage: P-medium

    Niko Matsakis at 2017-10-19 19:52:03

  6. I feel like we should at least try the fix -- though it may be hard to ascertain the impact entirely.

    Niko Matsakis at 2017-10-19 19:56:50

  7. @nikomatsakis do you have any idea how to fix it?

    Alex Burka at 2017-10-25 21:26:16

  8. I'm afraid fixing this is going to be problematic, because even we are using this "feature" in rustc. I'll post a PR with the fix (including changing our mis-uses), but will expect there to be breakage. If that is the case after a crater run, should we still include the fix as a warning?

    Esteban Kuber at 2018-10-26 02:13:33

  9. Wow, that's a lot of fallout in the PR! IMO yes, assuming crater shows nonzero breakage, add a warning at the definition site (callers too?) for macros 1.0 and fix it in macros 2.0.

    On Thu, Oct 25, 2018, 22:14 Esteban Kuber notifications@github.com wrote:

    I'm afraid fixing this is going to be problematic, because even we are using this "feature" in rustc https://github.com/rust-lang/rust/blob/4bd4e4130ed531a644263db26bf8461704215c77/src/libcore/iter/range.rs#L75. I'll post a PR with the fix (including changing our mis-uses), but will expect there to be breakage. If that is the case after a crater run, should we still include the fix as a warning?

    — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/44975#issuecomment-433263449, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3n0xEXdHT1jR698kC1GUcON_vvt6fks5uom_qgaJpZM4PqiGz .

    Alex Burka at 2018-10-26 13:51:06

  10. (callers too?)

    @durka It'd be nice to do, but I'm tempted at leaving devs to deal with the fallout after changing the signature by following the errors they'll get then instead of trying to preempt them, as extending the diagnostic to the callers would be a bigger PR, I fear.

    Esteban Kuber at 2018-10-26 15:15:22