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

Implement RFC 2580: Pointer metadata & VTable #81172

Merged
merged 12 commits into from
Feb 18, 2021

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Jan 18, 2021

RFC: rust-lang/rfcs#2580

Before merging this PR:


This PR extends the language with a new lang item for the Pointee trait which is special-cased in trait resolution to implement it for all types. Even in generic contexts, parameters can be assumed to implement it without a corresponding bound.

For this I mostly imitated what the compiler was already doing for the DiscriminantKind trait. I’m very unfamiliar with compiler internals, so careful review is appreciated.

This PR also extends the standard library with new unstable APIs in core::ptr and std::ptr:

pub trait Pointee {
    /// One of `()`, `usize`, or `DynMetadata<dyn SomeTrait>`
    type Metadata: Copy + Send + Sync + Ord + Hash + Unpin;
}

pub trait Thin = Pointee<Metadata = ()>;

pub const fn metadata<T: ?Sized>(ptr: *const T) -> <T as Pointee>::Metadata {}

pub const fn from_raw_parts<T: ?Sized>(*const (), <T as Pointee>::Metadata) -> *const T {}
pub const fn from_raw_parts_mut<T: ?Sized>(*mut (),<T as Pointee>::Metadata) -> *mut T {}

impl<T: ?Sized> NonNull<T> {
    pub const fn from_raw_parts(NonNull<()>, <T as Pointee>::Metadata) -> NonNull<T> {}

    /// Convenience for `(ptr.cast(), metadata(ptr))`
    pub const fn to_raw_parts(self) -> (NonNull<()>, <T as Pointee>::Metadata) {}
}

impl<T: ?Sized> *const T {
    pub const fn to_raw_parts(self) -> (*const (), <T as Pointee>::Metadata) {}
}

impl<T: ?Sized> *mut T {
    pub const fn to_raw_parts(self) -> (*mut (), <T as Pointee>::Metadata) {}
}

/// `<dyn SomeTrait as Pointee>::Metadata == DynMetadata<dyn SomeTrait>`
pub struct DynMetadata<Dyn: ?Sized> {
    // Private pointer to vtable
}

impl<Dyn: ?Sized> DynMetadata<Dyn> {
    pub fn size_of(self) -> usize {}
    pub fn align_of(self) -> usize {}
    pub fn layout(self) -> crate::alloc::Layout {}
}

unsafe impl<Dyn: ?Sized> Send for DynMetadata<Dyn> {}
unsafe impl<Dyn: ?Sized> Sync for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Debug for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Unpin for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Copy for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Clone for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Eq for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> PartialEq for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Ord for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> PartialOrd for DynMetadata<Dyn> {}
impl<Dyn: ?Sized> Hash for DynMetadata<Dyn> {}

API differences from the RFC, in areas noted as unresolved questions in the RFC:

  • Module-level functions instead of associated from_raw_parts functions on *const T and *mut T, following the precedent of null, slice_from_raw_parts, etc.
  • Added to_raw_parts

@SimonSapin SimonSapin added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Jan 18, 2021
@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2021
@SimonSapin
Copy link
Contributor Author

What’s the correct syntax for intra-doc links to a method of a primitive type? ./x doc library/std --stage 1 gives:

error: unresolved link to `to_raw_parts`
  --> library/core/src/ptr/metadata.rs:51:6
   |
51 | /// [`to_raw_parts`]: <*const _>::to_raw_parts
   |      ^^^^^^^^^^^^^^ no item named `to_raw_parts` in scope

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jan 18, 2021

What’s the correct syntax for intra-doc links to a method of a primitive type? ./x doc library/std --stage 1 gives:

error: unresolved link to `to_raw_parts`
  --> library/core/src/ptr/metadata.rs:51:6
   |
51 | /// [`to_raw_parts`]: <*const _>::to_raw_parts
   |      ^^^^^^^^^^^^^^ no item named `to_raw_parts` in scope

This was just implemented a few weeks ago and isn't available on beta: #80181

@SimonSapin
Copy link
Contributor Author

Thanks! Tests in that PR show that the answer (to the question I should have asked about the syntax for raw pointers specifically) is *const::foo, not <*const T>::foo.

isn't available on beta

That’s fine, the item with that doc-comment has #[cfg(not(bootstrap))] anyway since it depends on the new lang item.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jan 18, 2021

Thanks! Tests in that PR show that the answer (to the question I should have asked about the syntax for raw pointers specifically) is *const::foo, not <*const T>::foo.

Note that pointers are about to be put behind a feature gate because there's no way to distinguish *const from *mut currently: #81015

@SimonSapin
Copy link
Contributor Author

Indeed, I’ve added #![cfg_attr(not(bootstrap), feature(intra_doc_pointers))].

By the way, I’ve had to add it in both libcore and libstd. Presumably pub use core::ptr; causes those docs to be rendered again, and rustdoc looks at feature gates of "the current crate" rather than the crate that defines the item.


The ui/panic-handler/weak-lang-item.rs failure seems unrelated to the PR?

@jyn514
Copy link
Member

jyn514 commented Jan 18, 2021

The ui/panic-handler/weak-lang-item.rs failure seems unrelated to the PR?

Yeah that one seems flaky, it failed in #80965 too.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 22, 2021

