Don't use Res::Err without emitting an error 1/*

[lcnr]

We currently use Res::Err both to express "there was an error" and "there is no relevant resolution".

rust/src/librustc_middle/ty/context.rs

Lines 454 to 461 in 3fe4dd2

	pub fn qpath_res(&self, qpath: &hir::QPath<'_>, id: hir::HirId) -> Res {
	match *qpath {
	hir::QPath::Resolved(_, ref path) => path.res,
	hir::QPath::TypeRelative(..) => self
	.type_dependent_def(id)
	.map_or(Res::Err, |(kind, def_id)| Res::Def(kind, def_id)),
	}
	}

Before I work on changing all occurrences of the second meaning to Option<Res> I want to get some
feedback whether this change is actually desired.

cc @nikomatsakis @matthewjasper

[rust-highfive]

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

[nikomatsakis]

cc @petrochenkov

[nikomatsakis]

I am reading up a bit here to get more context, however my general opinion that we have to be very careful about things that signal "there was an error reported to the user". It's very common to use variants like that to skip downstream errors or avoid bits of work, and it's easy to wind up with unsoundness as a result.

So 👍 to having a clear distinction between "error reported" and "no relevant resolution", presuming at least that 'no relevant resolution' doesn't always imply an error has been reported.

Can you elaborate a bit more on what bits of downstream code want to distinguish between those two cases, though? I know it came up when we were discussing lazy normalization, but I think the context would be helpful.

[lcnr]

I first noticed this when refactoring query type_of for const generics.

rust/src/librustc_typeck/collect/type_of.rs

Lines 271 to 289 in 3fe4dd2

	// Try to use the segment resolution if it is valid, otherwise we
	// default to the path resolution.
	let res = segment.res.filter(|&r| r != Res::Err).unwrap_or(path.res);
	let generics = match res {
	Res::Def(DefKind::Ctor(..), def_id) => {
	tcx.generics_of(tcx.parent(def_id).unwrap())
	}
	Res::Def(_, def_id) => tcx.generics_of(def_id),
	res => {
	tcx.sess.delay_span_bug(
	DUMMY_SP,
	&format!(
	"unexpected anon const res {:?} in path: {:?}",
	res, path,
	),
	);
	return tcx.types.err;
	}
	};

Some correct qpaths have segments where res is None, for others res is Some(Err) and for others it's Some(actually_correct_res). I think that my current solution is not correct in all cases and something like this may break it. (We have an ICE here atm with seems unrelated though 🤷)

Looking at some other locations where Res::Err is used. I am quite suspicious of the following,
which already depends on this distinction afaict.

rust/src/librustc_typeck/check/pat.rs

Lines 824 to 828 in 3fe4dd2

	if res == Res::Err {
	self.set_tainted_by_errors();
	on_error();
	return self.tcx.types.err;
	}

[petrochenkov]

Before I work on changing all occurrences of the second meaning to Option<Res> I want to get some feedback whether this change is actually desired.

I think this is desirable at least after the main resolution pass, but I'd prefer to see this done in stages instead of changing all occurrences in one go. For example

• everything after AST -> HIR lowering
• AST -> HIR lowering
• everything after expansion / early resoluton
• expansion / early resolution (this may be harder because Res::Err is used in non-trivial ways for import stubs in namespaces where they don't import anything, and maybe other similar cases)

[bors]

☔ The latest upstream changes (presumably #69171) made this pull request unmergeable. Please resolve the merge conflicts.