Updating the "levenshtein-based" suggestions to include module imports... #30377
Conversation
wafflespeanut
changed the title
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
r? @Manishearth TODO: crates & inline paths (and tests should also be updated) |
|
Minor clarification: The |
wafflespeanut
force-pushed
the
levenshtein
branch
2 times, most recently
from
b42a8d2 to
a6263e4
Compare
| &source.as_str()); | ||
| if let Some((name, _)) = best_match { | ||
| lev_suggestion = | ||
| format!(". Did you mean to use the exported import `{}`?", |
There was a problem hiding this comment.
You know, the thing with pub use...
use food::bar;
mod baz {
pub struct foobar;
}
mod food {
pub use baz::foobar as baz;
}
There was a problem hiding this comment.
Maybe say "re-exported" instead for pub use ?
There was a problem hiding this comment.
Yeah, that should work :)
|
Please add tests for all the new suggestion cases (and list them in the PR too) |
|
@Manishearth you didn't address my question, btw? |
Switching to I'd be wary of coupling in the other change (making lev_distance private); there have been complaints recently about changes to libsyntax breaking plugins downstream, so until the compiler team firms firms up a policy regarding stability of libsyntax, I'd try to keep such so-called breaking changes isolated |
wafflespeanut
force-pushed
the
levenshtein
branch
2 times, most recently
from
6e27b49 to
8c68720
Compare
|
(I don't think anybody uses lev_distance outside of rustc, so I'm ambivalent on the private-ing) |
wafflespeanut
force-pushed
the
levenshtein
branch
from
8c68720 to
16ec960
Compare
| } | ||
| } | ||
| if let Some(n) = best { | ||
| let best_dist = Some(name.len()); |
There was a problem hiding this comment.
Not sure if this is okay, because our new function defines the maximum edit distance to be one-third of the characters in the given word. Here, the entire word's length (in bytes) is being used (seems rather rough to me). So, I've added an option for using custom edit distances. Check if this is okay...
There was a problem hiding this comment.
I'm fine with this. Be sure to update the doc comment explaining the third argument.
I don't think the best_dist variable is necessary here, just put the Some(name.len()) inline in the call to find_best_match.
wafflespeanut
force-pushed
the
levenshtein
branch
3 times, most recently
from
448fa66 to
88d5a07
Compare
| return SuggestionType::Macro(format!("{}!", macro_name)); | ||
| } | ||
|
|
||
| let mut names: Vec<Name> = Vec::new(); |
There was a problem hiding this comment.
We don't need a vector or any pushes here. This can be constructed via a map iterator (self.value_ribs.iter().rev().map(|(&k, _)| k)) and passed directly to find_best_match_for_name()
There was a problem hiding this comment.
Oh, right. Nice :)
|
Looks good to me with some nits. |
wafflespeanut
force-pushed
the
levenshtein
branch
from
88d5a07 to
8e01907
Compare
wafflespeanut
force-pushed
the
levenshtein
branch
from
8e01907 to
51ff171
Compare
wafflespeanut
changed the title
|
@bors r+ Thanks! |
|
📌 Commit 51ff171 has been approved by |
|
⌛ Testing commit 51ff171 with merge 4ce1daf... |
|
💔 Test failed - auto-win-gnu-32-opt |
|
@bors retry |
|
⚡ Previous build results for auto-linux-32-nopt-t, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt are reusable. Rebuilding only auto-linux-32-opt, auto-linux-64-debug-opt, auto-win-gnu-32-opt... |
fixes part of #30197