Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doctests don't run on nonexported macro_rules! macros #88038

Closed
inquisitivecrystal opened this issue Aug 14, 2021 · 7 comments · Fixed by #96630
Closed

Doctests don't run on nonexported macro_rules! macros #88038

inquisitivecrystal opened this issue Aug 14, 2021 · 7 comments · Fixed by #96630
Assignees
Labels
A-doctests Area: Documentation tests, run by rustdoc A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Aug 14, 2021

At present, doctests aren't run on macro_rules! macros that are not annotated with #[macro_export]. After #88019, it will become possible to fix this problem. However, doing so will cause tests to run that were skipped previously to run, which will likely cause test failures and may mess up people's CI. It might be helpful to provide some form of warning period first.

The relevant code is here:

rust/src/librustdoc/doctest.rs

Lines 1175 to 1184 in 5eacec9

hir::ItemKind::Macro(ref macro_def) => {
// FIXME(#88038): Non exported macros have historically not been tested,
// but we really ought to start testing them.
let def_id = item.def_id.to_def_id();
if macro_def.macro_rules && !self.tcx.has_attr(def_id, sym::macro_export) {
intravisit::walk_item(self, item);
return;
}
item.ident.to_string()
}

Fixing this issue technically just requires removing that special case. However, things are complicated by the fact that this will cause people's tests to suddenly fail. I'm not really sure how we should handle that. @jyn514, do you have any ideas?

@inquisitivecrystal inquisitivecrystal added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. A-doctests Area: Documentation tests, run by rustdoc labels Aug 14, 2021
@jyn514
Copy link
Member

jyn514 commented Aug 29, 2021

I think we should just change this; it's no different than #66059 in that the code was already broken before and we just didn't happen to run it. If you're really worried we can do a crater run but I would honestly be ok with just landing this.

@jyn514
Copy link
Member

jyn514 commented Apr 5, 2022

@rustbot label +E-easy +E-mentor

@rustbot rustbot added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 5, 2022
@jyn514
Copy link
Member

jyn514 commented Apr 5, 2022

Looks like #88019 accidentally prevented these doctests from being run even if --document-private-items is passed :(

@m-ysk
Copy link
Contributor

m-ysk commented May 2, 2022

@rustbot claim

JohnTitor added a commit to JohnTitor/rust that referenced this issue May 3, 2022
Include nonexported macro_rules! macros in the doctest target

Fixes rust-lang#88038

This PR aims to include nonexported `macro_rules!` macros in the doctest target. For more details, please see the above issue.
@bors bors closed this as completed in 3d18f94 May 5, 2022
@cuviper
Copy link
Member

cuviper commented May 12, 2022

FYI, rayon has an internal macro with an example that wasn't ready to be treated as a test:
https://github.com/rayon-rs/rayon/blob/9dc52d76b12f3da13b87dd509ff29a801177b8d3/src/delegate.rs#L14-L19

... discovered by failing in a CI run.

@cuviper
Copy link
Member

cuviper commented May 12, 2022

Hmm, I'm not even sure how to turn that into a working doctest, because the macro isn't exported.

The ignore(cannot-test-this-because-non-exported-macro) in #96630 is also telling -- then what's the point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-doctests Area: Documentation tests, run by rustdoc A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants