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

Type aliases can be silently shadowed #20702

Closed
aidanhs opened this issue Jan 7, 2015 · 13 comments
Closed

Type aliases can be silently shadowed #20702

aidanhs opened this issue Jan 7, 2015 · 13 comments
Labels
A-resolve Area: Name resolution

Comments

@aidanhs
Copy link
Member

aidanhs commented Jan 7, 2015

check.rs

extern crate libc;

use std::mem;

type VPtr = *mut ::libc::c_void;
type CbClosure<'s> = Fn<(&'s i32,), i32> + 's;

extern "C" fn callback(data: VPtr) {
    unsafe {
        let func: &CbClosure = mem::transmute::<VPtr, &CbClosure>(data);
        let x = 5;
        (*func)(&x);
    }
}

#[link(name = "extlib")]
extern {
   fn register_callback(cb: extern fn(VPtr)) -> i32;
   fn trigger_callback(val: VPtr);
}

fn main() {
    let func = |&mut: x: &i32| *x*2;
    go(func);
}

fn go<CbClosure>(func: CbClosure) {
    unsafe {
        register_callback(callback);
        let data: VPtr = mem::transmute::<&CbClosure, VPtr>(&func);
        trigger_callback(data);
    }
}

extlib.c

#include <stdint.h>

typedef void (*rust_callback)(void*);
rust_callback cb;

int32_t register_callback(rust_callback callback) {
    cb = callback;
    return 1;
}

void trigger_callback(void *val) {
  cb(val);
}
$ gcc extlib.c -fPIC -shared -o libextlib.so
$ rustc -L . check.rs
check.rs:10:32: 10:66 error: transmute called on types with different sizes: *mut libc::types::common::c95::c_void (64 bits) to &core::ops::Fn(&i32) -> i32 (128 bits)
check.rs:10         let func: &CbClosure = mem::transmute::<VPtr, &CbClosure>(data);
                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

But if you comment out the whole unsafe block in callback...

$ rustc -L . check.rs 
check.rs:8:24: 8:28 warning: unused variable: `data`, #[warn(unused_variables)] on by default
check.rs:8 extern "C" fn callback(data: VPtr) {

despite there being a conversion the other way in go. Surely these type cannot vary in size?

To compound my confusion, you can add the line below to after let data: [...] in go and it will compile - this is exactly the same line as in callback, with a different variable name.

let func2: &CbClosure = mem::transmute::<VPtr, &CbClosure>(data);
@aidanhs
Copy link
Member Author

aidanhs commented Jan 7, 2015

Ping #18101? (this worked with old closures)

@Diggsey
Copy link
Contributor

Diggsey commented Jan 7, 2015

It looks like you're shadowing "CbClosure" in the "go" function so the two transmutes have different operands: the callback transmute converts from a reference to any type implementing the trait "Fn<(&'s i32,), i32> + 's", to VPtr, while the callback in "go" converts from VPtr to a reference to any Sized type (the Sized bound is implicit for template arguments, unless you opt out).

The important point being that the Fn trait does not imply Sized, and only references to Sized types are implemented as "normal" pointers like VPtr: references to types which are not Sized are implemented as fat pointers, and transmuting from a fat pointer to a VPtr will fail because they are different sizes.

@kmcallister kmcallister added the A-typesystem Area: The type system label Jan 7, 2015
@aidanhs
Copy link
Member Author

aidanhs commented Jan 7, 2015

Ouch, that'll teach me to blindly copy and paste. Worth a warning I feel.

@aidanhs aidanhs changed the title mem::transmute is not symmetric with complaints about type sizes Type aliases can be silently shadowed Jan 7, 2015
@aidanhs
Copy link
Member Author

aidanhs commented Jan 7, 2015

Changed the title of the issue.

@aidanhs
Copy link
Member Author

aidanhs commented Jan 8, 2015

Going off track of the issue here somewhat:
After working on this a bit more and grepping the rust code, I've finally discovered (courtesy of tests) that type blah = trait aren't very useful unless you box it.
By itself it can't be used as a type of a variable (because it's not sized) and can't be used as a trait in a type parameter (because it's not a trait).
Is my understanding on this about right?

@Gankra
Copy link
Contributor

Gankra commented Jan 22, 2015

@aidanhs Yes, that sounds correct.

@Gankra
Copy link
Contributor

Gankra commented Jan 22, 2015

Fixed by #20728, btw. @huonw is this correct?

@Gankra Gankra closed this as completed Jan 22, 2015
@huonw
Copy link
Member

huonw commented Jan 22, 2015

I don't think so:

struct T;
impl T { fn method(&self) {} }

type U = T;

fn foo<U>(x: U) {
    x.method();
}

fn main() {}
<anon>:7:7: 5:15 error: type `U` does not implement any method in scope named `method`
<anon>:7     x.method();
               ^~~~~~~~

@huonw huonw reopened this Jan 22, 2015
@frewsxcv
Copy link
Member

frewsxcv commented Sep 7, 2015

Visiting for triage: this is still an issue

@steveklabnik
Copy link
Member

Triage: no change

@petrochenkov petrochenkov added A-resolve Area: Name resolution and removed A-typesystem Area: The type system labels Mar 1, 2017
@petrochenkov
Copy link
Contributor

I don't see an issue here.
Shadowing works as it should, names in inner scope shadow names in outer scopes.

I agree, it may be confusing, but not more confusing than

let a = 10;
{
    let a = 11;
    println!("{}", a); // WHY DOES IT PRINT 11, I SAID `a = 10` PREVIOUSLY?
}

@aidanhs
Copy link
Member Author

aidanhs commented Mar 1, 2017

Yes, I've cooled a bit on this. I think in general hints about shadowing could be better (where the compiler detects that removing a shadow could fix something) but that sounds like a whole new set of diagnostics.

@petrochenkov
Copy link
Contributor

Closing as per #20702 (comment)

in general hints about shadowing could be better (where the compiler detects that removing a shadow could fix something)

For these diagnostics to work with x.method() and mem::transmute mentioned above, name resolution pass needs to collect some alternative resolutions for all names and when some error occurs later in type checking, these the primary resolution needs to be replaced with these alternative resolutions and type checking needs to be redone.
Seems like a sufficiently large work. Probably needs a separate dedicated issue if people are interested in this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Name resolution
Projects
None yet
Development

No branches or pull requests

8 participants