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

Allow Ranges::contains to accept (e.g.) &str for Ranges<String> #35

Merged
merged 1 commit into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/internal/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,7 @@ impl<T: Hash + Eq + fmt::Debug> fmt::Debug for HashArena<T> {

impl<T: Hash + Eq> HashArena<T> {
pub fn new() -> Self {
HashArena {
data: FnvIndexSet::default(),
}
Self::default()
}

pub fn alloc(&mut self, value: T) -> Id<T> {
Expand All @@ -161,6 +159,14 @@ impl<T: Hash + Eq> HashArena<T> {
}
}

impl<T: Hash + Eq> Default for HashArena<T> {
fn default() -> Self {
Self {
data: FnvIndexSet::default(),
}
}
}

impl<T: Hash + Eq> Index<Id<T>> for HashArena<T> {
type Output = T;
fn index(&self, id: Id<T>) -> &T {
Expand Down
2 changes: 1 addition & 1 deletion src/internal/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl<DP: DependencyProvider> State<DP> {
let dep_incompats =
self.add_incompatibility_from_dependencies(package, version.clone(), dependencies);
self.partial_solution.add_package_version_incompatibilities(
package.clone(),
package,
version.clone(),
dep_incompats,
&self.incompatibility_store,
Expand Down
2 changes: 1 addition & 1 deletion src/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ pub fn resolve<DP: DependencyProvider>(
};

// Add that package and version if the dependencies are not problematic.
state.add_package_version_dependencies(p.clone(), v.clone(), dependencies);
state.add_package_version_dependencies(p, v.clone(), dependencies);
} else {
// `dep_incompats` are already in `incompatibilities` so we know there are not satisfied
// terms and can add the decision directly.
Expand Down
24 changes: 16 additions & 8 deletions version-ranges/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,11 @@ impl<V: Ord> Ranges<V> {
}

/// Returns true if self contains the specified value.
pub fn contains(&self, version: &V) -> bool {
pub fn contains<Q>(&self, version: &Q) -> bool
where
V: Borrow<Q>,
Q: ?Sized + PartialOrd,
{
self.segments
.binary_search_by(|segment| {
// We have to reverse because we need the segment wrt to the version, while
Expand Down Expand Up @@ -470,19 +474,23 @@ impl<V: Ord> Ord for Ranges<V> {
/// ^ ^ ^
/// less equal greater
/// ```
fn within_bounds<V: PartialOrd>(version: &V, segment: &Interval<V>) -> Ordering {
fn within_bounds<Q, V>(version: &Q, segment: &Interval<V>) -> Ordering
where
V: Borrow<Q>,
Q: ?Sized + PartialOrd,
{
let below_lower_bound = match segment {
(Excluded(start), _) => version <= start,
(Included(start), _) => version < start,
(Excluded(start), _) => version <= start.borrow(),
(Included(start), _) => version < start.borrow(),
(Unbounded, _) => false,
};
if below_lower_bound {
return Ordering::Less;
}
let below_upper_bound = match segment {
(_, Unbounded) => true,
(_, Included(end)) => version <= end,
(_, Excluded(end)) => version < end,
(_, Included(end)) => version <= end.borrow(),
(_, Excluded(end)) => version < end.borrow(),
};
if below_upper_bound {
return Ordering::Equal;
Expand Down Expand Up @@ -1304,7 +1312,7 @@ pub mod tests {

#[test]
fn always_contains_exact(version in version_strat()) {
assert!(Ranges::singleton(version).contains(&version));
assert!(Ranges::<u32>::singleton(version).contains(&version));
}

#[test]
Expand All @@ -1326,7 +1334,7 @@ pub mod tests {

#[test]
fn from_range_bounds(range in any::<(Bound<u32>, Bound<u32>)>(), version in version_strat()) {
let rv: Ranges<_> = Ranges::from_range_bounds(range);
let rv: Ranges<_> = Ranges::<u32>::from_range_bounds(range);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this means this is technically a breaking change. (Although we technically permit type inference regressions as semver compatible in Rust-library-land... Unless they are too disruptive.) I only mention this because I think version-ranges is published as a library?

Copy link
Member

@konstin konstin Dec 3, 2024

Choose a reason for hiding this comment

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

Agreed that this is technically a breaking changes, but i expect that real users use their Ranges as parameters to a function that has the generic parameter fix (the DependencyProvider has), so as an inference change I consider it acceptable.

assert_eq!(range.contains(&version), rv.contains(&version));
}

Expand Down
Loading