Avoid InferOk<'tcx, ()>

[nnethercote]

InferOk<'tcx, ()> is a silly type. This PR converts uses of it to something simpler.

r? @lcnr

[rustbot]

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred in engine.rs, potentially modifying the public API of ObligationCtxt.

cc @lcnr, @compiler-errors

[workingjubilee]

I do think this makes some of the code nicer, but doesn't this make it more annoying to refactor the locations in question? Since now they're a different type to begin with.

[nnethercote]

The InferOk<'tcx, ()> cases are well separated from the InferOk<'tcx, T> cases, and I was happy with how cleanly this change fell out once I started it. Let's see what @lcnr thinks.

[workingjubilee]

Hm, okay!

[lcnr]

I think the main reason to use InferOk over Vec<...> is that InferOk is #[must_use] and we want to make sure that people don't don't do the following, which implicitly drops the nested obligations

    whatever_typesystem_operation()?;

I think we currently lint on that but will stop to do so with this change.

[nnethercote]

Hmm, interesting. This has made me think about #[must_use] more than I have before :)

Result is marked with #[must_use]. AFAICT that means the discriminant must be checked, but the ok and error values within each variant don't have to be.

So this whatever_typesystem_operation()?; anti-pattern is possible for any function that returns a Result. I don't feel like it's something that is a problem in practice, though, or people would be using must_use much more, and putting all kinds of vanilla types in #[must_use] wrappers. Is there something about these methods that make this anti-pattern more likely? The relevant methods (with this PR's changes applied, the methods that return a UnitInferResult) are the following.

• At::{sub,sup,eq,eq_trace,relate}
• InferCtxt::{unify_query_response_instantiation_guess,unify_canonical_vars}
• InferCtxt::{coerce_predicate,subtype_predicate} (wrapped in Result)

Is it because they have side-effects as well as returning obligations?

The following snippet is interesting, it's a case where At::sup is called just for its side-effects:

rust/compiler/rustc_hir_typeck/src/method/probe.rs

Lines 1407 to 1413 in 18b1161

	// Guide the trait selection to show impls that have methods whose type matches
	// up with the `self` parameter of the method.
	let _ = self.at(&ObligationCause::dummy(), self.param_env).sup(
	DefineOpaqueTypes::Yes,
	xform_self_ty,
	self_ty,
	);

Interesting that the let _ is enough to satisfy the must_use on InferOk.

When dealing with Infer{Ok,Result}<'tcx, ()> I can see that the #[must_use] is useful, because it ensures you don't forget the obligations and think that that Infer{Ok,Result}<'tcx, ()> is equivalent to (). But removing the () makes it seem less likely that you would forget the obligations, because the obligations are all that's there. (For this reason, ObligationsResult would be a better name than UnitInferResult.)

I'm not wedded to this PR, I'm just trying to understand some of the subtleties.

[lcnr]

Is there something about these methods that make this anti-pattern more likely?

Is it because they have side-effects as well as returning obligations?

yeah, a lot of compiler contributors tend to not be aware that relating types can result in nested obligations, so iirc we accidentally dropped them in quite a few places in the past.

This is part of the reason the ObligationCtxt abstraction exists. It handles the nested obligations for you