rustdoc: Unify external_paths and paths cache map #86909
Conversation
rust-highfive
added
the
S-waiting-on-review
Stupremee
added
C-cleanup
This comment has been minimized.
This comment has been minimized.
The `formats::cache::Cache` fields `paths` and `external_paths` are now unified to a single `paths` map, which makes it easier to manage the paths and avoids using local paths as extern and vice versa. The `paths` map maps a `DefId` to an enum which can be either `Local` or `Extern` so there can't be any misusing.
Stupremee
force-pushed
the
unify-external_paths-and-paths
branch
from
0880e66 to
3fe671d
Compare
| }; | ||
|
|
||
| if did.is_local() { | ||
| cx.cache.exact_paths.insert(did, fqn); |
There was a problem hiding this comment.
Should we merge the exact_paths too?
There was a problem hiding this comment.
I don't really understand the point of this change. It's good that there's one map instead of two I guess, but that comes with some churn (I left comments about behavior that changed by accident) and it doesn't solve the underlying issue that Local is completely wrong: there are non-local DefIds in paths and local DefIds in extern_paths. Any bugs from #86798 are still present, they're just harder to see because you call things "Local" when they aren't.
| /// that node. This map contains local and external cached paths that are differentiated using | ||
| /// the [`CachedPath`] enum. | ||
| /// | ||
| /// This was previously known as `extern_paths` and `paths`. |
There was a problem hiding this comment.
I'd remove this comment, people can find it in the git history if they really care.
| /// This was previously known as `extern_paths` and `paths`. |
| path = if it.def_id.is_local() { | ||
| cx.current.join("/") | ||
| } else { | ||
| let (ref path, _) = cx.cache.external_paths[&it.def_id.expect_def_id()]; | ||
| path[..path.len() - 1].join("/") | ||
| path = match cx.cache.paths[&it.def_id.expect_def_id()] { | ||
| CachedPath::Extern(_, _) => cx.current.join("/"), | ||
| CachedPath::Local(ref path, _) => path[..path.len() - 1].join("/"), |
There was a problem hiding this comment.
This logic doesn't seem the same at all. What does Extern have to do with item.def_id.is_local()? Why do you use the code for Local that used to be used for external_paths?
| // documented locally though we always create the file to avoid dead | ||
| // links. | ||
| if implementors.is_empty() && !cx.cache.paths.contains_key(&did) { | ||
| if implementors.is_empty() && cx.cache.paths.get(&did).map_or(false, CachedPath::is_extern) |
There was a problem hiding this comment.
This is not the same logic.
| if implementors.is_empty() && cx.cache.paths.get(&did).map_or(false, CachedPath::is_extern) | |
| if implementors.is_empty() && cx.cache.paths.get(&did).map_or(true, CachedPath::is_extern) |
| CachedPath::Local(a, _) | CachedPath::Extern(a, _) => a, | ||
| }) | ||
| .0 | ||
| .unwrap() |
There was a problem hiding this comment.
| .unwrap() | |
| .expect("Trait should either be in local or external paths") |
|
Ohh I see. Yes that's true and I completely forgot that. So what you're saying is that the paths map should check any DefId and path if they match, so that local DefIds only point to |
|
Yes. In fact I would remove The problem comes that this is a change in behavior, and I think will cause some tests to fail :/ I don't know how to fix that; the current behavior has no rhyme nor reason, but rustdoc is depending on it anyway. But if you change it and the tests are easy to fix (🤞), then we don't have to worry about it. |
|
Now I understand. I was too fixed on unifying the paths maps and forgot about the original issue. 😅 |
|
I'm going to close this for now - if you still think this is a good change on its own, feel free to reopen. |
5 participants
The
formats::cache::Cachefieldspathsandexternal_pathsare nowunified to a single
pathsmap, which makes it easier to manage thepaths and avoids using local paths as extern and vice versa.
The
pathsmap maps aDefIdto an enum which can be eitherLocalorExternso there can't be any misusing.Resolves #86798
r? @jyn514