From 18f0f242e18349f89716a30a74d56003d01a6af2 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 4 Mar 2021 15:03:22 -0500 Subject: [PATCH] Give precedence to `html_root_url` over `--extern-html-root-url` by default, but add a way to opt-in to the previous behavior ## What is an HTML root url? It tells rustdoc where it should link when documentation for a crate is not available locally; for example, when a crate is a dependency of a crate documented with `cargo doc --no-deps`. ## What is the difference between `html_root_url` and `--extern-html-root-url`? Both of these tell rustdoc what the HTML root should be set to. `doc(html_root_url)` is set by the crate author, while `--extern-html-root-url` is set by the person documenting the crate. These are often different. For example, docs.rs uses `--extern-html-root-url https://docs.rs/crate-name/version` to ensure all crates have documentation, even if `html_root_url` is not set. Conversely, crates such as Rocket set `doc(html_root_url = "https://api.rocket.rs")`, because they prefer users to view the documentation on their own site. Crates also set `html_root_url` to ensure they have documentation when building locally when offline. This is unfortunate to require, because it's more work from the library author. It also makes it impossible to distinguish between crates that want to be viewed on a different site (e.g. Rocket) and crates that just want documentation to be visible offline at all (e.g. Tokio). I have authored a separate change to the API guidelines to no longer recommend doing this: https://github.com/rust-lang/api-guidelines/pull/230. ## Why change the default? In the past, docs.rs has been the main user of `--extern-html-root-url`. However, it's useful for other projects as well. In particular, Cargo wants to pass it by default when running `--no-deps` (https://github.com/rust-lang/cargo/issues/8296). Unfortunately, for these other use cases, the priority order is inverted. They want to give *precedence* to the URL the crate picks, and only fall back to the `--extern-html-root` if no `html_root_url` is present. That allows passing `--extern-html-root` unconditionally, without having to parse the source code to see what attributes are present. For docs.rs, however, we still want to keep the old behavior, so that all links on docs.rs stay on the site. --- src/librustdoc/clean/types.rs | 8 ++++++-- src/librustdoc/config.rs | 5 +++++ src/librustdoc/core.rs | 4 +--- src/librustdoc/formats/cache.rs | 13 +++++++------ src/librustdoc/lib.rs | 7 +++++++ src/test/rustdoc/auxiliary/html_root.rs | 2 ++ src/test/rustdoc/auxiliary/no_html_root.rs | 1 + .../rustdoc/extern-html-root-url-precedence.rs | 7 +++++++ src/test/rustdoc/extern-html-root-url.rs | 18 +++++++++++++++--- 9 files changed, 51 insertions(+), 14 deletions(-) create mode 100644 src/test/rustdoc/auxiliary/html_root.rs create mode 100644 src/test/rustdoc/auxiliary/no_html_root.rs create mode 100644 src/test/rustdoc/extern-html-root-url-precedence.rs diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 782ff8df17b58..7025574914331 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -168,6 +168,7 @@ impl ExternalCrate { crate fn location( &self, extern_url: Option<&str>, + extern_url_takes_precedence: bool, dst: &std::path::Path, tcx: TyCtxt<'_>, ) -> ExternalLocation { @@ -189,8 +190,10 @@ impl ExternalCrate { return Local; } - if let Some(url) = extern_url { - return to_remote(url); + if extern_url_takes_precedence { + if let Some(url) = extern_url { + return to_remote(url); + } } // Failing that, see if there's an attribute specifying where to find this @@ -202,6 +205,7 @@ impl ExternalCrate { .filter_map(|a| a.value_str()) .map(to_remote) .next() + .or(extern_url.map(to_remote)) // NOTE: only matters if `extern_url_takes_precedence` is false .unwrap_or(Unknown) // Well, at least we tried. } diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index e44158bc04230..eef6985ea30a0 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -233,6 +233,8 @@ crate struct RenderOptions { crate extension_css: Option, /// A map of crate names to the URL to use instead of querying the crate's `html_root_url`. crate extern_html_root_urls: BTreeMap, + /// Whether to give precedence to `html_root_url` or `--exten-html-root-url`. + crate extern_html_root_takes_precedence: bool, /// A map of the default settings (values are as for DOM storage API). Keys should lack the /// `rustdoc-` prefix. crate default_settings: FxHashMap, @@ -658,6 +660,8 @@ impl Options { let show_type_layout = matches.opt_present("show-type-layout"); let nocapture = matches.opt_present("nocapture"); let generate_link_to_definition = matches.opt_present("generate-link-to-definition"); + let extern_html_root_takes_precedence = + matches.opt_present("extern-html-root-takes-precedence"); if generate_link_to_definition && (show_coverage || output_format != OutputFormat::Html) { diag.struct_err( @@ -714,6 +718,7 @@ impl Options { themes, extension_css, extern_html_root_urls, + extern_html_root_takes_precedence, default_settings, resource_suffix, enable_minification, diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 512c4ed2d3cd6..e96eba2f17dbf 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -528,9 +528,7 @@ crate fn run_global_ctxt( let render_options = ctxt.render_options; let mut cache = ctxt.cache; - krate = tcx.sess.time("create_format_cache", || { - cache.populate(krate, tcx, &render_options.extern_html_root_urls, &render_options.output) - }); + krate = tcx.sess.time("create_format_cache", || cache.populate(krate, tcx, &render_options)); // The main crate doc comments are always collapsed. krate.collapsed = true; diff --git a/src/librustdoc/formats/cache.rs b/src/librustdoc/formats/cache.rs index 3d267ca50334d..d45f277a0a883 100644 --- a/src/librustdoc/formats/cache.rs +++ b/src/librustdoc/formats/cache.rs @@ -1,6 +1,4 @@ -use std::collections::BTreeMap; use std::mem; -use std::path::Path; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir::def_id::{CrateNum, DefId, CRATE_DEF_INDEX}; @@ -9,6 +7,7 @@ use rustc_middle::ty::TyCtxt; use rustc_span::symbol::sym; use crate::clean::{self, GetDefId, ItemId}; +use crate::config::RenderOptions; use crate::fold::DocFolder; use crate::formats::item_type::ItemType; use crate::formats::Impl; @@ -142,19 +141,21 @@ impl Cache { &mut self, mut krate: clean::Crate, tcx: TyCtxt<'_>, - extern_html_root_urls: &BTreeMap, - dst: &Path, + render_options: &RenderOptions, ) -> clean::Crate { // Crawl the crate to build various caches used for the output debug!(?self.crate_version); self.traits = krate.external_traits.take(); + let RenderOptions { extern_html_root_takes_precedence, output: dst, .. } = render_options; // Cache where all our extern crates are located // FIXME: this part is specific to HTML so it'd be nice to remove it from the common code for &e in &krate.externs { let name = e.name(tcx); - let extern_url = extern_html_root_urls.get(&*name.as_str()).map(|u| &**u); - self.extern_locations.insert(e.crate_num, e.location(extern_url, &dst, tcx)); + let extern_url = + render_options.extern_html_root_urls.get(&*name.as_str()).map(|u| &**u); + let location = e.location(extern_url, *extern_html_root_takes_precedence, dst, tcx); + self.extern_locations.insert(e.crate_num, location); self.external_paths.insert(e.def_id(), (vec![name.to_string()], ItemType::Module)); } diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index e02d92b11b844..34fe808dae2e4 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -294,6 +294,13 @@ fn opts() -> Vec { "NAME=URL", ) }), + unstable("extern-html-root-takes-precedence", |o| { + o.optflagmulti( + "", + "extern-html-root-takes-precedence", + "give precedence to `--extern-html-root-url`, not `html_root_url`", + ) + }), stable("plugin-path", |o| o.optmulti("", "plugin-path", "removed", "DIR")), stable("C", |o| { o.optmulti("C", "codegen", "pass a codegen option to rustc", "OPT[=VALUE]") diff --git a/src/test/rustdoc/auxiliary/html_root.rs b/src/test/rustdoc/auxiliary/html_root.rs new file mode 100644 index 0000000000000..4eb0b700f8f16 --- /dev/null +++ b/src/test/rustdoc/auxiliary/html_root.rs @@ -0,0 +1,2 @@ +#![doc(html_root_url="https://example.com/html_root")] +pub fn foo() {} diff --git a/src/test/rustdoc/auxiliary/no_html_root.rs b/src/test/rustdoc/auxiliary/no_html_root.rs new file mode 100644 index 0000000000000..c5c0bc606cd69 --- /dev/null +++ b/src/test/rustdoc/auxiliary/no_html_root.rs @@ -0,0 +1 @@ +pub fn bar() {} diff --git a/src/test/rustdoc/extern-html-root-url-precedence.rs b/src/test/rustdoc/extern-html-root-url-precedence.rs new file mode 100644 index 0000000000000..def6767ea47f6 --- /dev/null +++ b/src/test/rustdoc/extern-html-root-url-precedence.rs @@ -0,0 +1,7 @@ +// compile-flags:-Z unstable-options --extern-html-root-url core=https://example.com/core/0.1.0 --extern-html-root-takes-precedence + +// @has extern_html_root_url_precedence/index.html +// --extern-html-root should take precedence if `--takes-precedence` is passed +// @has - '//a/@href' 'https://example.com/core/0.1.0/core/iter/index.html' +#[doc(no_inline)] +pub use std::iter; diff --git a/src/test/rustdoc/extern-html-root-url.rs b/src/test/rustdoc/extern-html-root-url.rs index 60b7b28ae4acf..17eedcf2ab8dd 100644 --- a/src/test/rustdoc/extern-html-root-url.rs +++ b/src/test/rustdoc/extern-html-root-url.rs @@ -1,6 +1,18 @@ -// compile-flags:-Z unstable-options --extern-html-root-url core=https://example.com/core/0.1.0 +// compile-flags:-Z unstable-options --extern-html-root-url html_root=https://example.com/override --extern-html-root-url no_html_root=https://example.com/override +// aux-build:html_root.rs +// aux-build:no_html_root.rs +// NOTE: intentionally does not build any auxiliary docs + +extern crate html_root; +extern crate no_html_root; // @has extern_html_root_url/index.html -// @has - '//a/@href' 'https://example.com/core/0.1.0/core/iter/index.html' +// `html_root_url` should override `--extern-html-root-url` +// @has - '//a/@href' 'https://example.com/html_root/html_root/fn.foo.html' +#[doc(no_inline)] +pub use html_root::foo; + #[doc(no_inline)] -pub use std::iter; +// `--extern-html-root-url` should apply if no `html_root_url` is given +// @has - '//a/@href' 'https://example.com/override/no_html_root/fn.bar.html' +pub use no_html_root::bar;