Pass --crate-type to rustdoc

[Aaron1011]

This supports the corresponding rustc PR. To enable rustdoc to properly
document macros, we pass a new flag '--proc-macro-crate' when
documenting a proc-macro crate. This causes rustdoc to enable the
proc-macro compiler logic that runs when rustc is building a proc-macro
crate.

This flag is essentially a more restricted version of
'--crate-type=proc-macro'. I didn't think it was necessary to pass the
full '--crate-type' flag to rustdoc, when only two options would ever be
used (proc-macro vs anything else).

[rust-highfive]

r? @nrc

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

[Aaron1011]

The test failure is caused by cargo passing --proc-macro-crate to the current rustdoc, which doesn't understand it.

[alexcrichton]

Thanks for the report! For our own CI some version detection or feature detection will need to be done to get the test working on stable, but otherwise seems fine to add as soon as this is stable in rustdoc!

[Aaron1011]

As discussed on the corresponding rustc PR, we are now using the --crate-type flag instead of a new proc-macro-crate flag.

Unfortunately, this causes an enormous number of CI failures, since every test involving rustdoc will try to pass the (currently unsupported) --crate-type option. This will remain an issue until my rustc is merged and makes its way to stable.

@alexcrichton: What do you think is the best way of dealing with this?

[Aaron1011]

As suggested by @eddyb, I've modified this PR to only pass --crate-type to rustdoc if it's supported. However, I may have added the check in the wrong place - I'm not quite sure where the best place to put it is.

[Aaron1011]

This PR is no longer blocked - in fact, it is blocking the rustc PR.

[alexcrichton]

This looks good to me, thanks! Could this also add nightly-only tests which assert that the right flags are passed?

[ehuss]

I'm concerned about the performance implications of how this detects the flag support. It adds 50-80ms to every cargo invocation. It also runs twice unnecessarily.

Perhaps it be called once, and only if executing rustdoc? And if --crate-type is only used for proc-macro, maybe it could be further refined to only test if documenting a proc-macro?

[alexcrichton]

Oh oops sorry I figured this was using the same logic as rustc to cache results, but @ehuss you're right in that it's not. I think it'd be fine to reuse the same logic we have there though to basically cache results of rustdoc invocations on the filesystem.

[Aaron1011]

Could this also add nightly-only tests which assert that the right flags are passed?

@alexcrichton: Those tests would need to be done in a follow-up PR - right now, they will never pass, since the corresponding rustc PR is blocked on this PR.

[alexcrichton]

@Aaron1011 oh sorry I'm assuming that this lands in rustc first, then we land this in Cargo, then the Cargo submodule is updated. That way when we add nightly-only tests we'll be able to ensure this works before we land it.

[Aaron1011]

@ehuss @alexcrichton: I've modified the PR to cache the result of checking for --crate-type, and to only perform the check when rustdoc is run.

However, I think it's better to always run the check, not just when documenting a proc macro crate. The idea behind supporting --crate-type for rustdoc is that Cargo shouldn't have to care why rustdoc needs to know the crate type. If we later decide to make use of additional crate information in rustdoc, it shouldn't require any changes to Cargo.

[Aaron1011]

@alexcrichton: Unfortunately, it's not possible to land the rustc PR first, unless we make changes to it that would be immediately reverted. Rustdoc now runs all of the normal proc-macro checking logic that rustc does, which will error if a proc macro is declared in a non-proc-macro crate. Without Cargo passing --crate-type, rustdoc will believe that it's documenting a normal crate, and will error when we try to run cargo doc on the rustc-macros crate.

[alexcrichton]

Hm I'm not sure if that can be done regardless? That's a breaking change to rustdoc, right?

[Aaron1011]

@alexcrichton: Technically, it is a breaking change. However:

• It is completely invisible to anyone running cargo doc. It only affects anyone running rustdoc directly on a proc macro crate.
• For anyone who is running rustdoc, the fix is extremely simple: add --crate-type proc-macro to the rustdoc invocation.
• It allows us to fix several longstanding bugs in how rustdoc handling proc macros -

[alexcrichton]

Yes I understand your points, but that's fits the shape of every single breaking change we could land to Rust. Typically they have low impact, they're easy to fix, and they fix bugs. That being said we don't accept breaking changes in rust-lang/rust. There are users, albeit not the majority, running rustdoc directly and this would be a breaking change to them.

There's various strategies of handling breaking changes in rust-lang/rust to phase out behavior over time, and if that's what's required here then that should be employed rather than one breaking change.

[Aaron1011]

@alexcrichton: Fair enough. I've changed the rustc PR to emit a warning when --crate-type proc-macro isn't passed for a proc-macro crate.

[Aaron1011]

@alexcrichton I've modified the PR to check (and cache) the output of rustdoc --crate-type. All of the relevant tests ignore the flag, so they work on both stable and nightly.

[alexcrichton]

Thanks! Looks good to me, but instead of an Rc could the flag be calculated at construction of Compliation instead? (that way we're just storing a bool, not an Rc)

[Aaron1011]

@alexcrichton That would cause us to run rustdoc even when we're not running cargo doc

[alexcrichton]

Because it's cached I think that's fine, and this should be gracefully handling errors already if it turns out that rustdoc has issues.

[Mark-Simulacrum]

Ah, so one concern with running rustdoc where we weren't before is that e.g. in rustbuild we'll not always have a rustdoc around; to be clear that's not really a blocking concern here though since we can work around that (probably by faking --version output or so), just wanted to give a brief heads up. I think there's no platform today where we do ship a rustc and don't ship rustdoc so we should be fine in that respect.

[Aaron1011]

@Mark-Simulacrum: Note that it's fine for rustdoc to not exist - this will be interpreted as rustdoc not supporting --crate-type. If we ever need to invoke rustdoc for real, the actual error will show up.

[alexcrichton]

@bors: r+

[bors]

📌 Commit b68e854 has been approved by alexcrichton

[bors]

⌛ Testing commit b68e854 with merge b2d4f20...

[bors]

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing b2d4f20 to master...