Loop label hides variables with same name

[sfackler]

This compiled as of yesterday, but no longer does:

fn main() {
    let mut foo = ~[];
    'foo: for i in [1, 2, 3].iter() {
        foo.push(i);
    }
}

test.rs:4:9: 4:12 error: unresolved name `foo`.
test.rs:4         foo.push(i);
                  ^~~
error: aborting due to previous error

Renaming either the label or the variable fixes the error.

rustc 0.10-pre (329fcd4 2014-02-23 15:37:05 -0800)
host: x86_64-unknown-linux-gnu]

cc @edwardw

[edwardw]

Oh dear, this is bad. #12488 claims its first victim.

[huonw]

I think this should be as "simple" as making sure the renaming for lifetime variables only renames lifetime variables, and then the renaming for local variables only renaming non-lifetime-idents.

[edwardw]

Some discussions on IRC:

edwardw: to fix #12512, I feel like I must add a new AST node and then add a new visit_* or fold_* method
         to traits that traverse an AST. is that too big a change to propose?
...
eddyb: I guess we only perform renaming in one of 4 namespaces
edwardw: not after my hygiene label patch landed :)
edwardw: which also introduces the name clashing bug
eddyb: bleah, this kinda introduces the need for more resolve-like logic in expansion
       we could intertwine then, but that would cause all sorts of weird things
edwardw: eddyb: it already did https://github.com/mozilla/rust/issues/12512
eddyb: mkay, so what I would do instead, is have more renaming kinds
       one for each namespace
edwardw: exactly, that's why I'm thinking of having a Label AST node
...
eddyb: *record* the renaming
       expansion doesn't rename anything
       it just records renames
       (now that I think of it, more SyntaxContext_ variants will be harder than having another field in the Rename variant, like a RenameKind)
edwardw: that's another way to introduce namespaces for renaming
eddyb: mhmm
       and it's future-proof
edwardw: so you like this idea more than a Label AST node, yes?
eddyb: (I don't like it for other reasons, but it's the least threatening evil IMO)
edwardw: a Label node is kind evil, lol
eddyb: edwardw: well, yeah, I like how rename kinds can work even with namespaces, where just the AST shape won't help
       and/or would require duplicating resolve logic in expansion
       but it warrants a discussion with more than just the two of us
edwardw: sounds reasonably. let me sleep on the ideas and come up with a gist for further discussion

[edwardw]

@huonw, it is more complicated. In token level, a label is a LIFETIME. Then in expansion pass, it becomes ast::Ident, although not independent from ExprForLoop, ExprLoop, ExprBreak or ExprAgain. Then again in resolve pass, it lives in label_ribs, not value_ribs as other variables do, which I believe is the root cause.

This really is the identity (no pun intended) crisis for poor labels.

[huonw]

Well, what I said would semantically be a fix; it might not correspond exactly to how the code is structured in the compiler at the moment (one way would be detecting whether you're rewriting a ForLoop, Loop, Break or Again in the folder and only then modifying/not modifying as appropriate).

[edwardw]

That's exactly what annoys me now. Given the recursive nature of AST traversing, it is not clear how to distinguish one modification from another. (In the end, they are all Ident).

[huonw]

You could lift the renaming up one level, so it happens in fold_expr rather than fold_ident (this is pretty ugly).

Or, add a "ident source" field to fold_ident, passing it

enum IdentSource {
     PathSegment,
     TypeParam,
     Lifetime,
     LoopLabel,
}

and then adjusting the folder to pass this down into each fold_ident call (i.e. it has signature fn fold_ident(&mut self, id: Ident, source: IdentSource) -> Ident). This would allow you to check if you're meant to be renaming the ident, after adding a ident_type_to_fold: IdentSource field to the renaming folder (or some other technique to tell it to only rename lifetimes vs. locals).

(Also rather ugly, but probably less error prone.)

[edwardw]

Will it be sufficient to also address your concern about hygienic lifetimes?

[huonw]

I guess it will, if support for hygienic lifetimes is added.

[edwardw]

To be honest I feel a bit unsettled. It looks like a typical impedance mismatch somewhere in the parsing pipeline although I couldn't really put my finger on it, yet.

[huonw]

(Hm, I guess such a fine-grained IdentSource won't work in token-trees (and it needs to work on them), so it would just have to be PlainIdent vs LifetimeIdent, I guess.)

[edwardw]

Then it won't be able to handle label vs. true lifetime if we are to add hygienic lifetimes.

[huonw]

I think that's a fundamental problem that we can't solve; but, in any case, it doesn't seem unreasonable for all '<ident> things to live in the same namespace (especially if we ever get the explicit named-lifetimes-on-blocks that I think I've seen mentioned somewhere).

[edwardw]

How about

pub struct Ident {
    name: Name,
    ctxt: SyntaxContext,
    kind: IdentKind
}

pub enum IdentKind {
    PlainIdent,
    LifetimeIdent
}

Will this somehow intervene with codegen, which I haven't had a chance to look into?