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

Tracking issue for future-incompatibility lint tyvar_behind_raw_pointer #46906

Open
mikeyhew opened this issue Dec 21, 2017 · 22 comments
Open

Tracking issue for future-incompatibility lint tyvar_behind_raw_pointer #46906

mikeyhew opened this issue Dec 21, 2017 · 22 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@mikeyhew
Copy link
Contributor

mikeyhew commented Dec 21, 2017

This is the summary issue for the tyvar_behind_raw_pointer future-compatibility warning and other related errors. The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is the warning for?

This warning occurs when you call a method on a value whose type is a raw pointer to an unknown type (aka *const _ or *mut _), or a type that dereferences to one of these.

The most common case for this is casts, for example:

        let s = libc::getenv(k.as_ptr()) as *const _;
        s.is_null()

This can be fixed by giving a type annotation to the cast:

        let s = libc::getenv(k.as_ptr()) as *const libc::c_char;
        s.is_null()

The reason that this is going to become an error is because we are working on enabling arbitrary self types. Using that, you should be able to write functions such as:

#![feature(arbitrary_self_types)]

struct MyType;

impl MyType {
    fn is_null(self: *mut Self) -> bool {
        println!("?");
        true
    }
}

While that case is obviously silly, we can't prevent a sibling crate from implementing it, and such a function would make a call to s.is_null() when the type of s is an *mut _ ambiguous. Therefore, to avoid that potential breakage, you have to annotate the type of your raw pointer before the point of the method call.

After you fix these warnings, if you are working with raw pointers on nightly, you might want to check out #![feature(arbitrary_self_types)] yourself! It even works with trait objects:

#![feature(arbitrary_self_types, dyn_trait)]
trait Foo {
    fn example(self: *mut Self);
}

struct S;
impl Foo for S {
    fn example(self: *mut Self) {
        println!("Hello, I'm at {:?}", self);
    }
}

fn foo(foo: *mut dyn Foo) {
    foo.example();
}

fn main() {
    // This is 100% safe and not UB, even through I am calling `foo`
    // with a null pointer - this is a *raw* null pointer, and these are
    // always ok.
    foo(0 as *mut S);
}

While I'm at it, arbitrary_self_types also works for smart pointers, such as Rc<T> or Arc<T> (however, unfortunately we still have not figured out the best way to make it work with smart pointers to trait objects, so you can't yet create dyn Bar trait objects. We are planning on making it eventually work, so stay tuned!).

#![feature(arbitrary_self_types)]

use std::rc::Rc;

trait Bar {
    fn example(self: &Rc<Self>);
}

struct S;
impl Bar for S {
    fn example(self: &Rc<Self>) {
        println!("Hi I'm called on an Rc.");
        let _x = self.clone(); // You can call Rc::clone on Self!
    }
}

fn main() {
    Rc::new(S).example();
}

When will this warning become a hard error?

At the beginning of each 6-week release cycle, the Rust compiler team will review the set of outstanding future compatibility warnings and nominate some of them for Final Comment Period. Toward the end of the cycle, we will review any comments and make a final determination whether to convert the warning into a hard error or remove it entirely.

@kennytm kennytm added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 22, 2017
CAD97 added a commit to CAD97/rust that referenced this issue Dec 23, 2017
@mikeyhew
Copy link
Contributor Author

@arielb1 thanks for editing with the detailed explanation!

bors added a commit that referenced this issue Dec 25, 2017
Convert warning about `*const _` to a future-compat lint

#46664 was merged before I could convert the soft warning about method lookup on `*const _` into a future-compatibility lint. This PR makes that change.

fixes #46837
tracking issue for the future-compatibility lint: #46906

