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

impl AsRef<Uuid> for Uuid #743

Merged
merged 2 commits into from
Mar 18, 2024
Merged

impl AsRef<Uuid> for Uuid #743

merged 2 commits into from
Mar 18, 2024

Conversation

koshell
Copy link
Contributor

@koshell koshell commented Mar 14, 2024

Added the following AsRef impl:

impl AsRef<Uuid> for Uuid {
    #[inline]
    fn as_ref(&self) -> &Uuid {
        self
    }
}

Having this means you can write code like this that accepts Braced, Hyphenated, Simple, Urn, or Uuid:

fn find_with_uuid(uuid: impl AsRef<Uuid>) 

For completeness this is technical possible currently using something like this:

fn find_with_uuid(uuid: impl Borrow<Uuid>) 

However (and I am open to being corrected on this) my understanding is this is much less idiomatic then AsRef<T>. My understanding after reading over the std docs and some discussion online is:
Borrow<T> implies a function doesn't care if it gets T, &T, or &mut T.
AsRef<T> implies a function doesn't care what it gets as long as it can be represented as &T.

They both serve similar purposes (and have almost identical implementations). However AsRef<T> implies "I want &T and I don't mind if I need to do a low cost conversion to get it" where Borrow<T> implies "I don't care if you give me T or &T since I'll be borrowing it as &T anyway."

As an example the standard library recommends using AsRef<str> when you want a function to be able to accept any &str, &String, or String.

