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

Add lossless integer conversion by reference #90551

Conversation

vandenheuvel
Copy link
Contributor

@vandenheuvel vandenheuvel commented Nov 4, 2021

This pull request adds conversion of integer types by reference, e.g. i16::from(&1_u8).

This is useful for writing code that is generic over number types. Just like we have number operations by reference (e.g. std::ops::Add impls allow let _: u16 = 1_u16 + &1_u16), it can be necessary to convert number types by reference.

The alternative is cloning before converting, which is not always an option. Take for example the code below. When the right-hand side MaybeLarge2 is large type, such as an arbitrary precision type, the intermediate clone is too expensive, as it could require an allocation.

fn compute<'a, MaybeLarge1, MaybeLarge2>(left: &mut MaybeLarge1, right: &'a MaybeLarge2)
where
    MaybeLarge1: From<'a MaybeLarge2>,
    ... // A few std::ops and number traits
{
    if left.is_negative() {
        *left = From::from(right);
        // the alternative would be `*left = From::from(right.clone());`
    } else {
        left += right;
    }
    
    ...
}

Adding these conversion implementations makes such generic code possible.

The implementation could be constified with the rest of #87852.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2021
@rust-log-analyzer

This comment has been minimized.

@vandenheuvel vandenheuvel force-pushed the lossless_int_conversion_by_ref branch from 5f6f637 to 5c37c82 Compare November 4, 2021 08:56
@rust-log-analyzer

This comment has been minimized.

@vandenheuvel vandenheuvel force-pushed the lossless_int_conversion_by_ref branch from 5c37c82 to 0a9a71f Compare November 4, 2021 09:58
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

What is the motivation for adding these? It seems like most code can fairly easily dereference as needed to call the existing impls -- and if e.g. diagnostics are not so good today, then I would rather improve them then add this particular special case, personally.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2021
@vandenheuvel
Copy link
Contributor Author

@Mark-Simulacrum I wrote a PR description. Once #90560 is merged (perhaps you could take a look? I haven't gotten any response there...), I will rebase this.

@Mark-Simulacrum Mark-Simulacrum added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 10, 2021
@Mark-Simulacrum
Copy link
Member

Ok. I'm not sure I'm personally convinced - I think a pointer to an actual set of code (rather than theoretical) would help me.

r? @dtolnay for T-libs-api FCP since this would be insta stable

@vandenheuvel vandenheuvel force-pushed the lossless_int_conversion_by_ref branch from 0a9a71f to 02deb4c Compare November 10, 2021 12:38
@vandenheuvel
Copy link
Contributor Author

vandenheuvel commented Nov 10, 2021

This arose in a linear programming framework I'm developing. This trait is implemented by using an internal number type that might be arbitrary precision depending on the specific problem being solved. That problem is represented by an implementor of this trait, which in turn can have any number type to represent some problem. Some problems might need arbitrary precision, while others might be able to get by with small rational numbers or even integers or bools.

These two traits are essentially connected through traits like this one. Part of that alias is the From bound, which currently prohibits using this system with integers. The newtype pattern is incredibly cumbersome to use in this situation (especially because that trait alias will be restricted further in the future because new bounds will be added for different backing algorithms that require different operations).

@vandenheuvel vandenheuvel marked this pull request as ready for review November 10, 2021 13:28
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I agree with Mark in being a little skeptical about adding this.


// the alternative would be `*left = From::from(right.clone());`

As a counterproposal, I wonder how you feel about the downstream crate doing its trait bounds like this:

pub trait CheapCopy {
    type Type;
    fn cheap_copy(self) -> Self::Type;
}

impl CheapCopy for &i32 {
    type Type = i32;
    fn cheap_copy(self) -> Self::Type { *self }
}

struct BigInteger;
impl<'a> CheapCopy for &'a BigInteger {
    type Type = &'a BigInteger;
    fn cheap_copy(self) -> Self::Type { self }
}

pub type DoCheapCopy<T> = <T as CheapCopy>::Type;

fn compute<'a, MaybeLarge1, MaybeLarge2>(left: &mut MaybeLarge1, right: &'a MaybeLarge2)
where
    &'a MaybeLarge2: CheapCopy,
    MaybeLarge1: From<DoCheapCopy<&'a MaybeLarge2>>,
{
    if left.is_negative() {
        *left = From::from(right.cheap_copy());
    } else {
        // ...
    }
}

I figure you are going to want this anyway, so that you can switch to right: MaybeLarge2 and avoid requiring callers to do the silly thing of passing primitives by reference. This is an even cleaner result than the trait bounds you'd get from this PR, as you won't need explicit lifetime annotations anymore.

// The above impls, plus impl CheapCopy for i32 { type Type = i32 }.

fn compute<MaybeLarge1, MaybeLarge2>(left: &mut MaybeLarge1, right: MaybeLarge2)
where
    MaybeLarge2: CheapCopy,
    MaybeLarge1: From<DoCheapCopy<MaybeLarge2>>;

@bors
Copy link
Contributor

bors commented Nov 19, 2021

☔ The latest upstream changes (presumably #91033) made this pull request unmergeable. Please resolve the merge conflicts.

@dtolnay
Copy link
Member

dtolnay commented Nov 26, 2021

Gonna close but we can come back to this if someone decides they're still justified after my suggestion.

@dtolnay
Copy link
Member

dtolnay commented Nov 26, 2021

Thanks anyway!

@dtolnay dtolnay closed this Nov 26, 2021
@vandenheuvel
Copy link
Contributor Author

@dtolnay I'm still looking into your suggestion, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants