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.

Sign up for GitHub

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

Jump to bottom

Expand libstd struct case misspelling diagnostics #72988

Closed
wants to merge 3 commits into from
Closed

Expand libstd struct case misspelling diagnostics #72988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
731c4ea
add libstd struct misspelling type check diagnostics
chrissimpkins Jun 5, 2020
bf2f78e
convert to default suggestion format (from verbose) and change sugges…
chrissimpkins Jun 6, 2020
9441340
`libstd` => `std`; `name` => `names`
chrissimpkins Jun 6, 2020
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
242 changes: 242 additions & 0 deletions src/librustc_resolve/late/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,10 +353,252 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> {
}
_ => {}
}
if ns == TypeNS && path.len() == 1 {
// Case-insensitive test against most libstd struct names
// as another fallback
match res {
Some(Res::ToolMod) => {}
_ => {
let structs = self.get_case_insensitive_libstd_structs_matches(
&ident.name.to_ident_string(),
);
if structs.len() == 1 {
err.span_suggestion(
ident_span,
&format!("found a std struct with a similar name"),
format!("{}", structs[0]),
Applicability::MaybeIncorrect,
);
} else if structs.len() > 1 {
let mut struct_suggestions = Vec::new();
let message = "found std structs with similar names:";
for a_struct in structs.iter() {
struct_suggestions.push(format!("{}", a_struct));
}
err.span_suggestions(
ident_span,
message,
struct_suggestions.into_iter(),
Applicability::MaybeIncorrect,
);
}
}
}
}
}

(err, candidates)
}

/// Get a case-insensitive match with standard library
/// structs that are *not imported* in the prelude.
/// 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> {
Copy link
Member

Choose a reason for hiding this comment

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

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

if ident.name == lookup_ident.name
&& ns == namespace
&& !ptr::eq(in_module, parent_scope.module)
{
let res = name_binding.res();
if filter_fn(res) {
// create the path
let mut segms = path_segments.clone();
if lookup_ident.span.rust_2018() {
// crate-local absolute paths start with `crate::` in edition 2018
// FIXME: may also be stabilized for Rust 2015 (Issues #45477, #44660)
segms.insert(0, ast::PathSegment::from_ident(crate_name));
}
segms.push(ast::PathSegment::from_ident(ident));
let path = Path { span: name_binding.span, segments: segms };
// the entity is accessible in the following cases:
// 1. if it's defined in the same crate, it's always
// accessible (since private entities can be made public)
// 2. if it's defined in another crate, it's accessible
// only if both the module is public and the entity is
// declared as public (due to pruning, we don't explore
// outside crate private modules => no need to check this)
if !in_module_is_extern || name_binding.vis == ty::Visibility::Public {
let did = match res {
Res::Def(DefKind::Ctor(..), did) => this.parent(did),
_ => res.opt_def_id(),
};
if candidates.iter().all(|v: &ImportSuggestion| v.did != did) {
candidates.push(ImportSuggestion { did, descr: res.descr(), path });
}
}
}
}

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tyvm David! I will look into it this week. I really appreciate the review and will be in touch when I have updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

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}.

Copy link
Member

Choose a reason for hiding this comment

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

