Return available function types for BindingDecls.

[bazuzi]

Only return nullptr when we don't have an available QualType.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Samira Bazuzi (bazuzi)

Changes

Only return nullptr when we don't have an available QualType.

---

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

1 Files Affected:

• (modified) clang/lib/AST/DeclBase.cpp (+8-3)

diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
index 98a7746b7d997..129a119267890 100644
--- a/clang/lib/AST/DeclBase.cpp
+++ b/clang/lib/AST/DeclBase.cpp
@@ -1161,15 +1161,20 @@ int64_t Decl::getID() const {
 
 const FunctionType *Decl::getFunctionType(bool BlocksToo) const {
   QualType Ty;
-  if (isa<BindingDecl>(this))
-    return nullptr;
-  else if (const auto *D = dyn_cast<ValueDecl>(this))
+  if (const auto *D = dyn_cast<ValueDecl>(this))
     Ty = D->getType();
   else if (const auto *D = dyn_cast<TypedefNameDecl>(this))
     Ty = D->getUnderlyingType();
   else
     return nullptr;
 
+  if (Ty.isNull()) {
+    // BindingDecls do not have types during parsing, so return nullptr. This is
+    // the only known case where `Ty` is null.
+    assert(isa<BindingDecl>(this));
+    return nullptr;
+  }
+
   if (Ty->isFunctionPointerType())
     Ty = Ty->castAs<PointerType>()->getPointeeType();
   else if (Ty->isFunctionReferenceType())

[bazuzi]

Thanks for the quick review! Could you merge for me? I don't have write access.

[mizvekov]

Is this a bug fix? If so, do you have a test case for this?

If the change is supposed to be user or API visible, a release note explaining it would also be useful.

[bazuzi]

This is a bug fix, yes, but there are currently no tests at all for this function that I can find (and no test file corresponding to this file or its header), so I'm not sure what the scope of a new unit test should be. I also don't know how I would most realistically trigger the circumstance of a BindingDecl without a QualType set.

The recent changes to this function that introduced the issue, along with the previous two changes to this function (though 5 and 6 years ago now), do not include any tests or any mention of the changes in release notes (#89906 and #90495). I'm not aware of what magnitude of change qualifies for release notes, but this change seems pretty small to me.

[mizvekov]

Do you have a C++ source file example which presents misbehaviour when compiled through clang, which this change fixes?

If so, you can reduce that example and add is as a regression test. These go in clang/test/ and are basically composed of C++ source and some annotations to verify we are doing the right thing with them.

If you can post the example, we can give further advice.

[bazuzi]

For the following:

template <typename A, typename B>
struct Pair {
  Pair();

  A a;
  B b;
};

void target(int *i) {
  Pair<void (*)(int *), bool> p;
  auto [fp, b] = p;
  fp(i);
}

Running or compiling this code presents no misbehavior that I know of, but when running a dataflow analysis that queries getFunctionType() for fp, nullptr was returned, when in fact there was a valid type expected, roughly void (*)(int*).

[mizvekov]

Is this a dataflow analysis in upstream clang, or is this something you are developing?

[bazuzi]

This is a pointer nullability analysis being developed outside of clang. The relevant analysis code, including our workaround, can be seen at https://github.com/google/crubit/blob/main/nullability/inference/collect_evidence.cc#L757.

[bazuzi]

The fact that the code that revealed this bug was in a dataflow analysis isn't really relevant to the fix. I've added a unit test file with a new test limited only to the modified behavior of getFunctionType for BindingDecls.

[bazuzi]

Any additional questions on this @mizvekov?

[cor3ntin]

Nice tests, thanks. Do you need me to merge for you?

[bazuzi]

Yes, thanks!