Conflicting groups should handle conflicting inclusions automatically

[jtfmumm]

This adds support for inferring dependency group conflict sets from the directly defined conflicts in configuration. For example, if you declare a conflict between groups alpha and beta and dev includes beta, then we will infer a conflict between dev and alpha. We will also handle a conflict between two groups if they transitively include groups that conflict with each other. See #11232 for more details.

Closes #11232

[jtfmumm]

I've moved this file so that I can import DependencyGroups in conflicts.rs. It would have caused a circular dependency in uv-workspace.

[jtfmumm]

Can we assume cycle detection before this point? Otherwise, we'll have to return an error

[jtfmumm]

There's a failing test that wasn't running locally for some reason that shows we can't assume it at this point. I'll update.

[jtfmumm]

What I've done now is to just return if there's a Cycle error on toposort. I've left a FIXME comment indicating that we're currently bailing and allowing more detailed include cycle detection downstream. Is this the behavior we want? If so, I'll remove the FIXME tag.

[BurntSushi]

Hmmm cc @zanieb

One thing that strikes me here is that the cycle detection error would only kick in when a relevant conflicting group is declared? What happens if there aren't any conflicts declared? Is there cycle detection happening somewhere else?

[jtfmumm]

Cycle detection is happening somewhere, maybe in the resolver? But that happens after this code is called. With this new version where I just return (no error) when toposort() returns an error, then only the original conflict sets are sent along.

But it does seem weird to silently swallow the error. The reason I don't want it propagated is because better cycle detection happens elsewhere ("there was a cycle" vs. "there was a cycle: dev -> test -> dev"). So I could either warn the user in this method or match at the caller and warn there. Do either of those options make sense?

[BurntSushi]

I think what you're doing now is probably reasonable with a comment explaining what's going on here (basically, what you just said). @zanieb might have a different opinion though!

[zanieb]

I don't know what our current behavior for cycles is. Are we talking about cycles in inclusions or cycles in the conflicts?

1. If dev includes test and test includes dev don't we have a consistent set of dependencies? (just the union of test and dev?)

2. Similarly, if test is marked as conflicting with some other group foo then dev conflicts with foo too, the cycle doesn't matter here.

3. If test is marked as conflicting with dev, now the cycle is a problem because... they include each other and can't conflict. This applies to longer chains too, e.g., if we have test -> foo -> dev -> test a conflict between test and dev is "invalid".

If we don't have test coverage for each of these cases, we should add it. In reply to

One thing that strikes me here is that the cycle detection error would only kick in when a relevant conflicting group is declared?

This seems fine, because this form of cycle (3) is the only one that creates an unresolvable situation?

[charliermarsh]

Unless I'm misunderstanding, cycles in dependency groups are an error. Per the PEP:

Dependency Group Includes MUST NOT include cycles, and tools SHOULD report an error if they detect a cycle.

I think we have an DependencyGroupCycle error.

[jtfmumm]

Yeah, on main we definitely error on cycles. That's the behavior I'm relying on when just returning from the conflict expansion method when detecting a cycle during toposort.

[zanieb]

Oh, thanks for the correction. Maybe I was just thinking about feedback I wrote on the PEP.

[BurntSushi]

Nice work!

The failing duplicate_torch_and_sympy_because_of_wrong_inferences is somewhat worrisome, because that doesn't use dependency groups at all. So in theory, it shouldn't be impacted by the work here?

In your code, I wonder if something wonky is happening when no DependencyGroupSpecifier::IncludeGroup are found and thus no edges are added?

[BurntSushi]

Hmmm cc @zanieb

One thing that strikes me here is that the cycle detection error would only kick in when a relevant conflicting group is declared? What happens if there aren't any conflicts declared? Is there cycle detection happening somewhere else?

[charliermarsh]

Looks good from my perspective modulo the Cargo.lock changes which we should verify before merging.

[zanieb]

I don't quite understand this error, are we just repeating the groups here at the end? What's the value of that?

[charliermarsh]

I believe this is identical to the existing error we show in these cases, except "transitively" has been inserted. Perhaps this should be hashed out separately from this PR, since it doesn't seem to be a change? But for reference, the set at the end can be larger than "just" these two items. The set at the end is of arbitrary size, and the groups we list at the start are the activated, conflicting members.

[zanieb]

Oh, if it's not changing here that's fine and we can revisit it separately — I just looked at the test case not the implementation.