@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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// Excludes error types
// Excludes nightly only types
// Excludes types with case-sensitive macro names (e.g., `File` -> `file`)
// Excludes deprecated types (e.g., `std::str::LinesAny`)
let libstd_structs = [
"std::alloc::Layout",
"std::alloc::System",
"std::any::TypeId",
"std::ascii::EscapeDefault",
"std::cell::Cell",
"std::char::DecodeUtf16",
"std::char::EscapeDefault",
"std::char::EscapeUnicode",
"std::char::ToLowercase",
"std::char::ToUppercase",
"std::cmp::Reverse",
"std::collections::BTreeMap",
"std::collections::BTreeSet",
"std::collections::BinaryHeap",
"std::collections::HashMap",
"std::collections::HashSet",
"std::collections::LinkedList",
"std::collections::VecDeque",
"std::env::Args",
"std::env::ArgsOs",
"std::env::SplitPaths",
"std::env::Vars",
"std::env::VarsOs",
"std::ffi::CStr",
"std::ffi::CString",
"std::ffi::OsStr",
"std::ffi::OsString",
"std::fmt::DebugList",
"std::fmt::DebugMap",
"std::fmt::DebugSet",
"std::fmt::DebugStruct",
"std::fmt::DebugTuple",
"std::fmt::Formatter",
"std::fs::DirBuilder",
"std::fs::DirEntry",
"std::fs::FileType",
"std::fs::Metadata",
"std::fs::OpenOptions",
"std::fs::Permissions",
"std::fs::ReadDir",
"std::hash::BuildHasherDefault",
"std::io::BufReader",
"std::io::BufWriter",
"std::io::Bytes",
"std::io::Chain",
"std::io::Cursor",
"std::io::Empty",
"std::io::IoSlice",
"std::io::IoSliceMut",
"std::io::LineWriter",
"std::io::Lines",
"std::io::Repeat",
"std::io::Sink",
"std::io::Split",
"std::io::Stderr",
"std::io::StderrLock",
"std::io::Stdin",
"std::io::StdinLock",
"std::io::Stdout",
"std::io::StdoutLock",
"std::io::Take",
"std::iter::Chain",
"std::iter::Cloned",
"std::iter::Copied",
"std::iter::Cycle",
"std::iter::Empty",
"std::iter::Enumerate",
"std::iter::Filter",
"std::iter::FilterMap",
"std::iter::Flatten",
"std::iter::FromFn",
"std::iter::Fuse",
"std::iter::Inspect",
"std::iter::Map",
"std::iter::Once",
"std::iter::OnceWith",
"std::iter::Peekable",
"std::iter::Repeat",
"std::iter::RepeatWith",
"std::iter::Rev",
"std::iter::Scan",
"std::iter::Skip",
"std::iter::SkipWhile",
"std::iter::StepBy",
"std::iter::Successors",
"std::iter::Take",
"std::iter::TakeWhile",
"std::iter::Zip",
"std::marker::PhantomData",
"std::marker::PhantomPinned",
"std::mem::Discriminant",
"std::mem::ManuallyDrop",
"std::net::Incoming",
"std::net::Ipv4Addr",
"std::net::Ipv6Addr",
"std::net::SocketAddrV4",
"std::net::SocketAddrV6",
"std::net::TcpListener",
"std::net::TcpStream",
"std::net::UdpSocket",
"std::num::NonZeroI8",
"std::num::NonZeroI16",
"std::num::NonZeroI32",
"std::num::NonZeroI64",
"std::num::NonZeroI128",
"std::num::NonZeroU8",
"std::num::NonZeroU16",
"std::num::NonZeroU32",
"std::num::NonZeroU64",
"std::num::NonZeroU128",
"std::num::NonZeroUsize",
"std::num::Wrapping",
"std::ops::Range",
"std::ops::RangeFrom",
"std::ops::RangeFull",
"std::ops::RangeInclusive",
"std::ops::RangeTo",
"std::ops::RangeToInclusive",
"std::panic::AssertUnwindSafe",
"std::panic::Location",
"std::panic::PanicInfo",
"std::path::Ancestors",
"std::path::Components",
"std::path::PathBuf",
"std::path::PrefixComponent",
"std::pin::Pin",
"std::process::Child",
"std::process::ChildStderr",
"std::process::ChildStdin",
"std::process::ChildStdout",
"std::process::Command",
"std::process::ExitStatus",
"std::process::Output",
"std::process::Stdio",
"std::ptr::NonNull",
"std::rc::Rc",
"std::rc::Weak",
"std::str::Bytes",
"std::str::CharIndices",
"std::str::Chars",
"std::str::EncodeUtf16",
"std::str::EscapeDefault",
"std::str::EscapeUnicode",
"std::str::Lines",
"std::str::MatchIndices",
"std::str::RMatchIndices",
"std::str::RMatches",
"std::str::RSplit",
"std::str::RSplitN",
"std::str::RSplitTerminator",
"std::str::Split",
"std::str::SplitAsciiWhitespace",
"std::str::SplitN",
"std::str::SplitTerminator",
"std::str::SplitWhitespace",
"std::string::Drain",
"std::sync::Arc",
"std::sync::Barrier",
"std::sync::BarrierWaitResult",
"std::sync::Condvar",
"std::sync::Mutex",
"std::sync::MutexGuard",
"std::sync::Once",
"std::sync::RwLock",
"std::sync::RwLockReadGuard",
"std::sync::RwLockWriteGuard",
"std::sync::WaitTimeoutResult",
"std::sync::Weak",
"std::task::Context",
"std::task::RawWaker",
"std::task::RawWakerVTable",
"std::task::Waker",
"std::thread::Builder",
"std::thread::JoinHandle",
"std::thread::LocalKey",
"std::thread::Thread",
"std::thread::ThreadId",
"std::time::Duration",
"std::time::Instant",
"std::time::SystemTime",
];