r? @arielb1
MaloJaffre added a commit to MaloJaffre/parking_lot that referenced this issue Dec 29, 2017
requeue_threads` now requires an explicit type because of the `is_null`
method call later in the code.
See rust-lang/rust#46906
@SimonSapin
Copy link
Contributor

SimonSapin commented Jan 4, 2018

This warning occurs when you call a method on a value whose type is a raw pointer to an unknown type (aka *const _ or *mut _), or a type that dereferences to one of these.

(Emphasis added.)

I feel that this latter case is overly zealous. A pervasive pattern in Servo’s DOM can be simplified to this:

fn main() {
    // T = *mut _
    let mut constructor = RootedGuard::new(std::ptr::null_mut());

    // "warning: the type of this value must be known in this context"
    // But is this really ambiguous? Or does the warning have a false positive?
    let handle = constructor.handle_mut();

    // Type inference establishes `T = *mut JSObject` here.
    get_constructor_object_from_local_name(handle);
}

pub struct RootedGuard<T>(T);
pub struct MutableHandle<T>(T);
pub struct JSObject;

impl<T: Clone> RootedGuard<T> {
    pub fn new(x: T) -> Self { RootedGuard(x) }
    pub fn handle_mut(&mut self) -> MutableHandle<T> { unimplemented!() }
}

impl<T> std::ops::Deref for RootedGuard<T> {
    type Target = T;
    fn deref(&self) -> &T { &self.0 }
}

fn get_constructor_object_from_local_name(_: MutableHandle<*mut JSObject>) {}

As of 0a3761e the handle_mut call causes a tyvar_behind_raw_pointer warning which does not seem justified: handle_mut is an inherent method of RootedGuard<T> and so will always be picked before any method of T through auto-deref. arbitrary_self_types won’t change that as far as I can tell.

Please consider not warning in this case. This example is easy enough to fix (for example by replacing null_mut() with null_mut::<JSObject>()), but Servo has 1871 instances of this warning and adding as many type annotation would be a huge pain if they indeed don’t actually remove any ambiguity.

bors-servo pushed a commit to servo/servo that referenced this issue Jan 4, 2018
[Do not merge] Upgrade to rustc 1.24.0-nightly (0a3761e63 2018-01-03)

Do not merge (yet), as this might bring #19519 back.

Also blocked on rust-lang/rust#46906 (comment).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19683)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Jan 4, 2018
[Do not merge] Upgrade to rustc 1.24.0-nightly (0a3761e63 2018-01-03)

Do not merge (yet), as this might bring #19519 back.

Also blocked on rust-lang/rust#46906 (comment).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19683)
<!-- Reviewable:end -->
@mikeyhew
Copy link
Contributor Author

mikeyhew commented Jan 4, 2018

@SimonSapin

handle_mut is an inherent method of RootedGuard and so will always be picked before any method of T through auto-deref

During method lookup, T is *mut _. That _ could theoretically resolve to a type that has a method fn handle_mut(self: &mut RootedGuard<*mut Self>), which would result an an ambiguity on method lookup. Not to say that defining methods like this is very useful, but that's what's currently allowed with arbitrary_self_types.

Type inference establishes `T = *mut JSObject` here.
get_constructor_object_from_local_name(handle);

It's unfortunate that the type information from this function call isn't available during the earlier method lookup. I don't know much about how type inference works in the compiler, but it would be nice if cases like this could be improved.

@arielb1 any thoughts on a possible solution here?

bors-servo pushed a commit to servo/servo that referenced this issue Jan 4, 2018
[Do not merge] Upgrade to rustc 1.24.0-nightly (0a3761e63 2018-01-03)

Do not merge (yet), as this might bring #19519 back.

Also blocked on rust-lang/rust#46906 (comment).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19683)
<!-- Reviewable:end -->
@nikomatsakis
Copy link
Contributor

So, my thoughts are these:

There is really not much we can do to get more type information. We already do basically exhaust what remedies we have.

To get more, we'd have to do some big refactorings on the way that method lookup works. I'd like to do this, but it seems pretty distinct from this PR. We'd basically have to make method dispatch "delayable" until more type information is available.

I am a bit nervous about making this change however. I feel like we've seen a fair amount of code affected, though I don't have a real sample. Were any crater runs done to provide actual measurements?

At the worst, we could do the change only behind a feature-gate, and then use an epoch to enable access to custom self types.

bors-servo pushed a commit to servo/servo that referenced this issue Jan 4, 2018
[Do not merge] Upgrade to rustc 1.24.0-nightly (0a3761e63 2018-01-03)

Do not merge (yet), as this might bring #19519 back.

Also blocked on rust-lang/rust#46906 (comment).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19683)
<!-- Reviewable:end -->
@mikeyhew
Copy link
Contributor Author

mikeyhew commented Jan 5, 2018

@nikomatsakis no, no crater runs were done. It would definitely be good to do one

SimonSapin added a commit to servo/servo that referenced this issue Jan 5, 2018
SimonSapin added a commit to servo/servo that referenced this issue Jan 5, 2018
CasualX added a commit to CasualX/pelite that referenced this issue Jan 5, 2018
* Remove all `#[repr(packed)]` on the IMAGE structs due to a misunderstanding of the Windows.h struct definitions:
Only the old 16-bit headers want 2 byte packing, all the others want 4 byte packing. However all of these structs end up already aligned anyway.
Safeguard their implementation by asserting their sizes.

  Fixes rust-lang/rust#46043

* Explicitly specify the raw pointer type when casting from reference and calling a method on it.

  Fixes rust-lang/rust#46906
CasualX added a commit to CasualX/pelite that referenced this issue Jan 5, 2018
* Remove all `#[repr(packed)]` on the IMAGE structs due to a misunderstanding of the Windows.h struct definitions:
Only the old 16-bit headers want 2 byte packing, all the others want 4 byte packing. However all of these structs end up already aligned anyway.
Safeguard their implementation by asserting their sizes.

  Fixes rust-lang/rust#46043

* Explicitly specify the raw pointer type when casting from reference and calling a method on it.

  Fixes rust-lang/rust#46906
@nikomatsakis
Copy link
Contributor

@mikeyhew to that end, maybe you can prepare a PR that sets the "default level" for the lint to Deny, and then we can run a crater run?

@mikeyhew
Copy link
Contributor Author

mikeyhew commented Jan 6, 2018

@nikomatsakis done, see #47227

bluss added a commit to petgraph/petgraph that referenced this issue Jan 7, 2018
```
warning: the type of this value must be known in this context
   --> src/graphmap.rs:765:11
    |
765 |         a.cmp(&b)
    |           ^^^
    |
    = note: #[warn(tyvar_behind_raw_pointer)] on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46906 <rust-lang/rust#46906>
```
SimonSapin added a commit to servo/servo that referenced this issue Jan 9, 2018
Xanewok added a commit to Xanewok/rust-mozjs that referenced this issue Mar 8, 2018
As tracked at rust-lang/rust#46906, checked
with rustc 1.26.0-nightly (9cb18a92a 2018-03-02) here and using patched
mozjs in Servo (rustc 1.25.0-nightly (15a1e2844 2018-01-20)).
Xanewok added a commit to Xanewok/rust-mozjs that referenced this issue Mar 8, 2018
As tracked at rust-lang/rust#46906, checked
with rustc 1.26.0-nightly (9cb18a92a 2018-03-02) here and using patched
mozjs in Servo (rustc 1.25.0-nightly (15a1e2844 2018-01-20)).
bors-servo pushed a commit to servo/rust-mozjs that referenced this issue Mar 8, 2018
Fix tyvar_behind_raw_pointer warnings

As tracked at rust-lang/rust#46906, checked with rustc 1.26.0-nightly (9cb18a92a 2018-03-02) here and using patched mozjs in Servo (rustc 1.25.0-nightly (15a1e2844 2018-01-20)).

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/394)
<!-- Reviewable:end -->
mattico pushed a commit to mattico/rustup.rs that referenced this issue Apr 5, 2018
est31 added a commit to est31/cpp_demangle that referenced this issue Jun 4, 2018
fpagliughi added a commit to eclipse-paho/paho.mqtt.rust that referenced this issue Jun 28, 2018
@SSheldon
Copy link
Contributor

SSheldon commented Aug 5, 2018

I've got a case that hits this lint without using any casting and can be minimized down to:

fn make_vec() -> Vec<i32> {
    let mut dest = Vec::with_capacity(1);
    unsafe {
        ptr::write(dest.as_mut_ptr().offset(0), 17);
        dest.set_len(1);
    }
    dest
}

