[clang-tidy] Add new check bugprone-tagged-union-member-count (llvm#8…

…9925)

This patch introduces a new check to find mismatches between the number
of data members in a union and the number enum values present in
variant-like structures.

Variant-like types can look something like this:

```c++
struct variant {
    enum {
        tag1,
        tag2,
    } kind;
    union {
        int i;
        char c;
    } data;
};
```

The kind data member of the variant is supposed to tell which data
member of the union is valid, however if there are fewer enum values
than union members, then it is likely a mistake.

The opposite is not that obvious, because it might be fine to have more
enum values than union data members, but for the time being I am curious
how many real bugs can be caught if we give a warning regardless.

This patch also contains a heuristic where we try to guess whether the
last enum constant is actually supposed to be a tag value for the
variant or whether it is just holding how many enum constants have been
created.

Patch by Gábor Tóthvári!

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -78,6 +78,7 @@
 #include "SuspiciousStringviewDataUsageCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "SwitchMissingDefaultCaseCheck.h"
+#include "TaggedUnionMemberCountCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
@@ -229,6 +230,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-suspicious-stringview-data-usage");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "bugprone-swapped-arguments");
+    CheckFactories.registerCheck<TaggedUnionMemberCountCheck>(
+        "bugprone-tagged-union-member-count");
     CheckFactories.registerCheck<TerminatingContinueCheck>(
         "bugprone-terminating-continue");
     CheckFactories.registerCheck<ThrowKeywordMissingCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -73,6 +73,7 @@ add_clang_library(clangTidyBugproneModule
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
+  TaggedUnionMemberCountCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp

clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp

@@ -0,0 +1,199 @@
+//===--- TaggedUnionMemberCountCheck.cpp - clang-tidy ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TaggedUnionMemberCountCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallSet.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+static constexpr llvm::StringLiteral StrictModeOptionName = "StrictMode";
+static constexpr llvm::StringLiteral EnableCountingEnumHeuristicOptionName =
+    "EnableCountingEnumHeuristic";
+static constexpr llvm::StringLiteral CountingEnumPrefixesOptionName =
+    "CountingEnumPrefixes";
+static constexpr llvm::StringLiteral CountingEnumSuffixesOptionName =
+    "CountingEnumSuffixes";
+
+static constexpr bool StrictModeOptionDefaultValue = false;
+static constexpr bool EnableCountingEnumHeuristicOptionDefaultValue = true;
+static constexpr llvm::StringLiteral CountingEnumPrefixesOptionDefaultValue =
+    "";
+static constexpr llvm::StringLiteral CountingEnumSuffixesOptionDefaultValue =
+    "count";
+
+static constexpr llvm::StringLiteral RootMatchBindName = "root";
+static constexpr llvm::StringLiteral UnionMatchBindName = "union";
+static constexpr llvm::StringLiteral TagMatchBindName = "tags";
+
+namespace {
+
+AST_MATCHER_P2(RecordDecl, fieldCountOfKindIsOne,
+               ast_matchers::internal::Matcher<FieldDecl>, InnerMatcher,
+               StringRef, BindName) {
+  // BoundNodesTreeBuilder resets itself when a match occurs.
+  // So to avoid losing previously saved binds, a temporary instance
+  // is used for matching.
+  //
+  // For precedence, see commit: 5b07de1a5faf4a22ae6fd982b877c5e7e3a76559
+  clang::ast_matchers::internal::BoundNodesTreeBuilder TempBuilder;
+
+  const FieldDecl *FirstMatch = nullptr;
+  for (const FieldDecl *Field : Node.fields()) {
+    if (InnerMatcher.matches(*Field, Finder, &TempBuilder)) {
+      if (FirstMatch) {
+        return false;
+      } else {
+        FirstMatch = Field;
+      }
+    }
+  }
+
+  if (FirstMatch) {
+    Builder->setBinding(BindName, clang::DynTypedNode::create(*FirstMatch));
+    return true;
+  }
+  return false;
+}
+
+} // namespace
+
+TaggedUnionMemberCountCheck::TaggedUnionMemberCountCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      StrictMode(
+          Options.get(StrictModeOptionName, StrictModeOptionDefaultValue)),
+      EnableCountingEnumHeuristic(
+          Options.get(EnableCountingEnumHeuristicOptionName,
+                      EnableCountingEnumHeuristicOptionDefaultValue)),
+      CountingEnumPrefixes(utils::options::parseStringList(
+          Options.get(CountingEnumPrefixesOptionName,
+                      CountingEnumPrefixesOptionDefaultValue))),
+      CountingEnumSuffixes(utils::options::parseStringList(
+          Options.get(CountingEnumSuffixesOptionName,
+                      CountingEnumSuffixesOptionDefaultValue))) {
+  if (!EnableCountingEnumHeuristic) {
+    if (Options.get(CountingEnumPrefixesOptionName))
+      configurationDiag("%0: Counting enum heuristic is disabled but "
+                        "%1 is set")
+          << Name << CountingEnumPrefixesOptionName;
+    if (Options.get(CountingEnumSuffixesOptionName))
+      configurationDiag("%0: Counting enum heuristic is disabled but "
+                        "%1 is set")
+          << Name << CountingEnumSuffixesOptionName;
+  }
+}
+
+void TaggedUnionMemberCountCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, StrictModeOptionName, StrictMode);
+  Options.store(Opts, EnableCountingEnumHeuristicOptionName,
+                EnableCountingEnumHeuristic);
+  Options.store(Opts, CountingEnumPrefixesOptionName,
+                utils::options::serializeStringList(CountingEnumPrefixes));
+  Options.store(Opts, CountingEnumSuffixesOptionName,
+                utils::options::serializeStringList(CountingEnumSuffixes));
+}
+
+void TaggedUnionMemberCountCheck::registerMatchers(MatchFinder *Finder) {
+
+  auto UnionField = fieldDecl(hasType(qualType(
+      hasCanonicalType(recordType(hasDeclaration(recordDecl(isUnion())))))));
+
+  auto EnumField = fieldDecl(hasType(
+      qualType(hasCanonicalType(enumType(hasDeclaration(enumDecl()))))));
+
+  auto hasOneUnionField = fieldCountOfKindIsOne(UnionField, UnionMatchBindName);
+  auto hasOneEnumField = fieldCountOfKindIsOne(EnumField, TagMatchBindName);
+
+  Finder->addMatcher(recordDecl(anyOf(isStruct(), isClass()), hasOneUnionField,
+                                hasOneEnumField, unless(isImplicit()))
+                         .bind(RootMatchBindName),
+                     this);
+}
+
+bool TaggedUnionMemberCountCheck::isCountingEnumLikeName(StringRef Name) const {
+  if (llvm::any_of(CountingEnumPrefixes, [Name](StringRef Prefix) -> bool {
+        return Name.starts_with_insensitive(Prefix);
+      }))
+    return true;
+  if (llvm::any_of(CountingEnumSuffixes, [Name](StringRef Suffix) -> bool {
+        return Name.ends_with_insensitive(Suffix);
+      }))
+    return true;
+  return false;
+}
+
+std::pair<const std::size_t, const EnumConstantDecl *>
+TaggedUnionMemberCountCheck::getNumberOfEnumValues(const EnumDecl *ED) {
+  llvm::SmallSet<llvm::APSInt, 16> EnumValues;
+
+  const EnumConstantDecl *LastEnumConstant = nullptr;
+  for (const EnumConstantDecl *Enumerator : ED->enumerators()) {
+    EnumValues.insert(Enumerator->getInitVal());
+    LastEnumConstant = Enumerator;
+  }
+
+  if (EnableCountingEnumHeuristic && LastEnumConstant &&
+      isCountingEnumLikeName(LastEnumConstant->getName()) &&
+      (LastEnumConstant->getInitVal() == (EnumValues.size() - 1))) {
+    return {EnumValues.size() - 1, LastEnumConstant};
+  }
+
+  return {EnumValues.size(), nullptr};
+}
+
+void TaggedUnionMemberCountCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Root = Result.Nodes.getNodeAs<RecordDecl>(RootMatchBindName);
+  const auto *UnionField =
+      Result.Nodes.getNodeAs<FieldDecl>(UnionMatchBindName);
+  const auto *TagField = Result.Nodes.getNodeAs<FieldDecl>(TagMatchBindName);
+
+  assert(Root && "Root is missing!");
+  assert(UnionField && "UnionField is missing!");
+  assert(TagField && "TagField is missing!");
+  if (!Root || !UnionField || !TagField)
+    return;
+
+  const auto *UnionDef =
+      UnionField->getType().getCanonicalType().getTypePtr()->getAsRecordDecl();
+  const auto *EnumDef = llvm::dyn_cast<EnumDecl>(
+      TagField->getType().getCanonicalType().getTypePtr()->getAsTagDecl());
+
+  assert(UnionDef && "UnionDef is missing!");
+  assert(EnumDef && "EnumDef is missing!");
+  if (!UnionDef || !EnumDef)
+    return;
+
+  const std::size_t UnionMemberCount = llvm::range_size(UnionDef->fields());
+  auto [TagCount, CountingEnumConstantDecl] = getNumberOfEnumValues(EnumDef);
+
+  if (UnionMemberCount > TagCount) {
+    diag(Root->getLocation(),
+         "tagged union has more data members (%0) than tags (%1)!")
+        << UnionMemberCount << TagCount;
+  } else if (StrictMode && UnionMemberCount < TagCount) {
+    diag(Root->getLocation(),
+         "tagged union has fewer data members (%0) than tags (%1)!")
+        << UnionMemberCount << TagCount;
+  }
+
+  if (CountingEnumConstantDecl) {
+    diag(CountingEnumConstantDecl->getLocation(),
+         "assuming that this constant is just an auxiliary value and not "
+         "used for indicating a valid union data member",
+         DiagnosticIDs::Note);
+  }
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.h

@@ -0,0 +1,41 @@
+//===--- TaggedUnionMemberCountCheck.h - clang-tidy -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TAGGEDUNIONMEMBERCOUNTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TAGGEDUNIONMEMBERCOUNTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Gives warnings for tagged unions, where the number of tags is
+/// different from the number of data members inside the union.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/tagged-union-member-count.html
+class TaggedUnionMemberCountCheck : public ClangTidyCheck {
+public:
+  TaggedUnionMemberCountCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool StrictMode;
+  const bool EnableCountingEnumHeuristic;
+  const std::vector<StringRef> CountingEnumPrefixes;
+  const std::vector<StringRef> CountingEnumSuffixes;
+
+  std::pair<const std::size_t, const EnumConstantDecl *>
+  getNumberOfEnumValues(const EnumDecl *ED);
+  bool isCountingEnumLikeName(StringRef Name) const;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TAGGEDUNIONMEMBERCOUNTCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -103,6 +103,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-tagged-union-member-count
+  <clang-tidy/checks/bugprone/tagged-union-member-count>` check.
+
+  Gives warnings for tagged unions, where the number of tags is
+  different from the number of data members inside the union.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^