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

Fix name resolution in lexical scopes #32141

Merged
merged 5 commits into from
Mar 13, 2016

Conversation

jseyfried
Copy link
Contributor

Currently, resolve_item_in_lexical_scope does not check the "ribs" (type parameters and local variables). This can allow items that should be shadowed by type parameters to be named.

For example,

struct T { i: i32 }
fn f<T>() {
    let t = T { i: 0 }; // This use of `T` resolves to the struct, not the type parameter
}

mod Foo {
    pub fn f() {}
}
fn g<Foo>() {
    Foo::f(); // This use of `Foo` resolves to the module, not the type parameter
}

This PR changes resolve_item_in_lexical_scope so that it fails when the item is shadowed by a rib (fixes #32120).
This is a [breaking-change], but it looks unlikely to cause breakage in practice.

r? @nikomatsakis

@jseyfried
Copy link
Contributor Author

cc @petrochenkov @arielb1

@arielb1
Copy link
Contributor

arielb1 commented Mar 9, 2016

Can this cause silent changes to existing code (at least add an rpass test):

mod Bad {
    pub fn default<T>() -> T where T: Default + ::std::ops::Not<Output=T> {
        !T::default()
    }
}

fn foo<Bad>() -> Bad where Bad: Default + ::std::ops::Not<Output=Bad> {
    Bad::default()
}

fn main() {
    println!("{}", foo::<i32>());
}

@jseyfried
Copy link
Contributor Author

@arielb1 As your example demonstrates, this can cause silent changes to existing code, but only if

  • a type parameter shadows a module (violating camel/snake case convention) and
  • the first element of a path that currently resolves to the shadowed module could also resolve to the type parameter and everything would still resolve and typecheck,

which seems highly unlikely to occur in practice.

I believe the current test is sufficient to ensure that type parameters properly shadow items -- it would be virtually impossible for code that passes the current test to fail a corresponding rpass test by accident.

@nikomatsakis
Copy link
Contributor

Looks nice. I don't quite get what's going on with the refactoring of check_ribs, but I have confidence that it is correct. r=me with a comment (bonus points for adding similar comments to some of the other fns you touched ;)

@jseyfried
Copy link
Contributor Author

I commented resolve_name_in_lexical_scope.

I don't quite get what's going on with the refactoring of check_ribs

Now that resolve_item_in_lexical_scope also checks the ribs, the only difference between check_ribs=false and check_ribs=true (for all the functions with this argument) was that check_ribs=false excluded type parameters and local variables but check_ribs=true allowed them.

After checking that allowing type parameters and local variables where they used to be excluded would make no difference (modulo diagnostics), I refactored away this argument (treating it as always true).

@jseyfried jseyfried force-pushed the fix_resolution_in_lexical_scopes branch from b190e56 to f55908b Compare March 11, 2016 23:51
@bors
Copy link
Contributor

bors commented Mar 11, 2016

📌 Commit f55908b has been approved by jseyfried

@jseyfried jseyfried force-pushed the fix_resolution_in_lexical_scopes branch 2 times, most recently from 2902c8b to e926f28 Compare March 11, 2016 23:59
@jseyfried
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 12, 2016

📌 Commit e926f28 has been approved by nikomatsakis

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 12, 2016
…_scopes, r=nikomatsakis

Fix name resolution in lexical scopes

Currently, `resolve_item_in_lexical_scope` does not check the "ribs" (type parameters and local variables). This can allow items that should be shadowed by type parameters to be named.

For example,
```rust
struct T { i: i32 }
fn f<T>() {
    let t = T { i: 0 }; // This use of `T` resolves to the struct, not the type parameter
}

mod Foo {
    pub fn f() {}
}
fn g<Foo>() {
    Foo::f(); // This use of `Foo` resolves to the module, not the type parameter
}
```

This PR changes `resolve_item_in_lexical_scope` so that it fails when the item is shadowed by a rib (fixes rust-lang#32120).
This is a [breaking-change], but it looks unlikely to cause breakage in practice.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Mar 12, 2016

⌛ Testing commit e926f28 with merge 63906b6...

@bors
Copy link
Contributor

bors commented Mar 12, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Sat, Mar 12, 2016 at 7:31 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-gnu-32-opt
http://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/3415


Reply to this email directly or view it on GitHub
#32141 (comment).

@bors
Copy link
Contributor

bors commented Mar 13, 2016

⌛ Testing commit e926f28 with merge 06074ac...

bors added a commit that referenced this pull request Mar 13, 2016
…nikomatsakis

Fix name resolution in lexical scopes

Currently, `resolve_item_in_lexical_scope` does not check the "ribs" (type parameters and local variables). This can allow items that should be shadowed by type parameters to be named.

For example,
```rust
struct T { i: i32 }
fn f<T>() {
    let t = T { i: 0 }; // This use of `T` resolves to the struct, not the type parameter
}

mod Foo {
    pub fn f() {}
}
fn g<Foo>() {
    Foo::f(); // This use of `Foo` resolves to the module, not the type parameter
}
```

This PR changes `resolve_item_in_lexical_scope` so that it fails when the item is shadowed by a rib (fixes #32120).
This is a [breaking-change], but it looks unlikely to cause breakage in practice.

r? @nikomatsakis
@bors bors merged commit e926f28 into rust-lang:master Mar 13, 2016
@jseyfried jseyfried deleted the fix_resolution_in_lexical_scopes branch March 25, 2016 22:55
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.

Struct constructors can resolve to structs that are shadowed by type parameters
5 participants