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

Make unsync::Lazy<T, F> covariant in F #233

Merged
merged 4 commits into from
Jun 3, 2023

Conversation

danielhenrymantilla
Copy link
Contributor

"Continuation" from #230, which partially handles #167.

  • Incidentally, this also makes unsync::Lazy smaller in size in most cases, since T and F are now sharing the same union storage.

The sync can basically use the same logic (my PR paves the way for such a follow-up PR), only the whole thing would need to be moved to each of the possible implementations of sync::OnceCell, and special care to synchronization semantics will be in order, which I prefer to let somebody else do.

@danielhenrymantilla danielhenrymantilla changed the title Make unsync::Lazy<T, F> covariant Make unsync::Lazy<T, F> covariant in F Jun 2, 2023
Comment on lines +719 to +720
state: Cell<State>,
data: Data<T, F>,
Copy link
Contributor Author

@danielhenrymantilla danielhenrymantilla Jun 2, 2023

Choose a reason for hiding this comment

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

Note that we could make Lazy even more compact by moving state at the beginning of each union variant (and making the union be #[repr(C)]).

  • If T = [u8; 3], and F = u16, this would make Lazy<T, F> be 4 bytes rather than 6, for instance.
#[repr(C)] struct InOrder<T, U>(T, U);

#[repr(C)]
union Lazy<T, F = fn() -> T> {
    init:  MD<InOrder< Cell<State>, F     >>,
    value: MD<InOrder< Cell<State>, UC<T> >>,
    // convenience variant to keep the code symmetric.
    state: MD<InOrder< Cell<State>, ()    >>, 
}

@matklad
Copy link
Owner

matklad commented Jun 3, 2023

bors r+

Thanks!

@bors
Copy link
Contributor

bors bot commented Jun 3, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit e3b2260 into matklad:master Jun 3, 2023
@matklad
Copy link
Owner

matklad commented Jun 3, 2023

Ah, I think this doesn't actually work as we want it too 😭

I think we've lost dropchk here, no?

Before, we didn't have a custom drop, so a Lazy<&'a T> allowed dangling 'a at the point where the Lazy is dropped.

In this variation, we now have a custom drop, so that won't be allowed. There's a dropchk test for OnceCell, but not for lazy.

Is there some way to have #[may_dangle] on stable?

@matklad
Copy link
Owner

matklad commented Jun 3, 2023

@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Jun 3, 2023

Oh, good thing this wasn't released!

  • Note: let's try to add arrrr tests for Lazy too then 😅

Is there some way to have #[may_dangle] on stable?

Short answer is no; slightly longer answer is the post you've linked to, but which generalizes with great difficulty (it's very situation-specific)

Even longer answer, which I was planning to write a crate and blog post about it, is that with HKTs/GATs it is possible to define an advanced API that would let people opt-into their specific usage #[may_dangle] at the cost of having to use that type.

So for once_cell this is a big no, since it would be a major usability regression, in comparison of which lack-of-covariance over F pales.

  • But for those curious, the API would be something like LazyMayDangle<'a, Gat!(&'_ T)>, with Lazy<T> then being some kind of alias for LazyMayDangle<'static, Gat!(T)>.

Conclusion (to share back to #167): blocked due to lack of #[may_dangle]: only stdlib itself may realistically feature this on stable Rust, but stdlib is precisely the area where things ought to remain somewhat KISS 😅

I'm submitting a revert as we speak (my latest PR on the topic), and I apologize for the inconvenience, @matklad; I reckon it must have been quite a spam of notifications on your end, for a niche thing, and which hasn't panned out

danielhenrymantilla added a commit to danielhenrymantilla/once_cell that referenced this pull request Jun 3, 2023
This reverts commit e3b2260, reversing
changes made to 8f39b77.
bors bot added a commit that referenced this pull request Jun 4, 2023
236: Revert "Make `unsync::Lazy<T, F>` covariant in `F`" r=matklad a=danielhenrymantilla

Indeed, #233 introduced a regression w.r.t `#[may_dangle]` 😔, apologies 

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
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