[clang] Extend -Wunused-but-set-variable to static globals (#178342)

This PR extends the capability of -Wunused-but-set-variable to track and
diagnose static global variables that are assigned values within a
function but whose values are never used. This change complements
-Wunused-variable, which detects static globals that are neither set nor
used.

I created this change with the help of claude for some initial guidance.

Fixes #148361

Author: jpjepko

clang/docs/ReleaseNotes.rst

@@ -230,6 +230,9 @@ Attribute Changes in Clang
 
 Improvements to Clang's diagnostics
 -----------------------------------
+- ``-Wunused-but-set-variable`` now diagnoses file-scope variables with
+  internal linkage (``static`` storage class) that are assigned but never used. (#GH148361)
+
 - Added ``-Wlifetime-safety`` to enable lifetime safety analysis,
   a CFG-based intra-procedural analysis that detects use-after-free and related
   temporal safety bugs. See the

clang/include/clang/AST/Decl.h

@@ -1212,6 +1212,21 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
       && !isFileVarDecl();
   }
 
+  /// Returns true if this is a file-scope variable with internal linkage.
+  bool isInternalLinkageFileVar() const {
+    // Calling isExternallyVisible() can trigger linkage computation/caching,
+    // which may produce stale results when a decl's DeclContext changes after
+    // creation (e.g., OpenMP declare mapper variables), so here we determine
+    // it syntactically instead.
+    if (!isFileVarDecl())
+      return false;
+    // Linkage is determined by enclosing class/namespace for static data
+    // members.
+    if (getStorageClass() == SC_Static && !isStaticDataMember())
+      return true;
+    return isInAnonymousNamespace();
+  }
+
   /// Returns true if a variable has extern or __private_extern__
   /// storage.
   bool hasExternalStorage() const {

clang/include/clang/Sema/Sema.h

@@ -973,6 +973,10 @@ class Sema final : public SemaBase {
   /// unit because its type has no linkage and it's not extern "C".
   bool isExternalWithNoLinkageType(const ValueDecl *VD) const;
 
+  /// Determines whether the given source location is in the main file
+  /// and we're in a context where we should warn about unused entities.
+  bool isMainFileLoc(SourceLocation Loc) const;
+
   /// Obtain a sorted list of functions that are undefined but ODR-used.
   void getUndefinedButUsed(
       SmallVectorImpl<std::pair<NamedDecl *, SourceLocation>> &Undefined);
@@ -7009,6 +7013,9 @@ class Sema final : public SemaBase {
   /// Increment when we find a reference; decrement when we find an ignored
   /// assignment.  Ultimately the value is 0 if every reference is an ignored
   /// assignment.
+  ///
+  /// Uses canonical VarDecl as key so in-class decls and out-of-class defs of
+  /// static data members get tracked as a single entry.
   llvm::DenseMap<const VarDecl *, int> RefsMinusAssignments;
 
   /// Used to control the generation of ExprWithCleanups.

clang/lib/Sema/Sema.cpp

@@ -956,6 +956,12 @@ bool Sema::isExternalWithNoLinkageType(const ValueDecl *VD) const {
          !isFunctionOrVarDeclExternC(VD);
 }
 
+bool Sema::isMainFileLoc(SourceLocation Loc) const {
+  if (TUKind != TU_Complete || getLangOpts().IsHeaderFile)
+    return false;
+  return SourceMgr.isInMainFile(Loc);
+}
+
 /// Obtains a sorted list of functions and variables that are undefined but
 /// ODR-used.
 void Sema::getUndefinedButUsed(
@@ -1604,6 +1610,40 @@ void Sema::ActOnEndOfTranslationUnit() {
     emitAndClearUnusedLocalTypedefWarnings();
   }
 
+  if (!Diags.isIgnored(diag::warn_unused_but_set_variable, SourceLocation())) {
+    // Diagnose unused-but-set static globals in a deterministic order.
+    // Not tracking shadowing info for static globals; there's nothing to
+    // shadow.
+    struct LocAndDiag {
+      SourceLocation Loc;
+      PartialDiagnostic PD;
+    };
+    SmallVector<LocAndDiag, 16> DeclDiags;
+    auto addDiag = [&DeclDiags](SourceLocation Loc, PartialDiagnostic PD) {
+      DeclDiags.push_back(LocAndDiag{Loc, std::move(PD)});
+    };
+
+    // For -Wunused-but-set-variable we only care about variables that were
+    // referenced by the TU end.
+    for (const auto &Ref : RefsMinusAssignments) {
+      const VarDecl *VD = Ref.first;
+      // Only diagnose internal linkage file vars defined in the main file to
+      // match -Wunused-variable behavior and avoid false positives from
+      // headers.
+      if (VD->isInternalLinkageFileVar() && isMainFileLoc(VD->getLocation()))
+        DiagnoseUnusedButSetDecl(VD, addDiag);
+    }
+
+    llvm::sort(DeclDiags,
+               [](const LocAndDiag &LHS, const LocAndDiag &RHS) -> bool {
+                 // Sorting purely for determinism; matches behavior in
+                 // Sema::ActOnPopScope.
+                 return LHS.Loc < RHS.Loc;
+               });
+    for (const LocAndDiag &D : DeclDiags)
+      Diag(D.Loc, D.PD);
+  }
+
   if (!Diags.isIgnored(diag::warn_unused_private_field, SourceLocation())) {
     // FIXME: Load additional unused private field candidates from the external
     // source.

clang/lib/Sema/SemaDecl.cpp

@@ -1912,14 +1912,6 @@ bool Sema::mightHaveNonExternalLinkage(const DeclaratorDecl *D) {
   return !D->isExternallyVisible();
 }
 
-// FIXME: This needs to be refactored; some other isInMainFile users want
-// these semantics.
-static bool isMainFileLoc(const Sema &S, SourceLocation Loc) {
-  if (S.TUKind != TU_Complete || S.getLangOpts().IsHeaderFile)
-    return false;
-  return S.SourceMgr.isInMainFile(Loc);
-}
-
 bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const {
   assert(D);
 
@@ -1946,7 +1938,7 @@ bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const {
         return false;
     } else {
       // 'static inline' functions are defined in headers; don't warn.
-      if (FD->isInlined() && !isMainFileLoc(*this, FD->getLocation()))
+      if (FD->isInlined() && !isMainFileLoc(FD->getLocation()))
         return false;
     }
 
@@ -1957,7 +1949,7 @@ bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const {
     // Constants and utility variables are defined in headers with internal
     // linkage; don't warn.  (Unlike functions, there isn't a convenient marker
     // like "inline".)
-    if (!isMainFileLoc(*this, VD->getLocation()))
+    if (!isMainFileLoc(VD->getLocation()))
       return false;
 
     if (Context.DeclMustBeEmitted(VD))
@@ -1971,7 +1963,7 @@ bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const {
         VD->getMemberSpecializationInfo() && !VD->isOutOfLine())
       return false;
 
-    if (VD->isInline() && !isMainFileLoc(*this, VD->getLocation()))
+    if (VD->isInline() && !isMainFileLoc(VD->getLocation()))
       return false;
   } else {
     return false;
@@ -2215,6 +2207,11 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD,
       return;
   }
 
+  // Don't warn on volatile file-scope variables. They are visible beyond their
+  // declaring function and writes to them could be observable side effects.
+  if (VD->getType().isVolatileQualified() && VD->isFileVarDecl())
+    return;
+
   // Don't warn about __block Objective-C pointer variables, as they might
   // be assigned in the block but not used elsewhere for the purpose of lifetime
   // extension.
@@ -2227,7 +2224,7 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD,
   if (VD->hasAttr<ObjCPreciseLifetimeAttr>() && Ty->isObjCObjectPointerType())
     return;
 
-  auto iter = RefsMinusAssignments.find(VD);
+  auto iter = RefsMinusAssignments.find(VD->getCanonicalDecl());
   if (iter == RefsMinusAssignments.end())
     return;
 
@@ -2305,9 +2302,11 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) {
       DiagnoseUnusedDecl(D, addDiag);
       if (const auto *RD = dyn_cast<RecordDecl>(D))
         DiagnoseUnusedNestedTypedefs(RD, addDiag);
-      if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
+      // Wait until end of TU to diagnose internal linkage file vars.
+      if (auto *VD = dyn_cast<VarDecl>(D);
+          VD && !VD->isInternalLinkageFileVar()) {
         DiagnoseUnusedButSetDecl(VD, addDiag);
-        RefsMinusAssignments.erase(VD);
+        RefsMinusAssignments.erase(VD->getCanonicalDecl());
       }
     }
 

clang/lib/Sema/SemaExpr.cpp

@@ -20477,8 +20477,15 @@ static void DoMarkVarDeclReferenced(
   bool UsableInConstantExpr =
       Var->mightBeUsableInConstantExpressions(SemaRef.Context);
 
-  if (Var->isLocalVarDeclOrParm() && !Var->hasExternalStorage()) {
-    RefsMinusAssignments.insert({Var, 0}).first->getSecond()++;
+  // Only track variables with internal linkage or local scope.
+  // Use canonical decl so in-class declarations and out-of-class definitions
+  // of static data members in anonymous namespaces are tracked as a single
+  // entry.
+  const VarDecl *CanonVar = Var->getCanonicalDecl();
+  if ((CanonVar->isLocalVarDeclOrParm() ||
+       CanonVar->isInternalLinkageFileVar()) &&
+      !CanonVar->hasExternalStorage()) {
+    RefsMinusAssignments.insert({CanonVar, 0}).first->getSecond()++;
   }
 
   // C++20 [expr.const]p12:

clang/lib/Sema/SemaExprCXX.cpp

@@ -7504,7 +7504,7 @@ static void MaybeDecrementCount(
   if ((IsCompoundAssign || isIncrementDecrementUnaryOp) &&
       VD->getType().isVolatileQualified())
     return;
-  auto iter = RefsMinusAssignments.find(VD);
+  auto iter = RefsMinusAssignments.find(VD->getCanonicalDecl());
   if (iter == RefsMinusAssignments.end())
     return;
   iter->getSecond()--;

clang/test/C/C2y/n3622.c

@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -verify=good -pedantic -Wall -std=c2y %s
-// RUN: %clang_cc1 -verify=compat,expected -pedantic -Wall -Wpre-c2y-compat -std=c2y %s
-// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -std=c23 %s
-// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -std=c17 %s
+// RUN: %clang_cc1 -verify=good -pedantic -Wall -Wno-unused-but-set-variable -std=c2y %s
+// RUN: %clang_cc1 -verify=compat,expected -pedantic -Wall -Wno-unused-but-set-variable -Wpre-c2y-compat -std=c2y %s
+// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -Wno-unused-but-set-variable -std=c23 %s
+// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -Wno-unused-but-set-variable -std=c17 %s
 // good-no-diagnostics
 
 /* WG14 N3622: Clang 22

clang/test/Sema/Inputs/warn-unused-but-set-static-global-header.h

@@ -0,0 +1,3 @@
+// Header file for testing that header-defined static globals don't warn.
+static int header_set_unused = 0;
+static void header_init() { header_set_unused = 1; }

clang/test/Sema/warn-unused-but-set-static-global.c

@@ -0,0 +1,132 @@
+// RUN: %clang_cc1 -fsyntax-only -Wunused-but-set-variable -I %S/Inputs -verify %s
+
+// Test that header-defined static globals don't warn.
+#include "warn-unused-but-set-static-global-header.h"
+
+#define NULL (void*)0
+
+void *set(int size);
+void func_call(void *);
+
+static int set_unused; // expected-warning {{variable 'set_unused' set but not used}}
+static int set_and_used;
+static int only_used;
+static int addr_taken;
+extern int external_var;  // No warning (external linkage).
+extern int global_var;  // No warning (not static).
+
+void f1() {
+  set_unused = 1;
+  set_and_used = 2;
+
+  int x = set_and_used;
+  (void)x;
+
+  int y = only_used;
+  (void)y;
+
+  int *p = &addr_taken;
+  (void)p;
+
+  external_var = 3;
+  global_var = 4;
+}
+
+// Test across multiple functions.
+static int set_used1;
+static int set_used2;
+
+static int set1; // expected-warning {{variable 'set1' set but not used}}
+static int set2; // expected-warning {{variable 'set2' set but not used}}
+
+void f2() {
+  set1 = 1;
+  set_used1 = 1;
+
+  int x = set_used2;
+  (void)x;
+}
+
+void f3() {
+  set2 = 2;
+  set_used2 = 2;
+
+  int x = set_used1;
+  (void)x;
+}
+
+static volatile int vol_set;
+void f4() {
+  vol_set = 1;
+}
+
+// Read and use
+static int compound; // expected-warning{{variable 'compound' set but not used}}
+static volatile int vol_compound;
+static int unary; // expected-warning{{variable 'unary' set but not used}}
+static volatile int vol_unary;
+void f5() {
+  compound += 1;
+  vol_compound += 1;
+  unary++;
+  vol_unary++;
+}
+
+struct S {
+  int i;
+};
+static struct S s_set;  // expected-warning{{variable 's_set' set but not used}}
+static struct S s_used;
+void f6() {
+  struct S t;
+  s_set = t;
+  t = s_used;
+}
+
+// Multiple assignments
+static int multi; // expected-warning{{variable 'multi' set but not used}}
+void f7() {
+  multi = 1;
+  multi = 2;
+  multi = 3;
+}
+
+// Unused pointers
+static int *unused_ptr; // expected-warning{{variable 'unused_ptr' set but not used}}
+static char *str_ptr; // expected-warning{{variable 'str_ptr' set but not used}}
+void f8() {
+  unused_ptr = set(5);
+  str_ptr = "hello";
+}
+
+// Used pointers
+void a(void *);
+static int *used_ptr;
+static int *param_ptr;
+static int *null_check_ptr;
+void f9() {
+  used_ptr = set(5);
+  *used_ptr = 5;
+
+  param_ptr = set(5);
+  func_call(param_ptr);
+
+  null_check_ptr = set(5);
+  if (null_check_ptr == NULL) {}
+}
+
+// Function pointers (unused)
+static void (*unused_func_ptr)(); // expected-warning {{variable 'unused_func_ptr' set but not used}}
+void SetUnusedCallback(void (*f)()) {
+  unused_func_ptr = f;
+}
+
+// Function pointers (used)
+static void (*used_func_ptr)();
+void SetUsedCallback(void (*f)()) {
+  used_func_ptr = f;
+}
+void CallUsedCallback() {
+  if (used_func_ptr)
+    used_func_ptr();
+}