pubgrub-rs / pubgrub

PubGrub version solving algorithm implemented in Rust
https://pubgrub-rs.github.io/pubgrub/pubgrub/
Mozilla Public License 2.0
337 stars 29 forks source link

Avoid over-simplifying version ranges #228

Closed konstin closed 3 weeks ago

konstin commented 3 weeks ago

This is an upstream port of https://github.com/astral-sh/pubgrub/pull/12. It improves range formatting in error messages by avoiding to over-simplify ranges:

× No solution found when resolving dependencies:
-  ╰─▶ Because there is no version of werkzeug available matching ∅ and
-      flask>=3.0.0 depends on werkzeug>=3.0.0, flask>=3.0.0 is forbidden.
-      And because root depends on flask∅, version solving failed.
+  ╰─▶ Because there is no version of werkzeug available matching >=3.0.0 and
+      flask==3.0.0 depends on werkzeug>=3.0.0, flask==3.0.0 is forbidden.
+      And because root depends on flask==3.0.0, version solving failed.
Eh2406 commented 3 weeks ago

On one hand, there is something mathematically consistent about the current definition of simplify. It returns the smallest data structure that has the equivalent behavior. On the other hand, there is one production user of simplify and you want this change.

The new behavior can be wrapped around the old behavior (but not vice versa).

   pub fn new_simplify<'s, I, BV>(&self, versions: I) -> Self
    where
        I: Iterator<Item = BV> + 's,
        BV: Borrow<V> + 's,
    {
        // Do not simplify singletons
        if self.as_singleton().is_some() {
            return self.clone();
        } 

        let simp = self.simplify(versions);

        // Do not return null sets
        if simp == Self::empty() {
            self.clone()
        } else {
            simp
        }
    }

If https://github.com/astral-sh/uv/pull/3713 merges this could live there.

Am I missing some important perspective? Thoughts?

konstin commented 3 weeks ago

I'm fine with having this live in uv, @zanieb what do you think?

zanieb commented 3 weeks ago

I have no problem with this being in uv although I find it quite awkward that simplify will discard valuable user-facing information. Did we end up using simplify internally for solving performance or is it still only used for display?

Eh2406 commented 3 weeks ago

Did we end up using simplify internally for solving performance or is it still only used for display?

Not yet, but it is still planed.

I have no problem with this being in uv although I find it quite awkward that simplify will discard valuable user-facing information.

I would also ok merging this now as a "user-facing simplify" and add a "mathematical simplify" when/if I incorporate it into the algorithm (and it needs different property).

zanieb commented 3 weeks ago

Seems okay to implement it in uv for now but people who are writing error reports with simplified ranges will definitely need this kind of utility — maybe PubGrub's error reporting should expose and use this simplify wrapper.