break and continue are not hygienic

[flaper87]

Working on #6993 specifically on #12179 raised the fact that break and continue use Name as opposed of Ident. This was introduced in #9103 and unfortunately this change seems to have made them non-hygienic.

An example taken from @huonw comment:

#[feature(macro_rules)];

macro_rules! foo {
    ($e: expr) => {
        // $e shouldn't be able to interact with this 'x
        'x: loop { $e }
    }
}

fn main() {
    'x: loop {
        // i.e. this 'x should refer to the outer loop, but may be "captured"
        // by the 'x declaration inside foo
        foo!(break 'x);
        println!("should not be printed")
    }
}

Presumably, reverting the change should make them hygienic again.

I volunteer as a mentor for this fix.

[huonw]

Note that #9047 was caused by (semi-)hygienic break & continue.

[edwardw]

My first try here: edwardw@4e06bc6

I may not fully revert the change of #9103 since three new tests still fail.

[flaper87]

@edwardw Thanks, looking good. I think you can squash those test in a single test that ensures break and continue are hygienic.

Also, could you please add this test too?

macro_rules! foo {
    () => {
        // this should just break from this inner loop...
        'x: loop { break 'x; }
    }
}

'x: loop {
    // ...but it may refer to the outer 'x when used inside here
    foo!();
    println!("should be printed");
}

[flaper87]

I may not fully revert the change of #9103 since three new tests still fail.

@edwardw Sorry, did you mean that with your changes those tests still fail?

[edwardw]

Yes it was even with this change seconds ago: edwardw@9427d36

[edwardw]

I noticed two things. One is that ast::NodeId seems to play a more important role in trans now. The other is rustc::middle::trans::controlflow::trans_break_cont has been radically changed since #9103.

[huonw]

@edwardw I think you need to be renaming the labels as you register them, e.g.:

diff --git a/src/librustc/middle/resolve.rs b/src/librustc/middle/resolve.rs
index 7eb9b23..99ce3f9 100644
--- a/src/librustc/middle/resolve.rs
+++ b/src/librustc/middle/resolve.rs
@@ -5177,7 +5177,7 @@ impl Resolver {
                         let rib = label_ribs.get()[label_ribs.get().len() -
                                                    1];
                         let mut bindings = rib.bindings.borrow_mut();
-                        bindings.get().insert(label.name, def_like);
+                        bindings.get().insert(mtwt_resolve(label), def_like);
                     }

                     visit::walk_expr(this, expr, ());

(Just building this to test it now.)

(You'll need to adjust the comments too.)

[huonw]

Also, I'd recommend that you have some compile-fail tests to double/triple check the hygiene is working, e.g.

macro_rules! foo { () => { break 'x; } } //~ ERROR use of undeclared label `x`

fn main() {
    'x: loop { foo!() }
}

macro_rules! foo { ($e: expr) => { 'x: loop { $e } } }

fn main() {
    foo!(break 'x); //~ ERROR use of undeclared label `x`
}

[huonw]

Ah, so the problem is actually the macro expansion never renames these idents... see syntax::ext::expand, e.g. the new_pending_renames interactions in expand_stmt (expand_expr would need something similar).

[edwardw]

Tried to rename label of for loop: edwardw@9054b80 but the tests still fail.

[flaper87]

Just a quick note based on your last commit. You need to do the rename for both ExprForLoop and ExprLoop

[edwardw]

Oh, I leave ExprLoop out on purpose for now. I was hoping at least that for loop in main() and infinite loop in macro would work hygienically with the patch. Alas, even that fails.

[huonw]

@edwardw it might be easier to get feedback and iterate if you open a PR even if it's not ready (for commenting, etc.).

[edwardw]

Can we remove the E-easy label? This one is far from easy.