non-local-definitions lint probably shouldn't fire in doc comments

[TheBlueMatt]

Latest beta is generating non-local-definitions warnings in our doctests because we have some docs that describe #[macro_export]s, which of course end up inside a doctest function generating the lint.

cc tracking issue #120363

[Urgau]

Are you able to provide us with a sample code demonstrating the issue? Ideally one that is self-contained.

@rustbot label +E-needs-mcve

[TheBlueMatt]

Oops, sorry, needs #![doc(test(no_crate_inject, attr(deny(warnings))))], so maybe not all that interesting, but in some cases we set it in CI jobs to get advance warning of coming warnings and fail on new ones.

This fails on the playground on beta:

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

//! To use this crate you need to export a macro
//!```
//! #[macro_export]
//! macro_rules! a_macro { () => {} }
//!```

@rustbot label -E-needs-mcve

[Urgau]

Firstly I like to note that using #![deny(warnings)] (or equivalent) is generally considered to be an anti-pattern, see https://rust-unofficial.github.io/patterns/anti_patterns/deny-warnings.html

Secondly I think the lint is right to say that the macro_rules definition is non-local, let me explain:

• conceptually each doctest is a test embedded is a doc-comment
• each doctest is transformed by rustdoc into a function-test¹
• each test is the executed to verify that is passes (or fail)

So if I transform your example into a regular #[test] fn it would be something like this:

#[test]
fn doctest() {
    #[macro_export]
    macro_rules! a_macro { () => { todo!() } }
}

but let's imaging that there is another doctest, it could call the macro:

#[test]
fn doctest2() {
    a_macro!();
}

meaning that the macro_rules! definition is non-local, since it can be accessed outside of his local scope.

Note that currently rustdoc compiles each doctest into it's own crate, but as far as I know this isn't a guaranteed, and is even about to change: #123974. cc @GuillaumeGomez

@rustbot label -needs-triage -C-bug +C-discussion +T-compiler +T-rustdoc

Footnotes

1. there are some subtleties around compile_fail and other things, but lets ignore those ↩

[TheBlueMatt]

Firstly I like to note that using #![deny(warnings)] (or equivalent) is generally considered to be an anti-pattern, see https://rust-unofficial.github.io/patterns/anti_patterns/deny-warnings.html

I'm aware, we do it CI-only to avoid new warnings cropping up in new releases or PRs.

Secondly I think the lint is right to say that the macro_rules definition is non-local, let me explain:

As I noted in the OP, I'm aware these things compile into functions for test running, but I'd argue that doesn't make the lint correct - it's totally reasonable and expected to sometimes write doctests that have public functions and arguably public macros in them. If we demonstrate how the crate works with a macro_export we shouldn't get a lint warning (let alone one that "may become a hard error in the future"!).

The "doctests are secretly functions" bit is an implementation detail of the fact that they're tests, rather than something inherent in typechecked docs.

[GuillaumeGomez]

You can go around that by adding a fn main(). I think it's important in this case for people to know that the result is likely not what they're expecting.

[Urgau]

As I noted in the OP, I'm aware these things compile into functions for test running, but I'd argue that doesn't make the lint correct.

Sorry didn't pick up the "doctest function"; but whenever they are in doctests or not doesn't make the lint incorrect.

it's totally reasonable and expected to sometimes write doctests that have public functions and arguably public macros in them. If we demonstrate how the crate works with a macro_export we shouldn't get a lint warning

I agree this is useful, but it has nothing to do with the lint it-self, it's IMO tooling issue.

Thankfully, as pointed out by @GuillaumeGomez (thks), rustdoc allows you to define your own "test" function, by defining the "main" function. So a fix for your case would be:

  //! To use this crate you need to export a macro
  //! ```
  //! #[macro_export]
  //! macro_rules! a_macro { () => {} }
+ //! # fn main() {}
  //! ```

The "doctests are secretly functions" bit is an implementation detail of the fact that they're tests, rather than something inherent in typechecked docs.

I would argue that because it's an implementation detail, users can't rely on it, and thus can't assume that making a #[macro_export] macro_rules! is always global.

[TheBlueMatt]

You can go around that by adding a fn main().

Ah, thanks, maybe the lint in doctest contexts should print this as a suggestion as I assume some others (like me) aren't aware of this?

I think it's important in this case for people to know that the result is likely not what they're expecting.

ISTM with the current pre-#123974 state of things the result is generally what people writing doc tests like the above are expecting, so the lint doesn't really make sense. The post-#123974 version described by @Urgau above is indeed not what people are expecting but that seems like an issue with #123974 rather than a more general one?

[TheBlueMatt]

I would argue that because it's an implementation detail, users can't rely on it, and thus can't assume that making a #[macro_export] macro_rules! is always global.

If we want users to not rely on it the lint should probably be updated to indicate this to users as the behavior is incredibly surprising :).

[Urgau]

Ah, thanks, maybe the lint in doctest contexts should print this as a suggestion as I assume some others (like me) aren't aware of this?

Indeed, this would be a good suggestion. I will implement it.

@rustbot claim

[GuillaumeGomez]

I would argue that because it's an implementation detail, users can't rely on it, and thus can't assume that making a #[macro_export] macro_rules! is always global.

I think it should be the case, or at least before the doctest code.

[GuillaumeGomez]

Fixed by #124568, closing (as asked by @Urgau).