Skip to content

0.8.5 is a "breaking change" #271

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

Closed
ForsakenHarmony opened this issue Feb 22, 2023 · 5 comments · Fixed by #272
Closed

0.8.5 is a "breaking change" #271

ForsakenHarmony opened this issue Feb 22, 2023 · 5 comments · Fixed by #272

Comments

@ForsakenHarmony
Copy link

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();
   |                                      +++++

I think this was caused by #266, reverting to 0.8.4 fixes the error

@YoniFeng
Copy link
Contributor

Can confirm, I've been looking at the same the same issue wanting to bump string_cache for swc - swc-project/swc#6980

@realtimetodie
Copy link

0.8.5 needs to be yanked @mrobinson

@realtimetodie
Copy link

realtimetodie commented Feb 22, 2023

Blocks swc-project/swc#6973

@mrobinson
Copy link
Member

0.8.5 is now yanked.

@realtimetodie
Copy link

Thank you

bors-servo added a commit that referenced this issue 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 issue Feb 28, 2023
test: add common dependents' usage

As a response to #271, added tests with common usage.

I modified the project structure, so I could make use of the canonical (according to [the docs](https://doc.rust-lang.org/book/ch11-03-test-organization.html)) "external tests" support.
I'm not familiar with the nitty gritty and ramifications of this change, so absolutely looking for guidance/advice/instructions.

Concerns I'm aware of:
1. Integration tests can't have their own `build.rs`, so I moved it to be under the main crate. Does it matter that it'll run with `cargo build` now, where previously it would only run with `cargo test`
2. The `bench.rs` is apparently only supported in nightly? It didn't run with `cargo test`. Is it in use at all? How can I make sure that it runs properly under the new structure (with the same command that would run it previously)?
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 a pull request may close this issue.

4 participants