Skip to content

rustdoc: don't crash when an external trait's docs needs to import another trait - #48415

Merged
bors merged 4 commits into
rust-lang:masterfrom
QuietMisdreavus:traits-on-traits-on-traits
Feb 25, 2018
Merged

rustdoc: don't crash when an external trait's docs needs to import another trait#48415
bors merged 4 commits into
rust-lang:masterfrom
QuietMisdreavus:traits-on-traits-on-traits

Conversation

@QuietMisdreavus

Copy link
Copy Markdown
Contributor

Fixes#48414

When resolving intra-paths for an item, rustdoc needs to have information about their items on hand, for proper bookkeeping. When loading a path for an external item, it needs to load these items from their host crate, since their information isn't otherwise available. This includes resolving paths for those docs. which can cause this process to recurse. Rustdoc keeps a map of external traits in a RefCell<HashMap<DefId, Trait>>, and it keeps a borrow of this active when importing an external trait. In the linked crash, this led to a RefCell borrow error, panic, and ICE.

This PR manually releases the borrow while importing the trait, and also keeps a list of traits being imported at the given moment. The latter keeps rustdoc from infinitely recursing as it tries to import the same trait repeatedly.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @GuillaumeGomez

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2018
@Manishearth

Copy link
Copy Markdown
Member

r=me but needs a test

@Manishearth

Copy link
Copy Markdown
Member

The test I had was

pubtraitFoo{/// [x](::Bar)fnfoo();}pubtraitBar{/// [x](::Foo)fnfoo();}

and then

pubuse aux::Foo;pubuse aux::Bar;

@Manishearth

Copy link
Copy Markdown
Member

I have a test at https://github.com/manishearth/rust/tree/rustdoc-trait-ice , but haven't run it yet

@QuietMisdreavus

Copy link
Copy Markdown
ContributorAuthor

The test i wrote up was:

/// Woah, this trait links to [OtherTrait](OtherTrait)!pubtraitSomeTrait{}/// Woah, this trait links to [SomeTrait](SomeTrait)!pubtraitOtherTrait{}
#[doc(inline)]pubuse issue_48414::{SomeTrait,OtherTrait};

...which still stack-overflows. >_> Need to investigate.

@Manishearth

Copy link
Copy Markdown
Member

wooooooooooo

@QuietMisdreavus

Copy link
Copy Markdown
ContributorAuthor

It turns out, i wrote a && when i needed to have written a ||. 😓

@Manishearth

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Feb 22, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 8872e7b has been approved by Manishearth

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2018
…-traits, r=Manishearth
rustdoc: don't crash when an external trait's docs needs to import another trait
Fixesrust-lang#48414
When resolving intra-paths for an item, rustdoc needs to have information about their items on hand, for proper bookkeeping. When loading a path for an external item, it needs to load these items from their host crate, since their information isn't otherwise available. This includes resolving paths for those docs. which can cause this process to recurse. Rustdoc keeps a map of external traits in a `RefCell<HashMap<DefId, Trait>>`, and it keeps a borrow of this active when importing an external trait. In the linked crash, this led to a RefCell borrow error, panic, and ICE.
This PR manually releases the borrow while importing the trait, and also keeps a list of traits being imported at the given moment. The latter keeps rustdoc from infinitely recursing as it tries to import the same trait repeatedly.
bors added a commit that referenced this pull request Feb 25, 2018
@bors
bors merged commit 8872e7b into rust-lang:masterFeb 25, 2018
@QuietMisdreavus
QuietMisdreavus deleted the traits-on-traits-on-traits branch February 26, 2018 23:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@QuietMisdreavus@rust-highfive@Manishearth@bors@GuillaumeGomez