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

Rust 1.74 #16416

Draft
wants to merge 5 commits into
base: dw/use-latest-master-proof-systems
Choose a base branch
from
Draft

Conversation

martyall
Copy link
Member

@martyall martyall commented Dec 12, 2024

Depends on #16405 and proof-systems-2868 and proof-systems-2875

This PR updates the rust compiler to 1.74. It is a stepping stone in updating the
wasm-pack and wasm-bindgen-cli dependencies, but it is also good housekeeping to not fall too far behind.

@martyall
Copy link
Member Author

!ci-nightly-me

// simultaneously, and
// * the underlying pointer is in the correct state to be
// mutable, since we can call [`get_mut_unchecked`] in
// nightly, or can call [`get_mut`] and unwrap if this is
// the only live reference.
&mut *(((&*self.0) as *const Self::Target) as *mut Self::Target)
Copy link
Member Author

Choose a reason for hiding this comment

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

1.74 throws a compiler error here. There might be some other way to get it to stop complaining, but I think this is equivalent without the illegal cast

@martyall
Copy link
Member Author

@coveralls
Copy link

Coverage Status

coverage: 32.922%. first build
when pulling 2ec9256 on martin/rust-1.74
into 2cc26fa on dw/use-latest-master-proof-systems.

@@ -1,7 +1,7 @@
macro_rules! impl_caml_pointer {
($name: ident => $typ: ty) => {
#[derive(std::fmt::Debug, Clone, ::ocaml_gen::CustomType)]
pub struct $name(pub ::std::rc::Rc<$typ>);
pub struct $name(pub ::std::rc::Rc<std::cell::UnsafeCell<$typ>>);
Copy link
Member

Choose a reason for hiding this comment

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

Note as it is important for the Caml binding: UnsafeCell is implemented as:

#[lang = "unsafe_cell"]
#[stable(feature = "rust1", since = "1.0.0")]
#[repr(transparent)]
#[rustc_pub_transparent]
pub struct UnsafeCell<T: ?Sized> {
    value: T,
}

Copy link
Member

Choose a reason for hiding this comment

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

I guess we're adding a new indirection? We might lose some performances?

Copy link
Member Author

@martyall martyall Dec 13, 2024

Choose a reason for hiding this comment

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

Note as it is important for the Caml binding: UnsafeCell is implemented as:

The nightly tests were passing with this change, does this mean that that the ocaml bindings lib was able to resolve this on its own or is it just coincidence because maybe the runtime rep is the same

Copy link
Member Author

@martyall martyall Dec 15, 2024

Choose a reason for hiding this comment

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

I guess we're adding a new indirection? We might lose some performances?

My intuition is that this UnsafeCell should be optimized away. We can discuss

// simultaneously, and
// * the underlying pointer is in the correct state to be
// mutable, since we can call [`get_mut_unchecked`] in
// nightly, or can call [`get_mut`] and unwrap if this is
// the only live reference.
&mut *(((&*self.0) as *const Self::Target) as *mut Self::Target)
&mut *self.0.get()
Copy link
Member

Choose a reason for hiding this comment

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

Note: implementation of get.

    #[inline(always)]
    #[stable(feature = "rust1", since = "1.0.0")]
    #[rustc_const_stable(feature = "const_unsafecell_get", since = "1.32.0")]
    #[rustc_never_returns_null_ptr]
    pub const fn get(&self) -> *mut T {
        // We can just cast the pointer from `UnsafeCell<T>` to `T` because of
        // #[repr(transparent)]. This exploits std's special status, there is
        // no guarantee for user code that this will work in future versions of the compiler!
        self as *const UnsafeCell<T> as *const T as *mut T
    }

Copy link
Member

Choose a reason for hiding this comment

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

Nit: cargo fmt reformats this line. It seems we do not run in the Mina CI clippy nor fmt.

Copy link
Member Author

@martyall martyall Dec 13, 2024

Choose a reason for hiding this comment

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

Note: implementation of get.

This makes me think someone was trying to get around using UnsafeCell when they wrote this the first time. I would hope that this would be optimized away

@dannywillems
Copy link
Member

LGTM so far, modulo comments.

@dannywillems
Copy link
Member

Feel free to cherry-picked some changes from #16411

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.

3 participants