Update declaration message of extern linkage

[Krishna-13-cyber]

Summary

This patch aims to improve the diagnostic to much better precision of indicating extern declaration being followed after static declaration rather than just generic non-static declaration.

Example Testcase: https://godbolt.org/z/zMTda6MGG

[tbaeder]

clang/test/SemaCXX/extern_static.cpp

2

That's unnecessary, isn't it?

[Krishna-13-cyber]

• Updated with removing redundant flags

[cjdb]

I think we should fundamentally rethink this entire category of diagnostic. Rather than having a static diagnostic per offence stating what happened, we should instead have a single diagnostic that captures both what happened, why it's bad, and how to fix it. Something along the lines of

error: 'x' has been declared with incompatible linkage specifiers (static and extern); please pick exactly one
  extern int x;
  ^~~~~~
note: previous definition here
  static int x;
  ^~~~~~

It'd also be more robust from an engineering perspective, since it means that we won't need to add new diagnostic permutations every time a new linkage specifier is added.

[Krishna-13-cyber]

In D147888#4274763, @cjdb wrote:

I think we should fundamentally rethink this entire category of diagnostic. Rather than having a static diagnostic per offence stating what happened, we should instead have a single diagnostic that captures both what happened, why it's bad, and how to fix it. Something along the lines of

error: 'x' has been declared with incompatible linkage specifiers (static and extern); please pick exactly one
  extern int x;
  ^~~~~~
note: previous definition here
  static int x;
  ^~~~~~

It'd also be more robust from an engineering perspective, since it means that we won't need to add new diagnostic permutations every time a new linkage specifier is added.

In D147888#4274763, @cjdb wrote:

I think we should fundamentally rethink this entire category of diagnostic. Rather than having a static diagnostic per offence stating what happened, we should instead have a single diagnostic that captures both what happened, why it's bad, and how to fix it. Something along the lines of

error: 'x' has been declared with incompatible linkage specifiers (static and extern); please pick exactly one
  extern int x;
  ^~~~~~
note: previous definition here
  static int x;
  ^~~~~~

It'd also be more robust from an engineering perspective, since it means that we won't need to add new diagnostic permutations every time a new linkage specifier is added.

I will try giving this a shot
Thanks!

[Krishna-13-cyber]

I have tried a little modification from my side thinking on a beneficial note. I will make the changes to all the other test files as well if this diagnostic representation goes well after mentor review.

For refactoring and restructuring the whole of the re-declaration would need some time I have been through it and would initiate another patch for that,In the concern of giving or having just one diagnostic for getting all cases of re-declaration would also need multiple conditional or switch statements inside our function block.At present we have the same with conditional statements taking care of each linkage but it has multiple permutations of diagnostic messages which is nice but can be improved.GCC differs only for this case of extern linkage which can be better/precise in clang where as others seem to be more precise in clang than former as I worked out with good number of test cases regarding this.

[Krishna-13-cyber]

In D147888#4288149, @Krishna-13-cyber wrote:

I have tried a little modification from my side thinking on a beneficial note. I will make the changes to all the other test files as well if this diagnostic representation goes well after mentor review.

For refactoring and restructuring the whole of the re-declaration would need some time I have been through it and would initiate another patch for that,In the concern of giving or having just one diagnostic for getting all cases of re-declaration would also need multiple conditional or switch statements inside our function block.At present we have the same with conditional statements taking care of each linkage but it has multiple permutations of diagnostic messages which is nice but can be improved.GCC differs only for this case of extern linkage which can be better/precise in clang where as others seem to be more precise in clang than former as I worked out with good number of test cases regarding this.

Ping!

[tbaeder]

Ping.

[Krishna-13-cyber]

In D147888#4463163, @tbaeder wrote:

Ping.

Thanks for the ping! Does this require any further modification?
I think we will have to change the whole set of diagnostics if we go with this change of pick exactly one convention.

[aaron.ballman]

