Cognitive Complexity (step 1 out of 3+): name changes #3803

felix91gr · 2019-02-23T14:49:16Z

Following up on #3793

Overall checklist:

Name changes
MVP of functionality
Tests

After this PR, we will start working on the implementation itself.

felix91gr · 2019-02-24T04:01:43Z

Whoops, Travis failed because I didn't run the automatic naming update script

Manishearth · 2019-02-24T06:25:46Z

I think we have a way of registering renames so that the old name still works but warns about the lint being renamed.

Also, we should probably have something similar for the conf option, allowing people to migrate.

bors · 2019-02-24T15:36:55Z

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

felix91gr · 2019-02-24T23:31:53Z

That seems to be the case. The stability guarantees postulated for Clippy mention that a PR may never quite "remove" a Lint, only deprecate it and warn about its subsequent usage.

Might that be the cause of the error?

Should we be deprecating Cyclomatic Complexity and offering to use Cognitive Complexity (as an entirely different lint) instead?

(CC @oli-obk)

ghost · 2019-02-25T03:46:53Z

Take a look at #3554 if you want an example of renaming a lint.

felix91gr · 2019-02-25T03:54:25Z

Thanks @mikerite! (I'll continue tomorrow tho, now it's pretty late)

I think it's great that there's a mechanism in place for renaming lints 😊

clippy_lints/src/utils/conf.rs

felix91gr · 2019-02-25T16:20:18Z

This is strange. Github says there are conflicts with the cyclomatic_complexity.rs file 🤔

(Btw, I think the rename is now properly formalized!)

felix91gr · 2019-02-25T16:25:54Z

Nvm, I think I solved the conflicts!

felix91gr · 2019-02-25T18:37:16Z

Yay! All the checks have passed. Finally 😅
Now I think I understand a lot more about how Clippy is tested, tho, so for me it was very much worth the while.

felix91gr · 2019-02-25T18:41:13Z

To summarize, the PR now contains these changes:

Renaming of Cyclomatic Complexity to Cognitive Complexity
"Formal Renaming", i.e., Clippy now knows and can warn you about the deprecation of the cyclomatic_complexity lint if you're using it inside your code.
Updated the test for Formal Renaming so that this renaming is checked for in the future.
Updated auto-generated files with new names.
Updated README.md example of cognitive_complexity_threshold configuration.

What's blocking this PR:

How to tell the user about the renaming, if they are using the (now deprecated) cyclomatic_complexity_threshold value in their Clippy.toml configuration file.

felix91gr · 2019-02-25T23:38:41Z

Reading further into the define_Conf macro, I'm more and more certain that the key to putting "good errors for renamed values" in the handling of clippy.toml files, lies right there. But I got nothin': I'm barely able to interpret a macro_rules! macro and this one's beyond my league. Pls help 😅 .

bors · 2019-02-26T11:13:32Z

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

felix91gr · 2019-02-27T05:17:10Z

Alright, I think I've made it tell the user about the usage of the deprecated field cyclomatic_complexity_threshold, suggesting using the cognitive_complexity_threshold field instead. I've also added a test for it.

felix91gr · 2019-02-27T05:40:56Z

CI gives an odd error. Maybe I just missed an upstream change once again 🤔

Anyone got a clue?

detrumi · 2019-02-27T11:32:37Z

CI gives an odd error. Maybe I just missed an upstream change once again

Yes, you'll probably want to rebase so you get the latest fixes

clippy_lints/src/utils/conf.rs

felix91gr · 2019-02-27T20:23:38Z

The changes seemingly worked. Now I'm adding a test to check that the conf.rs file doesn't throw a false negative on a correct clippy.toml file.

felix91gr · 2019-02-27T21:01:47Z

Okay, all checks have passed and now conf.rs gives the appropiate error if and only if the user has an otherwise valid clippy.toml file with the cyclomatic-complexity-threshold field in it.

flip1995 · 2019-02-27T22:53:03Z

I have one more case that should be tested:

rust-clippy/tests/ui/cyclomatic_complexity.rs

Line 90 in da7aebc

#[clippy::cyclomatic_complexity = "0"]

It is currently possible to set the cyclomatic_complexity with an attribute. What is the output of the compiler if you sill use this attribute? Could you add a test for this?

felix91gr · 2019-02-28T05:08:02Z

It is currently possible to set the cyclomatic_complexity with an attribute. What is the output of the compiler if you sill use this attribute? Could you add a test for this?

It is apparently invisible. I made a test like this:

#[clippy::cyclomatic_complexity = "0"]
fn main() {
}

And the compiler and test said nothing (not even in stderr). It seems to only react when you're using the top-level configuration. This, for example:

#![warn(clippy::cyclomatic_complexity)]
#[clippy::cyclomatic_complexity = "0"]
fn main() {
}

Throws something like this:

error: lint `clippy::cyclomatic_complexity` has been renamed to `clippy::cognitive_complexity`
  --> $DIR/rename.rs:2:9
   |
LL | #![warn(clippy::cyclomatic_complexity)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::cognitive_complexity`
   |
   = note: `-D renamed-and-removed-lints` implied by `-D warnings`

Is that okay? Is the value-setter alone enough to trigger the lint? It should crash, shouldn't it? Then I think it didn't trigger at all, because I didn't see it crash. Maybe the setting is invisible to Clippy if you're not using the #![warn(clippy::cyclomatic_complexity)] line 🤔

flip1995 · 2019-02-28T10:19:56Z

Oh yeah, that's kind of the behavior I expected. From the RFC:

A tool that has an active name must check for unused lints/attibutes. For example, if rustfmt becomes active for attributes, and only recognises rustfmt::skip, it must produce a warning if a user uses rustfmt::foo in their code.

We already implemented a lint for unknown Clippy lints (#3161). This should also be done for Clippy attributes. I try to get to this today. I will notify you if I'm done.

flip1995 · 2019-02-28T15:54:19Z

#3830 will give you the ability to add the new attribute cognitive_complexity to the list of BUILTIN_ATTRIBUTES and deprecate the old attribute.

felix91gr · 2019-02-28T20:23:35Z

Your PR got merged, that's awesome!

Btw @flip1995, what kinds of attributes are there? "Builtin" is one, what are the other ones?

flip1995 · 2019-03-01T15:28:15Z

I don't think there are other types of attributes in Clippy. I just copied the naming from rustc https://github.com/rust-lang/rust/blob/1999a2288123173b2e487865c9a04386173025f7/src/libsyntax/feature_gate.rs#L828

In rustc there are for example "tool attributes", which are provided by tools like Clippy and rustfmt. Also there are "custom attributes".

oli-obk · 2019-03-01T15:52:55Z

clippy_lints/src/utils/attrs.rs

+    (
+        "cyclomatic_complexity",
+        DeprecationStatus::Replaced("cognitive_complexity"),
+    ),


So if I understand this list correctly, you'll need to add an entry for cognitive_complexity with DeprecationStatus::None, otherwise you'll get complaints about an unknown attribute when you start using #[clippy::cognitive_complexity(..)]

Ahh! I see. Thank you :D

felix91gr · 2019-03-01T15:58:02Z

(Unrelated to the attributes talk,) I'm getting another compilation error, but this time I'm almost certain I'm running the master toolchain.

This is the error

And these are the commands I ran before I got that result:

./setup-toolchain.sh
./ci/base-tests.sh

EDIT: CI shows the same error. Maybe Clippy got broken with the latest changes from the master toolchain? 🤔

flip1995 · 2019-03-01T16:04:09Z

The fix for this error is on it's way: #3823

felix91gr · 2019-03-01T17:00:43Z

So to recap, the PR has now:

Error for use in config files, with the error telling the user that they should change it to "cognitive-complexity". Tested
Error for use in .rs files (as top level #![warn(clippy::cyclomatic_complexity)]) telling the user that the lint has been renamed. Tested
Error for use in .rs files as builtin attribute, telling the user that the attribute has been renamed. Tested

I think all the user-facing errors have been implemented. Anything else I might be missing? @oli-obk ^^

clippy_lints/src/utils/conf.rs

oli-obk · 2019-03-05T18:51:29Z

Ok, this looks good to me now. Can you just squash your commits then I'll send it off to bors

felix91gr · 2019-03-05T18:52:43Z

Sure!

felix91gr · 2019-03-06T00:13:41Z

Okay after the squashing I had solved an old merge conflict leaving behind the wrong line, twice 😅 . I've been really sleepy today >.<

But I double checked it this time. The rebase should work! (still me -> 🤞)

bors · 2019-03-06T07:09:59Z

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

* Ran automatic naming update * Formalized rename of `cyclomatic_complexity` to `cognitive_complexity` ** Added the rename to `lib.rs` ** Added rename test * Added warning for deprecated key `cyclomatic_complexity_threshold` and tests for it * Added deprecation status for Clippy's builtin attribute * Updated tests for new builtin attribute renaming

felix91gr · 2019-03-06T13:09:43Z

Resolved upstream conflict and rebased again.

phansch

Just a small thing, otherwise it LGTM

tests/ui/rename.rs

phansch · 2019-03-06T13:41:20Z

@bors r=oli-obk

bors · 2019-03-06T13:41:22Z

📌 Commit ddc7180 has been approved by oli-obk

Cognitive Complexity (step 1 out of 3+): name changes Following up on #3793 **Overall checklist:** 1. **Name changes** 2. MVP of functionality 3. Tests After this PR, we will start working on the implementation itself.

bors · 2019-03-06T13:41:41Z

⌛ Testing commit ddc7180 with merge 00baf7a...

bors · 2019-03-06T14:08:54Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 00baf7a to master...

felix91gr · 2019-03-06T14:49:27Z

⛵️

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Feb 24, 2019

oli-obk reviewed Feb 25, 2019

View reviewed changes

clippy_lints/src/utils/conf.rs Show resolved Hide resolved

felix91gr commented Feb 27, 2019

View reviewed changes

clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 1, 2019

View reviewed changes

felix91gr mentioned this pull request Mar 3, 2019

lint: transmuting known null pointer to ref #628

Open

oli-obk reviewed Mar 4, 2019

View reviewed changes

clippy_lints/src/utils/conf.rs Outdated Show resolved Hide resolved

phansch suggested changes Mar 6, 2019

View reviewed changes

tests/ui/rename.rs Show resolved Hide resolved

phansch approved these changes Mar 6, 2019

View reviewed changes

bors merged commit ddc7180 into rust-lang:master Mar 6, 2019

bors mentioned this pull request Mar 6, 2019

Fix invalid while_let_on_iterator suggestion #3784

Closed

felix91gr mentioned this pull request Apr 15, 2019

Cognitive Complexity (step 2 (and maybe 3) out of 3+): implementation! #3963

Closed

5 tasks

ghost mentioned this pull request Aug 12, 2020

Move away form blacklist terminology #5896

Closed

Cognitive Complexity (step 1 out of 3+): name changes #3803

Cognitive Complexity (step 1 out of 3+): name changes #3803

Conversation

felix91gr commented Feb 23, 2019 • edited Loading

felix91gr commented Feb 24, 2019

Manishearth commented Feb 24, 2019

bors commented Feb 24, 2019

felix91gr commented Feb 24, 2019

ghost commented Feb 25, 2019

felix91gr commented Feb 25, 2019

felix91gr commented Feb 25, 2019 • edited Loading

felix91gr commented Feb 25, 2019

felix91gr commented Feb 25, 2019

felix91gr commented Feb 25, 2019 • edited Loading

felix91gr commented Feb 25, 2019

bors commented Feb 26, 2019

felix91gr commented Feb 27, 2019

felix91gr commented Feb 27, 2019

detrumi commented Feb 27, 2019

felix91gr commented Feb 27, 2019

felix91gr commented Feb 27, 2019

flip1995 commented Feb 27, 2019

felix91gr commented Feb 28, 2019

flip1995 commented Feb 28, 2019

flip1995 commented Feb 28, 2019

felix91gr commented Feb 28, 2019

flip1995 commented Mar 1, 2019

oli-obk Mar 1, 2019

Choose a reason for hiding this comment

felix91gr Mar 1, 2019

Choose a reason for hiding this comment

felix91gr commented Mar 1, 2019 • edited Loading

flip1995 commented Mar 1, 2019

felix91gr commented Mar 1, 2019 • edited Loading

oli-obk commented Mar 5, 2019

felix91gr commented Mar 5, 2019

felix91gr commented Mar 6, 2019

bors commented Mar 6, 2019

felix91gr commented Mar 6, 2019

phansch left a comment

Choose a reason for hiding this comment

phansch commented Mar 6, 2019

bors commented Mar 6, 2019

bors commented Mar 6, 2019

bors commented Mar 6, 2019

felix91gr commented Mar 6, 2019

felix91gr commented Feb 23, 2019 •

edited

Loading

felix91gr commented Feb 25, 2019 •

edited

Loading

felix91gr commented Feb 25, 2019 •

edited

Loading

felix91gr commented Mar 1, 2019 •

edited

Loading

felix91gr commented Mar 1, 2019 •

edited

Loading