Resolve links in hover documentation - #4873
Conversation
9b06786 to
ff60cda
Compare
| ModuleDef::Static(s) => Into::<StaticId>::into(s.clone()).resolver(db), | ||
| ModuleDef::Trait(t) => Into::<TraitId>::into(t.clone()).resolver(db), | ||
| ModuleDef::TypeAlias(t) => Into::<ModuleId>::into(t.module(db)).resolver(db), | ||
| // FIXME: This should be a resolver relative to `std/core` |
There was a problem hiding this comment.
Is there an easy way to get the std Module?
ea89336 to
6b9bd86
Compare
6b9bd86 to
48d858f
Compare
c89e617 to
d6f9e88
Compare
matklad
left a comment
matklad
left a comment
There was a problem hiding this comment.
Left a review.
I think the core interface is sound, but the implementation does a really uncomfortable amount of stuff. Luckely, it seems most of this is not required :)
| let namespace = Namespace::from_intra_spec(link_target); | ||
|
|
||
| // Strip prefixes/suffixes | ||
| let link_target = strip_prefixes_suffixes(link_target); |
There was a problem hiding this comment.
Seems like this could use some types?
struct DocLink {
path: &str,
namespace: Option<Namespace>,
}
impl FromStr for DocLink { ... }
There was a problem hiding this comment.
I've debated this for a little bit and I'm not sure I think it's worth the mental overhead of another type for the increased type safety.
More than happy to defer to your judgement here if you still think it's a good idea.
There was a problem hiding this comment.
I always err on the side of adding types, on the grounds that removing type later is easier than adding it.
df618c0 to
b34ad4b
Compare
| use hir_def::{db::DefDatabase, resolver::Resolver}; | ||
| use syntax::ast::Path; | ||
|
|
||
| pub fn resolve_doc_link<T: Resolvable + Clone, D: DefDatabase + HirDatabase>( |
There was a problem hiding this comment.
Can we make this a method of Docs trait?
There was a problem hiding this comment.
The docs trait is implemented for T: Into<AttrDef> which is a different set of types from those that implement Resolvable.
Conceptually they're not really the same thing. Resolvable really just means that we can find a base which we can then use to resolve documentation links. There isn't a strict requirement that something implementing Resolvable has, or could have, documentation.
| /// Try to resolve path to local documentation via intra-doc-links (i.e. `super::gateway::Shard`). | ||
| /// | ||
| /// See [RFC1946](https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md). | ||
| fn try_resolve_intra<T: Resolvable, D: DefDatabase + HirDatabase>( |
There was a problem hiding this comment.
It would be better if we can make this on-generic:
https://github.com/rust-analyzer/rust-analyzer/pull/5853/files
There was a problem hiding this comment.
Agreed, but I'm not sure it's easy/possible with the DefDatabase + HirDatabase bound (which also exists in Resolvable).
I know you can declare a trait:
trait DB: DefDatabase + HirDatabase {}And then create &dyn DB objects, but I don't think there's an easy way to get &dyn DefDatabase/&dyn HirDatabase out of those?
|
bors r+ Ok, this looks good now! |
|
I'll look into reshufling APIs here later today |
3 participants
This PR resolves links in hover documentation. Both the upcoming intra-doc-links style and the old "path-based" style.
Todo
Resolve issues with the markdown parser/get rid of itMigrate topulldown_cmark_to_cmarkFixes #503