Expand libstd struct case misspelling diagnostics #72988
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
chrissimpkins
changed the title
chrissimpkins
force-pushed
the
libstd-suggestions
branch
from
2d813b1
to
9381dff
Compare
This comment has been minimized.
This comment has been minimized.
add tests broaden libstd support add tests fmt eliminate match attempts on names < 2 chars addresses CI errors during match attempts against generic type names like "N". These will not be the correct suggestions. remove *Debug structs, overlaps with debug macro typo refactor match approach filter on the type namespace and filter out tool mod types update tests
chrissimpkins
force-pushed
the
libstd-suggestions
branch
from
2632bb9
to
731c4ea
Compare
|
@matthewjasper I think that this is ready for review after some edits to address issues that came up in other UI tests following the initial push. CI is failing on some UI tests that I did not add. Assuming that these changes are acceptable, let me know how you feel we should approach the Will wait on doing anything further until you have a chance to review. Let me know if there is anything that you need from me. Thanks! |
|
Generally we try to avoid "did you mean" in diagnostic suggestions. Instead, I would word this to indicate what we're suggesting and why we're suggesting it. Something like "the standard library contains a <item type> with a similar name". |
We have some work to do... ;) Happy to change it. Thanks for the feedback. |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@davidtwco David is this something that you would be willing to review? This is related to #73023 that you recently reviewed. |
|
r? @davidtwco I'll take a look at this shortly. |
Tyvm! |
| /// This is used for type checking diagnostics in cases when | ||
| /// the type is not in scope and the name includes case | ||
| /// misspelling (e.g., `Hashmap`, not `HashMap`). | ||
| fn get_case_insensitive_libstd_structs_matches(&self, needle: &str) -> Vec<String> { |
There was a problem hiding this comment.
I think we should take a different approach - I'd rather we avoid hard-coding std type names in the compiler (the compiler shouldn't need to be updated when std is changed for this diagnostic to continue to work) and I think we can make this more general, so it works for more than just std.
As you note in the PR description, we do Levenshtein matching on things that are already in-scope and suggest fixes in that case. We also already have suggestions for things that aren't in scope but where the name matches exactly.
The snippet below is where the compiler decides to make "did you mean to import this" suggestions:
rust/src/librustc_resolve/diagnostics.rs
Lines 660 to 693 in 50c0192
On line 660 is where it checks for an exact match - I'd experiment with doing a exact match or checking for a Levenshtein distance from that name that's under some threshold (look into lev_distance and find_best_match_for_name to see what can be re-used). I'd hope that would detect cases like this where we can import a name that's very similar. Alternatively, we could just change the comparison to compare the names without considering the case (as I understand it, that would be equivalent to what you're doing here?).
What we might also want to do is thread this information through in some way - e.g. changing ImportSuggestion to be an enum with Exact and Almost (you can come up with a better name than this) variants so that we can make the diagnostic separate these two types of suggestions:
error[E0412]: cannot find type `Hashmap` in this scope
--> src/main.rs:2:8
|
2 | m: Hashmap<String, ()>,
| ^^^^^^^ not found in this scope
|
help: consider importing one of these items
|
1 | use some::made:up::module::Hashmap;
|
help: consider importing one of these similarly named items
|
1 | use std::collections::HashMap;
|
This should be easier to maintain because we don't have to keep a list of types like you've got here, and it'd work for non-std types too.
There was a problem hiding this comment.
Tyvm David! I will look into it this week. I really appreciate the review and will be in touch when I have updates.
There was a problem hiding this comment.
Small nitpick on wording: help: consider importing one of these similarly named items → help: you might have meant to use {one of }the following similarly named item{s}.
There was a problem hiding this comment.
@chrissimpkins How are you getting with this? Feel free to open a work-in-progress PR if you’re unsure how to proceed or want some early feedback!
There was a problem hiding this comment.
Sorry for the delay! Busy week last week. I will get back to this during the week and let you know if I run into any issues.
| fn test_du16(_x: DecodeUTF16<()>){} | ||
| //~^ ERROR: cannot find type `DecodeUTF16` in this scope | ||
|
|
||
| fn test_edflt(_x: Escapedefault){} |
There was a problem hiding this comment.
I'd combine all of these tests into a single file - we're not gaining anything by having them separate - if we change the approach, it's very likely that if one fails then they all would and so having different tests just means we have more tests to run (likely to be marginally slower).
Also, feel free to --bless tests that you didn't add so that CI passes as soon as you update the PR - that way if all is well, we can just approve straight away.
|
@chrissimpkins This is a triage bump. |
|
@Muirrum Thanks for the reminder Owen. I don't know that any of this source will be used in what is required here based on the review. I'll close this and open a new PR when time permits me to dig into it again. |
Closes #72641
This PR adds an additional fallback
librustc_resolvediagnostic with libstd struct suggestions for case misspelling errors under the following conditions:std::str::LinesAny)Behavior: A user who enters a struct name with a libstd struct case misspelling for a type that is not in scope will receive the matching libstd struct(s) in a new suggestion. This approach was originally requested in #72641 for
Hashmap->HashMaponly. These changes broaden the scope of that report to cover many/most libstd structs with any case misspelling error, and support multiple struct suggestions when there are multiple case-insensitive name matches across the standard library. This is only used when the Levenshtein algorithm does not identify a typo candidate match.Before:
After: