[Clang][DiagnosticSemaKinds] combine diagnostic texts

[nickdesaulniers]

Summary

The diagnostic texts for warning on attributes that don't appear on the
initial declaration is generally useful. We'd like to re-use it in
D106030, but first let's combine two that already are very similar so we
may re-use it a third time in that commit.

Also, fix a few places that were using notePreviousDefinition to point
to declarations, to instead use diag::note_previous_declaration.

[aaron.ballman]

LGTM with some minor tweaks to avoid hasAttr followed by getAttr. Thanks!

clang/lib/Sema/SemaDecl.cpp

3355–3356

4168–4169

[nickdesaulniers]

• prefer getAttr to hasAttr+getAttr, remove another diag

[nickdesaulniers]

clang/lib/Sema/SemaDecl.cpp

3685

ah, this isn't quite right. It replaces warning of the first declaration with warning of the first definition.

This should instead use

Diag(Old->getLocation(), diag::note_previous_declaration);

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

3685

Good catch! I think we want to use note_previous_declaration in all three cases though, don't we? It's a bit weird that we diagnose "attribute not present on first declaration" but then try to note a definition. e.g., https://godbolt.org/z/jd9oG96sT

I know it's a bit of a drive-by, but I think it's probably worth correcting that as well. WDYT?

[nickdesaulniers]

clang/lib/Sema/SemaDecl.cpp

3685

Looks like we do that for WeakImportAttr, too. Will fix all cases and amend description.

[nickdesaulniers]

• more use of note_previous_declaration, fix WeakImportAttr, too

[nickdesaulniers]

clang/lib/Sema/SemaDecl.cpp

3686

should CXX11NoReturnAttr be dropAttr on New like the other cases, too?

[aaron.ballman]

LGTM!

clang/lib/Sema/SemaDecl.cpp

3686

I think it's better to leave it -- the user gets an error so they don't get object code out of it, but leaving the attribute as-if the user had specified everything properly means later diagnostics may behave a bit better. Probably doesn't matter all that much in this case, though, so I don't feel strongly.

[dschuff]

It looks like this error is intended to catch mismatches for attributes that can affect codegen such as noreturn (in which case it makes sense to have it as an error) but it also now fires for cases such as __attribute__(warning()) which often do not duplicate the attribute on the definition. Is that intended?

[nickdesaulniers]

In D107613#3040402, @dschuff wrote:

It looks like this error is intended to catch mismatches for attributes that can affect codegen such as noreturn (in which case it makes sense to have it as an error) but it also now fires for cases such as __attribute__(warning()) which often do not duplicate the attribute on the definition. Is that intended?

Duplicating the attribute on the definition is not needed for __attribute__(warning()).

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_undefined_internal_type : Extension<
   "ISO C++ requires a definition in this translation unit for "
   "%select{function|variable}0 %q1 because its type does not have linkage">,
   InGroup<DiagGroup<"undefined-internal-type">>;
 def warn_undefined_inline : Warning<"inline function %q0 is not defined">,
   InGroup<DiagGroup<"undefined-inline">>;
 def err_undefined_inline_var : Error<"inline variable %q0 is not defined">;
 def note_used_here : Note<"used here">;
 
-def err_internal_linkage_redeclaration : Error<
-  "'internal_linkage' attribute does not appear on the first declaration of %0">;
+def err_attribute_missing_on_first_decl : Error<
+  "%0 attribute does not appear on the first declaration">;
 def warn_internal_linkage_local_storage : Warning<
   "'internal_linkage' attribute on a non-static local variable is ignored">,
   InGroup<IgnoredAttributes>;
 
 def ext_internal_in_extern_inline : ExtWarn<
   "static %select{function|variable}0 %1 is used in an inline function with "
   "external linkage">, InGroup<StaticInInline>;
 def ext_internal_in_extern_inline_quiet : Extension<

 def warn_noreturn_function_has_return_expr : Warning<
   "function %0 declared 'noreturn' should not return">,
   InGroup<InvalidNoreturn>;
 def warn_falloff_noreturn_function : Warning<
   "function declared 'noreturn' should not return">,
   InGroup<InvalidNoreturn>;
 def err_noreturn_block_has_return_expr : Error<
   "block declared 'noreturn' should not return">;
-def err_noreturn_missing_on_first_decl : Error<
-  "function declared '[[noreturn]]' after its first declaration">;
-def note_noreturn_missing_first_decl : Note<
-  "declaration missing '[[noreturn]]' attribute is here">;
 def err_carries_dependency_missing_on_first_decl : Error<
   "%select{function|parameter}0 declared '[[carries_dependency]]' "
   "after its first declaration">;
 def note_carries_dependency_missing_first_decl : Note<
   "declaration missing '[[carries_dependency]]' attribute is here">;
 def err_carries_dependency_param_not_function_decl : Error<
   "'[[carries_dependency]]' attribute only allowed on parameter in a function "
   "declaration or lambda">;

clang/lib/Sema/SemaDecl.cpp

if (getLangOpts().MicrosoftExt) {
       Diag(New->getLocation(), diag::ext_static_non_static) << New;
       Diag(OldLocation, PrevDiag);
     } else {
       Diag(New->getLocation(), diag::err_static_non_static) << New;
       Diag(OldLocation, PrevDiag);
       return true;
     }
   }
 
-  if (New->hasAttr<InternalLinkageAttr>() &&
-      !Old->hasAttr<InternalLinkageAttr>()) {
-    Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
-        << New->getDeclName();
-    notePreviousDefinition(Old, New->getLocation());
+  if (const auto *ILA = New->getAttr<InternalLinkageAttr>())
+    if (!Old->hasAttr<InternalLinkageAttr>()) {
+      Diag(New->getLocation(), diag::err_attribute_missing_on_first_decl)
+          << ILA;
+      Diag(Old->getLocation(), diag::note_previous_declaration);
       New->dropAttr<InternalLinkageAttr>();
     }
 
   if (CheckRedeclarationModuleOwnership(New, Old))
     return true;
 
   if (!getLangOpts().CPlusPlus) {
     bool OldOvl = Old->hasAttr<OverloadableAttr>();
     if (OldOvl != New->hasAttr<OverloadableAttr>() && !Old->isImplicit()) {
       Diag(New->getLocation(), diag::err_attribute_overloadable_mismatch)
if (OldMethod && NewMethod) {
         return true;
       }
     }
 
     // C++11 [dcl.attr.noreturn]p1:
     //   The first declaration of a function shall specify the noreturn
     //   attribute if any declaration of that function specifies the noreturn
     //   attribute.
-    const CXX11NoReturnAttr *NRA = New->getAttr<CXX11NoReturnAttr>();
-    if (NRA && !Old->hasAttr<CXX11NoReturnAttr>()) {
-      Diag(NRA->getLocation(), diag::err_noreturn_missing_on_first_decl);
-      Diag(Old->getFirstDecl()->getLocation(),
-           diag::note_noreturn_missing_first_decl);
+    if (const auto *NRA = New->getAttr<CXX11NoReturnAttr>())
+      if (!Old->hasAttr<CXX11NoReturnAttr>()) {
+        Diag(NRA->getLocation(), diag::err_attribute_missing_on_first_decl)
+            << NRA;
+        Diag(Old->getLocation(), diag::note_previous_declaration);
       }
 
     // C++11 [dcl.attr.depend]p2:
     //   The first declaration of a function shall specify the
     //   carries_dependency attribute for its declarator-id if any declaration
     //   of the function specifies the carries_dependency attribute.
     const CarriesDependencyAttr *CDA = New->getAttr<CarriesDependencyAttr>();
     if (CDA && !Old->hasAttr<CarriesDependencyAttr>()) {
       Diag(CDA->getLocation(),
void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
 
   mergeDeclAttributes(New, Old);
   // Warn if an already-declared variable is made a weak_import in a subsequent
   // declaration
   if (New->hasAttr<WeakImportAttr>() &&
       Old->getStorageClass() == SC_None &&
       !Old->hasAttr<WeakImportAttr>()) {
     Diag(New->getLocation(), diag::warn_weak_import) << New->getDeclName();
-    notePreviousDefinition(Old, New->getLocation());
+    Diag(Old->getLocation(), diag::note_previous_declaration);
     // Remove weak_import attribute on new declaration.
     New->dropAttr<WeakImportAttr>();
   }
 
-  if (New->hasAttr<InternalLinkageAttr>() &&
-      !Old->hasAttr<InternalLinkageAttr>()) {
-    Diag(New->getLocation(), diag::err_internal_linkage_redeclaration)
-        << New->getDeclName();
-    notePreviousDefinition(Old, New->getLocation());
+  if (const auto *ILA = New->getAttr<InternalLinkageAttr>())
+    if (!Old->hasAttr<InternalLinkageAttr>()) {
+      Diag(New->getLocation(), diag::err_attribute_missing_on_first_decl)
+          << ILA;
+      Diag(Old->getLocation(), diag::note_previous_declaration);
       New->dropAttr<InternalLinkageAttr>();
     }
 
   // Merge the types.
   VarDecl *MostRecent = Old->getMostRecentDecl();
   if (MostRecent != Old) {
     MergeVarDeclTypes(New, MostRecent,
                       mergeTypeWithPrevious(*this, New, MostRecent, Previous));
     if (New->isInvalidDecl())
       return;

clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.noreturn/p1.cpp

 
 [[noreturn()]] void c(); // expected-error {{attribute 'noreturn' cannot have an argument list}}
 
 void d() [[noreturn]]; // expected-error {{'noreturn' attribute cannot be applied to types}}
 int d2 [[noreturn]]; // expected-error {{'noreturn' attribute only applies to functions}}
 
 [[noreturn]] int e() { b2(); } // ok
 
-int f(); // expected-note {{declaration missing '[[noreturn]]' attribute is here}}
-[[noreturn]] int f(); // expected-error {{function declared '[[noreturn]]' after its first declaration}}
+int f(); // expected-note {{previous declaration is here}}
+[[noreturn]] int f(); // expected-error {{'noreturn' attribute does not appear on the first declaration}}
 int f();
 
 [[noreturn]] int g();
 int g() { while (true) b(); } // ok
 [[noreturn]] int g();
 
 [[gnu::noreturn]] int h();
 

clang/test/Sema/attr-weak.c

 
 struct __attribute__((weak)) s0 {}; // expected-warning {{'weak' attribute only applies to variables, functions, and classes}}
 struct __attribute__((weak_import)) s1 {}; // expected-warning {{'weak_import' attribute only applies to variables and functions}}
 
 static int f() __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
 static int x __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
 
 // rdar://9538608
-int C; // expected-note {{previous definition is here}}
+int C; // expected-note {{previous declaration is here}}
 extern int C __attribute__((weak_import)); // expected-warning {{an already-declared variable is made a weak_import declaration}}
 
 static int pr14946_x;
 extern int pr14946_x  __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}
 
 static void pr14946_f();
 void pr14946_f() __attribute__((weak)); // expected-error {{weak declaration cannot have internal linkage}}

clang/test/Sema/internal_linkage.c

 // RUN: %clang_cc1 -fsyntax-only -verify -fdouble-square-bracket-attributes %s
 
 int var __attribute__((internal_linkage));
 int var2 __attribute__((internal_linkage,common)); // expected-error{{'common' and 'internal_linkage' attributes are not compatible}} \
                                                    // expected-note{{conflicting attribute is here}}
 int var3 __attribute__((common,internal_linkage)); // expected-error{{'internal_linkage' and 'common' attributes are not compatible}} \
                                                    // expected-note{{conflicting attribute is here}}
 
-int var4 __attribute__((common)); // expected-note{{previous definition is here}} expected-note{{conflicting attribute is here}}
+int var4 __attribute__((common)); // expected-note{{previous declaration is here}} expected-note{{conflicting attribute is here}}
 int var4 __attribute__((internal_linkage)); // expected-error{{'internal_linkage' and 'common' attributes are not compatible}} \
-                                            // expected-error{{'internal_linkage' attribute does not appear on the first declaration of 'var4'}}
+                                            // expected-error{{'internal_linkage' attribute does not appear on the first declaration}}
 
 int var5 __attribute__((internal_linkage)); // expected-note{{conflicting attribute is here}}
 int var5 __attribute__((common)); // expected-error{{'common' and 'internal_linkage' attributes are not compatible}}
 
 __attribute__((internal_linkage)) int f() {}
 struct __attribute__((internal_linkage)) S { // expected-warning{{'internal_linkage' attribute only applies to variables, functions, and classes}}
 };
 
 __attribute__((internal_linkage("foo"))) int g() {} // expected-error{{'internal_linkage' attribute takes no arguments}}
 
 int var6 [[clang::internal_linkage]];
 int var7 [[clang::internal_linkage]] __attribute__((common)); // expected-error{{'internal_linkage' and 'common' attributes are not compatible}} \
                                                    // expected-note{{conflicting attribute is here}}
 __attribute__((common)) int var8 [[clang::internal_linkage]]; // expected-error{{'internal_linkage' and 'common' attributes are not compatible}} \
                                                    // expected-note{{conflicting attribute is here}}

clang/test/SemaCXX/internal_linkage.cpp

 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
 
 int f() __attribute__((internal_linkage));
 
 class A;
 class __attribute__((internal_linkage)) A {
 public:
   int x __attribute__((internal_linkage)); // expected-warning{{'internal_linkage' attribute only applies to variables, functions, and classes}}
   static int y __attribute__((internal_linkage));
   void f1() __attribute__((internal_linkage));
   void f2() __attribute__((internal_linkage)) {}
   static void f3() __attribute__((internal_linkage)) {}
-  void f4(); // expected-note{{previous definition is here}}
-  static int zz; // expected-note{{previous definition is here}}
+  void f4(); // expected-note{{previous declaration is here}}
+  static int zz; // expected-note{{previous declaration is here}}
   A() __attribute__((internal_linkage)) {}
   ~A() __attribute__((internal_linkage)) {}
   A& operator=(const A&) __attribute__((internal_linkage)) { return *this; }
   struct {
     int z  __attribute__((internal_linkage)); // expected-warning{{'internal_linkage' attribute only applies to}}
   };
 };
 
-__attribute__((internal_linkage)) void A::f4() {} // expected-error{{'internal_linkage' attribute does not appear on the first declaration of 'f4'}}
+__attribute__((internal_linkage)) void A::f4() {} // expected-error{{'internal_linkage' attribute does not appear on the first declaration}}
 
-__attribute__((internal_linkage)) int A::zz; // expected-error{{'internal_linkage' attribute does not appear on the first declaration of 'zz'}}
+__attribute__((internal_linkage)) int A::zz; // expected-error{{'internal_linkage' attribute does not appear on the first declaration}}
 
 namespace Z __attribute__((internal_linkage)) { // expected-warning{{'internal_linkage' attribute only applies to}}
 }
 
 __attribute__((internal_linkage("foo"))) int g() {} // expected-error{{'internal_linkage' attribute takes no arguments}}
 
 [[clang::internal_linkage]] int h() {}