rustdoc: inject `#[macro_use] extern crate` in doctests for macros #33511

birkenfeld · 2016-05-09T09:42:46Z

Note: I tried to add a test to src/test/rustdoc, but the way in which these tests are called doesn't seem to allow extern crate testname; to work at all...

rust-highfive · 2016-05-09T09:42:53Z

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2016-05-09T14:59:09Z

Thanks! Could you elaborate a bit on why the added test wouldn't work? What did the error message look like?

Also, perhaps macros could always be imported? I could imagine a doctest for a normal function may also want to use the crate's macros as it's part of the API

birkenfeld · 2016-05-09T15:10:49Z

Thanks! Could you elaborate a bit on why the added test wouldn't work? What did the error message look like?

The external crate isn't found. This is not specific to the macro issue, but apparently there was no need for such a test yet (all test cases that use --test don't actually use the items defined in the test source).

---- mymacro!_0 stdout ----
        <anon>:1:14: 1:39 error: can't find crate for `issue_29286` [E0463]
<anon>:1 #[macro_use] extern crate issue_29286;
                      ^~~~~~~~~~~~~~~~~~~~~~~~~

Also, perhaps macros could always be imported? I could imagine a doctest for a normal function may also want to use the crate's macros as it's part of the API

That can be done as well; this was the more backwards compatible option (although I admit that the breakage is pretty hard to imagine). Should the "does it contain the crate name" check be removed too then?

alexcrichton · 2016-05-09T16:33:21Z

Oh this is actually a case where the test harness isn't quite up to the task. Right now it only runs rustdoc over the crate rather than compiling it ahead of time. You may be able to add a run-make test for now (or perhaps could modify src/tools/compiletest as well).

I'd leave in the crate name check for now, although in retrospect we probably should have just always added it. I suspect though that the addition of #[macro_use] shouldn't break too much (if anything)

durka · 2016-05-09T17:01:01Z

I think #[macro_use] should be there all the time, or never. There's already too much magic IMO in the test runner.

Either way, please also update the docs here which explain how test code gets wrapped in fn main etc.

birkenfeld · 2016-05-09T18:02:03Z

@durka good point, done.
@alexcrichton I found a run-make test that was already doing rustdoc, so I added the tests there (and renamed the test). Hope that's fine.

durka · 2016-05-09T18:05:30Z

src/doc/book/documentation.md

-   <mycrate>;` is inserted (note the lack of `#[macro_use]`).
+3. If the example does not contain `extern crate`, then `#[macro_use]
+   extern crate <mycrate>;` is inserted if the example is either for
+   a macro definition, or contains the crate name.


#[macro_use] is also mentioned in the paragraph below, and in the section "Documenting macros" -- both obsolete with this PR I believe.

Fixes: rust-lang#29286

durka · 2016-05-09T20:30:14Z

src/doc/book/documentation.md

 /// ```ignore
 /// fn foo() {
 /// ```
-# fn foo() {}
 ```

 The `ignore` directive tells Rust to ignore your code. This is almost never
 what you want, as it's the most generic. Instead, consider annotating it
 with `text` if it's not code, or using `#`s to get a working example that
 only shows the part you care about.


You seem to be ignoring this advice in this very file with the second commit...

That wasn't supposed to be part of this PR anyway.

steveklabnik · 2016-05-09T20:44:01Z

Also, perhaps macros could always be imported? I could imagine a doctest for a normal function may also want to use the crate's macros as it's part of the API

Yeah, I would be a fan of doing this.

alexcrichton · 2016-05-10T17:48:24Z

@bors: r+ f767373

All looks good to me, thanks @birkenfeld!

durka · 2016-05-10T17:49:55Z

Was there a resolution on whether macros should be imported when documenting a non-macro item?

durka · 2016-05-10T17:52:12Z

(If not, and this stays r+ed, I'll open a followup PR after this is merged so we can discuss there.)

alexcrichton · 2016-05-10T17:54:35Z

@durka for backwards compatibility we may not be able to do that, although in retrospect we should always have injected #[macro_use] extern crate foo;

durka · 2016-05-10T17:56:15Z

I don't see how adding #[macro_use] extern crate for macro tests is more backwards compatible than adding it anywhere we currently add extern crate?

alexcrichton · 2016-05-10T18:32:40Z

These tests are pretty fragile as they're syntactically creating a program, and if we didn't previously inject extern crate and we started doing so it may happen in the wrong location or conflict with another import. Most crates don't export macros nor do they shadow the standard library's macros, so importing macros is highly unlikely to break.

durka · 2016-05-10T18:36:09Z

If we're really worried about backcompat here and the justification for this PR as implemented is only that it won't affect a large number of tests... I think we shouldn't change it at all, unfortunately.

I'm not convinced that complicating the test runner is worth it. Yes, it works great in simple cases. But as soon as you get a weird resolve error and you have to comb through the docs trying to understand how it mangles the doctests, it backfires.

If you're testing a non-macro item and you want to use crate macros, now you need to add the manual #[macro_use] extern crate line (so as I'm writing this comment I realize that we should add that back into the docs somewhere). That corner case is going to be rather undiscoverable.

alexcrichton · 2016-05-10T18:50:02Z

@bors: r-

Ok, let's see what others think. cc @rust-lang/tools

durka · 2016-05-10T18:52:25Z

Sorry for being difficult :(

I remember being really frustrated when first learning how to document macros. This makes it easier, but harder to discover the fix when it breaks (which I believe it will, in crates that mix macro and non-macro items). So I'm torn.

bors · 2016-05-19T20:54:49Z

☔ The latest upstream changes (presumably #33742) made this pull request unmergeable. Please resolve the merge conflicts.

steveklabnik · 2016-06-06T22:47:41Z

r? @alexcrichton

alexcrichton · 2016-06-07T06:20:30Z

@durka ok, thinking more on this, I don't think I really see a reason to not land this. The way doc-blocks are tested is already voodoo, so that's not really the issue at hand here. Do you have a concrete use case where this goes awry? I think that this is fully backwards-compatible otherwise.

durka · 2016-06-07T14:56:35Z

I'm fine with it landing. Just two things:

I realized that the paragraphs in the book that I said were obsoleted by this are, unfortunately, not obsolete. The corner case where you will still need to override the "voodoo" is if you want to invoke a macro from your crate while documenting a non-macro item. (Previously it was any time you want to invoke a macro from your crate while documenting anything.)
The back-compat issue with putting in #[macro_use] everywhere is that you might have defined a macro in the crate that overrides an std macro, but intended to use the std macro in the crate, right? So, if your crate has
```
#[macro_export]
macro_rules! try { ($($t:tt)*) => { panic!("Do or do not; there is no try."); }
```
but in documenting some unrelated item you write try!("42".parse::<i32>()) without any extern crate lines, then you expect to get the regular try!. Is there any other problematic case? I apologize if I've misunderstood the back-compat concern.

So it seems relatively risk-free to add #[macro_use] extern crate to macro doc examples by default. Hopefully you invoke the macro in the example, so any macro doc examples out there already have the #[macro_use] extern crate line. It's possible that they wrote #[macro_use(just_this_one_macro)] extern crate and relied on some other macro not getting overridden, but that seems exceedingly unlikely. Again, I'm not sure if there's another back-compat concern that I'm missing.

alexcrichton · 2016-06-08T12:20:56Z

you might have defined a macro in the crate that overrides an std macro, but intended to use the std macro in the crate, right?

Yeah this perhaps could happen, but there's no way that would actually work out well in practice (the macro/crate would be unusable anyway), so I doubt that's actually happening anywhere.

I couldn't actually think of any backcompat hazard, which is why I think it's ok to land this.

durka · 2016-06-08T18:35:30Z

But then why don't we put #[macro_use] everywhere? I don't understand. But I'm not going to argue as long as we put the instructions on how to get around it back in the book somewhere.

alexcrichton · 2016-06-09T14:32:10Z

As I commented before

I'd leave in the crate name check for now, although in retrospect we probably should have just always added it. I suspect though that the addition of #[macro_use] shouldn't break too much (if anything)

There's also nothing to get around here, all this patch does is inject #[macro_use] for doctesting macros, if it was already present before then we won't do anything because the code already contains "extern crate". That's why I've come to the conclusion that there's likely to be literally no breakage here.

durka · 2016-06-09T17:37:45Z

The case where you still need to "get around" the voodoo is if you are not doctesting a macro but you do want to invoke a macro.

durka · 2016-06-09T17:38:38Z

Again I'm sorry to be a stick in the mud here but I don't want this to get any harder to figure out than it already is. Just now coaching a user on IRC who was frustrated with doctests to the point of not wanting to mark them as code.

alexcrichton · 2016-06-20T18:32:30Z

@birkenfeld do you want to rebase this and perhaps exclude the check for for_macro as well?

alexcrichton · 2016-07-19T04:12:19Z

Closing due to inactivity, but feel free to resubmit with a rebase!

rust-highfive assigned steveklabnik May 9, 2016

birkenfeld force-pushed the issue-29286 branch from e18fd0c to b5ded9a Compare May 9, 2016 18:00

durka reviewed May 9, 2016
View reviewed changes

birkenfeld force-pushed the issue-29286 branch from b5ded9a to f85b3c2 Compare May 9, 2016 18:15

rustdoc: inject #[macro_use] extern crate in doctests

f767373

Fixes: rust-lang#29286

birkenfeld force-pushed the issue-29286 branch from f85b3c2 to 2d4b129 Compare May 9, 2016 18:16

durka reviewed May 9, 2016
View reviewed changes

birkenfeld force-pushed the issue-29286 branch from 2d4b129 to f767373 Compare May 9, 2016 20:49

rust-highfive assigned alexcrichton and unassigned steveklabnik Jun 6, 2016

alexcrichton closed this Jul 19, 2016

birkenfeld deleted the issue-29286 branch August 7, 2016 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: inject `#[macro_use] extern crate` in doctests for macros #33511

rustdoc: inject `#[macro_use] extern crate` in doctests for macros #33511

birkenfeld commented May 9, 2016

rust-highfive commented May 9, 2016

alexcrichton commented May 9, 2016

birkenfeld commented May 9, 2016

alexcrichton commented May 9, 2016

durka commented May 9, 2016 •

edited

Loading

birkenfeld commented May 9, 2016

durka May 9, 2016

durka May 9, 2016

birkenfeld May 9, 2016

steveklabnik commented May 9, 2016

alexcrichton commented May 10, 2016

durka commented May 10, 2016

durka commented May 10, 2016

alexcrichton commented May 10, 2016

durka commented May 10, 2016

alexcrichton commented May 10, 2016

durka commented May 10, 2016 •

edited

Loading

alexcrichton commented May 10, 2016

durka commented May 10, 2016

bors commented May 19, 2016

steveklabnik commented Jun 6, 2016

alexcrichton commented Jun 7, 2016

durka commented Jun 7, 2016 •

edited

Loading

alexcrichton commented Jun 8, 2016

durka commented Jun 8, 2016

alexcrichton commented Jun 9, 2016

durka commented Jun 9, 2016

durka commented Jun 9, 2016

alexcrichton commented Jun 20, 2016

alexcrichton commented Jul 19, 2016

rustdoc: inject #[macro_use] extern crate in doctests for macros #33511

rustdoc: inject #[macro_use] extern crate in doctests for macros #33511

Conversation

birkenfeld commented May 9, 2016

rust-highfive commented May 9, 2016

alexcrichton commented May 9, 2016

birkenfeld commented May 9, 2016

alexcrichton commented May 9, 2016

durka commented May 9, 2016 • edited Loading

birkenfeld commented May 9, 2016

durka May 9, 2016

Choose a reason for hiding this comment

durka May 9, 2016

Choose a reason for hiding this comment

birkenfeld May 9, 2016

Choose a reason for hiding this comment

steveklabnik commented May 9, 2016

alexcrichton commented May 10, 2016

durka commented May 10, 2016

durka commented May 10, 2016

alexcrichton commented May 10, 2016

durka commented May 10, 2016

alexcrichton commented May 10, 2016

durka commented May 10, 2016 • edited Loading

alexcrichton commented May 10, 2016

durka commented May 10, 2016

bors commented May 19, 2016

steveklabnik commented Jun 6, 2016

alexcrichton commented Jun 7, 2016

durka commented Jun 7, 2016 • edited Loading

alexcrichton commented Jun 8, 2016

durka commented Jun 8, 2016

alexcrichton commented Jun 9, 2016

durka commented Jun 9, 2016

durka commented Jun 9, 2016

alexcrichton commented Jun 20, 2016

alexcrichton commented Jul 19, 2016

rustdoc: inject `#[macro_use] extern crate` in doctests for macros #33511

rustdoc: inject `#[macro_use] extern crate` in doctests for macros #33511

durka commented May 9, 2016 •

edited

Loading

durka commented May 10, 2016 •

edited

Loading

durka commented Jun 7, 2016 •

edited

Loading