Auto merge of rust-lang#8127 - dswij:8090, r=xFrednet

Fix `enum_variants` FP on prefixes that are not camel-case

closes rust-lang#8090

Fix FP on `enum_variants` when prefixes are only a substring of a camel-case word. Also adds some util helpers on `str_utils` to help parsing camel-case strings.

This changes how the lint behaves:

1. previously if the Prefix is only a length of 1, it's going to get ignored, i.e. these were previously ignored and now is warned
```rust
enum Foo {
    cFoo,
    cBar,
    cBaz,
}

enum Something {
    CCall,
    CCreate,
    CCryogenize,
}
```

2. non-ascii characters that doesn't have casing will not be split,
```rust
enum NonCaps {
    PrefixXXX,
    PrefixTea,
    PrefixCake,
}
```
will be considered as `PrefixXXX`, `Prefix`, `Prefix`, so this won't lint as opposed to fired previously.

changelog: [`enum_variant_names`] Fix FP when first prefix are only a substring of a camel-case word.

---

 (Edited by `@xFrednet` removed some non ascii characters)

clippy_lints/src/enum_variants.rs

@@ -2,8 +2,8 @@
 
 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 clippy_utils::str_utils::{camel_case_split, count_match_end, count_match_start};
+use rustc_hir::{EnumDef, Item, ItemKind, Variant};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::Span;
@@ -18,6 +18,12 @@ declare_clippy_lint! {
     /// Enumeration variant names should specify their variant,
     /// not repeat the enumeration name.
     ///
+    /// ### Limitations
+    /// Characters with no casing will be considered when comparing prefixes/suffixes
+    /// This applies to numbers and non-ascii characters without casing
+    /// e.g. `Foo1` and `Foo2` is considered to have different prefixes
+    /// (the prefixes are `Foo1` and `Foo2` respectively), as also `Bar螃`, `Bar蟹`
+    ///
     /// ### Example
     /// ```rust
     /// enum Cake {
@@ -120,72 +126,73 @@ impl_lint_pass!(EnumVariantNames => [
     MODULE_INCEPTION
 ]);
 
-fn check_variant(
-    cx: &LateContext<'_>,
-    threshold: u64,
-    def: &EnumDef<'_>,
-    item_name: &str,
-    item_name_chars: usize,
-    span: Span,
-) {
+fn check_enum_start(cx: &LateContext<'_>, item_name: &str, variant: &Variant<'_>) {
+    let name = variant.ident.name.as_str();
+    let item_name_chars = item_name.chars().count();
+
+    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())
+    {
+        span_lint(
+            cx,
+            ENUM_VARIANT_NAMES,
+            variant.span,
+            "variant name starts with the enum's name",
+        );
+    }
+}
+
+fn check_enum_end(cx: &LateContext<'_>, item_name: &str, variant: &Variant<'_>) {
+    let name = variant.ident.name.as_str();
+    let item_name_chars = item_name.chars().count();
+
+    if count_match_end(item_name, &name).char_count == item_name_chars {
+        span_lint(
+            cx,
+            ENUM_VARIANT_NAMES,
+            variant.span,
+            "variant name ends with the enum's name",
+        );
+    }
+}
+
+fn check_variant(cx: &LateContext<'_>, threshold: u64, def: &EnumDef<'_>, item_name: &str, span: Span) {
     if (def.variants.len() as u64) < threshold {
         return;
     }
-    for var in def.variants {
-        let name = var.ident.name.as_str();
-        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())
-        {
-            span_lint(
-                cx,
-                ENUM_VARIANT_NAMES,
-                var.span,
-                "variant name starts with the enum's name",
-            );
-        }
-        if count_match_end(item_name, &name).char_count == item_name_chars {
-            span_lint(
-                cx,
-                ENUM_VARIANT_NAMES,
-                var.span,
-                "variant name ends with the enum's name",
-            );
-        }
-    }
+
     let first = &def.variants[0].ident.name.as_str();
-    let mut pre = &first[..str_utils::camel_case_until(&*first).byte_index];
-    let mut post = &first[str_utils::camel_case_start(&*first).byte_index..];
+    let mut pre = camel_case_split(first);
+    let mut post = pre.clone();
+    post.reverse();
     for var in def.variants {
+        check_enum_start(cx, item_name, var);
+        check_enum_end(cx, item_name, var);
         let name = var.ident.name.as_str();
 
-        let pre_match = count_match_start(pre, &name).byte_count;
-        pre = &pre[..pre_match];
-        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() {
-                return;
-            }
-            if next.is_lowercase() {
-                let last = pre.len() - last.len_utf8();
-                let last_camel = str_utils::camel_case_until(&pre[..last]);
-                pre = &pre[..last_camel.byte_index];
-            } else {
-                break;
-            }
-        }
+        let variant_split = camel_case_split(&name);
 
-        let post_match = count_match_end(post, &name);
-        let post_end = post.len() - post_match.byte_count;
-        post = &post[post_end..];
-        let post_camel = str_utils::camel_case_start(post);
-        post = &post[post_camel.byte_index..];
+        pre = pre
+            .iter()
+            .zip(variant_split.iter())
+            .take_while(|(a, b)| a == b)
+            .map(|e| *e.0)
+            .collect();
+        post = post
+            .iter()
+            .zip(variant_split.iter().rev())
+            .take_while(|(a, b)| a == b)
+            .map(|e| *e.0)
+            .collect();
     }
     let (what, value) = match (pre.is_empty(), post.is_empty()) {
         (true, true) => return,
-        (false, _) => ("pre", pre),
-        (true, false) => ("post", post),
+        (false, _) => ("pre", pre.join("")),
+        (true, false) => {
+            post.reverse();
+            ("post", post.join(""))
+        },
     };
     span_lint_and_help(
         cx,
@@ -233,7 +240,6 @@ impl LateLintPass<'_> for EnumVariantNames {
     #[allow(clippy::similar_names)]
     fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) {
         let item_name = item.ident.name.as_str();
-        let item_name_chars = item_name.chars().count();
         let item_camel = to_camel_case(&item_name);
         if !item.span.from_expansion() && is_present_in_source(cx, item.span) {
             if let Some(&(ref mod_name, ref mod_camel)) = self.modules.last() {
@@ -283,7 +289,7 @@ impl LateLintPass<'_> for EnumVariantNames {
         }
         if let ItemKind::Enum(ref def, _) = item.kind {
             if !(self.avoid_breaking_exported_api && cx.access_levels.is_exported(item.def_id)) {
-                check_variant(cx, self.threshold, def, &item_name, item_name_chars, item.span);
+                check_variant(cx, self.threshold, def, &item_name, item.span);
             }
         }
         self.modules.push((item.ident.name, item_camel));

clippy_utils/src/str_utils.rs

@@ -68,6 +68,20 @@ pub fn camel_case_until(s: &str) -> StrIndex {
 /// ```
 #[must_use]
 pub fn camel_case_start(s: &str) -> StrIndex {
+    camel_case_start_from_idx(s, 0)
+}
+
+/// Returns `StrIndex` of the last camel-case component of `s[idx..]`.
+///
+/// ```
+/// # use clippy_utils::str_utils::{camel_case_start_from_idx, StrIndex};
+/// assert_eq!(camel_case_start_from_idx("AbcDef", 0), StrIndex::new(0, 0));
+/// assert_eq!(camel_case_start_from_idx("AbcDef", 1), StrIndex::new(3, 3));
+/// assert_eq!(camel_case_start_from_idx("AbcDefGhi", 0), StrIndex::new(0, 0));
+/// assert_eq!(camel_case_start_from_idx("AbcDefGhi", 1), StrIndex::new(3, 3));
+/// assert_eq!(camel_case_start_from_idx("Abcdefg", 1), StrIndex::new(7, 7));
+/// ```
+pub fn camel_case_start_from_idx(s: &str, start_idx: usize) -> StrIndex {
     let char_count = s.chars().count();
     let range = 0..char_count;
     let mut iter = range.rev().zip(s.char_indices().rev());
@@ -78,9 +92,13 @@ pub fn camel_case_start(s: &str) -> StrIndex {
     } 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 byte_index < start_idx {
+            break;
+        }
         if down {
             if c.is_uppercase() {
                 down = false;
@@ -98,9 +116,55 @@ pub fn camel_case_start(s: &str) -> StrIndex {
             return last_index;
         }
     }
+
     last_index
 }
 
+/// Get the indexes of camel case components of a string `s`
+///
+/// ```
+/// # use clippy_utils::str_utils::{camel_case_indices, StrIndex};
+/// assert_eq!(
+///     camel_case_indices("AbcDef"),
+///     vec![StrIndex::new(0, 0), StrIndex::new(3, 3), StrIndex::new(6, 6)]
+/// );
+/// assert_eq!(
+///     camel_case_indices("abcDef"),
+///     vec![StrIndex::new(3, 3), StrIndex::new(6, 6)]
+/// );
+/// ```
+pub fn camel_case_indices(s: &str) -> Vec<StrIndex> {
+    let mut result = Vec::new();
+    let mut str_idx = camel_case_start(s);
+
+    while str_idx.byte_index < s.len() {
+        let next_idx = str_idx.byte_index + 1;
+        result.push(str_idx);
+        str_idx = camel_case_start_from_idx(s, next_idx);
+    }
+    result.push(str_idx);
+
+    result
+}
+
+/// Split camel case string into a vector of its components
+///
+/// ```
+/// # use clippy_utils::str_utils::{camel_case_split, StrIndex};
+/// assert_eq!(camel_case_split("AbcDef"), vec!["Abc", "Def"]);
+/// ```
+pub fn camel_case_split(s: &str) -> Vec<&str> {
+    let mut offsets = camel_case_indices(s)
+        .iter()
+        .map(|e| e.byte_index)
+        .collect::<Vec<usize>>();
+    if offsets[0] != 0 {
+        offsets.insert(0, 0);
+    }
+
+    offsets.windows(2).map(|w| &s[w[0]..w[1]]).collect()
+}
+
 /// 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)]
@@ -231,4 +295,31 @@ mod test {
     fn until_caps() {
         assert_eq!(camel_case_until("ABCD"), StrIndex::new(0, 0));
     }
+
+    #[test]
+    fn camel_case_start_from_idx_full() {
+        assert_eq!(camel_case_start_from_idx("AbcDef", 0), StrIndex::new(0, 0));
+        assert_eq!(camel_case_start_from_idx("AbcDef", 1), StrIndex::new(3, 3));
+        assert_eq!(camel_case_start_from_idx("AbcDef", 4), StrIndex::new(6, 6));
+        assert_eq!(camel_case_start_from_idx("AbcDefGhi", 0), StrIndex::new(0, 0));
+        assert_eq!(camel_case_start_from_idx("AbcDefGhi", 1), StrIndex::new(3, 3));
+        assert_eq!(camel_case_start_from_idx("Abcdefg", 1), StrIndex::new(7, 7));
+    }
+
+    #[test]
+    fn camel_case_indices_full() {
+        assert_eq!(camel_case_indices("Abc\u{f6}\u{f6}DD"), vec![StrIndex::new(7, 9)]);
+    }
+
+    #[test]
+    fn camel_case_split_full() {
+        assert_eq!(camel_case_split("A"), vec!["A"]);
+        assert_eq!(camel_case_split("AbcDef"), vec!["Abc", "Def"]);
+        assert_eq!(camel_case_split("Abc"), vec!["Abc"]);
+        assert_eq!(camel_case_split("abcDef"), vec!["abc", "Def"]);
+        assert_eq!(
+            camel_case_split("\u{f6}\u{f6}AabABcd"),
+            vec!["\u{f6}\u{f6}", "Aab", "A", "Bcd"]
+        );
+    }
 }

tests/ui/enum_variants.rs

@@ -145,4 +145,10 @@ enum HIDataRequest {
     DeleteUnpubHIData(String),
 }
 
+enum North {
+    Normal,
+    NoLeft,
+    NoRight,
+}
+
 fn main() {}

tests/ui/enum_variants.stderr

@@ -6,6 +6,18 @@ LL |     cFoo,
    |
    = note: `-D clippy::enum-variant-names` implied by `-D warnings`
 
+error: all variants have the same prefix: `c`
+  --> $DIR/enum_variants.rs:14:1
+   |
+LL | / enum Foo {
+LL | |     cFoo,
+LL | |     cBar,
+LL | |     cBaz,
+LL | | }
+   | |_^
+   |
+   = help: remove the prefixes and use full paths to the variants instead of glob imports
+
 error: variant name starts with the enum's name
   --> $DIR/enum_variants.rs:26:5
    |
@@ -60,25 +72,25 @@ LL | | }
    |
    = help: remove the prefixes and use full paths to the variants instead of glob imports
 
-error: all variants have the same prefix: `WithOut`
-  --> $DIR/enum_variants.rs:81:1
+error: all variants have the same prefix: `C`
+  --> $DIR/enum_variants.rs:59:1
    |
-LL | / enum Seallll {
-LL | |     WithOutCake,
-LL | |     WithOutTea,
-LL | |     WithOut,
+LL | / enum Something {
+LL | |     CCall,
+LL | |     CCreate,
+LL | |     CCryogenize,
 LL | | }
    | |_^
    |
    = help: remove the prefixes and use full paths to the variants instead of glob imports
 
-error: all variants have the same prefix: `Prefix`
-  --> $DIR/enum_variants.rs:87:1
+error: all variants have the same prefix: `WithOut`
+  --> $DIR/enum_variants.rs:81:1
    |
-LL | / enum NonCaps {
-LL | |     Prefix的,
-LL | |     PrefixTea,
-LL | |     PrefixCake,
+LL | / enum Seallll {
+LL | |     WithOutCake,
+LL | |     WithOutTea,
+LL | |     WithOut,
 LL | | }
    | |_^
    |
@@ -108,5 +120,5 @@ LL | | }
    |
    = help: remove the postfixes and use full paths to the variants instead of glob imports
 
-error: aborting due to 11 previous errors
+error: aborting due to 12 previous errors