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

An initial section for libs docs #281

Merged
merged 15 commits into from
Dec 5, 2019
Merged

An initial section for libs docs #281

merged 15 commits into from
Dec 5, 2019

Conversation

KodrAus
Copy link
Contributor

@KodrAus KodrAus commented Nov 26, 2019

Recently in Libs we've been talking about putting together some docs with various things we try to keep in mind when working on std (inspired by .NET's runtime developer guide), and we thought the forge here would be a good home.

This PR is an attempt to bootstrap that doc. There's not a lot here yet, and what is there is probably both incomplete and plain incorrect :) It's just a starting point to iterate on, so any suggested corrections/enhancements would be greatly appreciated! Any thoughts on organization would also be good.

cc @rust-lang/libs

Rendered

src/libs/maintaining-std.md Outdated Show resolved Hide resolved
src/libs/maintaining-std.md Outdated Show resolved Hide resolved
src/libs/maintaining-std.md Outdated Show resolved Hide resolved
src/libs/maintaining-std.md Outdated Show resolved Hide resolved

### Could this affect inference?

New impls on public traits for public items may cause inference to break when there are generics involved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is limited to generics, right? Can't adding a second impl of some trait always cause inference failures, regardless of whether any of the impls is generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was mostly thinking of the case here where we'll optimistically type things when there's only a single trait impl. Say we have something like:

impl From<&'_ str> for Arc<str> { .. }

let b = Arc::from("a");

then we add:

impl From<&'_ str> for Arc<str> { .. }
+ impl From<&'_ str> for Arc<String> { .. }

then

let b = Arc::from("a");

will no longer compile, because we're relying on inference to figure out the T in Box<T>. I think this sort of thing comes up every now and again with types like Arc, and in new FromIterator impls.

Can we think of any other examples of real inference breakage that could make this section more useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another case to consider is deref coersion: rust-lang/rust#51916

KodrAus and others added 3 commits November 27, 2019 07:01
Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>
Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>
Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>
src/libs/maintaining-std.md Outdated Show resolved Hide resolved
src/libs/maintaining-std.md Outdated Show resolved Hide resolved
src/libs/maintaining-std.md Outdated Show resolved Hide resolved
src/libs/maintaining-std.md Outdated Show resolved Hide resolved

### Could `mem::forget` break assumptions?

Rust doesn't guarantee destructors will run, so code should avoid relying on them for safety. Remember, [everybody poops][Everybody Poops].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @rust-lang/wg-unsafe-code-guidelines

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems accurate in general. However, also see these UCG issues. And note that for some instances, we do guarantee things about destructors -- I am talking about the drop guarantee for pinned data. So, just because mem::forget is safe doesn't mean you can just drop arbitrary storage (like stack frames) without running the destructors. mem::forget leaks, that is okay -- but it doesn't deallocate without dropping, which is not (in general) okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks, I wasn't familiar with the drop guarantee for Pin!

The kind of undesirable invariant I was trying to catch with this section was just along the same lines as Vec::drain. Maybe we should reword this one around whether or not there are manual impls for Drop, and some of the things to keep in mind there? Like remembering that leaking is a thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is definitely worth mentioning that mem::forget is a thing, and that old Vec::drain and thread::spawn are unsound. But at the same time I think we should remind the reader that everything can leak does not imply that we can just deallocate any storage without calling destructors. This is an incorrect inference I have seen several people make.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what we're saying here is that, in general, you can't guarantee an impl of Drop will get called, but you must ensure that an impl of Drop will get called if its storage is being deallocated/repourposed through something like ptr::write? And that's on the burden of the caller to ptr::write to enforce?

Copy link
Member

@RalfJung RalfJung Nov 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Calling all relevant Drop impls is a precondition to deallocating or repurposing storage.

Or, in other words: there is no guarantee that an impl of Drop will get called. However, if the impl of Drop does not get called, then the memory backing that instance of the type must leak.

Note that this is true in general, i.e., when working with arbitrary T. Under more controlled circumstances, skipping Drop can be fine -- e.g. if you know that there is definitely nothing pinned in that storage you are repurposing.

src/libs/maintaining-std.md Outdated Show resolved Hide resolved
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this all up @KodrAus!

@KodrAus
Copy link
Contributor Author

KodrAus commented Dec 4, 2019

@rust-lang/infra would you be comfortable with @rust-lang/libs pushing to this with impunity? I think we'd like to be able to treat it as a bit of a living document that we can tweak as things come up in a low-ceremony way.

@Mark-Simulacrum
Copy link
Member

I think so (speaking personally). The forge is intended to be a place for the teams to gather together internal documentation for processes and other information. I personally think that edits should feel free to be made even without going through the PR process, though I also find that leads to breakage of CI frequently (seems like every time I try to sneak something in), so it may not be a good idea :) In some sense, it should be up to the team how y'all want to manage your pages on the forge.

I think the infra team is not strictly in charge of this area, too; I would feel free to proceed as you wish without gating on "us."

Co-Authored-By: Mark Rousskov <mark.simulacrum@gmail.com>
@KodrAus
Copy link
Contributor Author

KodrAus commented Dec 5, 2019

Thanks @Mark-Simulacrum! I guess we would need some permission tweaks then so that members of Libs could merge their PRs. I think it's fair to expect a PR to be opened (since that's straightforward to do in GitHub's UI already) and waiting for a green tick before merging.

@XAMPPRocky
Copy link
Member

Thank you for your PR! While permissions are being sorted out, for now @KodrAus should we merge this in, so that it's public?

@Mark-Simulacrum
Copy link
Member

I have granted the libs and compiler (while I'm at it) teams write access to this repository.

@alexcrichton alexcrichton merged commit ada544a into rust-lang:master Dec 5, 2019
@KodrAus KodrAus deleted the libs branch December 7, 2019 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants