Improve diagnostics when compared format strings have different numbers of specifiers

Author: apple-fcloutier

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -10018,8 +10018,8 @@ def note_previous_declaration_as : Note<
 
 def warn_printf_insufficient_data_args : Warning<
   "more '%%' conversions than data arguments">, InGroup<FormatInsufficientArgs>;
-def warn_format_cmp_insufficient_specifiers : Warning<
-  "fewer specifiers in format string than expected">, InGroup<FormatInsufficientArgs>;
+def warn_format_cmp_specifier_arity : Warning<
+  "%select{fewer|more}0 specifiers in format string than expected">, InGroup<FormatInsufficientArgs>;
 def warn_printf_data_arg_not_used : Warning<
   "data argument not used by format string">, InGroup<FormatExtraArgs>;
 def warn_format_invalid_conversion : Warning<

clang/lib/Sema/SemaChecking.cpp

@@ -8376,22 +8376,27 @@ bool CheckScanfHandler::HandleScanfSpecifier(
 static void CompareFormatSpecifiers(Sema &S, const StringLiteral *Ref,
                                     ArrayRef<EquatableFormatArgument> RefArgs,
                                     const StringLiteral *Fmt,
-                                    ArrayRef<EquatableFormatArgument> FmtArgs) {
+                                    ArrayRef<EquatableFormatArgument> FmtArgs,
+                                    const Expr *FmtExpr, bool InFunctionCall) {
   unsigned CommonArgs;
-  if (RefArgs.size() > FmtArgs.size()) {
-    CommonArgs = FmtArgs.size();
-    // "data argument not used by format string"
-    const auto &Arg = RefArgs[FmtArgs.size()];
-    S.Diag(Arg.getSourceLocation(), diag::warn_printf_data_arg_not_used)
-        << Arg.getSourceRange();
-    S.Diag(Fmt->getBeginLoc(), diag::note_format_cmp_with) << 1;
-  } else if (FmtArgs.size() > RefArgs.size()) {
+  if (FmtArgs.size() > RefArgs.size()) {
     CommonArgs = RefArgs.size();
-    // "more % conversions than data arguments"
-    const auto &Arg = FmtArgs[RefArgs.size()];
-    S.Diag(Fmt->getBeginLoc(), diag::warn_format_cmp_insufficient_specifiers)
-        << Arg.getSourceRange();
+    // "data argument not used by format string"
+    CheckFormatHandler::EmitFormatDiagnostic(
+        S, InFunctionCall, FmtExpr,
+        S.PDiag(diag::warn_format_cmp_specifier_arity) << 1,
+        FmtExpr->getBeginLoc(), false,
+        FmtArgs[RefArgs.size()].getSourceRange());
     S.Diag(Ref->getBeginLoc(), diag::note_format_cmp_with) << 1;
+  } else if (RefArgs.size() > FmtArgs.size()) {
+    CommonArgs = FmtArgs.size();
+    // "fewer specifiers in format string than expected"
+    CheckFormatHandler::EmitFormatDiagnostic(
+        S, InFunctionCall, FmtExpr,
+        S.PDiag(diag::warn_format_cmp_specifier_arity) << 0,
+        FmtExpr->getBeginLoc(), false, Fmt->getSourceRange());
+    S.Diag(Ref->getBeginLoc(), diag::note_format_cmp_with)
+        << 1 << RefArgs[FmtArgs.size()].getSourceRange();
   } else {
     CommonArgs = FmtArgs.size();
   }
@@ -8477,7 +8482,8 @@ static void CheckFormatString(
           DecomposePrintfHandler::GetSpecifiers(S, FExpr, Type, IsObjC,
                                                 inFunctionCall, FmtArgs)) {
         CompareFormatSpecifiers(S, ReferenceFormatString, RefArgs,
-                                FExpr->getFormatString(), FmtArgs);
+                                FExpr->getFormatString(), FmtArgs,
+                                Args[format_idx], inFunctionCall);
       }
     }
   } else if (Type == Sema::FST_Scanf) {

clang/test/Sema/format-string-matches.c

@@ -41,11 +41,10 @@ void vformat(const char *fmt, ...) {
 // MARK: -
 // Calling a function with format_matches diagnoses for incompatible formats.
 
-void cvt_percent(const char *c) __attribute__((format_matches(printf, 1, "%%"))); // expected-note{{comparing with this format string}}
+void cvt_percent(const char *c) __attribute__((format_matches(printf, 1, "%%"))); // expected-note 2{{comparing with this format string}}
 void cvt_at(const char *c) __attribute__((format_matches(NSString, 1, "%@"))); // \
-    expected-warning{{data argument not used by format string}} \
     expected-note{{comparing with this specifier}} \
-    expected-note{{comparing with this format string}}
+    expected-note 3{{comparing with this format string}}
 void cvt_c(const char *c) __attribute__((format_matches(printf, 1, "%c"))); // expected-note{{comparing with this specifier}}
 void cvt_u(const char *c) __attribute__((format_matches(printf, 1, "%u"))); // expected-note{{comparing with this specifier}}
 void cvt_hhi(const char *c) __attribute__((format_matches(printf, 1, "%hhi")));  // expected-note 3{{comparing with this specifier}}
@@ -75,12 +74,18 @@ void test_compatibility(void) {
     cvt_at("%p"); // expected-warning{{format specifier 'p' is incompatible with '@'}}
 
     cvt_percent("hello");
-    cvt_percent("%c"); // expected-warning{{fewer specifiers in format string than expected}}
+    cvt_percent("%c"); // expected-warning{{more specifiers in format string than expected}}
+
+    const char *const too_many = "%c"; // expected-note{{format string is defined here}}
+    cvt_percent(too_many); // expected-warning{{more specifiers in format string than expected}}
 }
 
 void test_too_few_args(void) {
-    cvt_at("a"); // expected-note{{comparing with this format string}}
-    cvt_at("%@ %@"); // expected-warning{{fewer specifiers in format string than expected}}
+    cvt_at("a"); // expected-warning{{fewer specifiers in format string than expected}}
+    cvt_at("%@ %@"); // expected-warning{{more specifiers in format string than expected}}
+
+    const char *const too_few = "a"; // expected-note{{format string is defined here}}
+    cvt_at(too_few); // expected-warning{{fewer specifiers in format string than expected}}
 }
 
 void cvt_several(const char *c) __attribute__((format_matches(printf, 1, "%f %i %s"))); // expected-note{{comparing with this specifier}}