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

*const Self method can't be called with *mut Self (arbitrary_self_types) #80258

Closed
osa1 opened this issue Dec 21, 2020 · 19 comments · Fixed by #82436
Closed

*const Self method can't be called with *mut Self (arbitrary_self_types) #80258

osa1 opened this issue Dec 21, 2020 · 19 comments · Fixed by #82436
Assignees
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull C-bug Category: This is a bug. F-arbitrary_self_types `#![feature(arbitrary_self_types)]` requires-nightly This issue requires a nightly compiler in some way. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@osa1
Copy link
Contributor

osa1 commented Dec 21, 2020

Using nightly 2020-12-03, this doesn't build: (play link)

#![feature(arbitrary_self_types)]

struct Test {}

impl Test {
    fn x(self: *const Self) {}
}

fn main() {
    let ptr: *mut Test = std::ptr::null_mut();
    ptr.x();
}

Error:

error[E0599]: no method named `x` found for raw pointer `*mut Test` in the current scope
  --> src/main.rs:11:9
   |
11 |     ptr.x();
   |         ^ method not found in `*mut Test`
   |
   = note: try using `<*const T>::as_ref()` to get a reference to the type behind the pointer: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref
   = note: using `<*const T>::as_ref()` on a pointer which is unaligned or points to invalid or uninitialized memory is undefined behavior

error: aborting due to previous error

I think similar to how &self methods can be called on &mut references, *const Self methods should be allowed to be called on *mut pointers.

@osa1 osa1 added the C-bug Category: This is a bug. label Dec 21, 2020
@osa1
Copy link
Contributor Author

osa1 commented Dec 21, 2020

Also note that I can assign/copy a *mut pointer to a *const variable just fine:

struct Test {}

fn main() {
    let ptr: *mut Test = std::ptr::null_mut();
    let ptr_: *const Test = ptr;
}

@jonas-schievink jonas-schievink added the F-arbitrary_self_types `#![feature(arbitrary_self_types)]` label Dec 21, 2020
@osa1
Copy link
Contributor Author

osa1 commented Dec 21, 2020

If you agree that this is a bug/ something to improve I'm happy to implement/fix this myself. Would be good to know where to start reading the code though..

@camelid camelid added A-raw-pointers Area: raw pointers, MaybeUninit, NonNull requires-nightly This issue requires a nightly compiler in some way. labels Dec 22, 2020
@camelid
Copy link
Member

camelid commented Dec 22, 2020

That does seem like a bug, though I'm not super knowledgeable about this so I'm not certain.

@camelid
Copy link
Member

camelid commented Dec 22, 2020

@osa1 Looks like the check is performed here:

fn check_method_receiver<'fcx, 'tcx>(
fcx: &FnCtxt<'fcx, 'tcx>,
fn_sig: &hir::FnSig<'_>,
method: &ty::AssocItem,
self_ty: Ty<'tcx>,
) {

and here:

/// Returns whether `receiver_ty` would be considered a valid receiver type for `self_ty`. If
/// `arbitrary_self_types` is enabled, `receiver_ty` must transitively deref to `self_ty`, possibly
/// through a `*const/mut T` raw pointer. If the feature is not enabled, the requirements are more
/// strict: `receiver_ty` must implement `Receiver` and directly implement
/// `Deref<Target = self_ty>`.
///
/// N.B., there are cases this function returns `true` but causes an error to be emitted,
/// particularly when `receiver_ty` derefs to a type that is the same as `self_ty` but has the
/// wrong lifetime. Be careful of this if you are calling this function speculatively.
fn receiver_is_valid<'fcx, 'tcx>(
fcx: &FnCtxt<'fcx, 'tcx>,
span: Span,
receiver_ty: Ty<'tcx>,
self_ty: Ty<'tcx>,
arbitrary_self_types_enabled: bool,
) -> bool {

@camelid
Copy link
Member

camelid commented Dec 22, 2020

By the way, a tip is to grep or ripgrep through the source for the name of the feature. In this case, I did rg 'arbitrary_self_types' compiler/.

@osa1
Copy link
Contributor Author

osa1 commented Dec 23, 2020

Is this comment relevant to this issue?

/// The parameter `explicit` indicates if this is an *explicit* dereference.
/// Some types -- notably unsafe ptrs -- can only be dereferenced explicitly.

Removing the if explicit guard in the pointer case in that code does not make any difference for this issue.

@camelid
Copy link
Member

camelid commented Dec 23, 2020

Is this comment relevant to this issue?

/// The parameter `explicit` indicates if this is an *explicit* dereference.
/// Some types -- notably unsafe ptrs -- can only be dereferenced explicitly.

Removing the if explicit guard in the pointer case in that code does not make any difference for this issue.

I’m not certain, but I don’t think so. The issue is not about deref but about pointer covariance (at least I think that’s the right word!). But I’m not knowledge enough to know where the issue is.

I recommend asking in the t-compiler/help stream on Zulip. Someone more knowledgeable about this part of the compiler can help you there.

@jonas-schievink jonas-schievink added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 11, 2021
@osa1
Copy link
Contributor Author

osa1 commented Jan 13, 2021

I think I have an idea on what's going on. When resolving method receivers, in these lines:

// skip types that are from a type error or that would require dereferencing
// a raw pointer
!step.self_ty.references_error() && !step.from_unsafe_deref

we don't consider Test (in my example above) as a potential receiver, as that would require dereferencing the pointer, which is unsafe.

However without considering Test as a potential receiver we can't consider &Test or *const Test as potential receivers. There's no other way currently to go from a *mut Test to &Test or *const Test.

I tried removing the !step.from_unsafe_deref guard in the code above to try this out but it did not fix this issue. So perhaps this is not the full story yet.

@osa1
Copy link
Contributor Author

osa1 commented Jan 13, 2021

This code seems relevant:

// Insert a `&*` or `&mut *` if this is a reference type:
if let ty::Ref(_, _, mutbl) = *step.self_ty.value.value.kind() {
pick.autoderefs += 1;
pick.autoref = Some(mutbl);
}

I think this is the code that, when the original receiver has type &T, tries receivers with types &*v or &mut *v (depending on mutability of the original receiver).

Unfortunately if I do the same treatment to pointers in that code I can no longer build stage 1 compiler, so can't even try it..

@osa1
Copy link
Contributor Author

osa1 commented Jan 14, 2021

Re: the code above, I have this diff:

diff --git a/compiler/rustc_typeck/src/check/method/probe.rs b/compiler/rustc_typeck/src/check/method/probe.rs
index d4631c465a3..b0febd5eccb 100644
--- a/compiler/rustc_typeck/src/check/method/probe.rs
+++ b/compiler/rustc_typeck/src/check/method/probe.rs
@@ -20,7 +20,7 @@
 use rustc_infer::infer::{self, InferOk, TyCtxtInferExt};
 use rustc_middle::middle::stability;
 use rustc_middle::ty::subst::{InternalSubsts, Subst, SubstsRef};
-use rustc_middle::ty::GenericParamDefKind;
+use rustc_middle::ty::{GenericParamDefKind, TypeAndMut};
 use rustc_middle::ty::{
     self, ParamEnvAnd, ToPolyTraitRef, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness,
 };
@@ -1111,9 +1111,16 @@ fn pick_by_value_method(
                 pick.autoderefs = step.autoderefs;

                 // Insert a `&*` or `&mut *` if this is a reference type:
-                if let ty::Ref(_, _, mutbl) = *step.self_ty.value.value.kind() {
-                    pick.autoderefs += 1;
-                    pick.autoref = Some(mutbl);
+                match *step.self_ty.value.value.kind() {
+                    ty::Ref(_, _, mutbl) => {
+                        pick.autoderefs += 1;
+                        pick.autoref = Some(mutbl);
+                    }
+                    ty::RawPtr(TypeAndMut { ty: _, mutbl }) => {
+                        pick.autoderefs += 1;
+                        pick.autoref = Some(mutbl);
+                    }
+                    _ => {}
                 }

                 pick

This causes build errors in libraries like

error[E0282]: type annotations needed
   --> library/core/src/fmt/num.rs:234:45
    |
209 | / macro_rules! impl_Display {
210 | |     ($($t:ident),* as $u:ident via $conv_fn:ident named $name:ident) => {
211 | |         fn $name(mut n: $u, is_nonnegative: bool, f: &mut fmt::Formatter<'_>) -> fmt::Result {
212 | |             // 2^128 is about 3*10^38, so 39 gives an extra byte of space
...   |
234 | |                     let d1 = (rem / 100) << 1;
    | |                                             ^ cannot infer type for type `{integer}`
...   |
290 | |     };
291 | | }
    | |_- in this expansion of `impl_Display!`
...
462 | /     impl_Display!(
463 | |         i8, u8, i16, u16, i32, u32, i64, u64, usize, isize
464 | |             as u64 via to_u64 named fmt_u64
465 | |     );
    | |______- in this macro invocation

error: aborting due to previous error

Anyone know how can that change cause this kind of errors?

@camelid
Copy link
Member

camelid commented Jan 15, 2021

Hmm, that is pretty weird. You might want to ask in #t-compiler/help on Zulip if you haven't already – somewhere there may be able to help.

@osa1
Copy link
Contributor Author

osa1 commented Jan 15, 2021

mergify bot pushed a commit to dfinity/motoko that referenced this issue Jan 19, 2021
Previously 1461f56 ported the GC to Rust. This patch ports rest of the RTS to
Rust and removes all C code.

Note that this is not 100% direct port. I rewrote some parts, and implemented
new tests. Notable changes:

- Float functions that were just wrappers of musl/libc float functions are
  removed. Compiler now directly calls musl/libc float functions. Only float
  function left in the RTS is `float_fmt`.

- We now use Rustc core's char functions instead of musl's. This allows us to
  handle cases where upper/lowercase of a character is multiple characters. For
  backwards compatibility we return the input chracter in these cases for now.

- Closure table is slightly refactored:
  - `FREE_SLOT` is no longer shifted, it holds the next free slot directly
  - The magic value `FULL` is gone, we check if the table is full by comparing
    `FREE_SLOT` with table's length field.

- Added a dummy return value to `wctomb` stub. Previously it wasn't returning
  anything even though it should return an `int`. (we should probably panic in
  this function)

- (s)leb128 encoding/decoding code is slightly refactored to use binary
  patterns instead of hexadecimal numbers for checking specific bits. For
  example we now use `0b1000_0000` to check the highest order bit, instead of
  `0x80` etc.

- `utf8_valid.c` is completely gone. We now use Rust core's `str::from_utf8` to
  validate UTF-8 strings.

- Lots of debug functions added. Notably, calling `debug::dump_heap` prints the
  entire heap using `ic0.debug_print`. Debug functions are only compiled in
  debug mode. (when Rust code is built without `--release`)

- Rust arithmetic operations have overflow and underflow checking in debug
  mode, so all arithmetic in the RTS is checked for those in debug mode now. We
  now use `add_wrapping` etc. methods in specific places to explicitly allow
  overflows and underflows.

- All methods that previously had a `*const` receiver now have a `*mut`
  receiver. This is because of the [rustc bug 80258][1] which doesn't allow
  passing `*mut` pointer to a method that expects `*const`, and having `*const`
  methods requires a lot of casts in use sites.

  (Note that `*mut` vs. `*const` pointers in Rust are not like `&mut` and `&`,
  unlike references, in pointers one can be coerced into the other. My
  understanding is `*mut` and `*const` exist for similar reasons they exist in
  C, to hint about mutability of the pointed memory.)

[1]: rust-lang/rust#80258

## New test suite for the RTS

We now have a test suite implemented in Rust where we can use the entire
standard library and other Rust libraries. All existing tests are ported
directly. AFL tests are ported as quickcheck tests. New quickcheck tests added
for text functions (iterators, concatenation, text-to-blob etc.).

The test suite runs on WASI. This makes it more realistic (it uses the same
`libtommath.a` as the real RTS) and easier to run on darwin etc. 

A previous version of this PR had the test suite running as a 32bit binary, i.e.
using cross-compilation. This worked, but caused issues with CI, hence
Joachim changed that. Old comments follow:

The test suite currently runs on native, but it needs to be compiled to a
32-bit platform as the RTS only supports 32-bit platforms, and it would be a
lot of work to make it work on 64-bit platforms. This is checked in run time
and the test suite fails (returns 1) with this meesage when run on 64-bit:

    Motoko RTS only works on 32-bit architectures

An easy way to run the test suite on x86/64 platforms is with:

    TOMMATHSRC=... cargo run --target=i686-unknown-linux-gnu

TOMMATHSRC should be the value of nix expression `nixpkgs.sources.libtommath`.
I didn't update nix files yet to run the tests on CI yet.

Differences in the RTS code when compiled to be linked with moc-generated code
and compiled for testing:

- The test suite runs on native, so we compile the RTS code to i686 when
  testing, instead of wasm32.

  Because both platforms are 32-bit this shouldn't cause too much changes, but
  there can still be subtle changes in how LLVM decides to lay out struct
  fields. For example, this C struct and its Rust equivalent:

      struct Test {
          uint32_t a;
          uint64_t b;
      };

  by default is 4 words on wasm32, but 3 words on i686. A change in this patch
  is we now make sure that all our sturcts are packed (with no padding between
  fields) by using `#[repr(packed)]` attributes.

- When compiled to native the RTS uses a different allocator, where
  `alloc_words` is just `malloc`, and we never free allocated objects.

  The GC module is not compiled when generating native.

  This could potentially cause OOM issues if we run longer/more tests, as
  32-bit platforms have 4G memory, but so far things work fine.

  In the future If memory limit becomes a problem we can allocate a (say, 1G)
  region in each test and free it afterwards.

  Finally, we could make the GC compile to native. I think this is not too
  difficult to do, but I decided to not invest time into this so far as this
  patch already took too long.

## Notes on using quickcheck

I found the Rust quickcheck library a bit lacking in some aspects. For example,
as far as I can see, there's no way to specify number of iterations for
individual tests. All we can do is setting the global number with an env
variable:

    QUICKCHECK_TESTS=100 cargo run --target=i686-unknown-linux-gnu

Secondly, it seems not possible to specify the seed, for individual tests, or
globally. This means even without any relevant changes a test may fail on CI
randomly (assuming there are bugs). When a test fails quickcheck tries to
shrink the argument and then print the arguments, so we can copy that from the
CI logs and make it a unit test, which is good. But random behavior on CI may
not be ideal.

I haven't compared proptest API to quickcheck to see if it solves any of these
problems.

## Cycle and binary size changes

CI reports

> In terms of gas, 3 tests improved and the mean change is -3.7%.
> In terms of size, 3 tests regressed and the mean change is +56.0%.

The size changes are for the most part the static `LOWERCASE_TABLE` from https://doc.rust-lang.org/src/core/unicode/unicode_data.rs.html#568 

## Future cleanup

The following things could be done as future cleanup

- Don't commit bindgen-generated tommath bindings, run bindgen in build time.
- Build and link tommath via Rust build script (`build.rs`) instead of manually in a Makefile, as we did in the tests when they were built manually.
- Or: Build and link `libtommath.a` using nix, and treat it more like a “normal system library”, so that we get nix caching.
@osa1
Copy link
Contributor Author

osa1 commented Feb 22, 2021

My previous attempt was completely wrong. I managed to make this work today. I can boot rustc, and the repro now works. However it's hacky, and I had to relax checks in two places to accept passing *mut to a method with *const receiver (as *mut A is not a subtype of *const A).

I think the right way to implement this would be to turn the argument (a *mut value) to *const by adding &*. If I understand the code correctly, currently this kind of thing is done via the Pick type with has fields for how many autoderefs to apply to the value, and whether to apply autoref or not (with given mutability). This type currently does not allow expressing converting a *mut A to *const A. I think the right way to fix this is by somehow updating the type so that we can express this conversion.

@RalfJung
Copy link
Member

by adding &mut *

Without understanding the details here, this sounds wrong -- &mut creates a reference, not a raw pointer, and that should certainly not be done implicitly when the source code just used raw pointers.

@osa1
Copy link
Contributor Author

osa1 commented Feb 23, 2021

(I had a typo in my comment above, I meant &* not &mut *)

@RalfJung How else can I convert a *mut T to a *const T? To summarize, I'm trying to make f.x() work when f is *mut T and receiver of x is *const T.

@osa1
Copy link
Contributor Author

osa1 commented Feb 23, 2021

Looking into the Pick type and its uses more, I think we don't really insert &* but in effect just cast the type. I may be wrong though, first time looking at this code. Relevant code:

@osa1
Copy link
Contributor Author

osa1 commented Feb 23, 2021

I pushed my code and submitted draft PR #82436. As the description says, it's a hacky proof-of-concept currently, but I can boot rustc with it and the original repro works. I'll be working on it.

@rustbot claim

@RalfJung
Copy link
Member

(I had a typo in my comment above, I meant &* not &mut *)

That's just as problematic.

How else can I convert a *mut T to a *const T?

You can just do x as *const T.

@osa1
Copy link
Contributor Author

osa1 commented Feb 23, 2021

That's just as problematic.

Right, I'm aware that dereferencing a pointer is unsafe and hence problematic to do automatically.

You can just do x as *const T.

That's what we do, yes. See my updates above.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 12, 2021
Allow calling *const methods on *mut values

This allows `*const` methods to be called on `*mut` values.

TODOs:

- [x] ~~Remove debug logs~~ Done.
- [x] ~~I haven't tested, but I think this currently won't work when the `self` value has type like `&&&&& *mut X` because I don't do any autoderefs when probing. To fix this the new code in `rustc_typeck::check::method::probe` needs to reuse `pick_method` somehow as I think that's the function that autoderefs.~~ This works, because autoderefs are done before calling `pick_core`, in `method_autoderef_steps`, called by `probe_op`.
- [x] ~~I should probably move the new `Pick` to `pick_autorefd_method`. If not, I should move it to its own function.~~ Done.
- [ ] ~~Test this with a `Pick` with `to_ptr = true` and `unsize = true`.~~ I think this case cannot happen, because we don't have any array methods with `*mut [X]` receiver. I should confirm that this is true and document this. I've placed two assertions about this.
- [x] ~~Maybe give `(Mutability, bool)` a name and fields~~ I now have a `to_const_ptr` field in `Pick`.
- [x] ~~Changes in `adjust_self_ty` is quite hacky. The problem is we can't deref a pointer, and even if we don't have an adjustment to get the address of a value, so to go from `*mut` to `*const` we need a special case.~~ There's still a special case for `to_const_ptr`, but I'm not sure if we can avoid this.
- [ ] Figure out how `reached_raw_pointer` stuff is used. I suspect only for error messages.

Fixes rust-lang#80258
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 13, 2021
Allow calling *const methods on *mut values

This allows `*const` methods to be called on `*mut` values.

TODOs:

- [x] ~~Remove debug logs~~ Done.
- [x] ~~I haven't tested, but I think this currently won't work when the `self` value has type like `&&&&& *mut X` because I don't do any autoderefs when probing. To fix this the new code in `rustc_typeck::check::method::probe` needs to reuse `pick_method` somehow as I think that's the function that autoderefs.~~ This works, because autoderefs are done before calling `pick_core`, in `method_autoderef_steps`, called by `probe_op`.
- [x] ~~I should probably move the new `Pick` to `pick_autorefd_method`. If not, I should move it to its own function.~~ Done.
- [ ] ~~Test this with a `Pick` with `to_ptr = true` and `unsize = true`.~~ I think this case cannot happen, because we don't have any array methods with `*mut [X]` receiver. I should confirm that this is true and document this. I've placed two assertions about this.
- [x] ~~Maybe give `(Mutability, bool)` a name and fields~~ I now have a `to_const_ptr` field in `Pick`.
- [x] ~~Changes in `adjust_self_ty` is quite hacky. The problem is we can't deref a pointer, and even if we don't have an adjustment to get the address of a value, so to go from `*mut` to `*const` we need a special case.~~ There's still a special case for `to_const_ptr`, but I'm not sure if we can avoid this.
- [ ] Figure out how `reached_raw_pointer` stuff is used. I suspect only for error messages.

Fixes rust-lang#80258
@bors bors closed this as completed in 98fbc09 Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull C-bug Category: This is a bug. F-arbitrary_self_types `#![feature(arbitrary_self_types)]` requires-nightly This issue requires a nightly compiler in some way. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants