arbitrary_self_types: add support for raw pointer `self` types #46664

mikeyhew · 2017-12-11T18:03:40Z

This adds support for raw pointer self types, under the arbitrary_self_types feature flag. Types like self: *const Self, self: *const Rc<Self>, self: Rc<*const Self are all supported. Object safety checks are updated to allowself: *const Self and self: *mut Self.

This PR does not add support for *const self and *mut self syntax. That can be added in a later PR once this code is reviewed and merged.

#44874

r? @arielb1

arielb1 · 2017-12-12T15:06:19Z

src/librustc_typeck/check/method/probe.rs

-                    let t = self.structurally_resolved_type(span, final_ty);
-                    assert_eq!(t, self.tcx.types.err);
-                    return None
+                    // don't report a type error if we dereferenced a raw pointer,


Is that really a problem in practice?

~~If then, I think we might want a warning cycle here.~~ This looks like a sort of expected inference breakage, so it should remain an error.

It broke something in libstd:

rust/src/libstd/sys_common/gnu/libbacktrace.rs

Line 76 in 707d070

let mut data = ptr::null();

Here's a minimal reproduction:

fn main() { let data = std::ptr::null(); let _ = &data as *const *const (); if data.is_null() {} }

I get that. I think we should probably make this a future-compat warning, or an error with the arbitrary_self_types feature flag set.

arielb1 · 2017-12-12T15:10:42Z

src/test/run-pass/arbitrary_self_types_raw_pointer_struct.rs

@@ -8,11 +8,17 @@
 // option. This file may not be copied, modified, or distributed


Could you also add a test for "complex" cases - i.e. Rc<*const Self> & *const Rc<Self>?

arielb1 · 2017-12-12T15:14:36Z

r=me with a test for complex cases added

I documented the "inference suppression" thing at the now-tracking-issue, but we need to look at it.

arielb1 · 2017-12-12T19:43:09Z

Actually, allowing for object-safe raw pointers is a bit controversial, because it means that fat raw pointers have some semantics.

aka:

trait Foo {
    fn bar(self: *const Self);
}

fn main() {
    let foo: *const Foo = mem::transmute([0usize, 0usize]);
    foo.bar(); // this is UB
}

I think that fat raw pointers already have some semantics - you can call size_of_val on them - but I'll rather not unilaterally specify more semantics for them.

@rfcbot fcp start

kennytm · 2017-12-13T07:35:38Z

@arielb1 you mean @rfcbot fcp merge?

BTW since transmute is unsafe and ptr::null is not defined for DSTs I don’t think this particular code sample is a big issue.

mikeyhew · 2017-12-13T14:50:20Z

@arielb1 size_of_val currently only takes a reference, not a raw pointer. I started a discussion in the rfcs repo about changing it to take *const T.

as @kennytm pointed out, the example above requires unsafe code to cause undefined behaviour. Using transmute to create a trait object is especially unsafe, and this is mentioned in the documentation of raw::TraitObject.

This PR in its current state (with object-safe raw pointer methods) exposes the following invariants about the language implementation:

trait object pointers will always be fat pointers, with the vtable(s) stored in the pointer, and
the vtable(s) will always be valid.

It is pretty much impossible for (1) not to be true, due to the nature of traits, and how any type can implement arbitrary traits. For a trait object to be a thin pointer, the vtable would have to be stored with the data, and that means every i32 that you cast to a trait object would have to be an extra word bigger to store the vtable to that trait. Did I say "pretty much" impossible? No, it's definitely impossible, because what if you cast a pointer to the same i32 field to two different trait objects?

(2) is maybe a bit more controversial, but this was discussed in #44932 because the assumption in making ptr::null(p) be equivalent to (p as *const ()).is_null() was that DST pointers would always have valid metadata. @dtolnay was the only person in that discussion to disagree, and he has since changed his mind and added that behaviour in a new PR #46094

arielb1 · 2017-12-13T17:39:35Z

r=me with the type error made into a future compat warning + a hard error with the feature gate enabled.

mikeyhew · 2017-12-14T17:52:50Z

@arielb1 should be good to go

kennytm · 2017-12-14T19:14:18Z

@mikeyhew CI is failing, ui/inference-variable-behind-raw-pointer.rs did not error out.

