Implements `const_deref` feature #49

uselessgoddess · 2022-07-24T17:44:58Z

const_deref feature should add almost all const functionality from std::borrow::Cow

Sorry for the code duplication

CAD97 · 2022-07-24T18:43:21Z

A less duplicated approach (but significantly more complex) would be to use a tt muncher to strip ~const:

macro_rules! cfg_const_trait {
    ( $($tt:tt)* ) => { cfg_const_trait_munch! { [] [$($tt)*] } };
}

#[cfg(not(feature = "const_deref"))]
macro_rules! cfg_const_trait_munch {
    ( [$($done:tt)*] [] ) => { $($done)* };

    // one arm is technically sufficient but more arms reduce recursion depth
    ( [$($done:tt)*] [                  ~const $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)*            ] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt             ~const $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a         ] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt $b:tt       ~const $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a $b      ] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt $b:tt $c:tt ~const $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a $b $c   ] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt $b:tt $c:tt $d:tt  $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a $b $c $d] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt                    $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a         ] [$($todo)*] } };
}

#[cfg(feature = "const_deref")]
macro_rules! cfg_const_trait_munch {
    ( [$($done:tt)*] [$($todo:tt)*] ) => { $($done)* $($todo)* };
}

uselessgoddess · 2022-07-24T20:08:23Z

Cool. But it doesn't work with const trait together const fn

uselessgoddess · 2022-07-24T20:09:34Z

I'm not pro in macros, can you explain what the reason may be

CAD97 · 2022-07-24T20:30:24Z

It's because all this macro is doing is going through the tokens and removing ~const and leaving all of the other tokens. Adding an arm that sees const fn and replaces it with fn is sufficient to allow that to work.

Essentially, all the macro is doing is taking tokens from the right [] and moving them into the left [], but recognizing ~const and const fn to treat them specially. Also, the macro doesn't recurse into {} blocks (which saves a lot of recursive calls), so it has to be used at the level that const shows up; that is, directly around the item whose signature includes ~const.

Since the macro only has to munch over the function declaration (since it jumps the body in on token tree), the following version works fine and is easier to follow:

macro_rules! cfg_const_trait {
    ( $($tt:tt)* ) => { cfg_const_trait_munch! { [] [$($tt)*] } };
}

#[cfg(not(feature = "const_deref"))]
macro_rules! cfg_const_trait_munch {
    ( [$($done:tt)*] [] ) => { $($done)* };

    ( [$($done:tt)*] [~const    $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)*   ] [$($todo)*] } };
    ( [$($done:tt)*] [ const fn $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* fn] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt     $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a] [$($todo)*] } };
}

#[cfg(feature = "const_deref")]
macro_rules! cfg_const_trait_munch {
    ( [$($done:tt)*] [$($todo:tt)*] ) => { $($done)* $($todo)* };
}

The additional arms are merely to make macro expansion take $\dfrac{n}{4}$ steps rather than $n$ steps.

The simplest way to allow wrapping impl const Trait would probably be to strip all const rather than just const as part of ~const or const fn. (Delete the two mentions of fn in the middle arm.) Doing something smarter (e.g. preserving const declarations) is not really within the scope of what macros can do.

uselessgoddess · 2022-07-24T20:40:45Z

Thanks, but this is too detailed. ~~I asked because the following code also doesn't work.~~
But I can't figure out why doesn't recurse into {} blocks

macro_rules! cfg_const_trait_munch {
    ( [$($done:tt)*] [] ) => { $($done)* };

    // one arm is technically sufficient but more arms reduce recursion depth
    ( [$($done:tt)*] [~const $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)*   ] [$($todo)*] } };
    ( [$($done:tt)*] [ const $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)*   ] [$($todo)*] } };
    ( [$($done:tt)*] [$a:tt  $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* $a] [$($todo)*] } };
}

uselessgoddess · 2022-07-24T20:42:14Z

Since the macro only has to munch over the function declaration (since it jumps the body in on token tree), the following version works fine and is easier to follow:

the following version contains link to my PR

uselessgoddess · 2022-07-24T20:46:02Z

Also, the macro doesn't recurse into {} blocks (which saves a lot of recursive calls), so it has to be used at the level that const shows up; that is, directly around the item whose signature includes ~const.

This confused me, because he was still able to remove ~const

CAD97 · 2022-07-24T21:08:55Z

Whoops, I forgot to put a link in at all (it was just [text]()).

The trick about not recursing into {} is that

this version gets stripped

impl<'a, T, U> Cow<'a, T, U>
where
    T: Beef + ?Sized,
    U: Capacity,
{
    cfg_const_trait! {
        /// Borrowed data.
        ///
        /// # Example
        ///
        /// ```rust
        /// use beef::Cow;
        ///
        /// let borrowed: Cow<str> = Cow::borrowed("I'm just a borrow");
        /// ```
        ///
        #[inline]
        pub const fn borrowed(val: &'a T) -> Self
        where T: ~const Beef,
        {
            todo!()
        }
    }
}

but this version doesn't.

cfg_const_trait! {
    impl<'a, T, U> Cow<'a, T, U>
    where
        T: Beef + ?Sized,
        U: Capacity,
    {
        /// Borrowed data.
        ///
        /// # Example
        ///
        /// ```rust
        /// use beef::Cow;
        ///
        /// let borrowed: Cow<str> = Cow::borrowed("I'm just a borrow");
        /// ```
        ///
        #[inline]
        pub const fn borrowed(val: &'a T) -> Self
        where T: ~const Beef,
        {
            todo!()
        }
    }
}

It is probably better to just apply the macro at each level it is needed, rather than adding a recursive call, but it is possible to implement by directly adding the recursion. (This has to be before the fallback arm, or else it will never be taken.)

    ( [$($done:tt)*] [{$($a:tt)*} $($todo:tt)*] ) => { cfg_const_trait_munch! { [$($done)* { cfg_const_trait!{$($a)*} }] [$($todo)*] } };

uselessgoddess · 2022-07-24T21:19:57Z

all this time I misunderstood $($t:tt)* and $t:tt. Thanks :)

CAD97 · 2022-07-24T22:13:38Z

Yep, tt stands for Token Tree, and matches an entire paired {}/[]/() and everything in the pair.

…fix`

CAD97

Note to reviewers: use "ignore whitespace changes" mode.

src/generic.rs

CAD97 · 2022-07-25T15:28:49Z

src/traits.rs

+    // for `~const Steak`
+    // I think that Steak (cooked beef) would contain "cooked" functions
+    // (computed at compile-time)
+    // However, `Vec::new()` also has const len/cap, but `std` does not implement it


The thought so far has been to not put const on methods where the type isn't constructible const (or in Vec's case, can only give one answer in const contexts).

I suspect this will relax in the future, but for the time being it allows greater impl flexibility especially w.r.t. future const allocation.

Co-authored-by: Christopher Durham <cad97@cad97.com>

uselessgoddess · 2022-07-25T15:47:17Z

@CAD97 Is panic considered an allowed style in const functions?
For example, unwrap_borrowed etc. can be const

CAD97 · 2022-07-25T16:00:01Z

It depends a bit. If the panic is a static string and does not need any formatting (i.e. panic!("message") and not panic!("message: {context}")), then it can be const.

Current practice seems to be that such methods should be made const only if

they currently do not use a formatting panic,
they have minimal reason to and are fine commiting not to using formatting panic to provide context, and
doing so does not require rewriting the function (e.g. unwrap is a formatting panic and thus not const).

So I'd say unwraps fall under not-const-yet, since unwrap has a connotation of providing a debug formatted context of the failed unwrap target when it fails. In const, use a match instead.¹

If our unwrap does not have a Debug bound, however, it seems reasonable to make unwrap const, since it can't do any non-const operations (e.g. Option::unwrap is unstably const as the failure case is dataless, unlike Result::unwrap, where the failure case prints E: Debug).

To match over beef::Cow, .into() -> std::Cow needs to be const. Or, as noted, .as_borrowed().unwrap() works via the unstably const Option::unwrap to strip the formatted context from the panic. ↩

uselessgoddess · 2022-07-25T16:25:29Z

To match over beef::Cow, .into() -> std::Cow needs to be const. Or, as noted, .as_borrowed().unwrap() works via the unstably const Option::unwrap to strip the formatted context from the panic.

But then it will prevent non-const implementation.
We'll have to use #[cfg], but it won't work after stabilize const features

uselessgoddess · 2022-07-25T19:39:56Z

I don't find a way to do something similar to if consteval from C++
Maybe now only TryFrom can be const, but Result::unwrap is not const now, users will have to use match

uselessgoddess · 2022-07-25T19:48:42Z

But it doesn't look like good code

uselessgoddess · 2022-07-25T20:21:32Z

I found a super unstable magic function))

uselessgoddess · 2022-07-25T20:24:03Z

It uses in std with a similar purpose. I can probably trust it

CAD97 · 2022-07-25T20:41:41Z

Note that const_eval_select is explicitly UB if there is any difference in the behavior of the const and nonconst arms.

But yes, since it's not possible at the moment to reconstruct the Owned value in const, just making [unwrap|as]_borrowed const is reasonable: [playground]

#![feature(const_refs_to_cell)] // new

impl<'a, T, U> Cow<'a, T, U>
where
    T: Beef + ?Sized,
    U: Capacity,
{
    #[inline]
    pub const fn as_borrowed(&self) -> Option<&'a T>
    where
        T: ~const Steak,
        U: ~const Capacity,
    {
        if self.is_borrowed() {
            Some(self.borrow())
        } else {
            None
        }
    }

    #[inline]
    pub const fn unwrap_borrowed(self) -> &'a T
    where
        T: ~const Steak,
        U: ~const Capacity,
    {
        match self.as_borrowed() {
            Some(borrowed) => borrowed,
            None => panic!("called `unwrap_borrowed` on an owned `beef::Cow`"),
        }
    }

    #[inline]
    pub const fn is_borrowed(&self) -> bool
    where
        U: ~const Capacity,
    {
        self.capacity().is_none()
    }

    #[inline]
    const fn borrow(&self) -> &T
    where
        T: ~const Steak,
    {
        unsafe { &*T::ref_from_parts::<U>(self.ptr, self.fat) }
    }

    #[inline]
    const fn capacity(&self) -> Option<U::NonZero>
    where
        U: ~const Capacity,
    {
        U::maybe(self.fat, self.cap)
    }
}

uselessgoddess · 2022-07-25T20:43:25Z

But yes, since it's not possible at the moment to reconstruct the Owned value in const, just making [unwrap|as]_borrowed const is reasonable:

I thought the same thing

…mplemented

uselessgoddess · 2022-07-27T18:00:13Z

src/generic.rs

+#[cfg(feature = "const_deref")]
+use core::marker::Destruct;
 use core::marker::PhantomData;
+#[cfg(not(feature = "const_deref"))]
+use core::marker::Sized as Destruct;


@CAD97 is there anything more invasive than Sized?

where Self: Sized is in fact a trivial bound that doesn't add any restrictions on the ungated impl, if that's what you're asking.

Yes. I just don't know anything more trivial.

T: ?Sized is the only weaker bound than T: ?Sized currently, or just T:.

Yes, but we can't create alias for this . On the other hand, Destruct used in one function.

uselessgoddess · 2022-07-27T18:02:25Z

src/generic.rs

+    // fixme: it should be unsafe :)
+    const fn from_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {


@maciejhirsz uses a lot of unsafe in the code, believing that Cow is created only in borrowed/owned
In this case, it is reasonable to make from_parts unsafe

uselessgoddess · 2022-07-27T18:03:05Z

src/generic.rs

+        let Cow { ptr, fat, cap, .. } = self;
+        (*ptr, *fat, *cap)
+    }
+
    // fixme: required `borrowed_from_raw_parts` - it has bug with the processing of structs


Suggested change

// fixme: required `borrowed_from_raw_parts` - it has bug with the processing of structs

// fixme: required `from_parts` - `cfg_const_deref` has problem in the processing of structs

``

uselessgoddess · 2022-07-27T18:30:18Z

src/generic.rs

+        where
+            T: ~const Steak,
+            U: ~const Capacity,
+            <T as ToOwned>::Owned: ~const Destruct


It is necessary? rustc suggest use it.

uselessgoddess · 2022-07-27T18:31:42Z

src/generic.rs

+                let (ptr, fat, cap) = Self::std_into_parts::<T, U>(cow);
+                Self::rt_from_cow(ptr, fat, cap)


Maybe there is a way

CAD97 · 2022-07-27T18:45:02Z

(The moment the code uses core::intrinsics is when it goes outside of the scope I'm comfortable reviewing. I'm absolutely not comfortable making an argument for the soundness of using const_eval_select, and relying on it makes reviewing the rest of the signatures pointless, since they're lying and being supported by const_eval_select.)

(I'm still not comfortable using const_eval_select even with the following change, but I'd be marginally more comfortable with moving the select into owned_{into|from}_parts using it with the argument calling the functions in a const context is necessarily UB to justify the select.)

(Generally, any feature without a tracking issue is a no-touch.)

uselessgoddess · 2022-07-27T18:50:34Z

Oh, no. It is sad))

uselessgoddess · 2022-07-27T18:58:12Z

May be users will be use .as_borrowed().unwrap() or .magic_into_std() in const context?
Also, const From/Into<std::Cow> will be added later

uselessgoddess · 2022-07-28T14:11:52Z

I re-read Safety zone in const_eval_select docs. Perhaps creating some const_eval function makes sense. After all, it will be the code that uses it will work the same in rt and ct.
But this feature does not have tracked issue. I now remove const From/Into<std::Cow>

uselessgoddess · 2022-07-28T17:45:13Z

@CAD97 why some functions require const_refs_to_cell feature?

CAD97 · 2022-07-28T19:11:17Z

Roughly, mutability behind an indirection (&mut) is unstable. &Cell presents the same questions as &mut, so is also unstable. In theory, what is actually needed/desired to be gated is doing a mutation through a reference, but just forbidding creation of mutable references is easier. (At least for &mut; &Cell is trickier to explain, but actually more complicated than &mut, since you can model unique &mut as just passing the value in and out.)

Starting with &T for an arbitrary T is fine, as this serves as an observation that T is free of cells; going from owned T to &T is gated.

Cow<'_, _, _> is potentially cell-containing via the cap: U::Field field, as is Option<U::NonZero>.

Option::is_none can be open coded with a match to avoid requiring const ref to (maybe) cell, and unwrap_borrowed can inline everything to only work on the generic types by value. But if we're enabling ~const anyway, const_refs_to_cell is fine to also enable; it's very likely to stabilize before ~const, and we're not actually using any of the in-question parts anyway (as evidenced by being able to inline to silence the gate).

As the feature gate error says, see rust-lang/rust#80384 for more information.

CAD97 · 2022-07-28T19:33:29Z

src/generic.rs

+    // fixme: it should be unsafe :)
+    const fn from_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {


Suggested change

// fixme: it should be unsafe :)

const fn from_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {

const unsafe fn from_raw_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {

Obviously the change needs to be propagated. Even though field access is safe and it is formally acceptable to have non-exported safe functions with safety requirements, it's much better practice to mark them unsafe.

Implements const_deref feature

471ea7e

uselessgoddess mentioned this pull request Jul 24, 2022

Beef can implement const Deref? #48

Open

Add some impl const traits

1ff6c64

uselessgoddess added 3 commits July 25, 2022 09:18

Remove code duplications via cfg_const_deref macro instead of `cfg_…

3a74eef

…fix`

Fix fmt in borrow

8ba8173

borrowed_from_raw_parts => borrowed_from_parts

b5fd4af

CAD97 reviewed Jul 25, 2022

View reviewed changes

Remove unnecessary cfg in borrowed_from_parts

eef0c3a

Co-authored-by: Christopher Durham <cad97@cad97.com>

uselessgoddess added 2 commits July 25, 2022 19:03

Add const_slice_from_raw_parts feature (required until rustc 1.64)

19be4be

Remove duplicated Default bound

cdd0532

const From<std::Cow> for beef and const From<beef::Cow> for std i…

38d64d6

…mplemented

uselessgoddess commented Jul 27, 2022

View reviewed changes

Remove const From/Int<std::Cow>

a76cdcd

CAD97 reviewed Jul 28, 2022

View reviewed changes

		// fixme: it should be unsafe :)
		const fn from_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {

	// fixme: required `borrowed_from_raw_parts` - it has bug with the processing of structs
	// fixme: required `from_parts` - `cfg_const_deref` has problem in the processing of structs
	``

		let (ptr, fat, cap) = Self::std_into_parts::<T, U>(cow);
		Self::rt_from_cow(ptr, fat, cap)

	// fixme: it should be unsafe :)
	const fn from_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {
	const unsafe fn from_raw_parts(ptr: NonNull<T::PointerT>, fat: usize, cap: U::Field) -> Self {

Implements const_deref feature #49

Are you sure you want to change the base?

Implements const_deref feature #49

Conversation

uselessgoddess commented Jul 24, 2022

CAD97 commented Jul 24, 2022

uselessgoddess commented Jul 24, 2022

uselessgoddess commented Jul 24, 2022

CAD97 commented Jul 24, 2022 • edited Loading

uselessgoddess commented Jul 24, 2022 • edited Loading

uselessgoddess commented Jul 24, 2022 • edited Loading

uselessgoddess commented Jul 24, 2022

CAD97 commented Jul 24, 2022

uselessgoddess commented Jul 24, 2022

CAD97 commented Jul 24, 2022

CAD97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uselessgoddess commented Jul 25, 2022 • edited Loading

CAD97 commented Jul 25, 2022

Footnotes

uselessgoddess commented Jul 25, 2022

uselessgoddess commented Jul 25, 2022 • edited Loading

uselessgoddess commented Jul 25, 2022

uselessgoddess commented Jul 25, 2022

uselessgoddess commented Jul 25, 2022 • edited Loading

CAD97 commented Jul 25, 2022 • edited Loading

uselessgoddess commented Jul 25, 2022

uselessgoddess Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uselessgoddess Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uselessgoddess Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAD97 commented Jul 27, 2022

uselessgoddess commented Jul 27, 2022

uselessgoddess commented Jul 27, 2022 • edited Loading

uselessgoddess commented Jul 28, 2022

uselessgoddess commented Jul 28, 2022

CAD97 commented Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Implements `const_deref` feature #49

Implements `const_deref` feature #49

CAD97 commented Jul 24, 2022 •

edited

Loading

uselessgoddess commented Jul 24, 2022 •

edited

Loading

uselessgoddess commented Jul 24, 2022 •

edited

Loading

uselessgoddess commented Jul 25, 2022 •

edited

Loading

uselessgoddess commented Jul 25, 2022 •

edited

Loading

uselessgoddess commented Jul 25, 2022 •

edited

Loading

CAD97 commented Jul 25, 2022 •

edited

Loading

uselessgoddess Jul 27, 2022 •

edited

Loading

uselessgoddess Jul 28, 2022 •

edited

Loading

uselessgoddess Jul 27, 2022 •

edited

Loading

uselessgoddess commented Jul 27, 2022 •

edited

Loading

CAD97 commented Jul 28, 2022 •

edited

Loading