non_local_definitions lint triggers when implementing trait for reference in doc test under beta 1.80.0

[tspiteri]

I tried this code:

#![doc(test(attr(deny(warnings))))]

/// ```rust
/// use l::T;
/// struct _S;
/// impl T for &'_ _S {}
/// ```
pub trait T {}

I expected to see this happen: No warning or error for cargo +beta test

Instead, this happened:

    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.04s
     Running unittests src/lib.rs (target/debug/deps/l-f86ba059936edb88)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests l

running 1 test
test src/lib.rs - T (line 3) ... FAILED

failures:

---- src/lib.rs - T (line 3) stdout ----
error: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> src/lib.rs:7:1
  |
7 | impl T for &'_ _S {}
  | ^^^^^-^^^^^------
  |      |     |
  |      |     `&'_ _S` is not local
  |      |     help: remove `&` to make the `impl` local
  |      `T` is not local
  |
  = note: `impl` may be usable in bounds, etc. from outside the expression, which might e.g. make something constructible that previously wasn't, because it's still on a publicly-visible type
  = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
help: move the `impl` block outside of this function `_doctest_main_src_lib_rs_3_0` and up 2 bodies
 --> src/lib.rs:4:38
  |
4 | fn main() { #[allow(non_snake_case)] fn _doctest_main_src_lib_rs_3_0() {
  |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5 | use l::T;
6 | struct _S;
  | --------- may need to be moved as well
  = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(warnings)]
  |         ^^^^^^^^
  = note: `#[deny(non_local_definitions)]` implied by `#[deny(warnings)]`

error: aborting due to 1 previous error

Couldn't compile the test.

failures:
    src/lib.rs - T (line 3)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

error: doctest failed, to rerun pass `--doc`

Meta

rustc +beta --version --verbose:

rustc 1.80.0-beta.1 (75ac3b633 2024-06-10)
binary: rustc
commit-hash: 75ac3b6331873133c4f7a10f2252afd6f3906c6a
commit-date: 2024-06-10
host: x86_64-unknown-linux-gnu
release: 1.80.0-beta.1
LLVM version: 18.1.7

[fmease]

Looks like the lint doesn't account for fundamental types like &.

[Urgau]

Looks like the lint doesn't account for fundamental types like &.

That's because they don't make an impl def local, quite the opposite; here is an example where despite not naming Local the compilation still pass, as inference is still able to resolve the type behind the reference, we therefore correctly report the impl def as a non local definition.

pub trait Trait {}

pub fn doctest() {
    struct Local;
    impl Trait for &Local {} // with this impl definition
}

fn do_stuff<U: Trait>() {}

fn main() {
    do_stuff::<&_>(); // this code will compile, as `_` will be resolved to `Local`
}

@tspiteri There are several ways of fixing the issue:

1. Remove the & (reference) from the impl definition
2. Make the doctest a stand-alone test by adding an fn main() {} to the test (we may want something like #124568 to automatically suggest it for impl definitions)

     /// ```rust
     /// use l::T;
     /// struct _S;
     /// impl T for &'_ _S {}
   + /// # fn main() {}
     /// ```

It should also be noted that denying warnings is an anti-pattern and is not recommanded.

@rustbot labels -C-bug +C-discussion

[fmease]

That's because they don't make an impl def local, quite the opposite here is an example where despite not naming Local the compilation still pass, as inference is still able to resolve the type behind the reference, correctly reporting the impl def as a non local definition.

Ah, I see! Thanks for the explainer, makes perfect sense

[tspiteri]

@tspiteri There are several ways of fixing the issue:

1. Remove the `&` (reference) from the impl definition

This here is a reduced test case. I originally hit this on a doc example that was explicitly on implementing a trait for a reference.

2. Make the doctest a stand-alone test by adding an `fn main() {}` to the test _(we may want something like [#124568](https://github.com/rust-lang/rust/pull/124568) to automatically suggest it for `impl` definitions)_
   ```diff
     /// ```rust
     /// use l::T;
     /// struct _S;
     /// impl T for &'_ _S {}
   + /// # fn main() {}
     /// ```
   ```

This is good, though it looks like a workaround rather than a fix.

It should also be noted that denying warnings is an anti-pattern and is not recommanded.

Usually it is an anti-pattern, but not for doc examples. For doc examples it makes sense to ensure that doc examples are not generating warnings. It is even given as an example in the official documentation.

[Urgau]

This is good, though it looks like a workaround rather than a fix.

There is no fix. This is not a bug (at least from rustc point of view, maybe T-rustdoc feels differently, but with #126245 coming together I would be surprised).

The impl definition is non-local, yet it was defined in a local context, in this case a non-standalone doc-test.
The same issue would arise with "regular" #[test] function, playground example.

Making the doc-test standalone (in the sense that it is guaranteed to be compiled and executed alone) removes most of the test harness and makes the test free of the warning. #126245 would introduce a dedicated standalone doctest attribute instead of the manual fn main() {}.

However one thing that could be considered to be a bug is the diagnostic output which should probably suggest making the doctest standalone.