Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

US: Add explanation for cognitive/cyclomatic complexity checks #1494

Closed
ivangalkin opened this issue Jun 11, 2018 · 2 comments · Fixed by #1546
Closed

US: Add explanation for cognitive/cyclomatic complexity checks #1494

ivangalkin opened this issue Jun 11, 2018 · 2 comments · Fixed by #1546
Assignees
Milestone

Comments

@ivangalkin
Copy link
Contributor

Hi All,

while reviewing of the #1398, I've noticed that the complexity checks create an issues without any explanation.
E.g. ClassComplexityCheck, FileComplexityCheck, FunctionCognitiveComplexityCheck or FunctionComplexityCheck create an issue on the class/file/function with a message like

"The complexity of this function is X which is greater than Xmax authorized."

The Java plugin breaks X into several summands, each of them is a secondary location, pointing to the complexity source (nesting, switch/case, loops, try/catch, inner declarations of classes or lambdas etc).

The resulting message looks like:

"The complexity of this function is X which is greater than Xmax authorized."
Line L0: nesting +1
Line L1: try statement +2
Line L2: continue statement +1 etc

Implementation in Java plugin:

https://github.com/SonarSource/sonar-java/blob/master/java-checks/src/main/java/org/sonar/java/checks/CognitiveComplexityMethodCheck.java
https://github.com/SonarSource/sonar-java/blob/master/java-frontend/src/main/java/org/sonar/java/ast/visitors/CognitiveComplexityVisitor.java

Example of working with multiple/secondary issue locations: #1447

@guwirth
Copy link
Collaborator

guwirth commented Jun 13, 2018

@ivangalkin maybe interesting to align it with Java, but I don't need it.

@ivangalkin
Copy link
Contributor Author

Hi @guwirth,

I used Java as example, but I believe that other plugins (incl. SonarCFamily) have this feature, too. Explanation about the source of [cognitive] complexity is optional, of course. However, if such additional information exists, it improves the sensitivity: now developer know why there is a code smell and which particular parts cause the "brain overload".

By the way, while writing this comment I found the duplicate US #1297. It contains a good illustration, how such issue should look like. It looks, like I am not the only person, who misses this feature.

@guwirth guwirth changed the title US (?): Add explanation for cognitive complexity checks US: Add explanation for cognitive complexity checks Jun 14, 2018
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Jun 19, 2018
* minor performance findings while looking at SonarOpenCommunity#1494
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Jun 21, 2018
* minor findings while looking at SonarOpenCommunity#1494
* test doesn show the actual values because of a wrong written assertion
@ivangalkin ivangalkin self-assigned this Jun 21, 2018
@ivangalkin ivangalkin changed the title US: Add explanation for cognitive complexity checks US: Add explanation for cognitive/cyclomatic complexity checks Jul 19, 2018
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Aug 5, 2018
* implement multi-line messages for squidsensor
ivangalkin referenced this issue in ivangalkin/sonar-cxx Aug 6, 2018
* this class is used for all scoped cyclomatic complexity checks
  * ClassComplexityCheck
  * FileComplexityCheck
  * FunctionComplexityCheck

* it respects the scopes (important for nested classes)
* it provides detailed information about complexity sources
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Aug 8, 2018
* replace custom recursive algorithm with the standard AstVisitor approach

  * no risk of StackOverflowError anymore
  * no need to track set of visited `AstNode`s - decrease run-time and memory footprint

* code simplification and clean-up
* the new code calcualtes an equal value for the cognitive complexity metric

this change is required for SonarOpenCommunity#1494
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Aug 8, 2018
1. implement multi-location messages for squidsensor
* move CxxReportIssue and CxxReportLocation from
  cxx-sensors to cxx-squid in order to satisfy dependencies

2. introduce CxxCyclomaticComplexityCheck
* this class is used for all scoped cyclomatic complexity checks
* implement
  * ClassComplexityCheck
  * FileComplexityCheck
  * FunctionComplexityCheck

* they respect the scopes (important for nested classes)
* they provide detailed information about complexity sources
* FileComplexityCheck previously had a wrong calculation:
  cyclomatic complexity of functions was always <correct score> + 1
  -> fixed now

This change addresses SonarOpenCommunity#1494:

* cyclomatic complexity checks (done)
* cognitive complexity checks (todo, depends on SonarOpenCommunity#1535
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Aug 16, 2018
1. implement multi-location messages for squidsensor
* move CxxReportIssue and CxxReportLocation from
  cxx-sensors to cxx-squid in order to satisfy dependencies

2. introduce CxxCyclomaticComplexityCheck
* this class is used for all scoped cyclomatic complexity checks
* implement
  * ClassComplexityCheck
  * FileComplexityCheck
  * FunctionComplexityCheck

* they respect the scopes (important for nested classes)
* they provide detailed information about complexity sources
* FileComplexityCheck previously had a wrong calculation:
  cyclomatic complexity of functions was always <correct score> + 1
  -> fixed now

This change addresses SonarOpenCommunity#1494:

* cyclomatic complexity checks (done)
* cognitive complexity checks (todo, depends on SonarOpenCommunity#1535
@guwirth guwirth added this to the 1.2 milestone Aug 26, 2018
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Aug 26, 2018
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Sep 5, 2018
* partial solution for SonarOpenCommunity#1547

MOTIVATION:

* SonarOpenCommunity#1494 made visible the fact, that preprocessor affects the complexity
metric. E.g. if function uses macros, the underlying (hidden) complexity
of theses macros increases the complexity of the function. This is wrong
because of many reasons:

1. Cognitive complexity:
Macros do not increase the cognitive complexity of the source code. To
the reader they are similar to the function invocations.
Example ASSERT_XX/EXPECT_XX macros of GTest.

2. Cyclomatic complexity:
Similar logic might be applied to the cyclomatic complexity: macro expansions
are similar to the function calls (which become inlined). In this case
the cyclomatic complexity of the caller should not be increased.
[Cyclomatic] complexity of the callee must be calculated separately.

3. Preprocessor doesn't distinguish between the macros, which
were declared inside of the analyzed project or were included from the
external header. Obviously the external source code (e.g. again very
complex GTest macros) should not affect the complexity of someone's
custom SonarQube project.

SOLUTION:

In general it's impossible to build a valid C/C++ AST without
performing of preprocessor step. The original source code might
be syntactically incorrect (e.g. if macros are used to "invent" some
domain specific language out of C/C++). All metrics of sonar-cxx
are however AST-based. (This is a very questionably decision
e.g. because #if/#ifdef etc might affect the resulting complexity or [NC]LOC
in dramatic way). So we cannot skip preprocessor step entirely and/or
perform metrics calculation/checks on the non-preprocessed code.

Sonar SSLR has a possibility to mark some tokens as generated.
This property (`Token::isGeneratedCode()`) is for example used
in order to ignore generated while
* [NC]LOC counting (see `AbstractLineLengthCheck<>`)
* CDP analysis (see `CxxCpdVisitor`) or
* Highlighting (see `CxxHighlighterVisitor`)

This patch ensures, that
* all `AstNode`s, which were created while macro expansion are marked as
generated
* all complexity related visitors ignore generated `AstNode`s

RESULT:

PRO:
1. The complexity of "callers" is not affected by macro anymore.
   This fact makes the complexity check look logical again.

CON:
1. the complexity of "callees" is not calculated at all. This
is correct in case of external macros, but wrong w.r.t. to the
project intern macros. Possible solution: preprocessor must try
to extract the function-like macro declaration into some special AST-subtree.
Afterwards complexity metrics/check will be able to calculate/check
macros as separate language construct.

By the way: it must be analyzed if and how macros affect other metrics.
For example LOC etc.

2. This patch might change the complexity related metrics. Depending
on how many macros were used in the analyzed project, the reduction
can be radical.
ivangalkin added a commit to ivangalkin/sonar-cxx that referenced this issue Oct 25, 2018
* complete list of `overloadable-operator`s
  (yes, alternative operators can be used for operator overloading too)
* complete `equalityExpression`, `andExpression`, `exclusiveOrExpression`, `inclusiveOrExpression`, `logicalAndExpression`, `logicalOrExpression`

fixes SonarOpenCommunity#1569

* add missing support of alternative operators to the complexity metrics (see # SonarOpenCommunity#1494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants