Make break and continue hygienic #12338

edwardw · 2014-02-17T06:08:28Z

Makes labelled loops hygiene by performing renaming of the labels defined in e.g. 'x: loop { ... } and then used in break and continue statements within loop body so that they act hygienically when used with macros.

Closes #12262.

huonw · 2014-02-17T07:05:20Z

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);


One way to check that this is doing something sensible is to write something like

let renamed = mtwt_resolve(label); info!("ExprLoop label: {:?} -> {:?}", label, renamed); bindings.get().insert(renamed, def_like);

and then run the compiler on the test cases like RUST_LOG=rustc::middle::resolve=info .../bin/rustc testcase.rs to check if it's being renamed or not.

(I suggest using info! rather than debug! for now so that you can avoid printing all the other debug! statements in this module and focus on just what you're interested in.)

emberian · 2014-02-17T07:23:44Z

cc @paulstansifer @jbclements

edwardw · 2014-02-17T08:33:39Z

src/test/run-pass/hygienic-continue-in-for-loop.rs

+fn main() {
+    'x: for _ in range(0, 1) {
+        // this 'x should refer to the outer loop, lexically
+        run_once!(continue 'x);


Interesting output from info! inside rustc::middle::resolve. I saw some phantom continue presumably because the same continue gets visited and renamed twice here and inside the macro above:

ExprLoop label: syntax::ast::Ident{name: 59u32, ctxt: 22u32} -> 258u32 ExprBreak/ExprAgain label: syntax::ast::Ident{name: 59u32, ctxt: 22u32} -> 258u32 ExprLoop label: syntax::ast::Ident{name: 59u32, ctxt: 26u32} -> 260u32 ExprBreak/ExprAgain label: syntax::ast::Ident{name: 59u32, ctxt: 26u32} -> 260u32 ExprBreak/ExprAgain label: syntax::ast::Ident{name: 59u32, ctxt: 26u32} -> 260u32

Judging from the same debug output, it seems that SyntaxContext is more important here. Where is label assigned a SyntaxContext?

Hm... it should only be visited once, in place in the expansion (since expansion runs and finishes before resolve starts). I guess resolve might be doing multiple passes...

But it certainly seems to be distinguishing between labels with the same name.

(I think the SyntaxContext is what is set by the ident renaming step of macro expansion.)

paulstansifer · 2014-02-17T15:19:27Z

Nifty!

alexcrichton · 2014-02-18T16:18:15Z

I don't personally understand the implementation of hygiene that much, but this seems pretty reasonable. Is this ready for merging?

alexcrichton · 2014-02-18T16:19:38Z

src/libsyntax/ext/expand.rs

@@ -166,7 +180,7 @@ pub fn expand_expr(e: @ast::Expr, fld: &mut MacroExpander) -> @ast::Expr {
            // `None => break ['<ident>];`
            let none_arm = {
                // FIXME #6993: this map goes away:


Should this be removed?

edwardw · 2014-02-18T16:34:42Z

No, it doesn't work yet. The label seems to be renamed twice by enclosing lexical scope and macro so the net effect is that break / continue still refers to the label inside macro.

edwardw · 2014-02-19T19:10:41Z

cc #10607. It looks like renaming can be costly.

edwardw · 2014-02-19T19:59:26Z

Let's at least get tests as comprehensive as possible first. Right now there are five of them, three run-pass and two compile-fail; all don't work yet.

jbclements · 2014-02-19T20:19:15Z

I'm not sure I'm reading your code correctly, but it looks like you might be performing renaming too eagerly, adding it to the pending renames and also applying it directly. This could explain your renaming-twice issue, or it could VERY WELL BE that I'm just misreading your code.

In any case: great to see people working on hygiene for loop labels!

edwardw · 2014-02-20T06:46:13Z

The current renaming approach does work for one case and one case only:

macro_rules! foo {
    () => { break 'x; }
}

pub fn main() {
    'x: for _ in range(0,1) { foo!() } //~ ERROR use of undeclared label `x`
}

Although the error message is not very accurate; it could be reported as:

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

pub fn main() {
    'x: for _ in range(0,1) { foo!() }
}

edwardw · 2014-02-21T06:35:43Z

I'm not sure the hygienic label can work using renaming given the current macro expansion mechanism. Consider the following:

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

fn main() {
    'x: loop {
        // this 'x should refer to the outer loop, lexically
        loop_x!(break 'x);
        assert!(false);
    }
}

As far as hygiene is concerned, there are three types of labels:

those defined and used as part of non-macro rust program
those used as arguments for macro invocation
those defined and used as part of macro definition

To work hygienically, type 1 and type 2 labels are lexically in one group and type 3 the other. If they clash, one group or the other need to be renamed. The renaming can only be performed on a fully realized AST tree. The problem is, given the current macro expansion mechanism, type 2 and type 3 labels are parsed together (in the macro expand function). After an AST has been constructed, they can no longer be distinguished from each other.

jbclements · 2014-02-21T07:30:19Z

No, I believe you misunderstand how renaming works. The renaming is applied to every hygienically-treated identifier, and it definitely does not require a fully-expanded AST. Without getting into the really interesting details, in this example the outer 'x would get a fresh rename that applies to the macro argument before the expansion of the macro occurs. Things can get really complicated, but I believe what you're looking at here is one of the simpler cases.

Again, if I understand you, you can try this using simple let-bound variables. Is there something that would make labels different?

edwardw · 2014-02-21T07:36:42Z

But the outer 'x and the macro argument 'x need to be renamed together, no? Or the first 'x inside the macro definition is renamed alone.

edwardw · 2014-02-21T07:46:30Z

Yes, my comment about renaming being performed on a fully expanded AST tree was totally wrong; it can be done in the process of expanding AST tree. The type 2 and type 3 labels being parsed together is till true so that I couldn't find a way to rename outer 'x and macro argument 'x together or the 'x inside macro definition alone.

jbclements · 2014-02-21T19:05:33Z

When you see the outer 'x, it goes on a list of pending renames that gets applied to the argument to the macro. Right?

edwardw · 2014-02-21T19:30:24Z

I think I figured it out. This fold.rs#L374 holds the key. After being tokenized and before being parsed, a label is represented as token::LIFETIME, not token::IDENT as other identifiers do. It should be folded as part of renaming process as well.

But #7743 blocked the renaming of lifetimes. I'm fixing that first right now.

huonw · 2014-02-22T12:49:03Z

This needs a rebase (feel free to have it rebased on top of your branch for #12451; GitHub normally automatically copes pretty well when one lands before the other).

edwardw · 2014-02-22T12:56:23Z

Will do a rebase. I'm fighting with lifetime resolving now since I turned on ast::LIFETIME rename; things usually go more broken first before they are finally worked out.

edwardw · 2014-02-22T23:19:44Z

The new tests should pass now. Although it works, to be honest I'm not entirely sure about this patch. But let's see what you guys have to say about it.

In the meantime, I need to run full test suite locally and add even more tests.

edwardw · 2014-02-23T05:52:03Z

Comment addressed and more tests added. I think this is ready. (Note: the patch should be applied on top of #12451 if reviewers are to try it locally).

EDIT: "Represent lifetimes as Names" one is actually equivalent to #12451.

Makes labelled loops hygiene by performing renaming of the labels defined in e.g. `'x: loop { ... }` and then used in break and continue statements within loop body so that they act hygienically when used with macros. Closes rust-lang#12262.

edwardw · 2014-02-23T14:30:54Z

r?

huonw · 2014-02-23T23:07:00Z

@edwardw since you now (mostly) understand how hygiene works, you could experiment with #9383 and/or #9384 if you feel like it (no obligation at all :) ).

edwardw · 2014-02-23T23:11:56Z

I'd like to try :P

Makes labelled loops hygiene by performing renaming of the labels defined in e.g. `'x: loop { ... }` and then used in break and continue statements within loop body so that they act hygienically when used with macros. Closes #12262.

fix: Fix broken async callback in join lines Fixes rust-lang#12338.

[`infinite_loops`]: fix incorrect suggestions on async functions/closures closes: rust-lang#12338 I intend to fix this in rust-lang#12421 but got distracted by some other problems in the same lint, delaying the process of closing the actual issue. So here's a separated PR that only focus on the issue and nothing else. --- changelog: [`infinite_loops`]: fix suggestion error on async functions/closures

huonw reviewed Feb 17, 2014
View reviewed changes

edwardw reviewed Feb 17, 2014
View reviewed changes

alexcrichton reviewed Feb 18, 2014
View reviewed changes

edwardw mentioned this pull request Feb 23, 2014

Consider pass macro invocation arguments as AST instead of TokenTree #12488

Closed

Make break and continue hygienic

386db05

Makes labelled loops hygiene by performing renaming of the labels defined in e.g. `'x: loop { ... }` and then used in break and continue statements within loop body so that they act hygienically when used with macros. Closes rust-lang#12262.

bors closed this Feb 24, 2014

bors merged commit 386db05 into rust-lang:master Feb 24, 2014

edwardw deleted the hygienic-break-continue branch February 24, 2014 05:35

edwardw mentioned this pull request Feb 27, 2014

Represent lifetimes as Names instead of Idents #12451

Merged

edwardw mentioned this pull request Mar 9, 2014

unhygienic named labels allowed in macros #9171

Closed

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022

Auto merge of rust-lang#12342 - lnicola:fix-join-lines, r=lnicola

3de03d4

fix: Fix broken async callback in join lines Fixes rust-lang#12338.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make break and continue hygienic #12338

Make break and continue hygienic #12338

edwardw commented Feb 17, 2014

huonw Feb 17, 2014

emberian commented Feb 17, 2014

edwardw Feb 17, 2014

huonw Feb 17, 2014

paulstansifer commented Feb 17, 2014

alexcrichton commented Feb 18, 2014

alexcrichton Feb 18, 2014

edwardw Feb 23, 2014

edwardw commented Feb 18, 2014

edwardw commented Feb 19, 2014

edwardw commented Feb 19, 2014

jbclements commented Feb 19, 2014

edwardw commented Feb 20, 2014

edwardw commented Feb 21, 2014

jbclements commented Feb 21, 2014

edwardw commented Feb 21, 2014

edwardw commented Feb 21, 2014

jbclements commented Feb 21, 2014

edwardw commented Feb 21, 2014

huonw commented Feb 22, 2014

edwardw commented Feb 22, 2014

edwardw commented Feb 22, 2014

edwardw commented Feb 23, 2014

edwardw commented Feb 23, 2014

huonw commented Feb 23, 2014

edwardw commented Feb 23, 2014

Make break and continue hygienic #12338

Make break and continue hygienic #12338

Conversation

edwardw commented Feb 17, 2014

huonw Feb 17, 2014

Choose a reason for hiding this comment

emberian commented Feb 17, 2014

edwardw Feb 17, 2014

Choose a reason for hiding this comment

huonw Feb 17, 2014

Choose a reason for hiding this comment

paulstansifer commented Feb 17, 2014

alexcrichton commented Feb 18, 2014

alexcrichton Feb 18, 2014

Choose a reason for hiding this comment

edwardw Feb 23, 2014

Choose a reason for hiding this comment

edwardw commented Feb 18, 2014

edwardw commented Feb 19, 2014

edwardw commented Feb 19, 2014

jbclements commented Feb 19, 2014

edwardw commented Feb 20, 2014

edwardw commented Feb 21, 2014

jbclements commented Feb 21, 2014

edwardw commented Feb 21, 2014

edwardw commented Feb 21, 2014

jbclements commented Feb 21, 2014

edwardw commented Feb 21, 2014

huonw commented Feb 22, 2014

edwardw commented Feb 22, 2014

edwardw commented Feb 22, 2014

edwardw commented Feb 23, 2014

edwardw commented Feb 23, 2014

huonw commented Feb 23, 2014

edwardw commented Feb 23, 2014