Skip to content
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

Find usages #1892

Merged
merged 19 commits into from Oct 23, 2019
Merged

Find usages #1892

merged 19 commits into from Oct 23, 2019

Conversation

@viorina
Copy link
Contributor

viorina commented Sep 21, 2019

Fixes #1622.

@@ -20,14 +20,14 @@ pub(crate) fn classify_name_ref(
db: &RootDatabase,
analyzer: &hir::SourceAnalyzer,
name_ref: &ast::NameRef,
) -> Option<NameRefKind> {
use NameRefKind::*;
) -> Option<NameKind> {

This comment has been minimized.

Copy link
@matklad

matklad Sep 21, 2019

Member

I wonder if we should just call this a Declaration and implement various presentation-like functions on this type? Definitely out of scope for this PR

@viorina viorina force-pushed the viorina:find-usages branch 2 times, most recently from 64c5eb0 to d0c1ea6 Oct 3, 2019
}

match src.ast {
ast::ModuleItem::FnDef(f) => def!(Function, f),

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member

I think it's better to delegate to Function::from_source here, rather than re-implement

AssocItem::TypeAlias(t) => t.module(db),
}
}
}

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member

It might makes sense to land these hir and from_source additions separately, before the bulk of find usages

This comment has been minimized.

Copy link
@viorina

viorina Oct 9, 2019

Author Contributor

Ok, but I'm not sure about from_source additions yet, so maybe I'll send them later.

}

match src.ast {
ast::ImplItem::FnDef(f) => def!(Function, f),

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member

I think it's better to delegate to Function::from_source here, rather than re-implement

@@ -58,12 +58,11 @@ pub(crate) fn reference_definition(
use self::ReferenceResult::*;

let analyzer = hir::SourceAnalyzer::new(db, file_id, name_ref.syntax(), None);

match classify_name_ref(db, &analyzer, name_ref) {
let name_kind = classify_name_ref(db, file_id, &analyzer, &name_ref).and_then(|d| Some(d.item));

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member
Suggested change
let name_kind = classify_name_ref(db, file_id, &analyzer, &name_ref).and_then(|d| Some(d.item));
let name_kind = classify_name_ref(db, file_id, &analyzer, &name_ref).map(|d| d.item);
@@ -0,0 +1,316 @@
//! FIXME: write short doc here

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member

might be a good occasion to actually write the docs.

$( if let Some($it) = ast::$ast::cast($node.clone()) $res else )*
{ $catch_all }
}};
}

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member

Sweet! Could you add this to ra_syntax, and remove visitors infra in a separate PR? this will close #1672

This comment has been minimized.

Copy link
@matklad

matklad Oct 4, 2019

Member

Oh, and I think it's more flexible to use :expr instead of :ident for node, but care must be taken to evaluate the expression only once

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Oct 4, 2019

Contributor

I think :expr is not allowd because of the following {.

This comment has been minimized.

Copy link
@viorina

viorina Oct 5, 2019

Author Contributor

Yeah, that's right: { can't follow :expr.
There's already an open PR for the issue, that's why I didn't add it separately.

This comment has been minimized.

Copy link
@matklad

matklad Oct 5, 2019

Member

right, it's fine to stick with $ident then I think. That PR seems stalled, so it's fine to open a new one

@viorina viorina force-pushed the viorina:find-usages branch 2 times, most recently from 2168f93 to b5b6fb2 Oct 11, 2019
for (file_id, text_range) in scope {
let text = db.file_text(file_id);
let parse = SourceFile::parse(&text);
let syntax = parse.tree().syntax().clone();

This comment has been minimized.

Copy link
@matklad

matklad Oct 12, 2019

Member

It's better to parse file only when we found a hit. Most files will not contain hits, and we can save time by not parsing them! This actually might be a good use-case for once_cell::unsync::Lazy

use super::{NameDefinition, NameKind};

pub(crate) struct SearchScope {
pub scope: Vec<(FileId, Option<TextRange>)>,

This comment has been minimized.

Copy link
@matklad

matklad Oct 14, 2019

Member

I've found that making non-trivial fields public at the start with "nah, I'll refactor later" leads to rather painful refactorings. Let's expose this as an impl Iterator<>

@viorina viorina force-pushed the viorina:find-usages branch 2 times, most recently from d274316 to 7be2d94 Oct 14, 2019
@@ -181,7 +181,6 @@ impl Module {
) -> Option<Self> {
let decl_id = match src.ast {
ModuleSource::Module(ref module) => {
assert!(!module.has_semi());

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

Why is this required? If module syntax has a ;, it's a declaration, and not a definition

@@ -296,7 +294,7 @@ mod tests {

#[test]
fn goto_definition_works_for_macros_from_other_crates() {
covers!(goto_definition_works_for_macros);
// covers!(goto_definition_works_for_macros);

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

We should either restore these marks, or remove them if they are not relevant anymore.

@@ -0,0 +1,175 @@
//! FIXME: write short doc here

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

might be a good idea to actually write a short doc here :)

@@ -1,13 +1,20 @@
//! FIXME: write short doc here

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

might be a good idea to write a short paragraph explaining how classification, text search and conformation steps work together

}

Some(RangeInfo::new(range, SourceChange::source_file_edits("rename", edit)))
return refs;

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member
Suggested change
return refs;
refs
Copy link
Member

matklad left a comment

Finished the review. Overall looks good to me, but test marks should be fixed/removed, the assert in ModuleSource resotred, and, preferably, the docs should be added!


let is_match = |file_id: FileId, name_ref: &ast::NameRef| -> bool {
let classified = classify_name_ref(db, file_id, &name_ref);
if let Some(d) = classified {

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

this might be slightly better if written as match with a guard

refs.push(FileRange { file_id, range });
}
} else if is_match(file_id, &name_ref) {
refs.push(FileRange { file_id, range });

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

this is a micro copy-paste. I think guarded continue would help here

if let Some(search_range) = search_range {
    if !range.is_subrange(&search_range) { continue; }
}
@@ -0,0 +1,110 @@
//! FIXME: write short doc here

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

A couple of sentences description would be helpful here

@@ -0,0 +1,99 @@
//! FIXME: write short doc here

use std::collections::HashSet;

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member
Suggested change
use std::collections::HashSet;
use rustc_hash::FxHashSet;

It's basically std::collections::HashSet, but with a faster hash function

ModuleSource::Module(m) => Some(m.syntax().text_range()),
ModuleSource::SourceFile(_) => None,
};
return [(file_id, range)].iter().cloned().collect();

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

seems like this return might be pulled up to if

ModuleSource::Module(m) => Some(m.syntax().text_range()),
ModuleSource::SourceFile(_) => None,
};
[(file_id, range)].iter().cloned().collect()

This comment has been minimized.

Copy link
@matklad

matklad Oct 21, 2019

Member

std::iter::Once, or just let mut res = FxHashMap::with_capcity(1) and insert manually

@viorina viorina changed the title [WIP] Find usages Find usages Oct 21, 2019
@viorina viorina force-pushed the viorina:find-usages branch from f6381cd to 76d6982 Oct 22, 2019
@viorina viorina force-pushed the viorina:find-usages branch from 76d6982 to a881ede Oct 22, 2019
@viorina viorina force-pushed the viorina:find-usages branch from a881ede to decfd28 Oct 22, 2019
@viorina

This comment has been minimized.

Copy link
Contributor Author

viorina commented Oct 22, 2019

Thanks, fixed it.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Oct 23, 2019

bors r+

🎉

Thanks @viorina , this is some awesome work!

bors bot added a commit that referenced this pull request Oct 23, 2019
Merge #1892
1892: Find usages r=matklad a=viorina

Fixes #1622.

Co-authored-by: Ekaterina Babshukova <ekaterina.babshukova@yandex.ru>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Oct 23, 2019

@bors bors bot merged commit decfd28 into rust-analyzer:master Oct 23, 2019
2 checks passed
2 checks passed
bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kjeremy

This comment has been minimized.

Copy link
Contributor

kjeremy commented Oct 23, 2019

This is AWESOME. As a test I renamed with_db and it renamed every usage and passed a cargo check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.