Refactoring: clean-up checks

[guwirth]

Clean-up the cxx plugin checks.

• there are checks which doing static code analysis: remove it - should be done by external tools
• overlapping functionality of checks: combine it to one check
• combine areas of functionality in subfolders

[guwirth]

from @jmecosta

@Bertk the idea to remove those was in case if we would implement those rules into the CxxChecks. Have i got the impression that @guwirth had a different view for those CxxChecks?

Anyway, for Vera because its mostly a linter. I supose replacing it with something more mainstream like cpplint with be more beneficial for end users.

for Rats, for security analysis i dont think there is no replacement for that.

CppLint could be a interesting replacement for linter, where the sensor actually runs the linter since it does require little configuration. the only dependcy is python, and that should be available in most boxes

from @guwirth

@Bertk and @jmecosta my idea is still:

• don't reinvent the wheel
• the plugin is only to forward issues from other tool to SQ

Think we have no chance to do the same work as other are already did: VS, Core Guideline Checker, Clang, Cppcheck, ... are too good to reimplement everything again...

from @jmecosta

@guwirth i dont know, if we have a grammar, it seems logic that we also have checks. otherwise just run a external tool collect those metrics and simplify the plugin a lot.

alternatively if really willing to just consume external stuff, integrate a clang static analyser by default that gives all metrics and provides basic checks. If those are handle with clang, there is much more possibility of community particiating on developing those. That means dropping totally the sslr stuff from the plugin.

By the way, thats what c# plugin does, so they use the roslyn compiler so they dont need to worry about supporting the grammar.

as far as i see you spend loads of time supporting the grammar only for the marginal gain of having some code metrics

from @Bertk

@guwirth , @jmecosta
We should definitely ask the users of the C++ Community plug-in which sensors are used and decide/vote on deprecating some outdated analyzer tools. My favorite candidates are RATS and Vera++ because the last updates of this tools were December 2013 for RATS and January 2015 for Vera++. The last updates of Vera++ added support for Python and Lua with the goal to replace Tcl. I think using RATS does not improve the security and I disabled the tool already 3 years ago in our build pipelines. There are better tools available and other ways to handle security requirements e.g. banned.h ...

I think the current implementation of AST based checkers and CppCheck are better alternatives. I used AST based checkers to implement custom rules and also extended the grammar for CUDA and C++/CLI.
I disagree with the last comment about the C# plug-in because there is only one compiler and we use many C++ compilers (VC++, gcc, clang). This will not change at least for the next decade.

We should continue source code analysis with AST but create checks and rules which are not available in other tools. This can be revised in 2020 with the next C++ standard.

from @jmecosta

I'm sure clang or derivatives can be used as frontend for many c flavours
https://clang.llvm.org.

But that's not the point, I'm only saying if everything is from external
tools, the usage of cxx grammar brings only a marginal benifit because
it's only use for metrics.

Now if you start developing new checks then effectively keeping tools like
Vera or rats makes no sense since what that provides can be implemented on
top of sslr.

Imo opinion it's a difficult road, because we are always affected by the
sslr versions and it's updates. So far it has been challenging to keep up
with the new versions.

But I'm not against or in favour of either alternative. And I would
probably vote on dropping those tools since they can anyway be provided as
external rules.

[ghost]

Please note that some users (like me) have implemented custom checks, using the sslr parser. If you remove it from the plugin, we will no longer be able to use our custom checks.

[guwirth]

@phcouton the idea is not to remove the sslr parser and AST (XPath support / custom checks). Idea is only to remove the checks which are doing static code analysis. The idea of the plugin is to use external tools for it. Reinventing the wheel makes no sense. From the design point of view there should be a clear separation: static code analysis checks = external tool.

Sample of checks to be removed: SwitchLastCaseIsDefaultCheck, UnnamedNamespaceInHeaderCheck, BooleanEqualityComparisonCheck, UsingNamespaceInHeaderCheck, ...

There is also another discussion to replace the parser #1479. Maybe you can add your five cent there.

[francoisferrand]

there are checks which doing static code analysis: remove it - should be done by external tools

