Uh oh!
There was an error while loading. Please reload this page.
Add trivial impl of Borrow<str> for Atom - #266
Merged
Merged
Conversation
jdm
commented
Dec 14, 2022
Member
@bors-servo r+ |
bors-servo
commented
Dec 14, 2022
Contributor
📌 Commit 13db052 has been approved by |
bors-servo
commented
Dec 14, 2022
Contributor
bors-servo added a commit
that referenced
this pull request
Dec 14, 2022
Add trivial impl of Borrow<str> for Atom This enables `Atom` to be used in methods like [`HashMap::entry_ref`](https://docs.rs/hashbrown/latest/hashbrown/hash_map/struct.HashMap.html#method.entry_ref).
bors-servo
commented
Dec 14, 2022
Contributor
💔 Test failed - checks-github |
adamreichold
commented
Dec 14, 2022
ContributorAuthor
I committed a Cargo lockfile which references once_cell 1.14.0 instead of the current 1.16.0 and should be compatible with the MSRV of 1.49.0. I also adjust the CI workflow to use this lockfile when testing Rust 1.49.0 in the CI even I am not completely sure whether this will work for all parts of the CI workflow. |
jdm
commented
Dec 15, 2022
Member
Oh, if the build failed it's because one of our dependencies now has a minimum Rust version that's higher than our version. There's nothing we can do about that except pin the older version in our dependencies, or update our MSRV. We just update our MSRV in that case. |
jdm
commented
Dec 15, 2022
Member
This enables Atom to be used in methods like HashMap::entry_ref.
adamreichold
commented
Dec 15, 2022
ContributorAuthor
I rebased on the post-merge |
jdm
commented
Dec 15, 2022
Member
@bors-servo r+ |
bors-servo
commented
Dec 15, 2022
Contributor
📌 Commit 37b459f has been approved by |
bors-servo
commented
Dec 15, 2022
Contributor
bors-servo
commented
Dec 16, 2022
Contributor
☀️ Test successful - checks-github |
bors-servo added a commit
that referenced
this pull request
Feb 23, 2023
Revert trivial impl of Borrow<str> for Atom #266 caused a breaking change (see #271) in 0.8.5 (which was yanked). This PR fixes#271 / will allow publishing new versions (so #268 can make it out). I've just started learning Rust, haven't been able to 100% wrap my head around the issue. The breaking change manifesting in the "browserlist-rs" crate: ```rust error[E0283]: type annotations needed --> {.. snip ..}\browserslist-rs-0.12.3\src\data\caniuse.rs:91:35 | 91 | let chrome = CANIUSE_BROWSERS.get(&"chrome".into()).unwrap(); | ^^^ ---------------- type must be known at this point | | | cannot infer type of the type parameter `Q` declared on the associated function `get` | = note: multiple `impl`s satisfying `Atom<BrowserNameAtomStaticSet>: Borrow<_>` found in the following crates: `core`, `string_cache`: - impl<Static> Borrow<str> for Atom<Static> where Static: StaticAtomSet; - impl<T> Borrow<T> for T where T: ?Sized; note: required by a bound in `AHashMap::<K, V, S>::get` --> {.. snip ..}\ahash-0.7.6\src\hash_map.rs:81:12 | 81 | K: Borrow<Q>, | ^^^^^^^^^ required by this bound in `AHashMap::<K, V, S>::get` help: consider specifying the generic argument | 91 | let chrome = CANIUSE_BROWSERS.get::<Q>(&"chrome".into()).unwrap(); | +++++ ``` `CANIUSE_BROWSERS` is a ```rust AHashMap<BrowserNameAtom, BrowserStat> ``` where `BrowserNameAtom` is an Atom generated with `string_cache_codegen`. The signature of the `get` function: ```rust pub fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V> where K: Borrow<Q>, Q: Hash + Eq, { self.0.get(k) } ``` `K` is the generic key type for the hash map. This is the exact same signature as the std lib HashMap, to which it delegates. When I debug 0.8.4 usage locally, the `From` that gets used for the `into()` is ```rust impl<'a, Static: StaticAtomSet> From<&'a str> for Atom<Static> { #[inline] fn from(string_to_add: &str) -> Self { Atom::from(Cow::Borrowed(string_to_add)) } } ``` It isn't clear to me how the `Borrow<str>` impl that was added "competes" with the std lib's implementation here. It doesn't seem relevant?
bors-servo added a commit
that referenced
this pull request
Feb 23, 2023
Revert trivial impl of Borrow<str> for Atom #266 caused a breaking change (see #271) in 0.8.5 (which was yanked). This PR fixes#271 / will allow publishing new versions (so #268 can make it out). I've just started learning Rust, haven't been able to 100% wrap my head around the issue. The breaking change manifesting in the "browserlist-rs" crate: ```rust error[E0283]: type annotations needed --> {.. snip ..}\browserslist-rs-0.12.3\src\data\caniuse.rs:91:35 | 91 | let chrome = CANIUSE_BROWSERS.get(&"chrome".into()).unwrap(); | ^^^ ---------------- type must be known at this point | | | cannot infer type of the type parameter `Q` declared on the associated function `get` | = note: multiple `impl`s satisfying `Atom<BrowserNameAtomStaticSet>: Borrow<_>` found in the following crates: `core`, `string_cache`: - impl<Static> Borrow<str> for Atom<Static> where Static: StaticAtomSet; - impl<T> Borrow<T> for T where T: ?Sized; note: required by a bound in `AHashMap::<K, V, S>::get` --> {.. snip ..}\ahash-0.7.6\src\hash_map.rs:81:12 | 81 | K: Borrow<Q>, | ^^^^^^^^^ required by this bound in `AHashMap::<K, V, S>::get` help: consider specifying the generic argument | 91 | let chrome = CANIUSE_BROWSERS.get::<Q>(&"chrome".into()).unwrap(); | +++++ ``` `CANIUSE_BROWSERS` is a ```rust AHashMap<BrowserNameAtom, BrowserStat> ``` where `BrowserNameAtom` is an Atom generated with `string_cache_codegen`. The signature of the `get` function: ```rust pub fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V> where K: Borrow<Q>, Q: Hash + Eq, { self.0.get(k) } ``` `K` is the generic key type for the hash map. This is the exact same signature as the std lib HashMap, to which it delegates. When I debug 0.8.4 usage locally, the `From` that gets used for the `into()` is ```rust impl<'a, Static: StaticAtomSet> From<&'a str> for Atom<Static> { #[inline] fn from(string_to_add: &str) -> Self { Atom::from(Cow::Borrowed(string_to_add)) } } ``` It isn't clear to me how the `Borrow<str>` impl that was added "competes" with the std lib's implementation here. It doesn't seem relevant?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This enables
Atomto be used in methods likeHashMap::entry_ref.