[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)

[Artem Dergachev via cfe-commits]

================
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     }
   }
 }
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+                                   UnsafeBufferUsageHandler &Handler,
+                                   bool EmitSuggestions) {
+#ifndef NDEBUG
+  Handler.clearDebugNotes();
+#endif
+
+  assert(D);
+
+  SmallVector<Stmt *> Stmts;
+
+  // We do not want to visit a Lambda expression defined inside a method
+  // independently. Instead, it should be visited along with the outer method.
+  // FIXME: do we want to do the same thing for `BlockDecl`s?
+  if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
+    if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+      return;
+  }
+
+  // Do not emit fixit suggestions for functions declared in an
+  // extern "C" block.
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    for (FunctionDecl *FReDecl : FD->redecls()) {
+      if (FReDecl->isExternC()) {
+        EmitSuggestions = false;
+        break;
+      }
+    }
+
+    Stmts.push_back(FD->getBody());
+
+    if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) {
+      for (const CXXCtorInitializer *CI : ID->inits()) {
+        Stmts.push_back(CI->getInit());
+      }
+    }
+  }
+
+  if (const auto *FD = dyn_cast<FieldDecl>(D)) {
----------------
haoNoQ wrote:

> AFAICT we are required to visit FieldDecl to handle the 0-ctor class use case.

I really want us to momentarily stop being preoccupied with "can" and focus on "should". This isn't a technical problem, this is a UI/UX problem.

There's no constructor. There's no binary code for constructor. The unsafe function is not invoked from any binary code generated from your example. It's the responsibility of the code that aggregate-initializes your aggregate to generate the call to the default initializer, so naturally it doesn't show up just yet.

The class is safe to use whenever the field is initialized explicitly. If they remove the field initializer, that'll break backwards compatibility. Maybe it's ok to keep the class as-is and use the attribute only as a reminder to the callers to call a better constructor.

Ultimately I think you're right and in most of such cases the best solution is to change the class to use a different, safe default initializer for the field. Which presumably exists, given that the attribute was placed on the currently used constructor. I think it's true that more users would complaint about "false negative" in the aggregate definition, or an awkwardly placed / hard-to-read warning at the curly braces (especially if the unsafe initializer is layered into several other layers of aggregates) than complain about false positive under "the class is intended to be safe as long as the field initializer is passed explicitly".

But this is something we should occasionally think about, and treat this as a very very special cornercase. Which doesn't even necessarily require an immediate solution - given that all unsafe code will eventually be flagged regardless. So I think if we go for explicitly supporting fields, we should probably exclude all non-aggregate classes from the callback, and leave an irritated comment explaining why we eventually agreed that it's a good idea 😅 

https://github.com/llvm/llvm-project/pull/91991