Macro hygiene for patterns breaks due to static/const in scope

[pnkfelix]

Does macro hygiene simply not work with respect to static identifiers in our model of syntax expansion?

For example:

macro_rules! foo {
    ($e:expr) => ({
        let value = $e;
        value
    })
}

fn main() {
    static value: i32 = 4; // comment out this line and things work
    let x = foo!(3);
    println!("x: {}", x);
}

I would have hoped that my use of the name value in the macro foo is irrelevant to the use of the name value in the body of main.

But currently one gets:

<anon>:3:13: 3:18 error: static variables cannot be referenced in a pattern, use a `const` instead
<anon>:3         let value = $e;
                     ^~~~~
<anon>:1:1: 6:2 note: in expansion of foo!
<anon>:10:13: 10:21 note: expansion site
error: aborting due to previous error
playpen: application terminated with error code 101

---

This is a shame because the box-desugaring #22181 expands into a { ... let value = ...; ... }, and there is a static binding of value in src/test/run-pass/drop-struct-as-object.rs

[pnkfelix]

(Also, even if I am wrong and we somehow do respect hygiene with respect to static identifiers, the pretty-printer probably needs a mode where it includes hygiene information to ensure identifiers are unique in the generated code, when the goal is to be able to recompile the output in the manner expected by our [pretty] test suite.)

[kmcallister]

We don't have hygiene for items at all. There's no way to resolve item definitions at the macro definition site, therefore something as simple as

macro_rules! foo {
    () => (None);
}

is un-hygienically capturing an item from the invocation's environment. This might be tolerable if it could not interfere with local bindings, but your example demonstrates this is not the case.

The problem here is that let x = ... can be either a definition or a use of x, depending on whether there's a const or static named x. This is pretty terrible, imo. This macro breakage only further illustrates the wisdom of distinguishing variables in patterns from structural matching (enum constructors, tuple structs, and const/static).

Rust uses capitalization for this distinction, but only by convention, not as part of the language. Perhaps there is a minimally-invasive way to enshrine the convention, just within the pattern grammar, where it seems most needed. More thoughts on this soon.

[kmcallister]

We could do this with two new pattern forms,

pat : ...
      | "its_an_item" pat
      | "its_a_variable" ident

(keywords / tokens tbb), where an unadorned ident is "inferred" based on the Unicode character class of the first character. I'd like to think and discuss a bit more before drafting an RFC.

[kmcallister]

Here is an example that does not involve let:

macro_rules! foo {
    ($e:expr) => {
        match $e {
            x => println!("{}", x),
        }
    }
}

fn main() {
    const x: u32 = 2;
    foo!(3);
}

<anon>:3:9: 5:10 error: non-exhaustive patterns: `_` not covered [E0004]
<anon>:3         match $e {
<anon>:4             x => println!("{}", x),
<anon>:5         }
<anon>:1:1: 7:2 note: in expansion of foo!
<anon>:11:5: 11:13 note: expansion site
<anon>:3:9: 5:10 help: pass `--explain E0004` to see a detailed explanation

[kmcallister]

the goal is to be able to recompile the output in the manner expected by our [pretty] test suite

I think that's a bug in our test suite. The compiler plugin docs explicitly point out that the --pretty expanded code can have a different meaning. I think rather that --pretty expanded,hygiene should output mangled identifiers -- currently it just comments each identifier. This should be a separate ticket though. But it is related: knowing whether to mangle an identifier's syntax context into its name on pretty printing a pattern requires figuring out whether it's a local binding or a use of an item.

[ghost]

@kmcallister

Personally I don't think the lack of general item name hygiene warrants a substantial change to the pattern grammar, especially this late in the game. You can also envision programs like the following:

fn foo() {}

macro_rules! invoke_foo(
    () => (foo())
);

fn main() {
    fn foo() { panic!(":("); }
    invoke_foo!();
}

and so I think the lack of hygiene for items is going to be problematic in other ways anyway.

As a user I don't necessarily mind your example not working as much as I think the diagnostic could be improved. I think even in the general case when you intend to use a binding as a catch-all pattern but it gets resolved to an item, we could be more precise and have

non-exhaustive patterns: `_` not covered`

be followed with a help note

perhaps you meant `x` to be a binding rather than a constant value pattern

[kmcallister]

I think the lack of hygiene for items is going to be problematic in other ways anyway

I agree, and fixing it is high on my list of priorities for macro rules two point oh. It'll be a big task, though.

I think interfering with local let bindings is worse than interfering with other items, though. Meanwhile there are other problems caused by the ambiguous binding structure of patterns.

[kmcallister]

Although it doesn't work today, I think we could accept x @ _ as unambiguous syntax for a variable binding, even when a const or an enum constructor named x happens to be in scope. It's tolerable, though pretty disappointing, to say that wildcards in match in macros should use x @ _. In fact we could write a lint for this — not by parsing macro_rules! definitions directly, but by walking the expanded AST and looking at every match that came out of a macro invocation. The main point of this lint is to move the error closer to the macro author, ideally into their test suite. Otherwise it's the unlucky user with a clashing const who gets a baffling error.

On the other hand I don't think

let x @ _ = ...

is an acceptable solution. let bindings are much more common than wildcard arms. And they're kind of the main point of macro hygiene. A let pattern is irrefutable, which means there's not much use for matching a const or a nullary enum constructor. Therefore, we could say that a bare ident as an irrefutable pattern is always a variable. This solves the hygiene problem, at the cost of making the pattern grammar less obvious.

I should reiterate that my preferred solution is a syntax change which makes all patterns locally unambiguous — probably by inferring from case, with explicit fallbacks like x @ _. However it is a scary change to make at this late hour.

[pnkfelix]

(closing as wont-fix; we cannot fix this backwards compatibly, and do not think it realistic to try to change this aspect of macro_rules! in time for 1.0.)

[kmcallister]

@pnkfelix: I think the fix where let x always parses as an ident because it's irrefutable is realistic on the 1.0 timeframe. If I drafted an RFC to that effect, would it be considered?

[pnkfelix]

@kmcallister I suspect I would support such an RFC, since it was with very heavy heart that I even closed this in the first place

[pnkfelix]

@kmcallister oh and the x @ _ trick is very interesting; I will have to investigate that further as a potential way to make robust binding forms within macros

[pnkfelix]

(reopening mostly to remind myself to look into why x @ _ does not work and whether I can easily fix it.)

[kmcallister]

look into why x @ _ does not work

I was just getting compiler errors before, but amazingly it gets worse: #22850

I think these are just bugs, though. We just need to be consistent about whether a_const_in_scope @ ... is a variable binding or a attempt to match the const (and it should clearly be a variable binding, imo).

[kmcallister]

I think situations where let x = ... parses as a reference to a const will never compile successfully. This code:

enum Foo { Bar }
const baz: Foo = Foo::Bar;
let baz = Foo::Bar;

produces

<anon>:4:9: 4:12 error: only irrefutable patterns allowed here
<anon>:4     let baz = Foo::Bar;
                 ^~~

even though, technically, the pattern is irrefutable.

This is good for us because it means that unconditionally parsing that as a binding occurrence of baz would be a backwards-compatible change. It's strictly increasing the set of programs that compile. I think the same goes for fixing x @ _.

[kmcallister]

Not going to have time to work on this for 1.0-final, sorry.

[pnkfelix]

I still want to try to look into this; assigning to self.

[mkpankov]

I also observed static variables cannot be referenced in a pattern in the following situation: http://is.gd/aAOIO0

static x: i32 = 0;

fn main() {
    println!("{}", say_hello());
}

fn say_hello() -> &str {
    "Hello World!"
}

fn foo(x: &i32) {
    &x
}

<anon>:11:8: 11:9 error: static variables cannot be referenced in a pattern, use a `const` instead
<anon>:11 fn foo(x: &i32) {
                 ^
error: aborting due to previous error
playpen: application terminated with error code 101

I find it strange, because I can't say I wanted to refer to that global static x in signature. I'd think formal parameters shouldn't be affected by statics at all.

[pnkfelix]

nota bene: The x @ _ trick may get fixed via PR #27349

[steveklabnik]

Today, this gives a different error:

error[E0530]: let bindings cannot shadow statics
  --> <anon>:3:13
   |
3  |         let value = $e;
   |             ^^^^^ cannot be named the same as a static
...
9  |     static value: i32 = 4; // comment out this line and things work
   |     ---------------------- a static `value` is defined here
10 |     let x = foo!(3);
   |             ------- in this macro invocation

I'm not sure if this ticket is still valid or not; I'm not sure that we're planning on making major changes to macro_rules, so this might be WONTFIX?

[Mark-Simulacrum]

I think the error we give is at least very clear so I'm going to close. It's unlikely we're going to make the changes that are asked for here for macro_rules specifically.

[upsuper]

Given that we now have editions, is it possible to fix this in a new edition so that we don't break backward compatibility?