let mut structs = Vec::new();
// abort for single character type names
if needle.len() < 2 {
return structs;
}
for item in libstd_structs.iter() {
// check the struct name in the module path
let struct_path: Vec<&str> = item.split("::").collect();
// case-insensitive comparison of names
if needle.to_lowercase() == struct_path.last().unwrap().to_lowercase() {
structs.push(item.to_string());
}
}
structs
}

/// Check if the source is call expression and the first argument is `self`. If true,
/// return the span of whole call and the span for all arguments expect the first one (`self`).
fn call_has_self_arg(&self, source: PathSource<'_>) -> Option<(Span, Option<Span>)> {
Expand Down
7 changes: 7 additions & 0 deletions src/test/ui/libstd-case-typo/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// checks case typos with libstd::alloc structs
fn main(){}

fn test_layout(_x: LayOut){}
//~^ ERROR: cannot find type `LayOut` in this scope
fn test_system(_x: system){}
//~^ ERROR: cannot find type `system` in this scope
21 changes: 21 additions & 0 deletions src/test/ui/libstd-case-typo/alloc.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error[E0412]: cannot find type `LayOut` in this scope
--> $DIR/alloc.rs:4:20
|
LL | fn test_layout(_x: LayOut){}
| ^^^^^^
| |
| not found in this scope
| help: found a std struct with a similar name: `std::alloc::Layout`

error[E0412]: cannot find type `system` in this scope
--> $DIR/alloc.rs:6:20
|
LL | fn test_system(_x: system){}
| ^^^^^^
| |
| not found in this scope
| help: found a std struct with a similar name: `std::alloc::System`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0412`.
5 changes: 5 additions & 0 deletions src/test/ui/libstd-case-typo/any.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// checks case typos with libstd::any structs
fn main(){}

fn test_typeid(_x: Typeid){}
//~^ ERROR: cannot find type `Typeid` in this scope
12 changes: 12 additions & 0 deletions src/test/ui/libstd-case-typo/any.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0412]: cannot find type `Typeid` in this scope
--> $DIR/any.rs:4:20
|
LL | fn test_typeid(_x: Typeid){}
| ^^^^^^
| |
| not found in this scope
| help: found a std struct with a similar name: `std::any::TypeId`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0412`.
5 changes: 5 additions & 0 deletions src/test/ui/libstd-case-typo/ascii.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// checks case typos with libstd::ascii structs
fn main(){}

fn test_escdef(_x: Escapedefault){}
//~^ ERROR: cannot find type `Escapedefault` in this scope
18 changes: 18 additions & 0 deletions src/test/ui/libstd-case-typo/ascii.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error[E0412]: cannot find type `Escapedefault` in this scope
--> $DIR/ascii.rs:4:20
|
LL | fn test_escdef(_x: Escapedefault){}
| ^^^^^^^^^^^^^ not found in this scope
|
help: found std structs with similar names:
|
LL | fn test_escdef(_x: std::ascii::EscapeDefault){}
| ^^^^^^^^^^^^^^^^^^^^^^^^^
LL | fn test_escdef(_x: std::char::EscapeDefault){}
| ^^^^^^^^^^^^^^^^^^^^^^^^
LL | fn test_escdef(_x: std::str::EscapeDefault){}
| ^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0412`.
5 changes: 5 additions & 0 deletions src/test/ui/libstd-case-typo/cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// checks case typos with libstd::cell structs
fn main(){}

fn test_cell(_x: cell<()>){}
//~^ ERROR: cannot find type `cell` in this scope
12 changes: 12 additions & 0 deletions src/test/ui/libstd-case-typo/cell.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0412]: cannot find type `cell` in this scope
--> $DIR/cell.rs:4:18
|
LL | fn test_cell(_x: cell<()>){}
| ^^^^
| |
| not found in this scope
| help: found a std struct with a similar name: `std::cell::Cell`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0412`.
17 changes: 17 additions & 0 deletions src/test/ui/libstd-case-typo/char.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// checks case typos with libstd::char structs
fn main(){}

fn test_du16(_x: DecodeUTF16<()>){}
//~^ ERROR: cannot find type `DecodeUTF16` in this scope

fn test_edflt(_x: Escapedefault){}
Copy link
Member

@davidtwco davidtwco Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

//~^ ERROR: cannot find type `Escapedefault` in this scope

fn test_euni(_x: Escapeunicode){}
//~^ ERROR: cannot find type `Escapeunicode` in this scope

fn test_tolow(_x: Tolowercase){}
//~^ ERROR: cannot find type `Tolowercase` in this scope

fn test_toupper(_x: Touppercase){}
//~^ ERROR: cannot find type `Touppercase` in this scope
Loading