[00:53:29] ---- [ui] ui/inference-variable-behind-raw-pointer.rs stdout ----
[00:53:29] 	
[00:53:29] error: ui test compiled successfully!
[00:53:29] status: exit code: 0
[00:53:29] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/inference-variable-behind-raw-pointer.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/inference-variable-behind-raw-pointer.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/inference-variable-behind-raw-pointer.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
[00:53:29] stdout:
[00:53:29] ------------------------------------------
[00:53:29] 
[00:53:29] ------------------------------------------
[00:53:29] stderr:
[00:53:29] ------------------------------------------
[00:53:29] {"message":"the type of this value must be known in this context","code":{"code":"E0619","explanation":"\nThe type-checker needed to know the type of an expression, but that type had not\nyet been inferred.\n\nErroneous code example:\n\n```compile_fail,E0619\nlet mut x = vec![];\nmatch x.pop() {\n    Some(v) => {\n        // Here, the type of `v` is not (yet) known, so we\n        // cannot resolve this method call:\n        v.to_uppercase(); // error: the type of this value must be known in\n                          //        this context\n    }\n    None => {}\n}\n```\n\nType inference typically proceeds from the top of the function to the bottom,\nfiguring out types as it goes. In some cases -- notably method calls and\noverloadable operators like `*` -- the type checker may not have enough\ninformation *yet* to make progress. This can be true even if the rest of the\nfunction provides enough context (because the type-checker hasn't looked that\nfar ahead yet). In this case, type annotations can be used to help it along.\n\nTo fix this error, just specify the type of the variable. Example:\n\n```\nlet mut x: Vec<String> = vec![]; // We precise the type of the vec elements.\nmatch x.pop() {\n    Some(v) => {\n        v.to_uppercase(); // Since rustc now knows the type of the vec elements,\n                          // we can use `v`'s methods.\n    }\n    None => {}\n}\n```\n"},"level":"warning","spans":[{"file_name":"/checkout/src/test/ui/inference-variable-behind-raw-pointer.rs","byte_start":563,"byte_end":570,"line_start":14,"line_end":14,"column_start":13,"column_end":20,"is_primary":true,"text":[{"text":"    if data.is_null() {}","highlight_start":13,"highlight_end":20}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"this will be made into a hard error in a future version of the compiler","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"warning[E0619]: the type of this value must be known in this context\n  --> /checkout/src/test/ui/inference-variable-behind-raw-pointer.rs:14:13\n   |\n14 |     if data.is_null() {}\n   |             ^^^^^^^\n   |\n   = note: this will be made into a hard error in a future version of the compiler\n\n"}
[00:53:29] 
[00:53:29] ------------------------------------------
[00:53:29] 
[00:53:29] thread '[ui] ui/inference-variable-behind-raw-pointer.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2774:8
[00:53:29] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:53:29] 
[00:53:29] 
[00:53:29] failures:
[00:53:29]     [ui] ui/inference-variable-behind-raw-pointer.rs
[00:53:29] 
[00:53:29] test result: FAILED. 666 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out

arielb1 · 2017-12-14T23:55:52Z

@mikeyhew

You should use a future-compat warning so you can use #![deny(warning_name)] to test it.

This does feel a little heavyweight, but I think the alternatives are worse. I'll ask @nikomatsakis about it.

mikeyhew · 2017-12-15T00:38:46Z

@kennytm so the error is that it compiles successfully? How do I tell the ui test suite that it's supposed to compile? Funnily enough, it passes on my machine, were ui tests changed since I last rebased off of master on Dec 10?

arielb1 · 2017-12-15T02:45:45Z

@mikeyhew

We did change UI tests so that they need to fail to compile unless you add this line to the start of the test (look for it in existing tests, e.g. https://github.com/rust-lang/rust/blob/master/src/test/ui/hello_world/main.rs):

// must-compile-successfully

mikeyhew · 2017-12-15T07:48:55Z

I found an old RFC PR that suggests allowing traits to opt into having the vtable stored with the data: rust-lang/rfcs#250. This goes against (1) in my comment above. However, since that feature would be opt-in for specific traits, it would be easy to make raw pointer methods non-object-safe for those traits only, either by checking for #[repr(internal_vtable)] or by introducing a new trait, e.g. FatDynSized and requiring it

mikeyhew · 2017-12-15T08:52:08Z

@kennytm you're confused by my comment?

arielb1 · 2017-12-15T14:52:07Z

Looks like you need to update a test

[01:02:25]  error[E0038]: the trait `Foo` cannot be made into an object
[01:02:25]    --> $DIR/arbitrary-self-types-not-object-safe.rs:40:33
[01:02:25]     |
[01:02:25]  40 |     let x = Box::new(5usize) as Box<Foo>;
[01:02:25]     |                                 ^^^^^^^^ the trait `Foo` cannot be made into an object
[01:02:25]     |
[01:02:25] -   = note: method `foo` has a non-standard `self` type. Only `&self`, `&mut self`, and `Box<Self>` are currently supported for trait objects
[01:02:25] +   = note: method `foo` has a non-standard `self` type
[01:02:25]  
[01:02:25]  error[E0038]: the trait `Foo` cannot be made into an object
[01:02:25]    --> $DIR/arbitrary-self-types-not-object-safe.rs:40:13
[01:02:25]     |
[01:02:25]  40 |     let x = Box::new(5usize) as Box<Foo>;
[01:02:25]     |             ^^^^^^^^^^^^^^^^ the trait `Foo` cannot be made into an object
[01:02:25]     |
[01:02:25] -   = note: method `foo` has a non-standard `self` type. Only `&self`, `&mut self`, and `Box<Self>` are currently supported for trait objects
[01:02:25] +   = note: method `foo` has a non-standard `self` type
[01:02:25]     = note: required because of the requirements on the impl of `std::ops::CoerceUnsized<std::boxed::Box<Foo>>` for `std::boxed::Box<usize>`
[01:02:25]  
[01:02:25]  error: aborting due to 2 previous errors
[01:02:25]  
[01:02:25]  
[01:02:25] failures:
[01:02:25]     [ui] ui/arbitrary-self-types-not-object-safe.rs

mikeyhew · 2017-12-17T09:10:55Z

looks like I need to rebase, because that test is under compile-fail for me

…ts, but not structs

…s long as we went through a raw pointer This avoids a break in backward compatibility in the following case: ``` let ptr = std::ptr::null(); let _ = &data as *const *const (); if data.null() {} ```

…nter self

had to tell the test suite that it's supposed to compile

…sage

arielb1 · 2017-12-18T20:34:01Z

@bors r+

bors · 2017-12-18T20:34:02Z

📌 Commit 5c656f0 has been approved by arielb1

bors · 2017-12-19T07:05:11Z

⌛ Testing commit 5c656f0 with merge a9f047c...

@arielb1

arbitrary_self_types: add support for raw pointer `self` types This adds support for raw pointer `self` types, under the `arbitrary_self_types` feature flag. Types like `self: *const Self`, `self: *const Rc<Self>`, `self: Rc<*const Self` are all supported. Object safety checks are updated to allow`self: *const Self` and `self: *mut Self`. This PR does not add support for `*const self` and `*mut self` syntax. That can be added in a later PR once this code is reviewed and merged. #44874 r? @arielb1

bors · 2017-12-19T09:43:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing a9f047c to master...

@arielb1

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

rust-highfive assigned arielb1 Dec 11, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2017

arielb1 reviewed Dec 12, 2017

View reviewed changes

arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 12, 2017

kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2017

mikeyhew force-pushed the raw_pointer_self branch from 426d647 to 1494927 Compare December 15, 2017 08:54

mikeyhew added 8 commits December 17, 2017 10:13

implement raw-pointer self. Works for traits, including trait objec…

361b3db

…ts, but not structs

changed some stuff in probe.rs and it broke libstd

083635e

don't emit a type error if autoderef ends in an inference variable, a…

e7d1542

…s long as we went through a raw pointer This avoids a break in backward compatibility in the following case: ``` let ptr = std::ptr::null(); let _ = &data as *const *const (); if data.null() {} ```

shorten lines for tidy

bff74fb

Add a feature_gate test for raw pointer self

cb9f552

Fix ICE in probe::confirm::extract_existential_trait_ref with raw poi…

8aac984

…nter self

update error message in test/compile-fail/issue-26194.rs

c5fc7c5

remove test for issue rust-lang#26194 because it is now redundant

428b025

mikeyhew added 6 commits December 17, 2017 10:13

Warn about inference variables behind raw pointers in method lookup

bc0439b

Add tests with *const Rc<Self> and similar self types

4cae2c0

better variable names in tests

0cc573a

add a ui test for the inference variable warning

63ce99c

fix ui test

23555b9

had to tell the test suite that it's supposed to compile

update ui/arbitrary-self-types-not-object-safe with shorter error mes…

5c656f0

…sage

mikeyhew force-pushed the raw_pointer_self branch from 1494927 to 5c656f0 Compare December 17, 2017 10:12

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 18, 2017

bors merged commit 5c656f0 into rust-lang:master Dec 19, 2017

pnkfelix mentioned this pull request Dec 19, 2017

E0619 diagnostic occurs twice in rustc source #46837

Closed

This was referenced Dec 21, 2017

Tracking issue for future-incompatibility lint tyvar_behind_raw_pointer #46906

Open

Convert warning about *const _ to a future-compat lint #46914

Merged

shepmaster mentioned this pull request Jan 23, 2019

Tracking issue for RFC 3519: arbitrary_self_types #44874

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arbitrary_self_types: add support for raw pointer `self` types #46664

arbitrary_self_types: add support for raw pointer `self` types #46664

mikeyhew commented Dec 11, 2017

arielb1 Dec 12, 2017 •

edited

Loading

mikeyhew Dec 12, 2017

arielb1 Dec 13, 2017

arielb1 Dec 12, 2017

arielb1 commented Dec 12, 2017 •

edited

Loading

arielb1 commented Dec 12, 2017

kennytm commented Dec 13, 2017

mikeyhew commented Dec 13, 2017

arielb1 commented Dec 13, 2017

mikeyhew commented Dec 14, 2017

kennytm commented Dec 14, 2017

arielb1 commented Dec 14, 2017 •

edited

Loading

mikeyhew commented Dec 15, 2017

arielb1 commented Dec 15, 2017 •

edited

Loading

mikeyhew commented Dec 15, 2017 •

edited

Loading

mikeyhew commented Dec 15, 2017

arielb1 commented Dec 15, 2017

mikeyhew commented Dec 17, 2017

arielb1 commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 19, 2017

bors commented Dec 19, 2017

		@@ -8,11 +8,17 @@
		// option. This file may not be copied, modified, or distributed

arbitrary_self_types: add support for raw pointer self types #46664

arbitrary_self_types: add support for raw pointer self types #46664

Conversation

mikeyhew commented Dec 11, 2017

arielb1 Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

mikeyhew Dec 12, 2017

Choose a reason for hiding this comment

arielb1 Dec 13, 2017

Choose a reason for hiding this comment

arielb1 Dec 12, 2017

Choose a reason for hiding this comment

arielb1 commented Dec 12, 2017 • edited Loading

arielb1 commented Dec 12, 2017

kennytm commented Dec 13, 2017

mikeyhew commented Dec 13, 2017

arielb1 commented Dec 13, 2017

mikeyhew commented Dec 14, 2017

kennytm commented Dec 14, 2017

arielb1 commented Dec 14, 2017 • edited Loading

mikeyhew commented Dec 15, 2017

arielb1 commented Dec 15, 2017 • edited Loading

mikeyhew commented Dec 15, 2017 • edited Loading

mikeyhew commented Dec 15, 2017

arielb1 commented Dec 15, 2017

mikeyhew commented Dec 17, 2017

arielb1 commented Dec 18, 2017

bors commented Dec 18, 2017

bors commented Dec 19, 2017

bors commented Dec 19, 2017

arbitrary_self_types: add support for raw pointer `self` types #46664

arbitrary_self_types: add support for raw pointer `self` types #46664

arielb1 Dec 12, 2017 •

edited

Loading

arielb1 commented Dec 12, 2017 •

edited

Loading

arielb1 commented Dec 14, 2017 •

edited

Loading

arielb1 commented Dec 15, 2017 •

edited

Loading

mikeyhew commented Dec 15, 2017 •

edited

Loading