I think the existing wording is pretty reasonable, changing "non-static declaration" into "extern declaration" isn't really giving the user any more information to help them resolve the issue. "please pick exactly one" doesn't seem likely to help the user either -- they already know there's a conflict, so picking one is already the solution the user is likely to have in mind. The hard part for the user is with figuring which one to pick, but we have no way to help them make that choice. So I'm not certain changes are needed in this space (I'm not opposed either), but I do think the idea @cjdb had to combine all these diagnostics into one using %select could be helpful. However, there are enough options that it could also be pretty complex to reason about. There's static, extern, and thread-local, as well as "non-" versions of each of those, so there are quite a few combinations.

[Krishna-13-cyber]

In D147888#4466998, @aaron.ballman wrote:

I think the existing wording is pretty reasonable, changing "non-static declaration" into "extern declaration" isn't really giving the user any more information to help them resolve the issue. "please pick exactly one" doesn't seem likely to help the user either -- they already know there's a conflict, so picking one is already the solution the user is likely to have in mind. The hard part for the user is with figuring which one to pick, but we have no way to help them make that choice. So I'm not certain changes are needed in this space (I'm not opposed either), but I do think the idea @cjdb had to combine all these diagnostics into one using %select could be helpful. However, there are enough options that it could also be pretty complex to reason about. There's static, extern, and thread-local, as well as "non-" versions of each of those, so there are quite a few combinations.

Yes I agree with this,
Changing the whole set of diagnostics will be quite challenging, but I think extern has an edge over non-static declaration when we are able to see the diagnostics in gcc if are not going with the pick-exactly one convention.

[aaron.ballman]

In D147888#4467002, @Krishna-13-cyber wrote:

In D147888#4466998, @aaron.ballman wrote:

I think the existing wording is pretty reasonable, changing "non-static declaration" into "extern declaration" isn't really giving the user any more information to help them resolve the issue. "please pick exactly one" doesn't seem likely to help the user either -- they already know there's a conflict, so picking one is already the solution the user is likely to have in mind. The hard part for the user is with figuring which one to pick, but we have no way to help them make that choice. So I'm not certain changes are needed in this space (I'm not opposed either), but I do think the idea @cjdb had to combine all these diagnostics into one using %select could be helpful. However, there are enough options that it could also be pretty complex to reason about. There's static, extern, and thread-local, as well as "non-" versions of each of those, so there are quite a few combinations.

Yes I agree with this,
Changing the whole set of diagnostics will be quite challenging, but I think extern has an edge over non-static declaration when we are able to see the diagnostics in gcc if are not going with the pick-exactly one convention.

I can sort of buy the argument that extern has a slight edge, but I think what would really help are test cases for all the various combinations so we can see them at once (static, extern. no linkage, thread local vs each counterpart). I'm concerned that the code you added conflicts with the existing code immediately below your changes and the standards wording quoted above your changes, but I left a comment on what would make me more comfortable.

clang/lib/Sema/SemaDecl.cpp

4641–4657

clang/test/SemaCXX/extern_static.cpp

7

Be sure to add the newline back.

Also, I think there should be a C run line for the test.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def note_extern_c_begins_here : Note<
   "extern \"C\" language linkage specification begins here">;
 def warn_weak_import : Warning <
   "an already-declared variable is made a weak_import declaration %0">;
 def ext_static_non_static : Extension<
   "redeclaring non-static %0 as static is a Microsoft extension">,
   InGroup<MicrosoftRedeclareStatic>;
 def err_non_static_static : Error<
-  "non-static declaration of %0 follows static declaration">;
+  "non-static declaration of %0 follows static declaration;Please pick exactly one (static or non-static)">;
+def err_extern_static : Error<
+  "extern declaration of %0 follows static declaration;Please pick exactly one (static or extern)">;
 def err_extern_non_extern : Error<
-  "extern declaration of %0 follows non-extern declaration">;
+  "extern declaration of %0 follows non-extern declaration;Please pick exactly one (non-extern or extern)">;
 def err_non_extern_extern : Error<
-  "non-extern declaration of %0 follows extern declaration">;
+  "non-extern declaration of %0 follows extern declaration;Please pick exactly one (extern or non-extern)">;
 def err_non_thread_thread : Error<
   "non-thread-local declaration of %0 follows thread-local declaration">;
 def err_thread_non_thread : Error<
   "thread-local declaration of %0 follows non-thread-local declaration">;
 def err_thread_thread_different_kind : Error<
   "thread-local declaration of %0 with %select{static|dynamic}1 initialization "
   "follows declaration with %select{dynamic|static}1 initialization">;
 def err_mismatched_owning_module : Error<

clang/lib/Sema/SemaDecl.cpp

void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
   //   For an identifier declared with the storage-class specifier
   //   extern in a scope in which a prior declaration of that
   //   identifier is visible,23) if the prior declaration specifies
   //   internal or external linkage, the linkage of the identifier at
   //   the later declaration is the same as the linkage specified at
   //   the prior declaration. If no prior declaration is visible, or
   //   if the prior declaration specifies no linkage, then the
   //   identifier has external linkage.
+
+  if (New->hasExternalStorage() &&
+      Old->getCanonicalDecl()->getStorageClass() == SC_Static &&
+      Old->isLocalVarDeclOrParm()) {
+    Diag(New->getLocation(), diag::err_extern_static) << New->getDeclName();
+    Diag(OldLocation, PrevDiag);
+    return New->setInvalidDecl();
+  }
+
   if (New->hasExternalStorage() && Old->hasLinkage())
     /* Okay */;
   else if (New->getCanonicalDecl()->getStorageClass() != SC_Static &&
            !New->isStaticDataMember() &&
            Old->getCanonicalDecl()->getStorageClass() == SC_Static) {
     Diag(New->getLocation(), diag::err_non_static_static) << New->getDeclName();
     Diag(OldLocation, PrevDiag);
     return New->setInvalidDecl();
   }
 
   // Check if extern is followed by non-extern and vice-versa.
   if (New->hasExternalStorage() &&
       !Old->hasLinkage() && Old->isLocalVarDeclOrParm()) {
     Diag(New->getLocation(), diag::err_extern_non_extern) << New->getDeclName();
     Diag(OldLocation, PrevDiag);
     return New->setInvalidDecl();
   }

clang/test/SemaCXX/extern_static.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -Wabstract-vbase-init
+void f(void)
+{
+    static int x; // expected-note {{previous definition is here}}
+    extern int x; // expected-error {{extern declaration of 'x' follows static declaration;Please pick exactly one (static or extern)}}
+}
 No newline at end of file