Move away form blacklist terminology #5896

mocsy · 2020-08-12T10:52:35Z

changelog: Move away form blacklist terminology

rust-highfive · 2020-08-12T10:52:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthiaskrgr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

flip1995 · 2020-08-12T12:54:55Z

Moderating note: I think we all know what discussions this type of PR can introduce, so I want to ask everyone to stay civil and have a productive discussion about this. (I really don't want to lock this PR)

cc @rust-lang/clippy

Do we want to do this? Recently all internal uses of black/white-list were renamed in the compiler. No user facing things were renamed though. This would definitely be user facing, both the lint name and the configuration name.

We have precedent for renaming a lint, because it could be offending to some people: #3521

oli-obk · 2020-08-12T13:26:25Z

Yea this definitely needs a renaming scheme, other than that lgtm

ghost · 2020-08-12T16:13:46Z

It's got to be done without breaking peoples config/code. The warnings should be emitted if the old name is used with instructions for using the new name. Don't crash with an error. #3803 shows how this can be done.

yaahc · 2020-08-12T17:41:04Z

Do we want to do this? Recently all internal uses of black/white-list were renamed in the compiler. No user facing things were renamed though. This would definitely be user facing, both the lint name and the configuration name.

Yes imo

matthiaskrgr · 2020-08-12T17:47:19Z

FTR this is the similar pr that was merged into rustc recently: rust-lang/rust@1e567c1

I also have not objections to merging this :) , but we should definitely have some kind of deprecation message! (As far as I can see there is none as of this version of this PR..?)

clippy_lints/src/blacklisted_name.rs

phansch · 2020-08-13T05:24:39Z

I'm also supportive of merging this (once the lint renaming handling has been added)

Ryan1729 · 2020-08-13T10:42:35Z

It's got to be done without breaking peoples config/code. The warnings should be emitted if the old name is used with instructions for using the new name. Don't crash with an error. #3803 shows how this can be done.

I was curious about what changes in #3803 were relevant to ensuring these qualities. As far as I can tell, only the following files are relevant:

Hopefully that helps anyone else who would otherwise find themselves reading the entire diff.

flip1995 · 2020-08-13T13:27:14Z

Alright, let's move forward with the renaming!

@mocsy 2 things have to be done:

Register the lint as renamed (use register_renamed for this)
Register the old configuration option as renamed. (See in utils/conf.rs how this was done with cyclomatic_complexity and do the same for this lint)

Each of the two steps require tests to be added. For 1. we already have a test file IIRC. For 2. add tests similar to the tests for cognitive_complexity linked above by @Ryan1729.

mocsy · 2020-08-13T16:06:09Z

Alright, let's move forward with the renaming!

@mocsy 2 things have to be done:

Register the lint as renamed (use register_renamed for this)

Register the old configuration option as renamed. (See in utils/conf.rs how this was done with cyclomatic_complexity and do the same for this lint)

Each of the two steps require tests to be added. For 1. we already have a test file IIRC. For 2. add tests similar to the tests for cognitive_complexity linked above by @Ryan1729.

All right, I'll do these steppes.

flip1995

There are tests missing for the old configuration name and the renaming of the lint.

Also some test files are still named *blacklist*, it would be great if you could rename them also.

flip1995 · 2020-08-18T16:05:45Z

clippy_lints/src/disallowed_name.rs

@@ -17,33 +17,33 @@ declare_clippy_lint! {
    /// ```rust
    /// let foo = 3.14;
    /// ```
-    pub BLACKLISTED_NAME,
+    pub DISALLOWED_NAME,


Let's rename it properly:

Suggested change

pub DISALLOWED_NAME,

pub DISALLOWED_NAMES,

I believe it's the lint definition which is in singular:

#![warn(clippy::blacklisted_name)]

not the toml entry:

blacklisted-names = ["toto", "tata", "titi"]

which is in plural.

Yeah, but it should be in plural, as per our lint naming conventions (point 3)

flip1995 · 2020-08-18T16:12:21Z

r? @flip1995

mocsy · 2020-08-25T09:53:31Z

There are tests missing for the old configuration name and the renaming of the lint.

Also some test files are still named *blacklist*, it would be great if you could rename them also.
These are done, please confirm this approach is good.

flip1995

There is still a test missing for the renamed configuration option (or did I miss this test?)

flip1995 · 2020-08-25T15:34:51Z

tests/ui/blacklisted_name.stderr

-LL | fn test(foo: ()) {}
-   |         ^^^
+LL | #![warn(clippy::blacklisted_name)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::disallowed_name`


Please include the rename tests in tests/ui/rename.rs

Okay, shall I remove tests/ui/blacklisted_name then?

Yes, after you have one #[warn(clippy::blacklisted_name)] in the rename test file you can remove this.

bors · 2020-09-13T16:25:01Z

☔ The latest upstream changes (presumably #6036) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

flip1995 · 2020-10-10T06:35:15Z

ping from triage @mocsy. Anything I can help you with to get this over the finish line?

giraffate · 2020-10-25T13:43:20Z

@flip1995 2 weeks have passed with no activity from previous ping, so this PR should be closed according to Rust triage procedure.

flip1995 · 2020-10-25T15:06:04Z

@rustbot modify labels: -S-waiting-on-author +S-inactive-closed

…shearth Remove "blacklist" terminology Picking up where #5896 left off, this renames the `blacklisted_name` lint to `disallowed_names` (pluralised for compliance with naming conventions). The old name is still available though is deprecated (both in the lint name, and in the TOML configuration file). This has been proposed/requested a few times, most recently in #7582 where `@xFrednet` suggested pinging the Clippy team when a PR was created hence... cc: `@rust-lang/clippy` changelog: [`disallowed_names`] lint replaces `blacklisted_name`

jonboh · 2023-10-10T18:21:09Z

@rustbot label -S-inactive-closed

rust-highfive assigned matthiaskrgr Aug 12, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 12, 2020

flip1995 added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Aug 12, 2020

phansch reviewed Aug 13, 2020

View reviewed changes

clippy_lints/src/blacklisted_name.rs Outdated Show resolved Hide resolved

mocsy force-pushed the master branch from 7853357 to d11cd67 Compare August 18, 2020 14:21

flip1995 requested changes Aug 18, 2020

View reviewed changes

rust-highfive assigned flip1995 and unassigned matthiaskrgr Aug 18, 2020

Andras Mocsary and others added 5 commits August 24, 2020 12:48

Move away form blacklist terminology

923d2d9

Lint rename and utils/conf rename

5ba20a0

Not all lints are defined properly

1d0286c

cargo dev fmt

4728151

Fix tests

1d8dd5b

mocsy force-pushed the master branch from d11cd67 to 1d8dd5b Compare August 24, 2020 11:37

mocsy requested a review from flip1995 August 25, 2020 10:02

flip1995 reviewed Aug 25, 2020

View reviewed changes

ghost mentioned this pull request Sep 22, 2020

New Lint: blacklisted_method #6073

Closed

flip1995 closed this Oct 25, 2020

rustbot added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 25, 2020

Manishearth mentioned this pull request Feb 16, 2021

Move away from "whitelist"/"blacklist" terminology rust-lang/rust-bindgen#1997

Closed

ghost mentioned this pull request Aug 17, 2021

Deprecate the term "blacklist" from lints #7582

Closed

bossmc mentioned this pull request Jun 8, 2022

Remove "blacklist" terminology #8974

Merged

rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move away form blacklist terminology #5896

Move away form blacklist terminology #5896

mocsy commented Aug 12, 2020

rust-highfive commented Aug 12, 2020

flip1995 commented Aug 12, 2020

oli-obk commented Aug 12, 2020

ghost commented Aug 12, 2020

yaahc commented Aug 12, 2020 •

edited

Loading

matthiaskrgr commented Aug 12, 2020

phansch commented Aug 13, 2020

Ryan1729 commented Aug 13, 2020

flip1995 commented Aug 13, 2020

mocsy commented Aug 13, 2020

flip1995 left a comment

flip1995 Aug 18, 2020

mocsy Aug 25, 2020

flip1995 Aug 25, 2020

flip1995 commented Aug 18, 2020

mocsy commented Aug 25, 2020

flip1995 left a comment

flip1995 Aug 25, 2020

mocsy Aug 26, 2020

flip1995 Aug 26, 2020

bors commented Sep 13, 2020

flip1995 commented Oct 10, 2020

giraffate commented Oct 25, 2020

flip1995 commented Oct 25, 2020

jonboh commented Oct 10, 2023

Move away form blacklist terminology #5896

Move away form blacklist terminology #5896

Conversation

mocsy commented Aug 12, 2020

rust-highfive commented Aug 12, 2020

flip1995 commented Aug 12, 2020

oli-obk commented Aug 12, 2020

ghost commented Aug 12, 2020

yaahc commented Aug 12, 2020 • edited Loading

matthiaskrgr commented Aug 12, 2020

phansch commented Aug 13, 2020

Ryan1729 commented Aug 13, 2020

flip1995 commented Aug 13, 2020

mocsy commented Aug 13, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Aug 18, 2020

Choose a reason for hiding this comment

mocsy Aug 25, 2020

Choose a reason for hiding this comment

flip1995 Aug 25, 2020

Choose a reason for hiding this comment

flip1995 commented Aug 18, 2020

mocsy commented Aug 25, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Aug 25, 2020

Choose a reason for hiding this comment

mocsy Aug 26, 2020

Choose a reason for hiding this comment

flip1995 Aug 26, 2020

Choose a reason for hiding this comment

bors commented Sep 13, 2020

flip1995 commented Oct 10, 2020

giraffate commented Oct 25, 2020

flip1995 commented Oct 25, 2020

jonboh commented Oct 10, 2023

yaahc commented Aug 12, 2020 •

edited

Loading