The warning disappears if you don't call offset(0) of if you add an explicit type for dest.

It seems a little weird because apparently the compiler can infer that dest is a Vec<i32>, in which case as_mut_ptr must return *mut i32 and then offset must return *mut i32, so it doesn't seem like a raw pointer to an unknown type.

Is this an error in the lint?

SSheldon added a commit to SSheldon/rust-dispatch that referenced this issue Aug 5, 2018
This was apparently tripping up the rust-lang/rust#46906 lint.
@arielb1
Copy link
Contributor

arielb1 commented Aug 11, 2018

@SSheldon

The compiler can't infer that dest is Vec<i32> (as opposed to Vec<_>).

Consider that someone could have implemented this function:

pub struct MyType;

impl MyType {
    pub fn offset(self: *mut Self, _offset: usize) -> *mut i32 {
        /* .. */
        1 as *mut i32
    }
}

That would obviously cause an ambiguity as dest could also be a Vec<MyType>.

Method lookup is conservative when it comes to adding inherent impls to unrelated types (as opposed to adding trait impls to existing traits), and therefore requires clarification.

@Centril Centril added C-future-incompatibility Category: Future-incompatibility lints and removed C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jan 13, 2019
vhdirk pushed a commit to vhdirk/capnproto-rust that referenced this issue May 8, 2019
yvt added a commit to yvt/ngspades that referenced this issue Apr 25, 2020
@vext01
Copy link
Contributor

vext01 commented Jun 18, 2020

I've got a case that hits this lint without using any casting.

I've just encountered the same issue on today's nightly:

    /// Creates an empty vector with capacity for `cap` values.                 
    pub fn with_capacity(cap: usize) -> Self {                                  
        let raw = RawVec::with_capacity(cap);                                   
        for i in 0..raw.capacity() {                                            
            unsafe {                                                            
                ptr::write(raw.ptr().offset(i as isize), T::default());         
            }                                                                   
        }                                                                       
        DefaultVec { raw }                                                      
    }

Gives:

warning: type annotations needed
  --> src/lib.rs:26:38
   |
26 |                 ptr::write(raw.ptr().offset(i as isize), T::default());
   |                                      ^^^^^^
   |
   = note: `#[warn(tyvar_behind_raw_pointer)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2018 edition!
   = note: for more information, see issue #46906 <https://github.com/rust-lang/rust/issues/46906>

As @arielb1 says, you can fix it by adding type annotations:

let raw: RawVec<T> = RawVec::with_capacity(cap);

But I think the error message was confusing. I'd have known what to do if it had said:

warning: type annotations needed
  --> src/lib.rs:23:..
   |
23 |                 let raw = RawVec::with_capacity(cap);
   |                     ^^^
   |

@nikomatsakis
Copy link
Contributor

It's probably time to move towards closing this compatibility lint, and maybe we can improve the diagnostics at the same time.

@orowith2os
Copy link

It appears I've come across this issue, though I don't get why it's happening despite me providing type annotations:

#[repr(C)]
struct Foo {
    a: u8,
    b: u8,
    c: u8,
    d: u8,
}

fn main() {
    let var = Foo { a: 1, b: 2, c: 3, d: 4 };
    let raw_pointer_array = &var as *const Foo as *const u8;
    let goofy_array: [u8; 4] = unsafe { raw_pointer_array.cast().read() };
    println!("{:?}", goofy_array);
}

@lcnr
Copy link
Contributor

lcnr commented Nov 10, 2023

it happens because the type of raw_pointer_array.cast() is *const _, i.e. a type variable behind a raw pointer. The lint is then emitted while selecting the method for read

@orowith2os
Copy link

It seems reasonable for it to infer from .read() that I want the type of .cast() to be *const [u8; 4]. If I'm not mistaken, this also happens in some similar capacity in type inference elsewhere too.

@fmease fmease changed the title Tracking issue for the tyvar_behind_raw_pointer compatibility lint Tracking issue for future-incompatibility lint tyvar_behind_raw_pointer Sep 14, 2024
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

No branches or pull requests