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

Back-compat risk: same label can be attached to distinct blocks in the same scope #21633

Closed
pnkfelix opened this issue Jan 25, 2015 · 8 comments · Fixed by #24162
Closed

Back-compat risk: same label can be attached to distinct blocks in the same scope #21633

pnkfelix opened this issue Jan 25, 2015 · 8 comments · Fixed by #24162
Assignees
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority
Milestone

Comments

@pnkfelix
Copy link
Member

The current Rust alpha allows one to attach two lifetimes with the same name to two distinct loop blocks.

This is a backwards compatibility risk for hypothesized future versions of Rust where the lifetime labels can be used in e.g. type annotations (rather than solely used for labelled break/continue statements).

Example:

fn main() {
    let d = 3;
    'a: for i in (0..d).rev() {
        println!("i: {}", i);
    }

    'a: for i in (d..d*2).rev() {
        println!("i: {}", i);
    }
}
@pnkfelix
Copy link
Member Author

Nominating for P-backcompat-lang, 1.0 beta.

@arielb1
Copy link
Contributor

arielb1 commented Jan 28, 2015

isn't this just ordinary shadowing?

@pnkfelix
Copy link
Member Author

@arielb1 in my opinion ordinary shadowing would be when an nested scope is labeled with the same lifetime as a scope is nested within.

I see the two scopes here as siblings, at least in terms of what we might use 'a for in type annotations.

(Having said that, there is probably a coherent semantics that allows this based on shadowing.)

@arielb1
Copy link
Contributor

arielb1 commented Jan 29, 2015

The 'a's scope is the for loop's, just like the i.

@pnkfelix
Copy link
Member Author

@arielb1 ah. okay yes, that is another coherent semantics, I think. Not sure if its the one I would pick, but I do not see any obvious holes in it.

@brson brson added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jan 29, 2015
@pnkfelix
Copy link
Member Author

Polish issue for 1.0, P-low.

@pnkfelix pnkfelix added the P-low Low priority label Jan 29, 2015
@pnkfelix pnkfelix added this to the 1.0 milestone Jan 29, 2015
@pnkfelix pnkfelix self-assigned this Apr 2, 2015
@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 3, 2015

(i spent a little while looking into this. It may be a little tricky to do at certain phases of the compiler, because the current for-loop expansion moves the label into the inside of a block (basically implicitly building in the scoping semantics outlined by @arielb1 above), so I don't think I can meaningfully do the analysis post expansion, at least, not without knowing enough to skip over such blocks as not introducing an inner scope.)

current branch is here: pnkfelix@fsk-detect-duplicate-loop-labels


semi-ironically, I think the fact that the loop-label ends up inside a block in the expansion is my own fault; due to #21984

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 7, 2015

@nikomatsakis suggests that we strengthen this to simply disallow any duplicate labels in the body of an entire function. That would certainly resolve my above problem.

Update: posted general warning that we'll be adopting this strategy to internals: http://internals.rust-lang.org/t/psa-rejecting-duplicate-loop-labels/1833

steveklabnik added a commit to steveklabnik/rust that referenced this issue Apr 10, 2015
bors added a commit that referenced this issue Apr 21, 2015
…ikomatsakis

Check for duplicate loop labels in function bodies.

See also: http://internals.rust-lang.org/t/psa-rejecting-duplicate-loop-labels/1833

The change, which we are putting in as future-proofing in preparation for future potential additions to the language (namely labeling arbitrary blocks and using those labels in borrow expressions), means that code like this will start emitting warnings:

```rust
fn main() {
    { 'a: loop { break; } }
    { 'a: loop { break; } }
}
```

To make the above code compile without warnings, write this instead:

```rust
fn main() {
    { 'a: loop { break; } }
    { 'b: loop { break; } }
}
```

Since this change is only introducing a new warnings, this change is non-breaking.

Fix #21633
alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 21, 2015
…abels

Check for duplicate loop labels in function bodies.

See also: http://internals.rust-lang.org/t/psa-rejecting-duplicate-loop-labels/1833

The change, which we are putting in as future-proofing in preparation for future potential additions to the language (namely labeling arbitrary blocks and using those labels in borrow expressions), means that code like this will start emitting warnings:

```rust
fn main() {
    { 'a: loop { break; } }
    { 'a: loop { break; } }
}
```

To make the above code compile without warnings, write this instead:

```rust
fn main() {
    { 'a: loop { break; } }
    { 'b: loop { break; } }
}
```

Since this change is only introducing a new warnings, this change is non-breaking.

Fix rust-lang#21633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. P-low Low priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants