Page MenuHomePhabricator

[clang-tidy] Implement sonarsource-function-cognitive-complexity check
Needs RevisionPublic

Authored by lebedev.ri on Aug 17 2017, 8:57 AM.

Details

Summary

Currently, there is basically just one clang-tidy check to impose some sanity limits on functions - clang-tidy-readability-function-size.
It is nice, allows to limit line count, total number of statements, number of branches, number of function parameters (not counting implicit this), nesting level.
However, those are simple generic metrics. It is still trivially possible to write a function, which does not violate any of these metrics, yet is still rather unreadable.

Thus, some additional, slightly more complicated metric is needed.
There is a well-known Cyclomatic complexity, but certainly has its downsides.
And there is a COGNITIVE COMPLEXITY by SonarSource, which is available for opensource on https://sonarcloud.io/.
I did ask them, and received an answer that it is it can be implemented in clang-tidy.

This check checks function Cognitive Complexity metric, and flags the functions with Cognitive Complexity exceeding the configured limit.
The default limit is 25, same as in 'upstream'.

The metric is implemented as per COGNITIVE COMPLEXITY by SonarSource specification version 1.2 (19 April 2017), with two notable exceptions:

  • preprocessor conditionals (#ifdef, #if, #elif, #else, #endif) are not accounted for. Could be done. Currently, upstream does not account for them either.
  • each method in a recursion cycle is not accounted for. It can't be fully implemented, because cross-translational-unit analysis would be needed, which is not possible in clang-tidy. Thus, at least right now, i completely avoided implementing it.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Oct 5 2017, 8:44 AM
clang-tidy/readability/FunctionCognitiveComplexityCheck.LICENSE.txt
13 ↗(On Diff #117824)

This isn't actually a license, however. I think we need to ask sonarsource either for a license statement they're comfortable with, or approach them with one in hand that they can approve. However, @dberlin might have more input here.

Adding @dberlin for licensing discussion questions.

Ping.

Yes, i agree that what i have added is not a directory, and not a proper license.
That is more of a template to hopefully stat moving things in the right direction.

To clarify, my problem with this Differential is that i'we yet to see any opinion on
the proposed check from the people who will actually have the final say whether
it will be accepted or not. It will be *really* sad if we go through all this trouble
with the legal matters, and then it will be NACK'ed...

(I've not had the chance to complete a full review, but these are some thoughts thus far.)

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
76 ↗(On Diff #117824)

I don't think this needs to be a member of the class -- it can be a static const char *[] at file scope, can it not?

77 ↗(On Diff #117824)

Are you intending to fix this -- that sounds rather serious?

97 ↗(On Diff #117824)

occurence -> occurrence

98 ↗(On Diff #117824)

Missing a full stop.

99 ↗(On Diff #117824)

I don't think the final adds value here.

102 ↗(On Diff #117824)

Why is this turned into a bit-field? If that is important, it should be of type unsigned to prevent differences between compilers (MSVC treats bit-fields differently than GCC and Clang).

118 ↗(On Diff #117824)

We use // instead of /// unless we're specifically documenting something for doxygen.

139 ↗(On Diff #117824)

Why is this commented out?

150 ↗(On Diff #117824)

Missing full-stop (here and elsewhere, I'll stop commenting on them).

191 ↗(On Diff #117824)

Please use = instead of () here.

210 ↗(On Diff #117824)

What's with the comment?

215–218 ↗(On Diff #117824)

I don't think this macro adds clarify for its two uses.

lebedev.ri marked 9 inline comments as done and an inline comment as not done.

@aaron.ballman, thank you for the review!

Rebased, addressed most of review notes.

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
76 ↗(On Diff #117824)

It seemed fitting to keep it in the class, because it is directly related to the previous enum.
But i guess i agree, it is better to move it back out of the class...

77 ↗(On Diff #117824)

I do intend to fix it, as soon as i'm aware of how to fix this.
Specifically, https://reviews.llvm.org/D36836#889350

Question:
Is it expected that clang-tidy somehow parses the DiagnosticIDs::Note's as FixIt's, and thus fails with the following assert:
...

I.e. what is the proper way to fix this, should i change the message, or change the code not to parse the message as FixIt?

99 ↗(On Diff #117824)

I don't think it hurts, either?
I *personally* prefer to specify it always, and remove if subclassing is needed.
But if you insist i can certainly drop it

102 ↗(On Diff #117824)

The comment right after this member variable should have explained it:

/// Criteria C is a bitfield. Even though Criteria is an unsigned char; and
/// only using 3 bits will still result in padding, the fact that it is a
/// bitfield is a reminder that it is important to min(sizeof(Detail))

It is already unsigned:

enum Criteria : unsigned char {
150 ↗(On Diff #117824)

Poured-in some full-stops. I doubt i caught all the places / did not add it in wrong places.

210 ↗(On Diff #117824)

An ugly attempt to align the same substring Optional<BinaryOperator::Opcode> on both lines.
I can either drop the comment, or do using OBO = Optional<BinaryOperator::Opcode>; and use it.

aaron.ballman added inline comments.Oct 21 2017, 6:29 AM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
111 ↗(On Diff #119091)

This static_assert should come with some further comments explaining that size being small is good, but not critical for correctness.

212 ↗(On Diff #119091)

Declare with its initialization below.

220 ↗(On Diff #119091)

Declare with its initialization below.

250 ↗(On Diff #119091)

Missing a closing paren in the comment.

275–276 ↗(On Diff #119091)

Would be better to just get the AST node directly instead of isa<>() and dyn_cast<>().

if (const auto *E = dyn_cast<IfStmt>(Node->getElse()))

317 ↗(On Diff #119091)

encounter function call -> encounter a function call

325 ↗(On Diff #119091)

typo: nonexistent

332 ↗(On Diff #119091)

if new operator -> if the new operator

333 ↗(On Diff #119091)

previous-one -> previous one

338 ↗(On Diff #119091)

creates new stack -> creates a new stack

347 ↗(On Diff #119091)

Can drop the const.

370 ↗(On Diff #119091)

This FIXME doesn't describe what needs to be fixed very well, you might want to expound (or fix it).

384 ↗(On Diff #119091)

regular -> Regular

391 ↗(On Diff #119091)

else are -> else are (remove extra space)

392 ↗(On Diff #119091)

nested -> Nested

411 ↗(On Diff #119091)

extra space between else and are

433 ↗(On Diff #119091)

if -> If

444 ↗(On Diff #119091)

The second non-standard parameter -> The parameter

448 ↗(On Diff #119091)

Explaination -> Explanation

495 ↗(On Diff #119091)

You only need to match function *definitions* (not declarations), correct? It might be useful to specify that in the matcher so that it matches less code.

513–514 ↗(On Diff #119091)

I would reword this to: Output all the basic increments of complexity.

520 ↗(On Diff #119091)

Increase on the -> Increase, on the

77 ↗(On Diff #117824)

This is one for @alexfh to answer, I think. My gut says that the code should not be parsing notes as fixits (as a separate commit).

99 ↗(On Diff #117824)

That's not the typical coding style for the project, so I would prefer to leave it off. It's reasonable to use when there are vtables involved, however.

102 ↗(On Diff #117824)

No, it's underlying type is unsigned char, but the type is Criteria. I meant the field itself needs to be unsigned. However, since that means you have to do more type casting, I think making the type uint8_t instead of a bit-field would be an improvement.

118 ↗(On Diff #117824)

This change is marked as done but wasn't applied consistently. For instance, see lines 130-131 where it switches between uses. I would use // consistently everywhere unless documenting a public interface in a header.

docs/clang-tidy/checks/readability-function-cognitive-complexity.rst
8 ↗(On Diff #119091)

as per -> as per the

85 ↗(On Diff #119091)

And this -> This

141 ↗(On Diff #119091)

I would drop Could be done.

lebedev.ri marked 28 inline comments as done.

Rebased.
Addressed @aaron.ballman review notes (mainly stylistic)

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
495 ↗(On Diff #119091)

Good idea!

102 ↗(On Diff #117824)

Better?

118 ↗(On Diff #117824)

OK.

aaron.ballman added inline comments.Oct 25 2017, 2:11 PM
clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
499 ↗(On Diff #119766)

Since we're restricting this, might as well add deleted and defaulted functions to the list of unless items (as I think those are still definitions).

102 ↗(On Diff #117824)

Much, thank you!

lebedev.ri marked 3 inline comments as done.

Rebased.
Tighten matchers.
Add an assert that the function we matched does have the body (the check analyses the function bodies, so if there is no body, there is nothing to do.)

clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
499 ↗(On Diff #119766)

Good idea!
It seems only the deleted were still matched, not defaulted.

lebedev.ri marked 2 inline comments as done.

Ping.

Rebased.
Silenced (via the -w flag in the RUN line) the Fix conflicts with existing fix! assertion since this differential does not actually cause it, but the testcase just happens to trigger it...
Reported it as PR35136 with minimal reproducer.

Adding @dberlin for licensing discussion questions.

@dberlin ping? I'm wondering if you had the chance to look at this? :)

lebedev.ri updated this revision to Diff 122053.Nov 8 2017, 3:48 AM

Ping.

Adding @dberlin for licensing discussion questions.

@dberlin ping? I'm wondering if you had the chance to look at this? :)

Ping.

Adding @dberlin for licensing discussion questions.

@dberlin ping? I'm wondering if you had the chance to look at this? :)

lebedev.ri marked an inline comment as done.
lebedev.ri retitled this revision from [clang-tidy] Implement readability-function-cognitive-complexity check to [clang-tidy] Implement sonarsource-function-cognitive-complexity check.
lebedev.ri added a subscriber: chandlerc.
  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

@chandlerc Hi!
@aaron.ballman has suggested for me to try to talk to you about this.
Is there some precedent for the licensing 'issue' at hand? Do you have any opinion?
@dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

If there are no further ideas, i'll try to contact sonarsource.

lebedev.ri changed the repository for this revision from rL LLVM to rCTE Clang Tools Extra.

Rebased.

  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

    @chandlerc Hi! @aaron.ballman has suggested for me to try to talk to you about this. Is there some precedent for the licensing 'issue' at hand? Do you have any opinion? @dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @llvm.org email address if useful.

  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

    @chandlerc Hi! @aaron.ballman has suggested for me to try to talk to you about this. Is there some precedent for the licensing 'issue' at hand? Do you have any opinion? @dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @llvm.org email address if useful.

Mailed and heard back (yay!).
They don't want to take active stance/first steps in this,
but they also did *not* retract that initial 'agreement' i quoted earlier (i did ask about that explicitly.).
So nothing is changed so far, this will have to wait until someone can volunteer their precious lawyer time.
(Can show the mail exchange if needed.)

alexfh requested changes to this revision.Mar 14 2018, 8:31 AM

Removing from my dashboard until the licensing stuff gets sorted out.

This revision now requires changes to proceed.Mar 14 2018, 8:31 AM
lebedev.ri updated this revision to Diff 154491.Jul 7 2018, 6:32 AM

Rebased, just to control bitrot, no changes.

Anything new here? Is there a LLVM foundation lawyer or something like that we can ask?

alexfh requested changes to this revision.Oct 1 2018, 6:19 AM

Anything new here? Is there a LLVM foundation lawyer or something like that we can ask?

I didn't notice any substantial changes w.r.t. license since Chandler's comment on Feb. 28th.

This revision now requires changes to proceed.Oct 1 2018, 6:19 AM
  • Rebased
  • As advised by @aaron.ballman, moved into it's own directory/module. Please review that, i'm not entirely sure i have done that fully correctly.

    @chandlerc Hi! @aaron.ballman has suggested for me to try to talk to you about this. Is there some precedent for the licensing 'issue' at hand? Do you have any opinion? @dberlin did not react to the pings, so i'm not sure i personally can come up with anything better for LICENSE.txt

Sadly, what you need here is legal advice for a good way to handle this, and I'm not a lawyer and so I can't really give you that advice.

To be clear, what you currently have isn't OK for several reasons, not least of which what Aaron brought up that this is not in fact a license.

If there are no further ideas, i'll try to contact sonarsource.

Unless Danny can volunteer his time, we finish with relicensing efforts

Hi @chandlerc !
I was just wondering what percentage of "finish" was meant here, and where at it's now?

and can devote the foundation's lawyer's (sadly precious) time to this, I think either you or sonarsource working to understand the best legal way to contribute this would be the best way forward. Sorry that this is a tricky situation. Happy to have a private discussion w/ your or sonarsources's lawyer via my @ llvm.org email address if useful.

Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2019, 2:58 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript