Adjust #[macro_export]/doctest help suggestion for non_local_defs lint

[Urgau]

This PR adjust the help suggestion of the non_local_definitions lint when encountering a #[macro_export] at top-level doctest.

So instead of a non-sentential help suggestion to move the macro_rules! up above the rustdoc-generated function. We now suggest users to declare their own function.

Fixes (partially, needs backport) #124534

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[fmease]

What if rustdoc added an internal #[rustc_*] attribute to the generated doc test fns which we would then look for here?

[Urgau]

That would be the best and that's partially what #123974 is trying to do by making doctest regular #[test] fn, but there are many subtleties.

When that PR lands we could look at improving this checks. In the meantime I would like to stay with this minimal approach so it can be backported easily.

[GuillaumeGomez]

Yes totally agree. Let's not add a hack to try to guess if we're inside a doctest or not please. ^^'

[Urgau]

In order to guess less, I changed the PR to use the UNSTABLE_RUSTDOC_TEST_PATH env, which is already used for diagnostic code.

It's not as great as a proper attribute but much better than looking a function names.

[GuillaumeGomez]

Nah it's fine. Much simpler than a brand new attribute.

[fee1-dead]

r? compiler

[GuillaumeGomez]

Wouldn't this better to make it a rustdoc lint instead?

EDIT: nevermind, got confused when reading "doctest" in the title. So from what I saw, the current code is a hack to detect whether it's a doctest or not. @fmease suggested to instead add a new crate level attribute when generating doctests so we're sure it's a doctest instead of trying to guess what it is, which I think it's a great idea.

[GuillaumeGomez]

Looks good to me. Great idea to use an environment variable set by rustdoc when running doctests!

[michaelwoerister]

So there is a path forward to getting rid of the UNSTABLE_RUSTDOC_TEST_PATH env var check?

[Urgau]

So there is a path forward to getting rid of the UNSTABLE_RUSTDOC_TEST_PATH env var check?

I think in the longer term we could a custom attribute.

Or we could right now use the fact that rustc crates a special FileName::DocTest for doctest when given UNSTABLE_RUSTDOC_TEST_PATH:

rust/compiler/rustc_driver_impl/src/lib.rs

Lines 506 to 514 in f5efc3c

	if let Ok(path) = env::var("UNSTABLE_RUSTDOC_TEST_PATH") {
	let line = env::var("UNSTABLE_RUSTDOC_TEST_LINE").expect(
	"when UNSTABLE_RUSTDOC_TEST_PATH is set \
	UNSTABLE_RUSTDOC_TEST_LINE also needs to be set",
	);
	let line = isize::from_str_radix(&line, 10)
	.expect("UNSTABLE_RUSTDOC_TEST_LINE needs to be an number");
	let file_name = FileName::doc_test_source_code(PathBuf::from(path), line);
	Ok(Some(Input::Str { name: file_name, input: src }))

and then in the code instead of looking for the env we could do something like this

let is_from_doctest = matches!(cx
   .tcx
   .sess
   .source_map()
   .span_to_filename(span), FileName:::DocTest(..));  // untested, but I think it would work

[GuillaumeGomez]

Or we could right now use the fact that rustc crates a special FileName::DocTest for doctest when given UNSTABLE_RUSTDOC_TEST_PATH:

Not something we can rely on so better not.

I think using the environment variable is good enough. Not sure if it's worth adding a new attribute for handling this case though...

[fmease]

Not something we can rely on so better not.

Why not? File::DocTest resides in rustc just like the lint. Its removal would trigger a build error if used in the lint.

[michaelwoerister]

I like the FileName:::DocTest idea better than using an env var.

[GuillaumeGomez]

Because it might be changed in the future when #123974 is merged (you can see the change here).

[michaelwoerister]

When that PR lands we could look at improving this checks. In the meantime I would like to stay with this minimal approach so it can be backported easily.

OK, I don't think we should block this PR on the env var issue. It looks like #123974 is going to change this quite a bit.

Feel free to r=me (unless you have a sudden insight on how to improve this 🙂)

Thanks for the review, @GuillaumeGomez!

[GuillaumeGomez]

We can always come back on this later on, nothing's ever definitive in compiler-land. 😺 And since it's not something users can directly use, we can make other changes easily. Anyway, since we got both rustdoc and compiler team approving this, let's go! Thanks everyone and thanks @Urgau for this improvement!

@bors r=michaelwoerister,GuillaumeGomez rollup

[bors]

📌 Commit 712560c has been approved by michaelwoerister,GuillaumeGomez

It is now in the queue for this repository.

[fmease]

Nominating for beta-backport (well, the T-compiler meeting is happening as we speak).
Removing T-rustdoc to avoid triggering the triagebot nomination posting on Zulip.

[apiraino]

Beta backport declined as per compiler team on Zulip.

Rather than backporting this (which seems to still a few open points, example about the wording), this should be reverted on nightly and then revert that.

(cc @Urgau @wesleywiser did I understand correctly the discussion?)

@rustbot label -beta-nominated