☔ The latest upstream changes (presumably #81160) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 29, 2021

I've created a tracking issue: #81513

@rust-log-analyzer

This comment has been minimized.

@SimonSapin
Copy link
Contributor Author

The RFC is merged, tracking issue numbers are in, and CI is green. This is only waiting for review now.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 2, 2021

r? @oli-obk

@jyn514
Copy link
Member

jyn514 commented Feb 17, 2021

@bors r-

This is unlikely to be spurious, there's a rustdoc bug here somewhere. @SimonSapin linking to pointers is still pretty buggy, I would avoid it here and use ../../pointer/method.whatever.html for now. If you can post the file doc/std/keyword.Self.html that would be helpful for debugging the issue. If you could make an MCVE that would be even better.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 17, 2021
@SimonSapin
Copy link
Contributor Author

If you could make an MCVE that would be even better.

That would be ideal of course, but I can not reproduce at all. I’ve rebased again just in case this is related to something merged into master very recently, and ./x.py test src/tools/linkchecker --stage 2 passes for me:

Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.14s
	finished in 6.496 seconds
Build completed successfully in 0:00:35

I assume the line with lower-cased finished is from the linkchecker tool running.

By the way, stages appear to be off by one between builds and docs. Stage 1 is the first where ./x.py build builds a standard library with #[cfg(not(bootstrap))], but stage 2 is the first where ./x.py doc documents a standard library built with #[cfg(not(bootstrap))].

Here is keyword.Self.html from my local build anyway: https://gist.github.com/SimonSapin/7d4763a08063ca7f2951e307618b6767. HTML IDs search and rustdoc-vars do appear to be unique in it, based on eyeballing with grep.

@ehuss
Copy link
Contributor

ehuss commented Feb 18, 2021

The error is caused by the lack of case-sensitivity on the filesystem. std\keyword.Self.html and std\keyword.self.html end up stomping on one another. Presumably you just had some bad luck where rustdoc was writing to both simultaneously, and it ended up corrupted, maybe? I'm not too familiar with how rustdoc works. Pretty sure it is unrelated to this PR.

By the way, stages appear to be off by one between builds and docs.

Can you explain more what you saw? If I run x.py doc --stage=1 library/std, I see the not(bootstrap) docs in build/*/doc/std/....

@jyn514
Copy link
Member

jyn514 commented Feb 18, 2021

The error is caused by the lack of case-sensitivity on the filesystem. std\keyword.Self.html and std\keyword.self.html end up stomping on one another. Presumably you just had some bad luck where rustdoc was writing to both simultaneously, and it ended up corrupted, maybe? I'm not too familiar with how rustdoc works. Pretty sure it is unrelated to this PR.

Ah ok, this is #25879 then.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Feb 18, 2021

📌 Commit cac71bf has been approved by oli-obk

@bors bors 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 Feb 18, 2021
@bors
Copy link
Contributor

bors commented Feb 18, 2021

⌛ Testing commit cac71bf with merge d1462d8...

@bors
Copy link
Contributor

bors commented Feb 18, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing d1462d8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 18, 2021
@bors bors merged commit d1462d8 into rust-lang:master Feb 18, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 18, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at d1462d855 Auto merge of #81172 - SimonSapin:ptr-metadata, r=oli-obk
##[group]Run src/ci/publish_toolstate.sh
src/ci/publish_toolstate.sh
env:
  SCCACHE_BUCKET: rust-lang-ci-sccache2
  DEPLOY_BUCKET: rust-lang-ci2
  TOOLSTATE_REPO: https://github.com/rust-lang-nursery/rust-toolstate
---
  CACHE_DOMAIN: ci-caches.rust-lang.org
  TOOLSTATE_REPO_ACCESS_TOKEN: ***
##[endgroup]
Cloning into 'rust-toolstate'...
/home/runner/work/rust/rust/src/tools/publish_toolstate.py:121: DeprecationWarning: 'U' mode is deprecated
📣 Toolstate changed by rust-lang/rust#81172!

Tested on commit rust-lang/rust@d1462d8558cf4551608457f63d9b999185ebf3bf.
Direct link to PR: <https://github.com/rust-lang/rust/pull/81172>

💔 miri on windows: test-fail → build-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-fail → build-fail (cc @eddyb @RalfJung @oli-obk).

  with open(path, 'rU') as f:
Traceback (most recent call last):
  File "/home/runner/work/rust/rust/src/tools/publish_toolstate.py", line 338, in <module>
    response = urllib2.urlopen(urllib2.Request(
  File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 522, in open
    req = meth(req)
  File "/usr/lib/python3.8/urllib/request.py", line 1281, in do_request_
    raise TypeError(msg)
TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
##[error]Process completed with exit code 1.

@SimonSapin SimonSapin deleted the ptr-metadata branch February 18, 2021 07:53
@SimonSapin
Copy link
Contributor Author

Can you explain more what you saw? If I run x.py doc --stage=1 library/std, I see the not(bootstrap) docs in build/*/doc/std/....

After running ./x.py doc library/std --open on the branch of this PR I did not see in the browser any of the APIs introduced by this branch. Then same with ./x.py doc library/std --open --stage 1. Only with ./x.py doc library/std --open --stage 2 did they show up. This is my config.toml:

profile = "compiler"
changelog-seen = 2

[build]
low-priority = true

@ehuss
Copy link
Contributor

ehuss commented Feb 18, 2021

The default for x.py doc without a stage is 0 (not a default I would recommend). On latest master, I just tried ./x.py doc library/std --open and saw the API is missing and then x.py doc library/std --open --stage 1 , and I see your new changes, so I'm not sure what happened. Maybe there was a stale browser cache from running the previous command?

I'm trying to think if there's some way a fingerprint error could cause it to fail to rebuild, but I can't think of a way to trigger that. When developing rustc, the fingerprint Cargo uses for a stage1 compiler is not very good (because it does not contain the SHA hash, it can't tell when the compiler changes), but in this scenario I don't think that matters.

@SimonSapin
Copy link
Contributor Author

and then x.py doc library/std --open --stage 1 , and I see your new changes

Ok, it’s possible I made some mistake on my end then. Thanks for trying to reproduce.

@yshui
Copy link
Contributor

yshui commented Jul 22, 2021

@SimonSapin Why isn't DynMetadata repr(C)? What is the intended way of passing the metadata through FFI and back?

@bjorn3
Copy link
Member

bjorn3 commented Jul 22, 2021

Responded on the tracking issue.

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 merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.