Auto merge of #7873 - xFrednet:7869-string-index-ice, r=Manishearth

Update `str` utils to prevent ICEs and FNs

This PR reworks some string handling for lints regarding enum naming. I hope the refactoring will prevent future ICEs and help with new bug free implementations.

It might be better to review this PR by going through the commits, as `clippy_utils::camel_case` was renamed to `clippy_utils::str_utils` and then changed further. GH sadly doesn't really make the changes that obvious 🙃

Not too much more to say. Have a nice day 🌞

---

Fixes: rust-lang/rust-clippy#7869

changelog: ICE Fix: [`enum_variant_names`] #7869

clippy_lints/src/enum_variants.rs

@@ -1,8 +1,8 @@
 //! lint on enum variants that are prefixed or suffixed by the same characters
 
-use clippy_utils::camel_case;
 use clippy_utils::diagnostics::{span_lint, span_lint_and_help};
 use clippy_utils::source::is_present_in_source;
+use clippy_utils::str_utils::{self, count_match_end, count_match_start};
 use rustc_hir::{EnumDef, Item, ItemKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -117,26 +117,6 @@ impl_lint_pass!(EnumVariantNames => [
     MODULE_INCEPTION
 ]);
 
-/// Returns the number of chars that match from the start
-#[must_use]
-fn partial_match(pre: &str, name: &str) -> usize {
-    let mut name_iter = name.chars();
-    let _ = name_iter.next_back(); // make sure the name is never fully matched
-    pre.chars().zip(name_iter).take_while(|&(l, r)| l == r).count()
-}
-
-/// Returns the number of chars that match from the end
-#[must_use]
-fn partial_rmatch(post: &str, name: &str) -> usize {
-    let mut name_iter = name.chars();
-    let _ = name_iter.next(); // make sure the name is never fully matched
-    post.chars()
-        .rev()
-        .zip(name_iter.rev())
-        .take_while(|&(l, r)| l == r)
-        .count()
-}
-
 fn check_variant(
     cx: &LateContext<'_>,
     threshold: u64,
@@ -150,7 +130,7 @@ fn check_variant(
     }
     for var in def.variants {
         let name = var.ident.name.as_str();
-        if partial_match(item_name, &name) == item_name_chars
+        if count_match_start(item_name, &name).char_count == item_name_chars
             && name.chars().nth(item_name_chars).map_or(false, |c| !c.is_lowercase())
             && name.chars().nth(item_name_chars + 1).map_or(false, |c| !c.is_numeric())
         {
@@ -161,7 +141,7 @@ fn check_variant(
                 "variant name starts with the enum's name",
             );
         }
-        if partial_rmatch(item_name, &name) == item_name_chars {
+        if count_match_end(item_name, &name).char_count == item_name_chars {
             span_lint(
                 cx,
                 ENUM_VARIANT_NAMES,
@@ -171,14 +151,14 @@ fn check_variant(
         }
     }
     let first = &def.variants[0].ident.name.as_str();
-    let mut pre = &first[..camel_case::until(&*first)];
-    let mut post = &first[camel_case::from(&*first)..];
+    let mut pre = &first[..str_utils::camel_case_until(&*first).byte_index];
+    let mut post = &first[str_utils::camel_case_start(&*first).byte_index..];
     for var in def.variants {
         let name = var.ident.name.as_str();
 
-        let pre_match = partial_match(pre, &name);
+        let pre_match = count_match_start(pre, &name).byte_count;
         pre = &pre[..pre_match];
-        let pre_camel = camel_case::until(pre);
+        let pre_camel = str_utils::camel_case_until(pre).byte_index;
         pre = &pre[..pre_camel];
         while let Some((next, last)) = name[pre.len()..].chars().zip(pre.chars().rev()).next() {
             if next.is_numeric() {
@@ -186,18 +166,18 @@ fn check_variant(
             }
             if next.is_lowercase() {
                 let last = pre.len() - last.len_utf8();
-                let last_camel = camel_case::until(&pre[..last]);
-                pre = &pre[..last_camel];
+                let last_camel = str_utils::camel_case_until(&pre[..last]);
+                pre = &pre[..last_camel.byte_index];
             } else {
                 break;
             }
         }
 
-        let post_match = partial_rmatch(post, &name);
-        let post_end = post.len() - post_match;
+        let post_match = count_match_end(post, &name);
+        let post_end = post.len() - post_match.byte_count;
         post = &post[post_end..];
-        let post_camel = camel_case::from(post);
-        post = &post[post_camel..];
+        let post_camel = str_utils::camel_case_start(post);
+        post = &post[post_camel.byte_index..];
     }
     let (what, value) = match (pre.is_empty(), post.is_empty()) {
         (true, true) => return,
@@ -266,14 +246,16 @@ impl LateLintPass<'_> for EnumVariantNames {
                             );
                         }
                     }
-                    if item.vis.node.is_pub() {
-                        let matching = partial_match(mod_camel, &item_camel);
-                        let rmatching = partial_rmatch(mod_camel, &item_camel);
+                    // The `module_name_repetitions` lint should only trigger if the item has the module in its
+                    // name. Having the same name is accepted.
+                    if item.vis.node.is_pub() && item_camel.len() > mod_camel.len() {
+                        let matching = count_match_start(mod_camel, &item_camel);
+                        let rmatching = count_match_end(mod_camel, &item_camel);
                         let nchars = mod_camel.chars().count();
 
                         let is_word_beginning = |c: char| c == '_' || c.is_uppercase() || c.is_numeric();
 
-                        if matching == nchars {
+                        if matching.char_count == nchars {
                             match item_camel.chars().nth(nchars) {
                                 Some(c) if is_word_beginning(c) => span_lint(
                                     cx,
@@ -284,7 +266,7 @@ impl LateLintPass<'_> for EnumVariantNames {
                                 _ => (),
                             }
                         }
-                        if rmatching == nchars {
+                        if rmatching.char_count == nchars {
                             span_lint(
                                 cx,
                                 MODULE_NAME_REPETITIONS,

clippy_utils/src/camel_case.rs

@@ -1,117 +0,0 @@
-/// Returns the index of the character after the first camel-case component of `s`.
-#[must_use]
-pub fn until(s: &str) -> usize {
-    let mut iter = s.char_indices();
-    if let Some((_, first)) = iter.next() {
-        if !first.is_uppercase() {
-            return 0;
-        }
-    } else {
-        return 0;
-    }
-    let mut up = true;
-    let mut last_i = 0;
-    for (i, c) in iter {
-        if up {
-            if c.is_lowercase() {
-                up = false;
-            } else {
-                return last_i;
-            }
-        } else if c.is_uppercase() {
-            up = true;
-            last_i = i;
-        } else if !c.is_lowercase() {
-            return i;
-        }
-    }
-    if up { last_i } else { s.len() }
-}
-
-/// Returns index of the last camel-case component of `s`.
-#[must_use]
-pub fn from(s: &str) -> usize {
-    let mut iter = s.char_indices().rev();
-    if let Some((_, first)) = iter.next() {
-        if !first.is_lowercase() {
-            return s.len();
-        }
-    } else {
-        return s.len();
-    }
-    let mut down = true;
-    let mut last_i = s.len();
-    for (i, c) in iter {
-        if down {
-            if c.is_uppercase() {
-                down = false;
-                last_i = i;
-            } else if !c.is_lowercase() {
-                return last_i;
-            }
-        } else if c.is_lowercase() {
-            down = true;
-        } else if c.is_uppercase() {
-            last_i = i;
-        } else {
-            return last_i;
-        }
-    }
-    last_i
-}
-
-#[cfg(test)]
-mod test {
-    use super::{from, until};
-
-    #[test]
-    fn from_full() {
-        assert_eq!(from("AbcDef"), 0);
-        assert_eq!(from("Abc"), 0);
-        assert_eq!(from("ABcd"), 0);
-        assert_eq!(from("ABcdEf"), 0);
-        assert_eq!(from("AabABcd"), 0);
-    }
-
-    #[test]
-    fn from_partial() {
-        assert_eq!(from("abcDef"), 3);
-        assert_eq!(from("aDbc"), 1);
-        assert_eq!(from("aabABcd"), 3);
-    }
-
-    #[test]
-    fn from_not() {
-        assert_eq!(from("AbcDef_"), 7);
-        assert_eq!(from("AbcDD"), 5);
-    }
-
-    #[test]
-    fn from_caps() {
-        assert_eq!(from("ABCD"), 4);
-    }
-
-    #[test]
-    fn until_full() {
-        assert_eq!(until("AbcDef"), 6);
-        assert_eq!(until("Abc"), 3);
-    }
-
-    #[test]
-    fn until_not() {
-        assert_eq!(until("abcDef"), 0);
-        assert_eq!(until("aDbc"), 0);
-    }
-
-    #[test]
-    fn until_partial() {
-        assert_eq!(until("AbcDef_"), 6);
-        assert_eq!(until("CallTypeC"), 8);
-        assert_eq!(until("AbcDD"), 3);
-    }
-
-    #[test]
-    fn until_caps() {
-        assert_eq!(until("ABCD"), 0);
-    }
-}

clippy_utils/src/lib.rs

@@ -37,7 +37,6 @@ pub mod sym_helper;
 #[allow(clippy::module_name_repetitions)]
 pub mod ast_utils;
 pub mod attrs;
-pub mod camel_case;
 pub mod comparisons;
 pub mod consts;
 pub mod diagnostics;
@@ -50,6 +49,7 @@ pub mod paths;
 pub mod ptr;
 pub mod qualify_min_const_fn;
 pub mod source;
+pub mod str_utils;
 pub mod sugg;
 pub mod ty;
 pub mod usage;

clippy_utils/src/str_utils.rs

@@ -0,0 +1,230 @@
+/// Dealing with sting indices can be hard, this struct ensures that both the
+/// character and byte index are provided for correct indexing.
+#[derive(Debug, Default, PartialEq, Eq)]
+pub struct StrIndex {
+    pub char_index: usize,
+    pub byte_index: usize,
+}
+
+impl StrIndex {
+    pub fn new(char_index: usize, byte_index: usize) -> Self {
+        Self { char_index, byte_index }
+    }
+}
+
+/// Returns the index of the character after the first camel-case component of `s`.
+///
+/// ```
+/// assert_eq!(camel_case_until("AbcDef"), StrIndex::new(6, 6));
+/// assert_eq!(camel_case_until("ABCD"), StrIndex::new(0, 0));
+/// assert_eq!(camel_case_until("AbcDD"), StrIndex::new(3, 3));
+/// assert_eq!(camel_case_until("Abc\u{f6}\u{f6}DD"), StrIndex::new(5, 7));
+/// ```
+#[must_use]
+pub fn camel_case_until(s: &str) -> StrIndex {
+    let mut iter = s.char_indices().enumerate();
+    if let Some((_char_index, (_, first))) = iter.next() {
+        if !first.is_uppercase() {
+            return StrIndex::new(0, 0);
+        }
+    } else {
+        return StrIndex::new(0, 0);
+    }
+    let mut up = true;
+    let mut last_index = StrIndex::new(0, 0);
+    for (char_index, (byte_index, c)) in iter {
+        if up {
+            if c.is_lowercase() {
+                up = false;
+            } else {
+                return last_index;
+            }
+        } else if c.is_uppercase() {
+            up = true;
+            last_index.byte_index = byte_index;
+            last_index.char_index = char_index;
+        } else if !c.is_lowercase() {
+            return StrIndex::new(char_index, byte_index);
+        }
+    }
+
+    if up {
+        last_index
+    } else {
+        StrIndex::new(s.chars().count(), s.len())
+    }
+}
+
+/// Returns index of the last camel-case component of `s`.
+///
+/// ```
+/// assert_eq!(camel_case_start("AbcDef"), StrIndex::new(0, 0));
+/// assert_eq!(camel_case_start("abcDef"), StrIndex::new(3, 3));
+/// assert_eq!(camel_case_start("ABCD"), StrIndex::new(4, 4));
+/// assert_eq!(camel_case_start("abcd"), StrIndex::new(4, 4));
+/// assert_eq!(camel_case_start("\u{f6}\u{f6}cd"), StrIndex::new(4, 6));
+/// ```
+#[must_use]
+pub fn camel_case_start(s: &str) -> StrIndex {
+    let char_count = s.chars().count();
+    let range = 0..char_count;
+    let mut iter = range.rev().zip(s.char_indices().rev());
+    if let Some((char_index, (_, first))) = iter.next() {
+        if !first.is_lowercase() {
+            return StrIndex::new(char_index, s.len());
+        }
+    } else {
+        return StrIndex::new(char_count, s.len());
+    }
+    let mut down = true;
+    let mut last_index = StrIndex::new(char_count, s.len());
+    for (char_index, (byte_index, c)) in iter {
+        if down {
+            if c.is_uppercase() {
+                down = false;
+                last_index.byte_index = byte_index;
+                last_index.char_index = char_index;
+            } else if !c.is_lowercase() {
+                return last_index;
+            }
+        } else if c.is_lowercase() {
+            down = true;
+        } else if c.is_uppercase() {
+            last_index.byte_index = byte_index;
+            last_index.char_index = char_index;
+        } else {
+            return last_index;
+        }
+    }
+    last_index
+}
+
+/// Dealing with sting comparison can be complicated, this struct ensures that both the
+/// character and byte count are provided for correct indexing.
+#[derive(Debug, Default, PartialEq, Eq)]
+pub struct StrCount {
+    pub char_count: usize,
+    pub byte_count: usize,
+}
+
+impl StrCount {
+    pub fn new(char_count: usize, byte_count: usize) -> Self {
+        Self { char_count, byte_count }
+    }
+}
+
+/// Returns the number of chars that match from the start
+///
+/// ```
+/// assert_eq!(count_match_start("hello_mouse", "hello_penguin"), StrCount::new(6, 6));
+/// assert_eq!(count_match_start("hello_clippy", "bye_bugs"), StrCount::new(0, 0));
+/// assert_eq!(count_match_start("hello_world", "hello_world"), StrCount::new(11, 11));
+/// assert_eq!(count_match_start("T\u{f6}ffT\u{f6}ff", "T\u{f6}ff"), StrCount::new(4, 5));
+/// ```
+#[must_use]
+pub fn count_match_start(str1: &str, str2: &str) -> StrCount {
+    // (char_index, char1)
+    let char_count = str1.chars().count();
+    let iter1 = (0..=char_count).zip(str1.chars());
+    // (byte_index, char2)
+    let iter2 = str2.char_indices();
+
+    iter1
+        .zip(iter2)
+        .take_while(|((_, c1), (_, c2))| c1 == c2)
+        .last()
+        .map_or_else(StrCount::default, |((char_index, _), (byte_index, character))| {
+            StrCount::new(char_index + 1, byte_index + character.len_utf8())
+        })
+}
+
+/// Returns the number of chars and bytes that match from the end
+///
+/// ```
+/// assert_eq!(count_match_end("hello_cat", "bye_cat"), StrCount::new(4, 4));
+/// assert_eq!(count_match_end("if_item_thing", "enum_value"), StrCount::new(0, 0));
+/// assert_eq!(count_match_end("Clippy", "Clippy"), StrCount::new(6, 6));
+/// assert_eq!(count_match_end("MyT\u{f6}ff", "YourT\u{f6}ff"), StrCount::new(4, 5));
+/// ```
+#[must_use]
+pub fn count_match_end(str1: &str, str2: &str) -> StrCount {
+    let char_count = str1.chars().count();
+    if char_count == 0 {
+        return StrCount::default();
+    }
+
+    // (char_index, char1)
+    let iter1 = (0..char_count).rev().zip(str1.chars().rev());
+    // (byte_index, char2)
+    let byte_count = str2.len();
+    let iter2 = str2.char_indices().rev();
+
+    iter1
+        .zip(iter2)
+        .take_while(|((_, c1), (_, c2))| c1 == c2)
+        .last()
+        .map_or_else(StrCount::default, |((char_index, _), (byte_index, _))| {
+            StrCount::new(char_count - char_index, byte_count - byte_index)
+        })
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn camel_case_start_full() {
+        assert_eq!(camel_case_start("AbcDef"), StrIndex::new(0, 0));
+        assert_eq!(camel_case_start("Abc"), StrIndex::new(0, 0));
+        assert_eq!(camel_case_start("ABcd"), StrIndex::new(0, 0));
+        assert_eq!(camel_case_start("ABcdEf"), StrIndex::new(0, 0));
+        assert_eq!(camel_case_start("AabABcd"), StrIndex::new(0, 0));
+    }
+
+    #[test]
+    fn camel_case_start_partial() {
+        assert_eq!(camel_case_start("abcDef"), StrIndex::new(3, 3));
+        assert_eq!(camel_case_start("aDbc"), StrIndex::new(1, 1));
+        assert_eq!(camel_case_start("aabABcd"), StrIndex::new(3, 3));
+        assert_eq!(camel_case_start("\u{f6}\u{f6}AabABcd"), StrIndex::new(2, 4));
+    }
+
+    #[test]
+    fn camel_case_start_not() {
+        assert_eq!(camel_case_start("AbcDef_"), StrIndex::new(7, 7));
+        assert_eq!(camel_case_start("AbcDD"), StrIndex::new(5, 5));
+        assert_eq!(camel_case_start("all_small"), StrIndex::new(9, 9));
+        assert_eq!(camel_case_start("\u{f6}_all_small"), StrIndex::new(11, 12));
+    }
+
+    #[test]
+    fn camel_case_start_caps() {
+        assert_eq!(camel_case_start("ABCD"), StrIndex::new(4, 4));
+    }
+
+    #[test]
+    fn camel_case_until_full() {
+        assert_eq!(camel_case_until("AbcDef"), StrIndex::new(6, 6));
+        assert_eq!(camel_case_until("Abc"), StrIndex::new(3, 3));
+        assert_eq!(camel_case_until("Abc\u{f6}\u{f6}\u{f6}"), StrIndex::new(6, 9));
+    }
+
+    #[test]
+    fn camel_case_until_not() {
+        assert_eq!(camel_case_until("abcDef"), StrIndex::new(0, 0));
+        assert_eq!(camel_case_until("aDbc"), StrIndex::new(0, 0));
+    }
+
+    #[test]
+    fn camel_case_until_partial() {
+        assert_eq!(camel_case_until("AbcDef_"), StrIndex::new(6, 6));
+        assert_eq!(camel_case_until("CallTypeC"), StrIndex::new(8, 8));
+        assert_eq!(camel_case_until("AbcDD"), StrIndex::new(3, 3));
+        assert_eq!(camel_case_until("Abc\u{f6}\u{f6}DD"), StrIndex::new(5, 7));
+    }
+
+    #[test]
+    fn until_caps() {
+        assert_eq!(camel_case_until("ABCD"), StrIndex::new(0, 0));
+    }
+}

tests/ui/crashes/ice-7869.rs

@@ -0,0 +1,7 @@
+enum Tila {
+    TyöAlkoi,
+    TyöKeskeytyi,
+    TyöValmis,
+}
+
+fn main() {}

tests/ui/crashes/ice-7869.stderr

@@ -0,0 +1,15 @@
+error: all variants have the same prefix: `Työ`
+  --> $DIR/ice-7869.rs:1:1
+   |
+LL | / enum Tila {
+LL | |     TyöAlkoi,
+LL | |     TyöKeskeytyi,
+LL | |     TyöValmis,
+LL | | }
+   | |_^
+   |
+   = note: `-D clippy::enum-variant-names` implied by `-D warnings`
+   = help: remove the prefixes and use full paths to the variants instead of glob imports
+
+error: aborting due to previous error
+

tests/ui/enum_variants.stderr

@@ -60,7 +60,7 @@ LL | | }
    |
    = help: remove the prefixes and use full paths to the variants instead of glob imports
 
-error: all variants have the same prefix: `With`
+error: all variants have the same prefix: `WithOut`
   --> $DIR/enum_variants.rs:81:1
    |
 LL | / enum Seallll {

tests/ui/match_ref_pats.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::match_ref_pats)]
-#![allow(clippy::equatable_if_let)]
+#![allow(clippy::equatable_if_let, clippy::enum_variant_names)]
 
 fn ref_pats() {
     {