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

rustdoc: extern crate at crate root #56898

Closed
Mark-Simulacrum opened this issue Dec 17, 2018 · 7 comments
Closed

rustdoc: extern crate at crate root #56898

Mark-Simulacrum opened this issue Dec 17, 2018 · 7 comments
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

https://crater-reports.s3.amazonaws.com/beta-1.32-1/beta-2018-12-05/reg/allocator_api-0.5.0/log.txt

Dec 11 04:12:37.657 INFO [stdout] error[E0468]: an `extern crate` loading macros must be at the crate root
Dec 11 04:12:37.657 INFO [stdout]  --> src/liballoc/boxed.rs:242:1
Dec 11 04:12:37.657 INFO [stdout]   |
Dec 11 04:12:37.657 INFO [stdout] 4 | extern crate allocator_api;
Dec 11 04:12:37.657 INFO [stdout]   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Dec 11 04:12:37.657 INFO [stdout] 

cc @rust-lang/rustdoc

@Mark-Simulacrum Mark-Simulacrum added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 17, 2018
@QuietMisdreavus
Copy link
Member

QuietMisdreavus commented Dec 17, 2018

Here's what one of those tests looks like...

    /// ```
    /// # #[macro_use]
    /// extern crate allocator_api;
    /// # test_using_global! {
    /// use allocator_api::Box;
    /// fn main() {
    ///     let x = Box::new(41);
    ///     let static_ref: &'static mut usize = Box::leak(x);
    ///     *static_ref += 1;
    ///     assert_eq!(*static_ref, 42);
    /// }
    /// # }
    /// ```

It looks like the new fn main parsing is missing the fact that the main function is inside a macro call. Are we going to need to run macro expansion before we can look for main functions? cc @rep-nop

@QuietMisdreavus
Copy link
Member

Wait, if we want to run macro expansion, that means we need a resolver, which means we need a full compiler session, not just the parsing session we currently have up. That means either spinning up two compiler sessions per doctest (which i fear will slow down doctests even more than they already are) or re-organizing run_test to run this processing after the compiler context is set up.

One thing we'll have to be wary about is if we run the processing "inline" with the compilation, we'll need to potentially run macro expansion twice - once on the initial doctest, once after adding fn main or extern crate my_crate. Either way, that change seems too big to try to add to beta in the next few weeks. Should we back out the libsyntax parsing from beta so we can work this out on nightly?

@Mark-Simulacrum
Copy link
Member Author

I think either backing that change out or perhaps disabling it if we detect a macro (any macro, no need for anything fancy) and going back to the old behavior are probably both valid options here.

We could also just regress here -- at least this crater run only picked up this 1 crate -- and really, wrapping main in a macro is quite rare I imagine.

@ehuss
Copy link
Contributor

ehuss commented Dec 17, 2018

I think this also hit criterion, but they have already updated: bheisler/criterion.rs@cf521b0

@QuietMisdreavus
Copy link
Member

I've opened #57019 on beta to use the text-based search if it found a macro invocation but not a main function, which should solve this issue.

bors added a commit that referenced this issue Jan 5, 2019
…=GuillaumeGomez

[beta] rustdoc: semi-revert libsyntax doctest parsing if a macro is wrapping main

Fixes #56898

This is a patch to doctest parsing on beta (that i plan to "forward-port" to master) that reverts to the old text-based `fn main` scan if it found a macro invocation in the doctest but did not find a main function. This should solve situations like `allocator_api` which wrap their main functions in a macro invocation, but doesn't solve something like the initial version of `quicli` which used a macro to *generate* the main function. (Properly doing the latter involves running macro expansion before checking, which is a bit too involved for a beta fix.)
@pietroalbini
Copy link
Member

Is this also fixed on stable or only on beta?

@QuietMisdreavus
Copy link
Member

Looks like it works on stable:

[misdreavus@tonberry allocator_api]$ cargo +stable test --doc
   Compiling semver-parser v0.7.0
   Compiling semver v0.9.0
   Compiling rustc_version v0.2.3
   Compiling allocator_api v0.5.0 (/home/misdreavus/clones/allocator_api)
    Finished dev [unoptimized + debuginfo] target(s) in 2.09s
   Doc-tests allocator_api

running 11 tests
test src/liballoc/boxed.rs - boxed::Box<T, A>::into_raw (line 186) ... ok
test src/liballoc/boxed.rs - boxed::Box<T, A>::from_raw_in (line 148) ... ok
test src/liballoc/boxed.rs - boxed::Box<T, A>::clone (line 320) ... ok
test src/liballoc/boxed.rs - boxed::Box<T, A>::clone_from (line 339) ... ok
test src/liballoc/boxed.rs - boxed::Box<T, A>::leak (line 224) ... ok
test src/liballoc/boxed.rs - boxed::Box<T, A>::new_in (line 47) ... ok
test src/liballoc/boxed.rs - boxed::Box<T, A>::leak (line 240) ... ok
test src/liballoc/boxed.rs - boxed::Box<T>::from_raw (line 119) ... ok
test src/liballoc/boxed.rs - boxed::Box<T>::new (line 87) ... ok
test src/liballoc/raw_vec.rs - raw_vec::RawVec<T, A>::double (line 278) ... ok
test src/liballoc/raw_vec.rs - raw_vec::RawVec<T, A>::reserve (line 484) ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

[misdreavus@tonberry allocator_api]$ rustdoc +stable --version
rustdoc 1.31.1 (b6c32da9b 2018-12-18)

The test also works on beta, but i haven't "forward-ported" the fix to nightly yet.

Centril added a commit to Centril/rust that referenced this issue Jan 12, 2019
…-parsing, r=GuillaumeGomez

rustdoc: use text-based doctest parsing if a macro is wrapping main

This is a "forward-port" of rust-lang#57019, intended to get rust-lang#56898 on nightly, since it's now fixed on beta (and already worked on stable).

To recap:

* The libsyntax-based doctest parsing now checks to see whether there is a top-level macro invocation in the doctest while it's checking for `fn main` and an `extern crate` statement.
* If it finds a macro invocation and *didn't* find `fn main`, then it performs the older text-based scan to allow doctests like the ones in `allocator_api` to still compile.

A "proper" fix will involve changing how `make_test` works to call it later in the `run_test` function, after the initial steps of compilation have completed. I've filed [a separate issue](rust-lang#57415) for that, though.
@pietroalbini pietroalbini added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants