fix: Exclude impls on the error type from impl enumeration

[Wilfred]

Previously, rust-analyzer would include impl methods from impl Foo for NoSuchType, which would lead to rust-analyzer reporting nonexistent type errors.

This is noticeable when depending on tokio with feature="fs" and cfg(test) enabled. The following code would produce an invalid type error.

use std::io::Read;
fn f<T>(mut o: Option<T>) {
    o.take().unwrap();
}

rust-analyzer was incorrectly resolving the .take() to Read::take() instead of Option::take().

Inside tokio, it uses mockall:mock! to define a MockFile, and then does impl Read for &'_ MockFile. However, since mockall is a dev dependency of tokio and we don't include dev dependencies of dependencies, rust-analyzer just sees impl Read for &'_ NoSuchType.

Instead, exclude traits whose self type is error.

AI disclosure: Partially written by Codex and GPT-5.5

[Veykril]

Not opposed to this, it sounds like the right thing to do here, but why is cfg(test) set for you in tokio which I presume is a dependency of your workspace? That asks for weird issues

[Wilfred]

Yeah, this is the curse of a monorepo: we don't really know which libraries you're actively working on, so I defensively set cfg(test) essentially everywhere so you can always go-to-def on test helpers. I noticed this when using rust-project as a discover command.

It looks like I should modify rust-project to exclude cfg(test) on imported libraries like tokio, since our monorepo (using reindeer) doesn't actually have dev-dependencies for imported libraries so cfg(test) is genuinely wrong there.

The bug was a spooky action-at-a-distance issue: adding tokio to the transitive set of dependencies causes spurious red squiggles and it took me a while to get to the bottom of it. This fix does feel appropriately defensive.

I see CI is red, I'll fix that too.

[ChayimFriedman2]

Generally that's something that came up multiple times in the past. In each of them we ended up finding the problematic impl and resolving its issue, but I agree this is something we want in general as the solver is quite sensitive to such impls.

However I think references_non_lt_error() is too strong condition, checking for error type itself should be enough IMO.

View changes since this review

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Wilfred]

@ChayimFriedman2 how about this, using references_only_ty_error instead? That seems to be a narrower condition.

I did play with if matches!(trait_ref.skip_binder().self_ty().kind(), TyKind::Error(_)) but that doesn't work with my original repro, since the impl is on &NoSuchType.

[ChayimFriedman2]

I'm a bit on the edge about this. On one hand, const errors are indeed more commonly used by bugs in r-a than type errors. On the other hand... removing the impl due to any type error feels a bit like going too far. Maybe, and given that this is the most problematic case, only filter it if it has an error constructor? That is, peeling references, raw pointers(?), arrays(?), slices(?), pattern types, and unsafe binders.

But I'm not sure myself.

[Wilfred]

That makes sense to me: the only really problematic case is instances of impl Foo for SomeMissingType leaking everywhere. I've updated the PR and things like impl Foo for Option<SomeMissingType> seem to work correctly.

[Wilfred]

Something like this? I've added a references_non_lt_error() call and also tweaked the helper to be trait specific, I think it reads better.

[Wilfred]

OK, moved the logic, and done another pass on making the test small and more self-documenting.