Adding an `impl for type defined outside of crate` causes every associated method call to be reported as an error

[udoprog]

Code

/// This is not a valid implementation for the current crate.
impl dyn core::fmt::Display {}

fn foo(values: &[u32]) -> Option<&u32> {
    values.get(0)
}

Current output

As expected, we get an error indicating:


error[E0116]: cannot define inherent `impl` for a type outside of the crate where the type is defined
 --> src/lib.rs:1:1
  |
1 | impl dyn core::fmt::Display {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl for type defined outside of crate.
  |
  = note: define and implement a trait or new type instead

But we also get the following "bonus" error:

error[E0599]: no method named `get` found for reference `&[u32]` in the current scope
 --> src/lib.rs:5:12
  |
5 |     values.get(0)
  |            ^^^
  |
help: there is a method `ge` with a similar name
  |
5 |     values.ge(0)
  |            ~~

Some errors have detailed explanations: E0116, E0599.
For more information about an error, try `rustc --explain E0116`.
error: could not compile `playground` (lib) due to 2 previous errors

If for example we add the impl to the tokio crate, we get:

error: could not compile `tokio` (lib) due to 3370 previous errors; 9 warnings emitted

Desired output

Preferably, the impl for type defined outside of crate being an illegal implementation should not cause other seemingly unrelated errors.

Rust Version

rustc 1.79.0 (129f3b996 2024-06-10)
binary: rustc
commit-hash: 129f3b9964af4d4a709d1383930ade12dfe7c081
commit-date: 2024-06-10
host: x86_64-unknown-linux-gnu
release: 1.79.0
LLVM version: 18.1.7

[steffahn]

This seems to be a regression in 1.78

[steffahn]

Bisection points to somewhere here

  commit[0] 2024-02-14: Auto merge of #120847 - oli-obk:track_errors9, r=compiler-errors
  commit[1] 2024-02-15: Auto merge of #121125 - ehuss:fix-small-cstr, r=Mark-Simulacrum
  commit[2] 2024-02-15: Auto merge of #121131 - matthiaskrgr:rollup-mo3b8nz, r=matthiaskrgr
  commit[3] 2024-02-15: Auto merge of #116564 - oli-obk:evaluated_static_in_metadata, r=RalfJung,cjgillot
  commit[4] 2024-02-15: Auto merge of #120931 - chenyukang:yukang-cleanup-hashmap, r=michaelwoerister
  commit[5] 2024-02-15: Auto merge of #119863 - tmiasko:will-wake, r=m-ou-se
  commit[6] 2024-02-15: Auto merge of #121142 - GuillaumeGomez:rollup-5qmksjw, r=GuillaumeGomez
  commit[7] 2024-02-15: Auto merge of #119338 - compiler-errors:upcast-plus-autos, r=lcnr
  commit[8] 2024-02-15: Auto merge of #121133 - tmiasko:skip-coroutines, r=cjgillot

That’s almost certainly

• Continue compilation even if inherent impl checks fail #121113

cc @oli-obk

[steffahn]

This change in test output is actually a bad change that was already visible in the tests when #121113 was merged. The function

// Allowed
fn in_impl_Trait_in_return() -> impl IntoIterator<Item = impl IntoIterator> {
    vec![vec![0; 10], vec![12; 7], vec![8; 3]]
+   //~^ ERROR: no function or associated item named `into_vec` found for slice `[_]`
}

should not gain any errors pointing to it. It's even clearly marked as an “Allowed” case in the comments.
(This looks like a clear oversight in review.) [Also cc @compiler-errors.]

[steffahn]

For context, this issue can easily make a single change in a rust crate cause hundreds of unrelated errors at once, as reported on: https://users.rust-lang.org/t/pub-use-breaks-my-compiler/117983

Those hundreds of unrelated errors can then easily hide the actual problem.

---

Looking further through the changed tests in #121113, I also think that the following is a bad change:

struct Foo<T: ?Sized>(T);
impl Foo<dyn Send + Sync> {
    fn abc() -> bool { //~ ERROR duplicate definitions with name `abc`
        false
    }
}

impl Foo<dyn Sync + Send> {
    fn abc() -> bool {
        true
    }
}

fn main() {
    assert_eq!(<Foo<dyn Send + Sync>>::abc(), false);
+   //~^ ERROR: multiple applicable items in scope
    assert_eq!(<Foo<dyn Sync + Send>>::abc(), true);
+   //~^ ERROR: multiple applicable items in scope
}

Adding a second definition of abc does not make existing callers wrong, so reporting an error there is just distracting from the actual problem.

[steffahn]

This doesn’t just make methods fail, but all inherent associated items, AFAICT. E.g. here’s an associated constant (inspired by the third bad error I’ve noticed #121113 has added to already existing tests).

impl () {}

fn f() {
    let x = i8::MIN;
}

(among the errors:)

error[E0599]: no associated item named `MIN` found for type `i8` in the current scope
 --> src/lib.rs:4:17
  |
4 |     let x = i8::MIN;
  |                 ^^^ associated item not found in `i8`
  |

[compiler-errors]

I'll look into it. I agree the existing implementation is a bit goofy.

[compiler-errors]

This is two issues:

1. We act as if no inherent impls exist if we have an invalid inherent impl like impl () {}:
   • I think this is worth fixing and I've got a PR coming up soon.
2. We continue typechecking if there are overlapping incoherent impls: Adding an impl for type defined outside of crate causes every associated method call to be reported as an error #127798 (comment)
   • I'm not as convinced that this is worth suppressing. While it may cause some confusion, it really is an ambiguity -- if you are concerned with this behavior, I'm happy to keep discussing, and the fix is not very difficult. please file a new bug so we can separate that out, though.

[steffahn]

I think, the second issue (ambiguity on overlap) is acceptable either way; at least all of the errors also point to the relevant impls anyway; and it’s only affecting the overlapping trait, so no risk of causing hundreds or thousands of error messages suddenly.

I think this is worth fixing and I've got a PR coming up soon.

This is great to hear, thanks for the quick analysis. Otherwise (if it wasn’t so easily fixed) I would have suggested a revert of #121113 as a quick fix, or at least a partial revert, as far as the tcx.ensure().crate_inherent_impls(()) case goes.