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.

Sign up for GitHub

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

Jump to bottom

[WIP] subtle: breaking changes for a hypothetical v3.0 #136

Closed
wants to merge 1 commit into from
Closed

[WIP] subtle: breaking changes for a hypothetical v3.0 #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ name = "subtle"
# - update html_root_url
# - update README if necessary by semver
# - if any updates were made to the README, also update the module documentation in src/lib.rs
version = "2.6.0"
version = "3.0.0-pre"
edition = "2018"
authors = ["Isis Lovecruft <[email protected]>",
"Henry de Valence <[email protected]>"]
Expand Down
53 changes: 25 additions & 28 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ impl Choice {
///
/// This function only exists as an **escape hatch** for the rare case
/// where it's not possible to use one of the `subtle`-provided
/// where it's not possible to use one of the `subtle`-provided
/// trait impls.
///
/// **To convert a `Choice` to a `bool`, use the `From` implementation instead.**
Expand Down Expand Up @@ -383,14 +384,19 @@ impl ConstantTimeEq for cmp::Ordering {
}
}

/// Marker trait for types whose [`Clone`] impl operates in constant-time.
pub trait ConstantTimeClone: Clone {}

impl<T: Copy> ConstantTimeClone for T {}

/// A type which can be conditionally selected in constant time.
///
/// This trait also provides generic implementations of conditional
/// assignment and conditional swaps.
//
// #[inline] is specified on these function prototypes to signify that they
#[allow(unused_attributes)] // should be in the actual implementation
pub trait ConditionallySelectable: Copy {
pub trait ConditionallySelectable: ConstantTimeClone {
/// Select `a` or `b` according to `choice`.
///
/// # Returns
Expand Down Expand Up @@ -467,7 +473,7 @@ pub trait ConditionallySelectable: Copy {
/// ```
#[inline]
fn conditional_swap(a: &mut Self, b: &mut Self, choice: Choice) {
let t: Self = *a;
let t: Self = a.clone();
a.conditional_assign(&b, choice);
b.conditional_assign(&t, choice);
}
Expand Down Expand Up @@ -575,7 +581,7 @@ impl ConditionallySelectable for Choice {
#[cfg(feature = "const-generics")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do make breaking changes, we can completely remove this feature since it's now MSRV-compatible

impl<T, const N: usize> ConditionallySelectable for [T; N]
where
T: ConditionallySelectable,
T: ConditionallySelectable + Copy,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately we can't support anything other than Copy types here, since [T; N] receives a Copy impl for T: Copy, and that means it will receive ConstantTimeClone when T: Copy.

This means we can't add a blanket impl of ConstantTimeClone to [T; N] where T: Copy because it would overlap with the other blanket impls outlined above.

We could potentially add bounds like: Self: ConstantTimeClone, T: ConditionallySelectable + ConstantTimeClone, but that wouldn't actually be usable since [T; N] only receives a ConstantTimeClone impl for Copy types as described above.

What would be really nice here is some way of expressing disjoint bounds using hypothetical new type system features, e.g. being able to impl<T> ConstantTimeClone for [T; N] where T: ConstantTimeClone + !Copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be nice not to explicitly use Copy here just to leave the door open for the blanket impl outlined above though

{
#[inline]
fn conditional_select(a: &Self, b: &Self, choice: Choice) -> Self {
Expand Down Expand Up @@ -741,49 +747,34 @@ impl<T> CtOption<T> {

/// Returns a `None` value if the option is `None`, otherwise
/// returns a `CtOption` enclosing the value of the provided closure.
/// The closure is given the enclosed value or, if the option is
/// `None`, it is provided a dummy value computed using
/// `Default::default()`.
/// The closure is given the enclosed value.
///
/// This operates in constant time, because the provided closure
/// is always called.
#[inline]
pub fn map<U, F>(self, f: F) -> CtOption<U>
where
T: Default + ConditionallySelectable,
T: ConditionallySelectable,
F: FnOnce(T) -> U,
{
CtOption::new(
f(T::conditional_select(
&T::default(),
&self.value,
self.is_some,
)),
self.is_some,
)
CtOption::new(f(self.value), self.is_some)
}

/// Returns a `None` value if the option is `None`, otherwise
/// returns the result of the provided closure. The closure is
/// given the enclosed value or, if the option is `None`, it
/// is provided a dummy value computed using `Default::default()`.
/// given the enclosed value.
///
/// This operates in constant time, because the provided closure
/// is always called.
#[inline]
pub fn and_then<U, F>(self, f: F) -> CtOption<U>
where
T: Default + ConditionallySelectable,
T: ConditionallySelectable,
F: FnOnce(T) -> CtOption<U>,
{
let mut tmp = f(T::conditional_select(
&T::default(),
&self.value,
self.is_some,
));
tmp.is_some &= self.is_some;

tmp
let mut ret = f(self.value);
ret.is_some &= self.is_some;
ret
}

/// Returns `self` if it contains a value, and otherwise returns the result of
Expand All @@ -797,7 +788,10 @@ impl<T> CtOption<T> {
let is_none = self.is_none();
let f = f();

Self::conditional_select(&self, &f, is_none)
CtOption::new(
T::conditional_select(&self.value, &f.value, is_none),
Choice::conditional_select(&self.is_some, &f.is_some, is_none),
)
}

/// Convert the `CtOption<T>` wrapper into an `Option<T>`, depending on whether
Expand All @@ -817,7 +811,10 @@ impl<T> CtOption<T> {
}
}

impl<T: ConditionallySelectable> ConditionallySelectable for CtOption<T> {
impl<T> ConditionallySelectable for CtOption<T>
where
T: ConditionallySelectable + Copy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a similar problem where where we can't add a conditional ConstantTimeClone impl to CtOption<T> because it derives Copy and so already can receive the blanket impl.

{
fn conditional_select(a: &Self, b: &Self, choice: Choice) -> Self {
CtOption::new(
T::conditional_select(&a.value, &b.value, choice),
Expand Down
Loading