Refactor librustdoc html backend #73767
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
GuillaumeGomez
requested changes
Jun 26, 2020
src/librustdoc/html/render.rs
Outdated
| @@ -112,6 +113,9 @@ struct Context { | |||
| id_map: Rc<RefCell<IdMap>>, | |||
| pub shared: Arc<SharedContext>, | |||
| pub cache: Arc<Cache>, | |||
| pub parent: Rc<RefCell<Renderer>>, | |||
There was a problem hiding this comment.
Just a note (for myself and any other potential reviewer): those 3 variables were before in the run function which became split in the FormatRenderer trait.
src/librustdoc/formats/mod.rs
Outdated
| } | ||
|
|
||
| renderer.after_krate(&krate)?; | ||
| renderer.after_run(diag) |
There was a problem hiding this comment.
Note: this allows to check if any errors has occurred during the run, which allows to make the code inside after_krate a bit lighter.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
betamos
reviewed
Jun 26, 2020
tmandry
reviewed
Jun 26, 2020
|
☔ The latest upstream changes (presumably #73779) made this pull request unmergeable. Please resolve the merge conflicts. |
jyn514
reviewed
Jun 27, 2020
P1n3appl3
force-pushed
the
rustdoc-formats
branch
from
84ecd62 to
986f9a0
Compare
|
@jyn514 I couldn't find those PRs, do you mean they'll conflict because of moving |
GuillaumeGomez
approved these changes
Jul 6, 2020
There was a problem hiding this comment.
Looks good to me! Considering this is a huge PR (even if mainly doing code moves), I'll wait for at least one more review from someone on the @rust-lang/rustdoc team (ping @ollie27 )
|
☔ The latest upstream changes (presumably #74117) made this pull request unmergeable. Please resolve the merge conflicts. |
P1n3appl3
force-pushed
the
rustdoc-formats
branch
from
986f9a0 to
ad31823
Compare
|
@ollie27 Friendly ping, would you be able to review this? |
|
☔ The latest upstream changes (presumably #74330) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Perhaps another person on @rust-lang/rustdoc can give this a once-over. I nominate... r? @Manishearth |
|
@jyn514 wanna review this? |
P1n3appl3
force-pushed
the
rustdoc-formats
branch
from
ad31823 to
4c4f300
Compare
|
☔ The latest upstream changes (presumably #74710) made this pull request unmergeable. Please resolve the merge conflicts. |
Manishearth
approved these changes
Jul 24, 2020
jyn514
reviewed
Jul 24, 2020
| } | ||
| } | ||
|
|
||
| impl PathError for Error { |
There was a problem hiding this comment.
Pre-existing, but do you know why rustdoc uses a separate error type and trait here? It has wrappers around both io::Error and std::error::Error.
| } | ||
|
|
||
| impl PathError for Error { | ||
| fn new<S, P: AsRef<Path>>(e: S, path: P) -> Error |
There was a problem hiding this comment.
Pre-existing, but this would be better taking P: Into<PathBuf> instead which avoids an unnecessary allocation if passing a PathBuf.
Comment on lines
+42
to
+47
| /// Maintains a mapping of local crate `DefId`s to the fully qualified name | ||
| /// and "short type description" of that node. This is used when generating | ||
| /// URLs when a type is being linked to. External paths are not located in | ||
| /// this map because the `External` type itself has all the information | ||
| /// necessary. | ||
| pub paths: FxHashMap<DefId, (Vec<String>, ItemType)>, |
There was a problem hiding this comment.
Pre-existing, but would it be better to use LocalDefId if these are only for the current crate?
There was a problem hiding this comment.
Probably, although it's kind of convenient that all the mappings in Cache use a full DefId. Otherwise I think you'd need to add checks that a given item is local before pulling out it's LocalDefId and looking it up in those maps.
Comment on lines
+34
to
+40
| /// Maps a type ID to all known implementations for that type. This is only | ||
| /// recognized for intra-crate `ResolvedPath` types, and is used to print | ||
| /// out extra documentation on the page of an enum/struct. | ||
| /// | ||
| /// The values of the map are a list of implementations and documentation | ||
| /// found on that implementation. | ||
| pub impls: FxHashMap<DefId, Vec<Impl>>, |
There was a problem hiding this comment.
Do you know if this includes types from other crates as well?
There was a problem hiding this comment.
Nope, don't see std or other types in there unless i'm reexporting them. It does have the numeric primitives + char and bool but I assume that's either from the prelude or just because they're special.
There was a problem hiding this comment.
Got it. In general, if it doesn't have types from other crates it should use LocalDefId but that can be delayed to a later PR.
| parent: Option<DefId>, | ||
| parent_idx: Option<usize>, | ||
| search_type: Option<IndexItemFunctionType>, | ||
| pub struct IndexItem { |
There was a problem hiding this comment.
If you make this fully pub, rustc won't warn about dead code.
Suggested change
| pub struct IndexItem { | |
| pub(crate) struct IndexItem { |
There was a problem hiding this comment.
Can't restrict it since it's exposed through Cache
| stripped_mod: bool, | ||
| masked_crates: FxHashSet<CrateNum>, | ||
|
|
||
| pub search_index: Vec<IndexItem>, |
There was a problem hiding this comment.
It looks like there is both a formats::cache and a html::render::cache. Maybe this belongs with render? I don't see why backends other than HTML would need a JS search index.
There was a problem hiding this comment.
Yeah it would definitely be better to have search_index in html::render::SharedContext but it requires a bit of work to separate out the logic to construct the index vs the cache because they're currently using the same DocFolder pass.
| @@ -361,7 +280,7 @@ impl Serialize for Generic { | |||
|
|
|||
| /// Full type of functions/methods in the search index. | |||
| #[derive(Debug)] | |||
| struct IndexItemFunctionType { | |||
| pub struct IndexItemFunctionType { | |||
There was a problem hiding this comment.
Suggested change
| pub struct IndexItemFunctionType { | |
| pub(crate) struct IndexItemFunctionType { |
There was a problem hiding this comment.
This one gets exposed by the cache so it's got to stay public (until we can move search index related stuff to html::render::SharedContext)
ollie27
added
the
T-rustdoc
jyn514
added
the
C-cleanup
|
📌 Commit cee8023 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit cee8023 with merge f1be9a90a04ae76a2798a3d0999fa651100289ce... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
A bajillion test failures of the form: |
tmandry
reviewed
Jul 29, 2020
|
@bors r+ |
|
📌 Commit 29df050 has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup=iffy This will cause lots of conflicts if it tries to merge at the same time as another rustdoc PR. |
|
@jyn514 rollups already ignore PRs that have merge conflicts when creating them |
|
☀️ Test successful - checks-actions, checks-azure |
This was referenced Jul 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves several types out of the librustdoc::html module so that they can be used by a future json backend. These changes are a re-implementation of some work done 6 months ago by @GuillaumeGomez. I'm currently working on said json backend and will put up an RFC soon with the proposed implementation.
There are a couple of changes that are more substantial than relocating structs to a different module:
Cacheis no longer part of thehtml::render::Contexttype and therefor it needs to be explicitly passed to any functions that access it.html::render::runhas been rewritten to use theFormatRenderertrait which should allow different backends to re-use the driving code.r? @GuillaumeGomez
cc @tmandry @betamos