Skip to content

DocCommentOwner and symbols #186

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

Closed
kjeremy opened this issue Nov 1, 2018 · 7 comments
Closed

DocCommentOwner and symbols #186

kjeremy opened this issue Nov 1, 2018 · 7 comments

Comments

@kjeremy
Copy link
Contributor

kjeremy commented Nov 1, 2018

I'm sketching out some sort of hover support and as a first step was just going to display any documentation associated with a FileSymbol symbol. I was thinking of making an Option<string> on FileSymbol for documentation. FnDef is already a DocCommentOwner and I believe at least Module should be. Does it make sense to implement DocCommentOwner for the ones below too?

fn to_symbol(node: SyntaxNodeRef) -> Option<FileSymbol> {
    // ...
    visitor()
        .visit(decl::<ast::FnDef>)
        .visit(decl::<ast::StructDef>)
        .visit(decl::<ast::EnumDef>)
        .visit(decl::<ast::TraitDef>)
        .visit(decl::<ast::Module>)
        .visit(decl::<ast::TypeDef>)
        .visit(decl::<ast::ConstDef>)
        .visit(decl::<ast::StaticDef>)
        .accept(node)?
}
@matklad
Copy link
Member

matklad commented Nov 2, 2018

Does it make sense to implement DocCommentOwner for the ones below too?

Yep!

@matklad
Copy link
Member

matklad commented Nov 2, 2018

I was thinking of making an Option on FileSymbol for documentation.

SGTM, though ideally it would be better to load docstrings for FileSymbols on demand. I think LSP has some support for this even (the method was called resolveSymbol IIRC)

@kjeremy
Copy link
Contributor Author

kjeremy commented Nov 2, 2018

Actually should I just make a NameOwner a DocCommentOwner?

@matklad
Copy link
Member

matklad commented Nov 2, 2018

Probably not: pattern bindings (let x = 92) are NameOwners, but can't have docstrings.

@kjeremy
Copy link
Contributor Author

kjeremy commented Nov 2, 2018

Hm I don't see anything like resolveSymbol in the LSP that has anything to do with documentation. Maybe there should be something like symbol_documentation(FileId, FileSymbol) -> Cancelable<Option<String>> on analysis. An alternative would be instead of a FileSymbol it could take a TextUnit.

@kjeremy
Copy link
Contributor Author

kjeremy commented Nov 5, 2018

@matklad This is possibly the stupidest question ever but how the heck can I go from a SyntaxNodeRef to an ast::DocCommentOwner? Right now the best I can come up with is a bunch of find_node_at_offset<T> calls but I was hoping there would be a way to pass in ast::DocCommentsOwner.

@matklad
Copy link
Member

matklad commented Nov 5, 2018

@kjeremy there's no easy way to do that, unfortunately. You basically have to check manually for each concrete type which can be a DocCommentOwner. I think we can generate some helper to do that, but the API wouldn't be too nice. See this code which deals with a similar problem for name owners:

https://github.com/rust-analyzer/rust-analyzer/blob/43665eb166e1bd0319a1e13a97b753a536e4b4d2/crates/ra_editor/src/symbols.rs#L74-L94

bors bot added a commit that referenced this issue Nov 7, 2018
196: Hover: Show documentation r=matklad a=kjeremy

Closes #186 

Co-authored-by: Jeremy A. Kolb <[email protected]>
bors bot added a commit that referenced this issue Nov 7, 2018
196: Hover: Show documentation r=matklad a=kjeremy

Closes #186 

Co-authored-by: Jeremy A. Kolb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants