Handle universal vs. fork markers with ResolverMarkers

[konstin]

• Use a dedicated ResolverMarkers check in the fork state. This is better than the MarkerTree::And(Vec::new()) check.
• Report the timing correct naming universal resolution instead of two spaces around an empty string when there are no markers.
• On resolution error, show the split that we're in. I'm not sure how to word this, since we're doing a universal resolution until we fork, so the trace may contain information from requirements that are not part of this fork.

[BurntSushi]

The improvements here look good.

With respect to MarkerTree and Option<MarkerTree>, my idea here was to collapse states that weren't semantically distinct. I now see that the is_universal check was added later as a way of determining whether the resolver was in "universal" mode or not. (And this is doubly confusing because the resolver is in universal mode when MarkerTree::is_universal is false.) The idea was that the canonical way of determining whether the resolver was in universal mode or not is whether the resolver has a marker environment, and not whether there's a marker expression that is trivially true or not. So for example, this code:

uv/crates/uv-resolver/src/resolver/mod.rs

Lines 309 to 318 in 545bbf9

	if !state.markers.is_universal() {
	if let Some(requires_python) = state.requires_python.as_ref() {
	debug!(
	"Solving split {} (requires-python: {})",
	state.markers, requires_python
	);
	} else {
	debug!("Solving split {}", state.markers);
	}
	}

Should I think be using self.markers (which, confusingly, is a MarkerEnvironment) to determine whether to emit log messages about a split.

This one can too:

uv/crates/uv-resolver/src/resolver/mod.rs

Lines 585 to 590 in 545bbf9

	if markers.is_universal() {
	debug!(
	"Pre-fork split universal took {:.3}s",
	start.elapsed().as_secs_f32()
	);
	} else {

But this one is trickier:

uv/crates/uv-resolver/src/fork_urls.rs

Lines 42 to 47 in 545bbf9

	return if fork_markers.is_universal() {
	Err(ResolveError::ConflictingUrls(
	package_name.clone(),
	conflicting_url,
	))
	} else {

And I think it's that case that caused MarkerTree::is_universal to come into existence. And I agree that is_universal is Not Good, and that given the choice between "checking for a single trivially true marker expression" and "using an option," the latter is better. But I'm worried about Option<MarkerTree> carrying subtly different semantics for the None case depending on where we are in the code. But maybe we don't yet have the right abstractions to deal with this nicely.

[BurntSushi]

Hmmm. So I very intentionally used MarkerTree instead of Option<MarkerTree>. For example, in this routine, I don't think there is any semantic difference between None and Some(MarkerTree::And(vec![])). They are redundant with one another, which I find confusing, because it injects an extra state into the type system that you need to think about. Moreover, there are perhaps cases where "a marker that is always true" and a "possibly absent marker" are semantically distinct, and IMO that's when we should use Option<MarkerTree>.

[konstin]

Do we ever get a fork with Some(MarkerTree::And(vec![]))? It seems to me that we shouldn't fork in that case since we're still doing a universal resolution.

[BurntSushi]

I don't believe so, but that invariant isn't encoded into the type system. It's more like it's manifest in the concept of forking itself. It's not something you can easily employ local reasoning about when looking at just the types.

I think this is a fine change for now. I think this change is probably better than the status quo. :-)

[konstin]

I encoded it in the type system using ResolverMarkers. Probably needs a fresh read tomorrow morning, but the api feels a good bit better this way.

[BurntSushi]

Hmmm. So I think the confusing thing about this name is that, if I'm understanding correctly, ResolverMarkers::Universal is used when the resolver is not in universal mode. It has the right semantics, but the name is I think confusable.

I'm actually finding it quite difficult to come up with a good name here. I think the problem is in using the markers (and not the marker environment) to identify whether we're employing a possibly-forking strategy or not.

Maybe... ResolverMarkers::AlwaysTrue? ¯\_(ツ)_/¯

[konstin]

The markers represent a trinary state - specific environment, universal resolution, resolution in a fork - so i changed the type to express that too.

[BurntSushi]

If we have a newtype for this, I think I'd rather see these case analyses collapsed. Perhaps a method like ResolverMarkers::possible_to_satisfy or something.

[BurntSushi]

Yeah I think this is better. I think we can still collapse/encapsulate case analyses, but that can be a follow-up.