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

dswij · 2021-12-15T13:44:02Z

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:

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

enum Foo {
    cFoo,
    cBar,
    cBaz,
}

enum Something {
    CCall,
    CCreate,
    CCryogenize,
}

non-ascii characters that doesn't have casing will not be split,

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)

rust-highfive · 2021-12-15T13:44:05Z

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

tests/ui/enum_variants.stderr

xFrednet · 2021-12-23T09:16:35Z

I would like to review this, I hope it's okay to steal the assignment ^^

r? @xFrednet

(It's on my todo list later today 🙃 )

llogiq · 2021-12-23T09:43:21Z

Sure! It's all yours, @xFrednet !

xFrednet

Thank you for the changes, overall, they look good to me and the code is also more readable now. I found some things which can probably be improved, but nothing major. I've also been a bit more nit picky than usual in str_utils 😅. Afterwards, it should be ready to be merged 🙃

clippy_lints/src/enum_variants.rs

clippy_utils/src/str_utils.rs

xFrednet · 2021-12-23T14:32:21Z

clippy_utils/src/str_utils.rs

+/// StrIndex::new(6, 6)]);
+/// assert_eq!(camel_case_indexes("abcDef"), vec![StrIndex::new(3, 3), StrIndex::new(6, 6)]);
+/// ```
+pub fn camel_case_indexes(s: &str) -> Vec<StrIndex> {


Suggested change

pub fn camel_case_indexes(s: &str) -> Vec<StrIndex> {

pub fn camel_case_indices(s: &str) -> Vec<StrIndex> {

Same for the documentation

xFrednet · 2021-12-23T15:16:56Z

clippy_utils/src/str_utils.rs

+    let offsets = camel_case_indexes(s);
+    let mut idxs_iter = offsets.iter().map(|str_idx| str_idx.byte_index).peekable();
+    let idxs: Vec<usize> = if let Some(&idx) = idxs_iter.peek() {
+        if idx == 0 {
+            idxs_iter.collect()
+        } else {
+            Vec::<usize>::from([0]).into_iter().chain(idxs_iter).collect()
+        }
+    } else {
+        return vec![s];
+    };
+    let split_points: Vec<(&usize, &usize)> = idxs[..idxs.len() - 1].iter().zip(&idxs[1..]).collect();
+
+    split_points.iter().map(|(&start, &stop)| &s[start..stop]).collect()


All of these collects seem a bit much to me. I would suggest the following rewrite 🙃

let offsets = camel_case_indexes(s); if offsets[0] != 0 { offsets.insert(0, 0); } offsets.as_slice().windows(2).map(|win| { let start = win[0].byte_index; let end = win[1].byte_index; &s[start..end] }).collect()

Yeah, this seems much better, I was trying to be too fancy there 😅

This also often happens to me, it's just more fun to go fancy ^^. BTW, there is also a std::iter::once() function to create a one time iterator, that's more efficient than Vec::<usize>::from([0]).into_iter() as it doesn't allocate any memory. 🙃

clippy_lints/src/enum_variants.rs

tests/ui/enum_variants.stderr

xFrednet

LGTM. One last NIT, Rust and Clippy have a no-merge policy, meaning that no merge-commits should be added in PRs. Could you please remove the merge commit from this branch and rebase on master. I'm guessing the easiest way is to drop the commit first with git rebase -i HEAD~9 (This opens the rebase interface) and afterwards using git rebase upstream/master or something similar depending on your setup. Let me know if you need help with that.

Then I think it's ready to be merged 🙃

`enum_variant_names` will consider characters with no case to be a part of prefixes/suffixes substring that are compared. This means `Foo1` and `Foo2` has different prefixes (`Foo1` and `Foo2` prefix respeectively). This applies to all non-ascii characters with no casing.

dswij · 2021-12-27T05:55:45Z

@xFrednet BTW, this is rebased and ready to be re-reviewed 🙂

xFrednet · 2021-12-27T11:24:33Z

Awesome thank you very much! This looks good to me 👍

@bors r+

(Bors seems to have some problems since yesterday. Let's hope that it runs now 🙃 )

bors · 2021-12-27T11:24:35Z

📌 Commit b82c9ce has been approved by xFrednet

bors · 2021-12-27T11:24:41Z

⌛ Testing commit b82c9ce with merge ab66862...

Fix `enum_variants` FP on prefixes that are not camel-case closes #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 { Prefix的, PrefixTea, PrefixCake, } ``` will be considered as `Prefix的`, `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.

bors · 2021-12-27T11:31:22Z

💔 Test failed - checks-action_test

xFrednet · 2021-12-27T11:53:04Z

@bors retry

bors · 2021-12-27T11:53:11Z

⌛ Testing commit b82c9ce with merge e5293c5...

Fix `enum_variants` FP on prefixes that are not camel-case closes #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)

bors · 2021-12-27T11:55:11Z

💔 Test failed - checks-action_dev_test

xFrednet · 2021-12-27T12:09:46Z

@bors retry (restarting the failed job worked on the normal CI, let's see it now)

bors · 2021-12-27T12:09:52Z

⌛ Testing commit b82c9ce with merge 3f2bb30...

Fix `enum_variants` FP on prefixes that are not camel-case closes #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)

bors · 2021-12-27T12:40:42Z

💔 Test failed - checks-action_test

dswij · 2021-12-27T14:39:28Z

Lots of failed downloads on GH action, I suppose we can wait for a while 🙂

 Warning: Failed to download action 'https://api.github.com/repos/actions/download-artifact/tarball/18f0f591fbc635562c815484d73b6e8e3980482e'. Error: The operation was canceled.
Warning: Back off 12.276 seconds before retry.
Warning: Failed to download action 'https://api.github.com/repos/actions/download-artifact/tarball/18f0f591fbc635562c815484d73b6e8e3980482e'. Error: The operation was canceled.
Warning: Back off 15.908 seconds before retry.
Error: The operation was canceled.

xFrednet · 2021-12-28T12:01:17Z

GitHub had some outages, not everything seems to work again. let's try it one more time

@bors retry

bors · 2021-12-28T12:01:24Z

⌛ Testing commit b82c9ce with merge 56ccd30...

bors · 2021-12-28T12:15:39Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 56ccd30 to master...

rust-highfive assigned llogiq Dec 15, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 15, 2021

dswij force-pushed the 8090 branch from 93c5821 to 51d6e5f Compare December 22, 2021 15:19

dswij commented Dec 22, 2021

View reviewed changes

tests/ui/enum_variants.stderr Show resolved Hide resolved

dswij marked this pull request as ready for review December 23, 2021 05:39

dswij changed the title ~~[WIP] Fix enum_variants FP on prefixes that are not camel-case~~ Fix enum_variants FP on prefixes that are not camel-case Dec 23, 2021

rust-highfive assigned xFrednet and unassigned llogiq Dec 23, 2021

xFrednet reviewed Dec 23, 2021

View reviewed changes

dswij force-pushed the 8090 branch from fdbb3ca to 84204ff Compare December 24, 2021 04:38

xFrednet approved these changes Dec 24, 2021

View reviewed changes

dswij added 8 commits December 25, 2021 21:54

Refactor enum_variants

f327f0e

Fix False Positive on enum_variants when prefixes are not camel-case

d58fdfb

Add str_util helpers to split camelcase strings

c76e2d1

update enum_variants test

df2e4d1

Update str_utils test

8ed723b

Fix reversed suggestion on postfix

c8f016f

Some minor cleanup

6f7e5cb

dswij force-pushed the 8090 branch from 84204ff to b82c9ce Compare December 25, 2021 14:12

dswij requested a review from xFrednet December 27, 2021 05:55

xFrednet approved these changes Dec 27, 2021

View reviewed changes

bors merged commit 56ccd30 into rust-lang:master Dec 28, 2021

bors mentioned this pull request Dec 28, 2021

new lint: borrow_deref_ref #7930

Merged

dswij deleted the 8090 branch January 20, 2022 12:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dswij commented Dec 15, 2021 •

edited by xFrednet

Loading

rust-highfive commented Dec 15, 2021

xFrednet commented Dec 23, 2021

llogiq commented Dec 23, 2021

xFrednet left a comment

xFrednet Dec 23, 2021

xFrednet Dec 23, 2021

dswij Dec 24, 2021

xFrednet Dec 24, 2021

xFrednet left a comment

dswij commented Dec 27, 2021

xFrednet commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

xFrednet commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

xFrednet commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

dswij commented Dec 27, 2021

xFrednet commented Dec 28, 2021

bors commented Dec 28, 2021

bors commented Dec 28, 2021

	pub fn camel_case_indexes(s: &str) -> Vec<StrIndex> {
	pub fn camel_case_indices(s: &str) -> Vec<StrIndex> {

Fix enum_variants FP on prefixes that are not camel-case #8127

Fix enum_variants FP on prefixes that are not camel-case #8127

Conversation

dswij commented Dec 15, 2021 • edited by xFrednet Loading

rust-highfive commented Dec 15, 2021

xFrednet commented Dec 23, 2021

llogiq commented Dec 23, 2021

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Dec 23, 2021

Choose a reason for hiding this comment

xFrednet Dec 23, 2021

Choose a reason for hiding this comment

dswij Dec 24, 2021

Choose a reason for hiding this comment

xFrednet Dec 24, 2021

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

dswij commented Dec 27, 2021

xFrednet commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

xFrednet commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

xFrednet commented Dec 27, 2021

bors commented Dec 27, 2021

bors commented Dec 27, 2021

dswij commented Dec 27, 2021

xFrednet commented Dec 28, 2021

bors commented Dec 28, 2021

bors commented Dec 28, 2021

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

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

dswij commented Dec 15, 2021 •

edited by xFrednet

Loading