Skip to content

Support multiple crate versions in --extern-html-root-url #143465

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion compiler/rustc_metadata/src/creader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::owned_slice::OwnedSlice;
use rustc_data_structures::svh::Svh;
use rustc_data_structures::sync::{self, FreezeReadGuard, FreezeWriteGuard};
use rustc_data_structures::unord::UnordMap;
use rustc_errors::DiagCtxtHandle;
use rustc_expand::base::SyntaxExtension;
use rustc_fs_util::try_canonicalize;
Expand Down Expand Up @@ -68,6 +69,9 @@ pub struct CStore {
/// This crate has a `#[alloc_error_handler]` item.
has_alloc_error_handler: bool,

/// Names that were used to load the crates via `extern crate` or paths.
resolved_externs: UnordMap<Symbol, CrateNum>,

/// Unused externs of the crate
unused_externs: Vec<Symbol>,
}
Expand Down Expand Up @@ -268,6 +272,22 @@ impl CStore {
self.metas[cnum] = Some(Box::new(data));
}

/// Save the name used to resolve the extern crate in the local crate
///
/// The name isn't always the crate's own name, because `sess.opts.externs` can assign it another name.
/// It's also not always the same as the `DefId`'s symbol due to renames `extern crate resolved_name as defid_name`.
pub(crate) fn set_resolved_extern_crate_name(&mut self, name: Symbol, extern_crate: CrateNum) {
self.resolved_externs.insert(name, extern_crate);
}

/// Crate resolved and loaded via the given extern name
/// (corresponds to names in `sess.opts.externs`)
///
/// May be `None` if the crate wasn't used
pub fn resolved_extern_crate(&self, externs_name: Symbol) -> Option<CrateNum> {
self.resolved_externs.get(&externs_name).copied()
}

pub(crate) fn iter_crate_data(&self) -> impl Iterator<Item = (CrateNum, &CrateMetadata)> {
self.metas
.iter_enumerated()
Expand Down Expand Up @@ -494,6 +514,7 @@ impl CStore {
alloc_error_handler_kind: None,
has_global_allocator: false,
has_alloc_error_handler: false,
resolved_externs: UnordMap::default(),
unused_externs: Vec::new(),
}
}
Expand Down Expand Up @@ -533,7 +554,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
// We're also sure to compare *paths*, not actual byte slices. The
// `source` stores paths which are normalized which may be different
// from the strings on the command line.
let source = self.cstore.get_crate_data(cnum).cdata.source();
let source = data.source();
if let Some(entry) = self.sess.opts.externs.get(name.as_str()) {
// Only use `--extern crate_name=path` here, not `--extern crate_name`.
if let Some(mut files) = entry.files() {
Expand Down Expand Up @@ -1302,6 +1323,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {
let path_len = definitions.def_path(def_id).data.len();
self.cstore.update_extern_crate(
cnum,
name,
ExternCrate {
src: ExternCrateSource::Extern(def_id.to_def_id()),
span: item.span,
Expand All @@ -1320,6 +1342,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> {

self.cstore.update_extern_crate(
cnum,
name,
ExternCrate {
src: ExternCrateSource::Path,
span,
Expand Down
19 changes: 17 additions & 2 deletions compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,14 +630,29 @@ impl CStore {
}
}

pub(crate) fn update_extern_crate(&mut self, cnum: CrateNum, extern_crate: ExternCrate) {
/// Track how an extern crate has been loaded
///
/// * the `name` is for [`Self::set_resolved_extern_crate_name`]
/// * `extern_crate` is for diagnostics
pub(crate) fn update_extern_crate(
&mut self,
cnum: CrateNum,
name: Symbol,
extern_crate: ExternCrate,
) {
debug_assert_eq!(extern_crate.dependency_of, LOCAL_CRATE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
debug_assert_eq!(extern_crate.dependency_of, LOCAL_CRATE);
// this function should not be called on transitive dependencies
debug_assert_eq!(extern_crate.dependency_of, LOCAL_CRATE);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it to the assert's message

self.set_resolved_extern_crate_name(name, cnum);
self.update_extern_crate_inner(cnum, extern_crate);
}

fn update_extern_crate_inner(&mut self, cnum: CrateNum, extern_crate: ExternCrate) {
let cmeta = self.get_crate_data_mut(cnum);
if cmeta.update_extern_crate(extern_crate) {
// Propagate the extern crate info to dependencies if it was updated.
let extern_crate = ExternCrate { dependency_of: cnum, ..extern_crate };
let dependencies = mem::take(&mut cmeta.dependencies);
for &dep_cnum in &dependencies {
self.update_extern_crate(dep_cnum, extern_crate);
self.update_extern_crate_inner(dep_cnum, extern_crate);
}
self.get_crate_data_mut(cnum).dependencies = dependencies;
}
Expand Down
5 changes: 5 additions & 0 deletions src/doc/rustdoc/src/unstable-features.md
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,11 @@ flags to control that behavior. When the `--extern-html-root-url` flag is given
one of your dependencies, rustdoc use that URL for those docs. Keep in mind that if those docs exist
in the output directory, those local docs will still override this flag.

Crate names in this flag refer to the names in the [extern prelude], and support crates renamed
via the `--extern` `rustc` flag.
Comment on lines +398 to +399
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a few improvements that could be made here:

  1. the original crate name is used as a fallback if there is no match in the extern prelude
  2. the fact that renaming crates via --extern logically follows from the fact we are using the extern prelude
  3. renaming via extern crate as is also supported.
  4. note that cargo's package renaming works via --extern and is thus supported.

I think this should be split into at least two sentences, one tersely and precisely describing the behavior (extern prelude with fallback), and another describing the consequence of this behavior (e.g. support for various methods of renaming, backwards compatibility)


[extern prelude]: https://doc.rust-lang.org/reference/names/preludes.html#extern-prelude

## `-Z force-unstable-if-unmarked`

Using this flag looks like this:
Expand Down
26 changes: 20 additions & 6 deletions src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::mem;
use rustc_attr_data_structures::StabilityLevel;
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap, FxIndexSet};
use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, DefIdSet};
use rustc_metadata::creader::CStore;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::Symbol;
use tracing::debug;
Expand Down Expand Up @@ -158,18 +159,31 @@ impl Cache {
assert!(cx.external_traits.is_empty());
cx.cache.traits = mem::take(&mut krate.external_traits);

let render_options = &cx.render_options;
let extern_url_takes_precedence = render_options.extern_html_root_takes_precedence;
let dst = &render_options.output;

// Make `--extern-html-root-url` support the same names as `--extern` whenever possible
let cstore = CStore::from_tcx(tcx);
for (name, extern_url) in &render_options.extern_html_root_urls {
if let Some(crate_num) = cstore.resolved_extern_crate(Symbol::intern(name)) {
let e = ExternalCrate { crate_num };
let location = e.location(Some(extern_url), extern_url_takes_precedence, dst, tcx);
cx.cache.extern_locations.insert(e.crate_num, location);
}
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the doc url is emitted in the json, doesn't that make this fixme comment wrong? if so, i would replace it with a note saying that this needs to be common because it is actually emitted by both backends.

for &crate_num in tcx.crates(()) {
let e = ExternalCrate { crate_num };

let name = e.name(tcx);
let render_options = &cx.render_options;
let extern_url = render_options.extern_html_root_urls.get(name.as_str()).map(|u| &**u);
let extern_url_takes_precedence = render_options.extern_html_root_takes_precedence;
let dst = &render_options.output;
let location = e.location(extern_url, extern_url_takes_precedence, dst, tcx);
cx.cache.extern_locations.insert(e.crate_num, location);
cx.cache.extern_locations.entry(e.crate_num).or_insert_with(|| {
let extern_url =
render_options.extern_html_root_urls.get(name.as_str()).map(|u| &**u);
e.location(extern_url, extern_url_takes_precedence, dst, tcx)
});
cx.cache.external_paths.insert(e.def_id(), (vec![name], ItemType::Module));
}

Expand Down
8 changes: 8 additions & 0 deletions tests/rustdoc/extern/extern-html-alias.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//@ compile-flags:-Z unstable-options --extern-html-root-url extern_prelude_name=https://renamed.example.com --extern-html-root-url empty=https://bad.invalid
//@ aux-crate:extern_prelude_name=empty.rs

extern crate extern_prelude_name as internal_ident_name;

//@ has extern_html_alias/index.html
//@ has - '//a/@href' 'https://renamed.example.com/empty/index.html'
pub use internal_ident_name as yet_different_name;
Comment on lines +2 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. internal_ident_name is a misleading name, as it actually does exist in extern prelude as an alias (i.e. ::internal_ident_name would be valid here, in this case both exist in the extern prelude)
  2. we should test that extern crate as aliases are handled correctly
  3. we should test the fallback behavior (using crate name)

Doing all this would probably require at least a second aux-crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolver.extern_prelude confused me, because it contains only core, std, and extern_prelude_name, and not internal_ident_name.

I've renamed things to avoid referring to the extern prelude.

Loading