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

clang: Handle old C++ standard aliases #2816

Open
cmorve-te opened this issue Nov 19, 2024 · 8 comments · May be fixed by #2824
Open

clang: Handle old C++ standard aliases #2816

cmorve-te opened this issue Nov 19, 2024 · 8 comments · May be fixed by #2824
Assignees
Milestone

Comments

@cmorve-te
Copy link
Contributor

clangtidy.xml includes, for example, both a clang-diagnostic-c++1y-extensions rule and a clang-diagnostic-c++14-extensions rule. They are logically the same rule, though.

If somebody is scanning the output of an old version of clang, and "clang-diagnostic-c++1y-extensions" is found, it should probably be treated as a clang-diagnostic-c++14-extensions match.

AFAICT this applies to the following rules

  • C++14
    clang-diagnostic-c++1y-extensions -> clang-diagnostic-c++14-extensions

  • C++17
    clang-diagnostic-c++1z-compat -> clang-diagnostic-c++17-compat
    clang-diagnostic-c++1z-compat-mangling -> clang-diagnostic-c++17-compat-mangling
    clang-diagnostic-c++1z-extensions -> clang-diagnostic-c++17-extensions

  • C++20
    clang-diagnostic-c++2a-compat -> clang-diagnostic-c++20-compat
    clang-diagnostic-c++2a-compat-pedantic -> clang-diagnostic-c++20-compat-pedantic
    clang-diagnostic-c++2a-extensions -> clang-diagnostic-c++20-extensions

  • C++23
    clang-diagnostic-c++2b-extensions -> clang-diagnostic-c++23-extensions
    clang-diagnostic-pre-c++2b-compat -> clang-diagnostic-pre-c++23-compat
    clang-diagnostic-pre-c++2b-compat-pedantic -> clang-diagnostic-pre-c++23-compat-pedantic

  • C++26
    clang-diagnostic-c++2c-compat -> clang-diagnostic-c++26-compat
    clang-diagnostic-c++2c-extensions -> clang-diagnostic-c++26-extensions
    clang-diagnostic-pre-c++2c-compat -> clang-diagnostic-pre-c++26-compat
    clang-diagnostic-pre-c++2c-compat-pedantic -> clang-diagnostic-pre-c++26-compat-pedantic

@guwirth guwirth added this to the 2.2.0 milestone Nov 20, 2024
@guwirth
Copy link
Collaborator

guwirth commented Nov 20, 2024

Normally, the sensors support all the rules of the current version and also those of the previous versions. With cxx 2.2.x we can think about cleaning up and removing outdated rules. For me, the decisive factor is always which version of the tool is included in larger Linux distributions. These rules should at least be supported:

  • Ubuntu 22.04.5 LTS => clang 14?
  • Debian 11 “Bullseye” => clang 11?
  1. Can someone give more feedback of which clang versions are in use?
  2. For example, could we only support the clang 14 rules and remove all older rules?

@guwirth
Copy link
Collaborator

guwirth commented Nov 22, 2024

Mapping a rule ID can lead to an issue being moved from old code to new code. See also Issue identification mechanisms to learn how new issues are calculated. In some cases the mechanism is doing a backdating.

In case we are doing a mapping the correct way is to register the mapping during repository creation.

Description:
https://javadocs.sonarsource.org/latest/org/sonar/api/server/rule/RulesDefinition.Rule.html#deprecatedRuleKeys()

RulesDefinition:
https://github.com/SonarSource/sonar-plugin-api/blob/bb901cd17c9ab2cf2df18e0ad9062188e5afc543/plugin-api/src/main/java/org/sonar/api/server/rule/RulesDefinition.java#L605

CxxClangTidyRuleRepository:

rule.addDeprecatedRuleKey("ClangTidy", rule.key()); // V1.3 repository name

@guwirth
Copy link
Collaborator

guwirth commented Nov 25, 2024

Asking SonarSource for details to deprecated rules: https://community.sonarsource.com/t/handling-of-deprecated-rules/131032

@guwirth guwirth linked a pull request Nov 26, 2024 that will close this issue
@guwirth guwirth self-assigned this Nov 26, 2024
@guwirth
Copy link
Collaborator

guwirth commented Nov 26, 2024

@cmorve-te I'm playing around with it to find a correct solution.

@guwirth
Copy link
Collaborator

guwirth commented Nov 27, 2024

Summary:

  1. Rule status: status does not affect the operation of a rule and has no impact on its issues, essentially only the status is displayed in the Web UI. There are three possible rule statuses: beta, ready, and deprecated. Sometimes, rules are first issued in beta status and then moved to ready. Most rules are in ready status; ready to be used in production. When Sonar developers realize that a rule no longer makes sense, they first deprecate the rule, then eventually drop it.

Hint: Recommendation is to mark a rule first deprecated and remove/replace it in a later version than.

  1. addDeprecatedRuleKey: If you want to rename the key of a rule or change its repository or both, register the rule's previous repository and key This will allow SonarQube to support "issue re-keying" for this rule. If the repository and/or key of an existing rule is changed without declaring deprecated keys, existing issues for this rule, created under the rule's previous repository and/or key, will be closed and new ones will be created under the issue's new repository and/or key.

Hint: Rules marked deprecated with addDeprecatedRuleKey must no longer be in the repository, not even with the status deprecated. If this hint is ignored, the server start is aborted and the error message java.lang.IllegalStateException: The following rule keys are declared both as deprecated and used key ... is written to the LOG file.

  1. If a sensor should support the old and new rule in a report during the analysis, the plugin must implement this itself.
  • Mark rules as deprecated in the repository and no mapping in the sensor. Old rules/issues are displayed as deprecated.
  • Remove deprecated rules in the repository and thus discard associated issues
  • Remove deprecated rules in the repository and map to new rules in the plugin/sensor. Issues are all displayed under the new key.

@guwirth
Copy link
Collaborator

guwirth commented Nov 27, 2024

@cmorve-te: I played around with it, see description above.

I tend to the following solution:

  1. Mark deprecated rules in the repository (XML) with deprecated. This means old/new rule/key is in the repository.
  2. Do no mapping in the sensor. People using older versions of the tool see the rules as deprecated. Others using the latest version get the issues with new rule/key.
  3. After some time remove the deprecated rules from repository (XML) and creating a mapping with addDeprecatedRuleKey where a successor is available. If still old key is used it can be made visible with CXX Unknown Rule

@cmorve-te
Copy link
Contributor Author

Mark deprecated rules in the repository (XML) with deprecated. This means old/new rule/key is in the repository.

What do we achieve with this?
It seems to me the deprecated thing is something Sonar can do because the tool (SonarQube) that creates the "warnings" and the thing that creates the rules (SonarQube) are the same thing. I don't think I see the point when third-party tools are creating the warnings.

It's a way of saying "we will stop supporting your old toolchain soon". But C++ developers would be happy to be able to use coroutines, modules and the latest and the greatest, if they are using an old toolchain it's because it's imposed by an external entity. Telling them "it's old" it's not going to change anything, they will just say "I know, I wish I could update it".

Do no mapping in the sensor. People using older versions of the tool see the rules as deprecated. Others using the latest version get the issues with new rule/key.

Do we want people updating their toolchain to get a "new" issue that's actually the same one that already existed, just with a different name?

Some people will be using multiple toolchains (building for different targets), they will get the same issue duplicated, with different names.

What's the problem with your original idea of just adding the addDeprecatedRuleKey rule now (deleting the standalone old one)? From what I understand:

  • All the existing "clang-diagnostic-c++1y-extensions" issues will magically transform into "clang-diagnostic-c++14-extensions". I would argue this is good, not a problem.
  • If you use the new toolchain, everything will be directly "clang-diagnostic-c++14-extensions".
  • If you use the old toolchain, new issues will still be "clang-diagnostic-c++14-extensions" (not sure if this requires also manual mapping in the plugin, but it would be added if needed)
  • If you use both the old and new toolchains, it will be a single issue: clang-diagnostic-c++14-extensions, not duplicated.

@cmorve-te
Copy link
Contributor Author

What's the problem with your original idea of just adding the addDeprecatedRuleKey rule now (deleting the standalone old one)?

i.e. https://github.com/SonarOpenCommunity/sonar-cxx/pull/2824/files seems fine to me. Isn't it?

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