[clang][nullability] Don't return null fields from getReferencedDecls().

[martinboehme]

The patch includes a repro for a case where we were returning a null FieldDecl
when calling getReferencedDecls() on the InitListExpr for a union.

Also, I noticed while working on this that RecordInitListHelper has a bug
where it doesn't work correctly for empty unions. This patch also includes a
repro and fix for this bug.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

The patch includes a repro for a case where we were returning a null FieldDecl
when calling getReferencedDecls() on the InitListExpr for a union.

Also, I noticed while working on this that RecordInitListHelper has a bug
where it doesn't work correctly for empty unions. This patch also includes a
repro and fix for this bug.

---

Full diff: https://github.com/llvm/llvm-project/pull/94983.diff

5 Files Affected:

• (modified) clang/docs/tools/clang-formatted-files.txt (+1)
• (modified) clang/lib/Analysis/FlowSensitive/ASTOps.cpp (+7-4)
• (added) clang/unittests/Analysis/FlowSensitive/ASTOpsTest.cpp (+88)
• (modified) clang/unittests/Analysis/FlowSensitive/CMakeLists.txt (+1)
• (modified) llvm/utils/gn/secondary/clang/unittests/Analysis/FlowSensitive/BUILD.gn (+1)

diff --git a/clang/docs/tools/clang-formatted-files.txt b/clang/docs/tools/clang-formatted-files.txt
index dee51e402b687..4866bd4aee634 100644
--- a/clang/docs/tools/clang-formatted-files.txt
+++ b/clang/docs/tools/clang-formatted-files.txt
@@ -622,6 +622,7 @@ clang/tools/libclang/CXCursor.h
 clang/tools/scan-build-py/tests/functional/src/include/clean-one.h
 clang/unittests/Analysis/CFGBuildResult.h
 clang/unittests/Analysis/MacroExpansionContextTest.cpp
+clang/unittests/Analysis/FlowSensitive/ASTOpsTest.cpp
 clang/unittests/Analysis/FlowSensitive/CNFFormula.cpp
 clang/unittests/Analysis/FlowSensitive/DataflowAnalysisContextTest.cpp
 clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
diff --git a/clang/lib/Analysis/FlowSensitive/ASTOps.cpp b/clang/lib/Analysis/FlowSensitive/ASTOps.cpp
index 38b5f51b7b2f0..27d42a7b50856 100644
--- a/clang/lib/Analysis/FlowSensitive/ASTOps.cpp
+++ b/clang/lib/Analysis/FlowSensitive/ASTOps.cpp
@@ -100,7 +100,8 @@ getFieldsForInitListExpr(const InitListT *InitList) {
   std::vector<const FieldDecl *> Fields;
 
   if (InitList->getType()->isUnionType()) {
-    Fields.push_back(InitList->getInitializedFieldInUnion());
+    if (const FieldDecl *Field = InitList->getInitializedFieldInUnion())
+      Fields.push_back(Field);
     return Fields;
   }
 
@@ -137,9 +138,11 @@ RecordInitListHelper::RecordInitListHelper(
   // it doesn't do this -- so we create an `ImplicitValueInitExpr` ourselves.
   SmallVector<Expr *> InitsForUnion;
   if (Ty->isUnionType() && Inits.empty()) {
-    assert(Fields.size() == 1);
-    ImplicitValueInitForUnion.emplace(Fields.front()->getType());
-    InitsForUnion.push_back(&*ImplicitValueInitForUnion);
+    assert(Fields.size() <= 1);
+    if (!Fields.empty()) {
+      ImplicitValueInitForUnion.emplace(Fields.front()->getType());
+      InitsForUnion.push_back(&*ImplicitValueInitForUnion);
+    }
     Inits = InitsForUnion;
   }
 
diff --git a/clang/unittests/Analysis/FlowSensitive/ASTOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/ASTOpsTest.cpp
new file mode 100644
index 0000000000000..cd1c076ab09e6
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/ASTOpsTest.cpp
@@ -0,0 +1,88 @@
+//===- unittests/Analysis/FlowSensitive/ASTOpsTest.cpp --------------------===//
+//
+// 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 "clang/Analysis/FlowSensitive/ASTOps.h"
+#include "TestingSupport.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <memory>
+
+namespace {
+
+using namespace clang;
+using namespace dataflow;
+
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::initListExpr;
+using ast_matchers::match;
+using ast_matchers::selectFirst;
+using test::findValueDecl;
+using testing::IsEmpty;
+using testing::UnorderedElementsAre;
+
+TEST(ASTOpsTest, RecordInitListHelperOnEmptyUnionInitList) {
+  // This is a regression test: The `RecordInitListHelper` used to assert-fail
+  // when called for the `InitListExpr` of an empty union.
+  std::string Code = R"cc(
+    struct S {
+      S() : UField{} {};
+
+      union U {} UField;
+    };
+  )cc";
+  std::unique_ptr<ASTUnit> Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++17"});
+  auto &ASTCtx = Unit->getASTContext();
+
+  ASSERT_EQ(ASTCtx.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  auto *InitList = selectFirst<InitListExpr>(
+      "init",
+      match(initListExpr(hasType(cxxRecordDecl(hasName("U")))).bind("init"),
+            ASTCtx));
+  ASSERT_NE(InitList, nullptr);
+
+  RecordInitListHelper Helper(InitList);
+  EXPECT_THAT(Helper.base_inits(), IsEmpty());
+  EXPECT_THAT(Helper.field_inits(), IsEmpty());
+}
+
+TEST(ASTOpsTest, ReferencedDeclsOnUnionInitList) {
+  // This is a regression test: `getReferencedDecls()` used to return a null
+  // `FieldDecl` in this case (in addition to the correct non-null `FieldDecl`)
+  // because `getInitializedFieldInUnion()` returns null for the syntactic form
+  // of the `InitListExpr`.
+  std::string Code = R"cc(
+    struct S {
+      S() : UField{0} {};
+
+      union U {
+        int I;
+      } UField;
+    };
+  )cc";
+  std::unique_ptr<ASTUnit> Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++17"});
+  auto &ASTCtx = Unit->getASTContext();
+
+  ASSERT_EQ(ASTCtx.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  auto *InitList = selectFirst<InitListExpr>(
+      "init",
+      match(initListExpr(hasType(cxxRecordDecl(hasName("U")))).bind("init"),
+            ASTCtx));
+  ASSERT_NE(InitList, nullptr);
+  auto *IDecl = cast<FieldDecl>(findValueDecl(ASTCtx, "I"));
+
+  EXPECT_THAT(getReferencedDecls(*InitList).Fields,
+              UnorderedElementsAre(IDecl));
+}
+
+} // namespace
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index cfabb80576bc1..12fee5dc2789c 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS
 
 add_clang_unittest(ClangAnalysisFlowSensitiveTests
   ArenaTest.cpp
+  ASTOpsTest.cpp
   CFGMatchSwitchTest.cpp
   ChromiumCheckModelTest.cpp
   DataflowAnalysisContextTest.cpp
diff --git a/llvm/utils/gn/secondary/clang/unittests/Analysis/FlowSensitive/BUILD.gn b/llvm/utils/gn/secondary/clang/unittests/Analysis/FlowSensitive/BUILD.gn
index e16ca31b81a8d..780a69f1f3299 100644
--- a/llvm/utils/gn/secondary/clang/unittests/Analysis/FlowSensitive/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang/unittests/Analysis/FlowSensitive/BUILD.gn
@@ -18,6 +18,7 @@ unittest("ClangAnalysisFlowSensitiveTests") {
     "//llvm/lib/Testing/Support",
   ]
   sources = [
+    "ASTOpsTest.cpp",
     "ArenaTest.cpp",
     "CFGMatchSwitchTest.cpp",
     "ChromiumCheckModelTest.cpp",

[ymand]

Thanks! I'm once again struck by how much this project would benefit from Nullability annotations/checking.

[martinboehme]

Thanks! I'm once again struck by how much this project would benefit from Nullability annotations/checking.

Indeed -- that was my thought too.