Revert trivial impl of Borrow<str> for Atom

[YoniFeng]

#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:

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

AHashMap<BrowserNameAtom, BrowserStat>

where BrowserNameAtom is an Atom generated with string_cache_codegen.

The signature of the get function:

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

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?

[adamreichold]

It isn't clear to me how the Borrow impl that was added "competes" with the std lib's implementation here.
It doesn't seem relevant?

With the additional Borrow impl, you should be able to call

let chrome = CANIUSE_BROWSERS.get("chrome").unwrap();

without a conversion to BrowserNameAtom. So I guess the trivial impl of Into (going from T to T) and the new Borrow could also make the original invocation work.

[adamreichold]

But I noticed that we actually have to revert this for correctness reasons which we oversaw in the original PR: impl Borrow<Q> for K requires that Q and K have equivalent Hash implementations, but Atom uses

impl<Static: StaticAtomSet> Hash for Atom<Static> {
    #[inline]
    fn hash<H>(&self, state: &mut H)
    where
        H: Hasher,
    {
        state.write_u32(self.get_hash())
    }
}

without actually hashing the contents as str does, so the Borrow impl is incorrect in any case. Sorry, for not noticing this earlier.

[YoniFeng]

It isn't clear to me how the Borrow impl that was added "competes" with the std lib's implementation here.
It doesn't seem relevant?

With the additional Borrow impl, you should be able to call

let chrome = CANIUSE_BROWSERS.get("chrome").unwrap();

without a conversion to BrowserNameAtom. So I guess the trivial impl of Into (going from T to T) and the new Borrow could also make the original invocation work.

Right, I get that with the additional Borrow impl, you can directly use the str.
But I don't get why &str.into() breaks.
i.e. why is the coercion ambiguous between

           - impl<Static> Borrow<str> for Atom<Static>
             where Static: StaticAtomSet;
           - impl<T> Borrow<T> for T
             where T: ?Sized;

According to expression precedence, .into() is stronger than the & borrow.
Based on the impl<'a, Static: StaticAtomSet> From<&'a str> for Atom<Static> I mentioned above, shouldn't we now have an Atom at our hands?
Evidently not, because I don't see how borrowing an Atom could result in the ambiguous coercion options shown in the error.

What am I missing :(

But I noticed that we actually have to revert this for correctness reasons which we oversaw in the original PR: impl Borrow<Q> for K requires that Q and K have equivalent Hash implementations, but Atom uses

impl<Static: StaticAtomSet> Hash for Atom<Static> {
    #[inline]
    fn hash<H>(&self, state: &mut H)
    where
        H: Hasher,
    {
        state.write_u32(self.get_hash())
    }
}

without actually hashing the contents as str does, so the Borrow impl is incorrect in any case. Sorry, for not noticing this earlier.

Ha, the example in the Borrow doc is for exactly this scenario.
Since there's no way to enforce this statically, how is one supposed to be aware of this "gotcha"/unenforced constraint?

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 9c7b0aa has been approved by jdm

[bors-servo]

⌛ Testing commit 9c7b0aa with merge 50d3b20...

[bors-servo]

💔 Test failed - checks-github

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 4e45fde has been approved by jdm

[bors-servo]

⌛ Testing commit 4e45fde with merge df9093d...

[bors-servo]

☀️ Test successful - checks-github
Approved by: jdm
Pushing df9093d to master...

[YoniFeng]

@mrobinson @jdm

Can we publish a new version?
What's the policy after 0.8.5 got yoinked? is it just 0.8.6?