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: Don't try to load source files from external crates #70828

Merged
merged 1 commit into from
Apr 9, 2020
Merged
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
1 change: 1 addition & 0 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1937,6 +1937,7 @@ impl Clean<Span> for rustc_span::Span {
let hi = sm.lookup_char_pos(self.hi());
Span {
filename,
cnum: lo.file.cnum,
Copy link
Member

Choose a reason for hiding this comment

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

Heh this is really simple. Although, perhaps a better thing to do would be to keep lo.file? And remove the span_to_filename call, which is then wasteful.

Also cc @Aaron1011 I hope this will continue to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually what I originally tried but it ended up quite a bit more complicated than this. The main advantage to keeping the full SourceFile is that rustdoc won't have to load the source files from disk again. I plan to do that as a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

@eddyb: This looks good to me: were you thinking of imported spans/hygiene?

Copy link
Member

Choose a reason for hiding this comment

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

Mostly knowing the CrateNum a file came from.

loline: lo.line,
locol: lo.col.to_usize(),
hiline: hi.line,
Expand Down
4 changes: 3 additions & 1 deletion src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_ast::util::comments::strip_doc_comment_decoration;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_hir::def_id::{CrateNum, DefId};
use rustc_hir::def_id::{CrateNum, DefId, LOCAL_CRATE};
use rustc_hir::lang_items;
use rustc_hir::Mutability;
use rustc_index::vec::IndexVec;
Expand Down Expand Up @@ -1357,6 +1357,7 @@ pub enum VariantKind {
#[derive(Clone, Debug)]
pub struct Span {
pub filename: FileName,
pub cnum: CrateNum,
pub loline: usize,
pub locol: usize,
pub hiline: usize,
Expand All @@ -1368,6 +1369,7 @@ impl Span {
pub fn empty() -> Span {
Span {
filename: FileName::Anon(0),
cnum: LOCAL_CRATE,
loline: 0,
locol: 0,
hiline: 0,
Expand Down
6 changes: 3 additions & 3 deletions src/librustdoc/html/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ use rustc_data_structures::flock;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_feature::UnstableFeatures;
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::Mutability;
use rustc_middle::middle::privacy::AccessLevels;
use rustc_middle::middle::stability;
Expand Down Expand Up @@ -1623,14 +1623,14 @@ impl Context {
_ => return None,
};

let (krate, path) = if item.def_id.is_local() {
let (krate, path) = if item.source.cnum == LOCAL_CRATE {
if let Some(path) = self.shared.local_sources.get(file) {
(&self.shared.layout.krate, path)
} else {
return None;
}
} else {
let (krate, src_root) = match *self.cache.extern_locations.get(&item.def_id.krate)? {
let (krate, src_root) = match *self.cache.extern_locations.get(&item.source.cnum)? {
(ref name, ref src, Local) => (name, src),
(ref name, ref src, Remote(ref s)) => {
root = s.to_string();
Expand Down
5 changes: 3 additions & 2 deletions src/librustdoc/html/sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::html::format::Buffer;
use crate::html::highlight;
use crate::html::layout;
use crate::html::render::{Error, SharedContext, BASIC_KEYWORDS};
use rustc_hir::def_id::LOCAL_CRATE;
use rustc_span::source_map::FileName;
use std::ffi::OsStr;
use std::fs;
Expand Down Expand Up @@ -37,8 +38,8 @@ impl<'a> DocFolder for SourceCollector<'a> {
if self.scx.include_sources
// skip all synthetic "files"
&& item.source.filename.is_real()
// skip non-local items
&& item.def_id.is_local()
// skip non-local files
&& item.source.cnum == LOCAL_CRATE
{
// If it turns out that we couldn't read this file, then we probably
// can't read any of the files (generating html output from json or
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to close #70757 without fixing the thing where include_sources would be set to false? I guess it might be hard to actually support only being able to emit some of the files, and is much less relevant now.

Feel free to leave a FIXME comment and open an issue about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess we can leave #70757 open to track removing this error case.

Expand Down
15 changes: 15 additions & 0 deletions src/test/rustdoc/auxiliary/external-macro-src.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// compile-flags:--remap-path-prefix={{src-base}}=/does-not-exist
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Surprised {{src-base}} exists, based on the normalization rules I would've expected it to be $SRC_DIR.


#![doc(html_root_url = "https://example.com/")]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test that uses thread_local! from libstd and checks that the resulting docs have doc.rust-lang.org URLs? Since that was my smallest example for #70757 (but I guess I left it at the end of this comment: #70025 (comment)).

The libstd URLs should be already valid today, so it'd be a good test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've now added a test using thread_local!.


#[macro_export]
macro_rules! make_foo {
() => {
pub struct Foo;
impl Foo {
pub fn new() -> Foo {
Foo
}
}
}
}
15 changes: 15 additions & 0 deletions src/test/rustdoc/external-macro-src.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// aux-build:external-macro-src.rs
// ignore-tidy-linelength

#![crate_name = "foo"]

#[macro_use]
extern crate external_macro_src;

// @has foo/index.html '//a[@href="../src/foo/external-macro-src.rs.html#4-15"]' '[src]'

// @has foo/struct.Foo.html
// @has - '//a[@href="https://example.com/src/external_macro_src/external-macro-src.rs.html#8"]' '[src]'
// @has - '//a[@href="https://example.com/src/external_macro_src/external-macro-src.rs.html#9-13"]' '[src]'
// @has - '//a[@href="https://example.com/src/external_macro_src/external-macro-src.rs.html#10-12"]' '[src]'
make_foo!();
2 changes: 1 addition & 1 deletion src/test/rustdoc/issue-26606.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
extern crate issue_26606_macro;

// @has issue_26606/constant.FOO.html
// @has - '//a/@href' '../src/issue_26606/auxiliary/issue-26606-macro.rs.html#3'
// @has - '//a/@href' '../src/issue_26606_macro/issue-26606-macro.rs.html#3'
make_item!(FOO);
6 changes: 6 additions & 0 deletions src/test/rustdoc/thread-local-src.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#![crate_name = "foo"]

// @has foo/index.html '//a[@href="../src/foo/thread-local-src.rs.html#1-6"]' '[src]'

// @has foo/constant.FOO.html '//a/@href' 'https://doc.rust-lang.org/nightly/src/std/'
thread_local!(pub static FOO: bool = false);