I understand there is a significant effort in keeping the static analysis code in the plugin. However, there is a significant use case for this, which is not handled through external tools.
When using the 'integrated' static code analysis checks, we get 2 features:

• The checks are automatically executed. There is no need to run some extra steps to generate a report, which can eventually be fed to sonar-runner.
• The checks are configured centrally. The list of checks as well as their configuration is configured centrally in the quality profile. This centralization is a key feature of Sonarqube, and should be supported IMO by the C++ community plugin.

Both these feature make the integration a really significant feature. True, the integrated checks are quite limited, but they provide a few things already, with a much better workflow and simplified configuration. For exemple, clang-tidy and clang-format may indeed allow must more complete and accurate checks, but they are also significantly more complicated to setup, with no easy option for centralizing the rules.

So basically I may be fine with removing the integrated checks, but only when the plugin gains the ability to both run the external tools (to generate the reports) and configure this tools in the Quality Profile.

[guwirth]

Hi @Typz,

thanks for you feedback.

I understand there is a significant effort in keeping the static analysis code in the plugin.

No it's not really the effort. The effort was quite low in the past. Problem is more:

• The current list of static code analysis checks is more or less a random (and small) collection of checks. The initial idea to replace external tools by internal checks stagnates. Looking to the pure number of checks in cppcheck>300, pclint>1400, vc>700, ... makes clear that there is no chance to reinvent the wheel. And also the question why we should spend the effort?
• Current checks are confusing the users. They think we are supporting static code analyses but we don't.
• SonarSource deprecates more and more APIs (Use sslr-squid-bridge 2.7 #1484, Refactoring: remove deprecated com.sonar.sslr.api.Preprocessor #471, ...).
• SonarSource is working full-time on this and their result is quit poor. Cppcheck is much better than their static code analysis (and Cppcheck is poor compared to other static code analysers).

when the plugin gains the ability to both run the external tools (to generate the reports)

I would not change the current behaviour. Running external tools is in most cases part of an already existing CI chain. Question again here: Why reinventing the wheel? What can we do better as e.g. Jenkins? The rest is to create an XML report and copy it to an folder. What we can (and should) improve is XSLT support.

Regards,

[ivangalkin]

additionally to the arguments of @guwirth I would like to mention

• the performance aspect: many checks implement the visitor interface, but in fact just read the whole file in memory and run the regexp over the full content. Also it is not possible (yet) to run the checks in parallel.
• the complexity aspect: some of checks require a preprocessor step (e.g. in order to detect all defines or find missing includes). The CxxPreprocessor however is a source of
  a) many run-time log warnings because it failed to find some include. It could be a configuration problem or an issue (#include quoted form: preprocessor search order #1225), but it's never a useful information, because compiler does this job better
  b) design problems: source highlighting is done after the source code become preprocessed. This is logically incorrect and introduces additional complexity (see e.g. Highlighting error for concatenated strings on multiple lines #1468)
  c) maintenance problems, since preprocessor is de-facto deprecated in the new version of sslr-squid-bridge (see Use sslr-squid-bridge 2.7 #1484)

W.r.t. the preprocessor (CxxPreprocessor): if we will remove the dependent checks and don't plan to implement any semantic analysis (e.g. SymbolTable #1401) I would suggest to remove the preprocessor, too. If we have plans regarding the SymbolTable I suggest using an open-source parser e.g. #1479

[francoisferrand]

Mind me, I am not against removing the checks as such. I just think they have huge advantage, of allowing to "driving" the analysis process. If you are running CI only, this may not be significant: you can add many steps in your CI chain. However, the advantage becomes significant when you consider that you want your developper to also be able to run the tests locally: in that case, it is quite convenient to be able to define how to run the analysis in a simple file.

As already mentionned, the other advantage is that the configuration of the analysis (e.g. indent width...) is defined centrally on SonarQube, which is quite nice for enforcing compagny-wide rules.

Now, there are indeed many "practical" reasons to remove the checks: but I still think the workflow they provide is slightly better. A way out of this however may be to integrate the configuration and running of the external tools in the plugins: this way the configuration can be defined on SonarQube and the analysis can be driven by sonar-runner if needed, while re-using existing high-quality analysis engines.

[guwirth]

closed with #1704