Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Ident with Name in for loops #12179

Closed
wants to merge 1 commit into from

Conversation

flaper87
Copy link
Contributor

Cleaning up Ident usage in rustc. This patch replaces Ident usage in ExprFor and ExprForLoop

cc #6993

@flaper87
Copy link
Contributor Author

FWIW, I'll keep working on this cleanup. If we want to hold this PR off and merge them all, that's fine.

@huonw
Copy link
Member

huonw commented Feb 11, 2014

Doesn't this remove the possibility for 'foo: for x in bar() { ... } to be hygienic? i.e.

macro_rules! breakfoo( () => { break 'foo; } )
'foo: for x in bar() { breakfoo!() }

would compile and immediately break. I'm not sure (a) if I'm understanding this correctly, and (b) if we want this poor hygiene

(@paulstansifer @jbclements is my assessment above accurate: using Name rather than Ident makes it non-hygienic?)

@jbclements
Copy link
Contributor

@huonw your understanding sounds correct to me.

@alexcrichton
Copy link
Member

Is the issue correct in that we want to have a Name on label instead of an Ident? Or does this implementation just need some touch-up around the edges?

@flaper87
Copy link
Contributor Author

@alexcrichton I'd like to hold this patch until I verify @huonw's concerns w.r.t this change making for-loops non-hygienic. FWIW, I don't think this specific change makes for loops non-hygienic because what it is doing is replacing a bunch of ident.name with name.

At least for for loops, it looks like the only thing we're actually using from Ident is the name. I'm not sure about removing Ident all around, though.

@huonw
Copy link
Member

huonw commented Feb 13, 2014

FWIW, I don't think this specific change makes for loops non-hygienic because what it is doing is replacing a bunch of ident.name with name.

That's exactly what the difference between being hygienic and not is; the non-Name part of the Ident is what tells the compiler about where an ident comes from (i.e. which "macro expansion context" the ident was created in). Our loops are probably not hygienic at the moment, but hopefully they will be in future and so removing this only to add it back in later seems counter-productive.

@flaper87
Copy link
Contributor Author

@huonw I know. At this point I'd recommend discussing #6993 in the next meeting since it's targeting more than just for-loops and AFAIK some clean up was done already. /cc @alexcrichton

@huonw
Copy link
Member

huonw commented Feb 14, 2014

To be clear, lack of hygiene will mean (at least) one of the following will be incorrect:

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

or

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

'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")
}

(I don't have a compiler with me to see if either of these are currently broken.)

@huonw
Copy link
Member

huonw commented Feb 14, 2014

@flaper87 also, it's worth noting that #6993 is specifically "replace Ident by Name after libsyntax", i.e. libsyntax (and presumably the resolve pass) still needs Idents.

@huonw
Copy link
Member

huonw commented Feb 14, 2014

That said, it seems break and continue explicitly use Names: #9103.

@flaper87
Copy link
Contributor Author

also, it's worth noting that #6993 is specifically "replace Ident by Name after libsyntax", i.e. libsyntax (and presumably the resolve pass) still needs Idents.

Yeah, bad wording from my side!

I tested something similar to the first example yday and forgot to comment back here. Anyway, I gave both your examples a try with and without this change, as expected the behavior was the same in both cases. The first example works as expected whereas the second one doesn't.

Presumably, reverting #9103 should make our for-loops hygienic again.

I'll close this PR and work on bringing the hygienic behavior back.

Thanks @huonw and @alexcrichton

@flaper87 flaper87 closed this Feb 14, 2014
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 25, 2024
[`multiple_crate_versions`]: add a configuration option for allowed duplicate crates

Closes rust-lang#12176

changelog: [`multiple_crate_versions`]: add a configuration option for allowed duplicate crates
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 25, 2024
Suggest existing configuration option if one is found

While working on/testing rust-lang#12179, I made the mistake of using underscores instead of dashes for the field name in the clippy.toml file and ended up being confused for a few minutes until I found out what's wrong. With this change, clippy will suggest an existing field if there's one that's similar.
```
1 | allow_mixed_uninlined_format_args = true
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: perhaps you meant: `allow-mixed-uninlined-format-args`
```
(in hindsight, the current behavior of printing all the config options makes it obvious in most cases but I still think a suggestion like this would be nice to have)

I had to play around with the value a bit. A max distance of 5 seemed a bit too strong since it'd suggest changing `foobar` to `msrv`, which seemed odd, and 4 seemed just good enough to detect a typo of five underscores.

changelog: when an invalid field in clippy.toml is found, suggest the closest existing one if one is found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants