Skip to content

Improve some rough edges. #15

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

Closed
wants to merge 8 commits into from
Closed

Improve some rough edges. #15

wants to merge 8 commits into from

Conversation

Alextopher
Copy link

@Alextopher Alextopher commented May 25, 2023

Stop checking in *_unchecked methods

Enabling debug assertions in *_unchecked methods violates the principal of least surprise. Remove some debug assertions and update the doc comments explaining method behavior in more detail.

Add transmute<N, D>(self)

I think transmute might not be the best name. I'm open to suggestions.

I've added some missing functionality allowing users to conveniently change the ratio of an rc. This help makes more patterns expressive, albeit in unsafe.

rust-lang/rust#77708

There were a number of "fixme"s in StaticRcRef blocked on an ICE. That problem seems to be fixed now, so I've gone ahead and brought StaticRcRef to it's intended place.

cargo fmt, cargo clippy, cargo hack

  • I've formatted the repo with cargo fmt.
  • Made a few improvements suggested by cargo clippy.
  • Tested the crate with cargo-hack and verified that for any subset of features flags all tests pass.

After this PR I'm happy to add some github actions to make these steps proper CI.

Copy link
Owner

@matthieu-m matthieu-m left a comment

Choose a reason for hiding this comment

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

A lot of good improvements, in general.

My main concerns are with the 4th commit:

  • The removal of debug assertions is not motivated, and I'd like to understand the exact motivation for it.
  • The documentation changes switch from documenting intent to documenting implementation, and once again I'd like to understand the exact motivation for it.

In general, I tend to prefer checking as many assumptions as possible in Debug, and being as vague as I can about exact implementation in documentation. This is not dogma, more of a guideline, so I don't necessarily mind departing from it, but I would only do so for a good reason, and in the absence of alternative.

src/lib.rs Outdated
#![cfg_attr(feature = "nightly-generator-trait", feature(generator_trait))]
// https://github.com/rust-lang/rust/issues/43122
#![cfg_attr(feature = "nightly-generator-trait", feature(generator_trait))] // Unused ????
#![cfg_attr(feature = "nightly-dispatch-from-dyn", feature(dispatch_from_dyn))] // Unused ????
Copy link
Owner

Choose a reason for hiding this comment

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

I see you added this Unused comment, if they are, maybe they can be removed?

Copy link
Author

Choose a reason for hiding this comment

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

I'll look into the git blame to see why it was originally added 😄

Copy link
Author

Choose a reason for hiding this comment

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

They are certainly used - I'm not sure what I missed when I added those comments.

Copy link
Owner

Choose a reason for hiding this comment

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

I do believe they are used. Specifically, I do believe that users on nightly can "activate" them by compile the static-rc crate with the nightly-generator-trait and nightly-dispatch-from-dyn features.

I would recommend removing the added comments, and leaving them as is.

@@ -542,9 +535,6 @@ impl<T: ?Sized, const NUM: usize, const DEN: usize> StaticRc<T, NUM, DEN> {
where
AssertEqType!(NUM, A + B): Sized,
{
#[cfg(debug_assertions)]
let (left, right) = Self::validate_pair(left, right);
Copy link
Owner

Choose a reason for hiding this comment

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

What is your motivation for removing those debug aids?

Copy link
Author

Choose a reason for hiding this comment

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

I found it surprising that an "unchecked" function was still preforming checks. I had assumed joining would simply just forget one side and transmute the other, which is actually what I wanted for my use case. In my opinion there should be supported ways to opt out of the validity requirement for this crate. Given that it generally doesn't seem to be unsound nor UB. Leaking memory is safe.

Copy link
Owner

Choose a reason for hiding this comment

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

In my opinion there should be supported ways to opt out of the validity requirement for this crate.

I see.

I agree that opting out -- temporarily -- of the validity requirement for this crate should definitely be possible. An unchecked into_parts + from_parts, and an unchecked transmute, sound like worthy additions in this regard.

Given that it generally doesn't seem to be unsound nor UB. Leaking memory is safe.

You are correct that leaking memory is safe, but that is not all that is happening here. If you were to accidentally pass two StaticRc with mismatching pointers:

  • You would be forgetting part of the ownership of the right pointer -- which would ultimately lead to a memory leak. That is sound.
  • You would, however, be "claiming" more ownership of the left pointer than you ought to -- which would ultimately lead to releasing its memory whilst other StaticRc to it, aka use-after-free, which is definitely unsound.

This is why it is actually unsound to have mismatching pointers in join_unchecked, and why the method is labelled unsafe.

The method, in the tradition of the standard library, is not marked "unchecked" as a guarantee that no check is ever performed, but instead as a note that not all checks may be performed and it's thus the responsibility of the caller to pay attention to the safety invariants. It then actually performs the checks in Debug for "friendliness" reasons: notifying the user of mistakes, and elides them in Release for performance reason.

@Alextopher
Copy link
Author

I think you have a fair perspective. I was drawn towards this crate because I wanted fine control over how I alias data to get maximum performance, and felt that I couldn't communicate my invariants without giving up performance - or I would have to get clever with unsafe. After some reflection I think adding a documented transmute method (that has been signed off as verifiably working) improves the experience enough I don't feel too strongly about having unchecked changed.

@Alextopher
Copy link
Author

Alextopher commented May 28, 2023

On that note, would this be sound?

    #[inline(always)]
    pub unsafe fn transmute<const A: usize, const B: usize>(self) -> StaticRc<T, A, B> {
        transmute(self)
    }

@matthieu-m
Copy link
Owner

On that note, would this be sound?

    #[inline(always)]
    pub unsafe fn transmute<const A: usize, const B: usize>(self) -> StaticRc<T, A, B> {
        transmute(self)
    }

Soundness is a property of the whole program... so it would depend how it's used. If someone has a 1/3 pointer and 2/3 pointer and transmutes them to make 2 1/2 pointers, then that's sound. On the other hand, if they transmute them to make 2 1/1 pointers, then they'll have a use-after free and everything goes downhill from there.

I do note that your usecase seems fulfilled by StaticRc::into_raw and StaticRc::from_raw (the latter being unsafe).

I do see the value as going to a raw pointer, as then NonNull makes not claim about their share of ownership, while I am more skeptical about transmute: the resulting StaticRc is then misleading -- its actual ownership doesn't match its share -- when the entire purpose of StaticRc is to track such share accurately. Once you subvert one StaticRc, you somewhat lose confidence into all StaticRc of the program, as any could have been derived from the subverted one.

@Alextopher Alextopher closed this by deleting the head repository Oct 11, 2023
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.

2 participants