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

Add a check for swapped words when we can't find an identifier #67849

Merged
merged 3 commits into from
Jan 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions src/libsyntax/util/lev_distance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,16 @@ where
T: Iterator<Item = &'a Symbol>,
{
let max_dist = dist.map_or_else(|| cmp::max(lookup.len(), 3) / 3, |d| d);
let name_vec: Vec<&Symbol> = iter_names.collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to avoid this collect by cloning the iterator instead and passing that to find_match_by_sorted_words, if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm not sure how to do that. If we call cloned(), iter_names is moved and we can't use that iterator again, and we still can't use the cloned one twice. Is there another way to clone it so that we can still use the original iter_names afterwards?


let (case_insensitive_match, levenstein_match) = iter_names
let (case_insensitive_match, levenshtein_match) = name_vec
.iter()
.filter_map(|&name| {
let dist = lev_distance(lookup, &name.as_str());
if dist <= max_dist { Some((name, dist)) } else { None }
})
// Here we are collecting the next structure:
// (case_insensitive_match, (levenstein_match, levenstein_distance))
// (case_insensitive_match, (levenshtein_match, levenshtein_distance))
.fold((None, None), |result, (candidate, dist)| {
(
if candidate.as_str().to_uppercase() == lookup.to_uppercase() {
Expand All @@ -73,10 +75,31 @@ where
},
)
});

// Priority of matches:
// 1. Exact case insensitive match
// 2. Levenshtein distance match
// 3. Sorted word match
if let Some(candidate) = case_insensitive_match {
Some(candidate) // exact case insensitive match has a higher priority
Some(*candidate)
} else if levenshtein_match.is_some() {
levenshtein_match.map(|(candidate, _)| *candidate)
} else {
levenstein_match.map(|(candidate, _)| candidate)
find_match_by_sorted_words(name_vec, lookup)
}
}

fn find_match_by_sorted_words<'a>(iter_names: Vec<&'a Symbol>, lookup: &str) -> Option<Symbol> {
iter_names.iter().fold(None, |result, candidate| {
if sort_by_words(&candidate.as_str()) == sort_by_words(lookup) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you're only doing comparison on the sorted elements...

Some(**candidate)
} else {
result
}
})
}

fn sort_by_words(name: &str) -> String {
let mut split_words: Vec<&str> = name.split('_').collect();
split_words.sort();
split_words.join("_")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...I wonder if it wouldn't be more efficient to return a Vec<&str> here.

That being said, this is only done in a case where we're already emitting an error, so it should be fine.

}
6 changes: 6 additions & 0 deletions src/libsyntax/util/lev_distance/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,11 @@ fn test_find_best_match_for_name() {
find_best_match_for_name(input.iter(), "aaaa", Some(4)),
Some(Symbol::intern("AAAA"))
);

let input = vec![Symbol::intern("a_longer_variable_name")];
assert_eq!(
find_best_match_for_name(input.iter(), "a_variable_longer_name", None),
Some(Symbol::intern("a_longer_variable_name"))
);
})
}
4 changes: 4 additions & 0 deletions src/test/ui/suggestions/issue-66968-suggest-sorted-words.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
fn main() {
let a_longer_variable_name = 1;
println!("{}", a_variable_longer_name); //~ ERROR E0425
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
error[E0425]: cannot find value `a_variable_longer_name` in this scope
--> $DIR/issue-66968-suggest-sorted-words.rs:3:20
|
LL | println!("{}", a_variable_longer_name);
| ^^^^^^^^^^^^^^^^^^^^^^ help: a local variable with a similar name exists: `a_longer_variable_name`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0425`.