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?

[huonw]

That makes every Ident a byte (and with alignment, probably a word) larger, likely to cause a noticeable increase in compiler memory use. (Feel free to experiment with it, though.)

[edwardw]

Another (hopefully better) idea. There are two separated concerns here, AST shape and hygiene. They are orthogonal to each other so we treat them as such. Keep current AST nodes intact but make MTWT family of methods take an additional argument which indicates the namespace for hygiene operations:

pub enum IdentKind {
    PlainIdent,
    LifetimeIdent
}

mtwt_resolve(...) => mtwt_resolve(..., IdentKind)
new_rename(...) => new_rename(..., IdentLind)
new_mark(...) => new_mark(..., IdentKind)

Implementation-wise, we simply have multiple SCTable's, one for each IdentKind.

[huonw]

@edwardw I guess that might work, but it seems like you still end with things being renamed incorrectly. However, you have more experience than me of the implementation of hygiene, so if you think it might work, experiment away.

[huonw]

A concrete example of value and lifetime names interacting (just of the top of my head). If we had a "lifetime shorthand" syntax that allowed converting

fn foo<'a>(x: &'a Foo) -> &'a Bar { ... }

into

fn foo(x: &Foo) -> &'x Bar { ... }

presumably the variable and lifetime would need renaming together.

[huonw]

Nominating.

[edwardw]

Bump, we still need to make a decision about this.

[huonw]

Loop-label hygiene has actually been filed before: #9171 (not closing either of these as dupes, since this bug isn't actually about loop hygiene directly, e.g. one way to stop this being confusing would be a compiler error about labels and variables sharing a name).

[alexcrichton]

Is there a nice summary of the status of both this issue and #12563? Both have quite long conversations which seem to go into much detail, and it would be nice to see a more concise description of what's going on.

[edwardw]

(My) Summary

Why is this issue happening?

The hygienic label works by renaming itself with respect to its lexical scope. The renaming of a label needs to be propagated into its loop body for break or continue to pick up so that they refer to the right renamed label. Unfortunately, both label and normal identifier are represented as ast::Ident so any identifiers inside that loop body with the same name will pick up the renaming too and be erroneously (IMO) be renamed.

What #12563 has proposed

#12563 tried to add namespaces to renaming operation so that label and normal identifier won't collide.

jbclements' concern and suggestion

@jbclements claims that such behavior may very well be what we want. By hardcoding namespace, we may restrict the ability of macro expansion. He then suggests to have one flat namespace as-is but try to distinguish them 'symbolically', i.e. label not only has ' in it textually but also compiler internally. It is another way of doing namespace if I understand him correctly, only more free-form.

[pnkfelix]

Assigning 1.0, P-backcompat-lang.

[jbclements]

Could we arrange a video call on this topic? I think it might clarify things.

[pnkfelix]

@jbclements I imagine such a thing could be done, though I would not be the one to arrange it. You may want to send an email with perhaps a suggestion of whom to include on the call?

(Also, I'm still curious to know the original motivation for your suggestion that we make lifetimes Names instead of Idents, I haven't been able the determine the motivation for that.)

[jbclements]

I hadn't made them hygienic, so I thought we might as well clean that up and just make them Names. If we're going to make them hygienic, though, I cheerfully retract that suggestion entirely.

[pnkfelix]

@jbclements ah, i see the light: we hadn't had the necessary machinery to support them as properly hygienic, so it was misleading to classify them as Idents, okay.

But yes, I think we should go the other direction and make them properly hygienic idents. Okay okay.

[edwardw]

Now the hygienic label has been documented: PR 13106. It would be better if we could decide what to do about this bug.