(Ideally the standard library would implement something like:

impl<T> AsRef<T> for T {
    #[inline]
    fn as_ref(&self) -> &T {
        self
    }
}

However currently this isn't possible due to some other conflicting traits so it needs to be implemented manually for each type.)

@@ -1745,7 +1752,7 @@ mod tests {
fn test_as_bytes() {
let u = new();
let ub = u.as_bytes();
let ur = u.as_ref();
let ur: &[u8] = u.as_ref();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New AsRef made this ambiguous.

@@ -1767,7 +1774,7 @@ mod tests {
use crate::std::{convert::TryInto, vec::Vec};

let u = new();
let ub = u.as_ref();
let ub: &[u8] = u.as_ref();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New AsRef made this ambiguous.

@KodrAus
Copy link
Member

KodrAus commented Mar 18, 2024

Thanks for the PR @koshell. I'm a bit reluctant to add this since it does break inference. I'm not sure how widely relied on that is, but since we do already have a way to abstract over Uuid and its various format types via Borrow I'd rather err on the side of caution on not introduce this. Adding a generic bound on Borrow<T> is perfectly idiomatic, the trait itself is just less widely pushed because of its additional implementation constraints that you can't enforce through the type system.

@koshell
Copy link
Contributor Author

koshell commented Mar 18, 2024

That's fair. I will say the documentation for AsRef in the std library does say this:

Reflexivity

Ideally, AsRef would be reflexive, i.e. there would be an impl<T: ?Sized> AsRef for T with as_ref simply returning its argument unchanged. Such a blanket implementation is currently not provided due to technical restrictions of Rust’s type system (it would be overlapping with another existing blanket implementation for &T where T: AsRef which allows AsRef to auto-dereference, see “Generic Implementations” above).

A trivial implementation of AsRef for T must be added explicitly for a particular type T where needed or desired. Note, however, that not all types from std contain such an implementation, and those cannot be added by external code due to orphan rules.

This suggests that while it isn't possible to currently enforce it, it should be assumed to be reflexive (and could possibly become an automatic implementation in a future version of rust-std).

The fact that it creates some inference issues in this case is mildly annoying but I think would only be a minor semver change since your just expanding a trait. The same inference problem could theoretically happen with any new AsRef implementation for a type.

Would a feature flag for the implementation be preferable? Since there is no way to implement this outside of this crate that would mean people could enable/disable it as they saw fit. Ideally if nobody reports any issues you could just include the feature flag in the default flags after a time. I'm happy to implement that in the branch if that would be better.

Copy link
Member

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

I've spent some more time thinking about this, and I've come around to adding impl AsRef<Uuid> for Uuid. The inference breakage is unfortunate, but I realised I avoid the pattern of let b = a.as_ref() in code anyways unless there's a type bound either on the let binding or through a generic bound on the type of a that isn't reliant on inference anyways. AsRef without a concrete bound somewhere is not only brittle, it's also difficult to tell from reading the source what you're actually asking for.

So I think we're potentially breaking a pattern I'd generally recommend against anyways, and there's no logical reason you can't or shouldn't add a reflexive AsRef.

@KodrAus
Copy link
Member

KodrAus commented Mar 18, 2024

The build break here should be fixed if you rebase on main.

@KodrAus
Copy link
Member

KodrAus commented Mar 18, 2024

I'm putting together a new release so will merge this one in and deal with any breakage in a follow-up. Thanks again @koshell!

@KodrAus KodrAus merged commit 9415ed4 into uuid-rs:main Mar 18, 2024
9 of 21 checks passed
mergify bot referenced this pull request in andrzejressel/pulumi-wasm Mar 19, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [uuid](https://togithub.com/uuid-rs/uuid) | workspace.dependencies | minor | `1.7.0` -> `1.8.0` |

---

### Release Notes

<details>
<summary>uuid-rs/uuid (uuid)</summary>

### [`v1.8.0`](https://togithub.com/uuid-rs/uuid/releases/tag/1.8.0)

[Compare Source](https://togithub.com/uuid-rs/uuid/compare/1.7.0...1.8.0)

#### ⚠️ Potential Breakage ⚠️

A new `impl AsRef<Uuid> for Uuid` bound has been added, which can break inference on code like:

```rust
let b = uuid.as_ref();
```

You can fix these by explicitly typing the result of the conversion:

```rust
let b: &[u8] = uuid.as_ref();
```

or by calling `as_bytes` instead:

```rust
let b = uuid.as_bytes();
```

#### What's Changed

-   docs: fix small spelling mistake by [@&#8203;bengsparks](https://togithub.com/bengsparks) in [https://github.com/uuid-rs/uuid/pull/737](https://togithub.com/uuid-rs/uuid/pull/737)
-   serde serialize_with support by [@&#8203;dakaizou](https://togithub.com/dakaizou) in [https://github.com/uuid-rs/uuid/pull/735](https://togithub.com/uuid-rs/uuid/pull/735)
-   Fix up CI builds by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [https://github.com/uuid-rs/uuid/pull/744](https://togithub.com/uuid-rs/uuid/pull/744)
-   Only add `wasm-bindgen` as a dependency on `wasm32-unknown-unknown` by [@&#8203;emilk](https://togithub.com/emilk) in [https://github.com/uuid-rs/uuid/pull/738](https://togithub.com/uuid-rs/uuid/pull/738)
-   impl AsRef<Uuid> for Uuid by [@&#8203;koshell](https://togithub.com/koshell) in [https://github.com/uuid-rs/uuid/pull/743](https://togithub.com/uuid-rs/uuid/pull/743)
-   Add v6 to v8 draft link to README by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [https://github.com/uuid-rs/uuid/pull/746](https://togithub.com/uuid-rs/uuid/pull/746)
-   Add a workflow for running cargo outdated by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [https://github.com/uuid-rs/uuid/pull/745](https://togithub.com/uuid-rs/uuid/pull/745)
-   Prepare for 1.8.0 release by [@&#8203;KodrAus](https://togithub.com/KodrAus) in [https://github.com/uuid-rs/uuid/pull/747](https://togithub.com/uuid-rs/uuid/pull/747)

#### New Contributors

-   [@&#8203;bengsparks](https://togithub.com/bengsparks) made their first contribution in [https://github.com/uuid-rs/uuid/pull/737](https://togithub.com/uuid-rs/uuid/pull/737)
-   [@&#8203;dakaizou](https://togithub.com/dakaizou) made their first contribution in [https://github.com/uuid-rs/uuid/pull/735](https://togithub.com/uuid-rs/uuid/pull/735)
-   [@&#8203;emilk](https://togithub.com/emilk) made their first contribution in [https://github.com/uuid-rs/uuid/pull/738](https://togithub.com/uuid-rs/uuid/pull/738)
-   [@&#8203;koshell](https://togithub.com/koshell) made their first contribution in [https://github.com/uuid-rs/uuid/pull/743](https://togithub.com/uuid-rs/uuid/pull/743)

**Full Changelog**: uuid-rs/uuid@1.7.0...1.8.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/andrzejressel/pulumi-wasm).
kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Mar 19, 2024
Bumps uuid from 1.7.0 to 1.8.0.

Release notes
Sourced from uuid's releases.

1.8.0
⚠️ Potential Breakage ⚠️
A new impl AsRef<Uuid> for Uuid bound has been added, which can break inference on code like:
let b = uuid.as_ref();
You can fix these by explicitly typing the result of the conversion:
let b: &[u8] = uuid.as_ref();
or by calling as_bytes instead:
let b = uuid.as_bytes();
What's Changed

docs: fix small spelling mistake by @​bengsparks in uuid-rs/uuid#737
serde serialize_with support by @​dakaizou in uuid-rs/uuid#735
Fix up CI builds by @​KodrAus in uuid-rs/uuid#744
Only add wasm-bindgen as a dependency on wasm32-unknown-unknown by @​emilk in uuid-rs/uuid#738
impl AsRef for Uuid by @​koshell in uuid-rs/uuid#743
Add v6 to v8 draft link to README by @​KodrAus in uuid-rs/uuid#746
Add a workflow for running cargo outdated by @​KodrAus in uuid-rs/uuid#745
Prepare for 1.8.0 release by @​KodrAus in uuid-rs/uuid#747

New Contributors

@​bengsparks made their first contribution in uuid-rs/uuid#737
@​dakaizou made their first contribution in uuid-rs/uuid#735
@​emilk made their first contribution in uuid-rs/uuid#738
@​koshell made their first contribution in uuid-rs/uuid#743

Full Changelog: uuid-rs/uuid@1.7.0...1.8.0



Commits

0f2aaae Merge pull request #747 from uuid-rs/cargo/1.8.0
01d16c3 prepare for 1.8.0 release
e4746bc Merge pull request #745 from uuid-rs/ci/outdated
d0396ad Merge pull request #746 from uuid-rs/chore/draft-link
9415ed4 Merge pull request #743 from koshell/main
951e8e3 Merge pull request #738 from rerun-io/emilk/wasm-bindgen-only-on-web
101aa84 add v6 to v8 draft link to README
84dcbba run outdated on a schedule
ca952b1 add a workflow for running cargo outdated
abe995a Make the toml longer, more complicated, and functional
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
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