[SEMA] ignore duplicate declaration specifiers from typeof exprs

[nickdesaulniers]

Summary

Create a third diagnostic type for duplicate declaration specifiers. Previously, we had an ExtWarn and a Warning. This change adds a third, Extension, which only warns when -pedantic is set, staying silent otherwise.

The common cases where each are used are roughly:
Extension: pre-C99 (added by this change)
ExtWarn: C++
Warning: _Noreturn, inline, other C++ keywords

Where the preexisting behavior is mostly kept intact. This change intentionally removes warnings for duplicated _Atomic and restrict, since they were not available until C11 and C99 respectively, and we not longer wish to warn for duplicates for C99 and newer.

Fixes PR32985.

[nickdesaulniers]

git-clang-format HEAD~

[joel]

I built Linux using this patch applied to trunk and it worked for me. Thanks!

[manojgupta]

As per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80868, I thought GCC also emits this error but only with -pedantic. So probably should keep this error but under -Wextra or another appropriate group?

[nickdesaulniers]

• warn only if -pedantic, add gcc bug test cases

[nickdesaulniers]

• git-clang-format HEAD~

[lebedev.ri]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

This ideally needs positive tests. E.g.:

• -std=c89
• -std=c89 -pedantic
• -std=gnu99
• -std=gnu99 -pedantic
• -std=c99
• -std=c99 -pedantic

16 ↗

(On Diff #166352)

There is no otherwise check prefix in the run lines.

[manojgupta]

lgtm. But someone more familiar with these code paths should approve.

[nickdesaulniers]

• move test to new file, use check-prefix for both cases

[nickdesaulniers]

• add line numbers to match specific warning lines

[srhines]

lib/Sema/SemaType.cpp

1682 ↗

(On Diff #166357)

This is broken for C11 and C17 (even before you touch anything). As we just talked about, let's have a helper function to detect the oldest version (and maybe even that should get promoted as a LANGOPT).

[nickdesaulniers]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

Since typeof is a gnu extension, its use constitutes an error for all non gnu C standards, so it's moot to check for duplicate const specifiers from typeof exprs.

Since we're trying to match GCC's behavior here, GCC does not warn for -std=gnu99 or -std=gnu99 -pedantic so I will add those test cases.

[nickdesaulniers]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

https://godbolt.org/z/3trZdl

[nickdesaulniers]

• also run test on gnu99+

lib/Sema/SemaType.cpp

1682 ↗

(On Diff #166357)

From the declarations in include/clang/Frontend/LangStandards.def, newer versions of C bitwise OR together flags of previous versions. So C99 sets C99 flag, C11 sets C99 and C11 flags, and C17 sets C99, C11, and C17 flags.

So this should be correct (though even to me, this looks wrong on first glance). Thanks to @george.burgess.iv and you for help in looking into this.

[nickdesaulniers]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

Ah, I can still put CHECKs for errors. Will add additional tests.

[nickdesaulniers]

• add ISO C tests, handle typedef case new tests found

[nickdesaulniers]

• remove debug statments

[nickdesaulniers]

• condense CHECK-prefixes into CHECK for const const

[nickdesaulniers]

test/Sema/gnu89-const.c

41–46 ↗

(On Diff #166740)

gah, CNU typo!

[nickdesaulniers]

• fix typo s/CNU/GNU/g and update NOTEs

[nickdesaulniers]

• adjust wording in comment

[nickdesaulniers]

This now matches GCC AFAICT. My only question is:

Should GCC instead warn for the typedef case for -std=c89 (non pedantic), according to C90 6.5.3?

[nickdesaulniers]

Should GCC instead warn for the typedef case for -std=c89 (non pedantic), according to C90 6.5.3?

Seems like yes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80868#c6
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87435
But let's wait a bit to see what the GCC folks say. If the newer bug doesn't get closed, then I need to rework this patch.

[rsmith]

The right way to produce diagnostics under pedantic mode is to model them as Extension or ExtWarn in the .td file, not by checking the Pedantic diagnostic option directly. If this is an intentional GNU extension too, that makes things a bit more complex (one approach would be to have two diagnostics, one for "this is ill-formed" and another for "this is a GNU extension").

[nickdesaulniers]

• split duplicate_declspec into Extension and Extwarn from Extwarn.
• rename ext_duplicate_declspec to be the Extension, not the Extwarn.
• Fix C++ case.
• Use tablegen'd Extension rather than checking Pedantic diag option.
• Update tests that started failing due to this change.
• Consolidate checks for duplicate declspecs into 1 file.
• Move cases for PR8264 into my added test case, extending them for various language standards.
• Update my tests/rename the new test file name.

[nickdesaulniers]

Ok, I think this is ready for rereview.

test/Parser/atomic.c

5 ↗

(On Diff #167565)

Note to reviewers: this deletion was intentional. It is not copied over to the new test file test/Sema/dupl-declspec.c because we now DONT want to warn for duplicates in C99+, and _Atomic was not available before C11.

test/Sema/declspec.c

43 ↗

(On Diff #167565)

Note to reviewers; this deletion was intentional. It is not copied over to the new test file test/Sema/dupl-declspec.c with the rest of the PR8264 cases because we now DONT want to warn for duplicates in C99+, and restrict was not available before then (ie. C90).

The rest of these cases were moved to the new test file test/Sema/dupl-declspec.c and exhaustively tested against all current C standards.

[nickdesaulniers]

test/FixIt/fixit.c

50 ↗

(On Diff #167565)

Ah, this was a case I should add one last test for. I think reviewers can still start reviewing the rest. This should just add one additional test case.

[nickdesaulniers]

• add back test for typedef typedef
• add test for dupl _Atomic and restrict

[nickdesaulniers]

test/FixIt/fixit.c

50 ↗

(On Diff #167565)

Added test case back.

test/Parser/atomic.c

5 ↗

(On Diff #167565)

Actually, we can still test that it does not appear.

test/Sema/declspec.c

43 ↗

(On Diff #167565)

Actually, we can still test that it does not appear.

[nickdesaulniers]

• move back untouched test case

[rsmith]

Downgrading the C89 warning for duplicate qualifiers from typedefs / __typeof from ExtWarn to Extension seems very reasonable to me. However, I don't see a justification for removing the warning for duplicate explicit (non-typedef) qualifiers in C99, nor for downgrading the corresponding ExtWarn to an Extension in our other language modes; that seems like a regression.

include/clang/Basic/DiagnosticSemaKinds.td

197

Please follow our naming convention for diagnostic names: both Extension and ExtWarn diagnostics should start ext_.

lib/Sema/DeclSpec.cpp

442–443

This doesn't look like a correct change. When IsExtension is false, the duplicate is not ill-formed, so we shouldn't use an ExtWarn diagnostic (which results in an error under -pedantic-errors).

860–861

We still want to warn on duplicate decl specifiers in C99 mode.

[nickdesaulniers]

lib/Sema/DeclSpec.cpp

442–443

If the duplicate is not ill-formed, then isn't the original code incorrect, since it produces a warning?

[nickdesaulniers]

[SEMA] split ExtWarn dupl-decl-spec's into Extension and ExtWarn

For types deduced from typedef's and typeof's, don't warn for duplicate
declaration specifiers in C90 unless -pedantic.

Create a third diagnostic type for duplicate declaration specifiers.
Previously, we had an ExtWarn and a Warning. This change adds a third,
Extension, which only warns when -pedantic is set, staying silent
otherwise.

Fixes PR32985.

[nickdesaulniers]

Well that didn't do what I wanted...(sorry, still learning arc). Going to abandon this and push a much simpler fix.

[nickdesaulniers]

Created: https://reviews.llvm.org/D52849

include/clang/Basic/DiagnosticSemaKinds.td

 def err_designator_into_flexible_array_member : Error<
   "designator into flexible array member subobject">;
 def note_flexible_array_member : Note<
   "initialized flexible array member %0 is here">;
 def ext_flexible_array_init : Extension<
   "flexible array initialization is a GNU extension">, InGroup<GNUFlexibleArrayInitializer>;
 
 // Declarations.
-def ext_duplicate_declspec : ExtWarn<"duplicate '%0' declaration specifier">,
+def ext_duplicate_declspec : Extension<"duplicate '%0' declaration specifier">,
+  InGroup<DuplicateDeclSpecifier>;
+def ext_warn_duplicate_declspec : ExtWarn<"duplicate '%0' declaration specifier">,
   InGroup<DuplicateDeclSpecifier>;
 def warn_duplicate_declspec : Warning<"duplicate '%0' declaration specifier">,
   InGroup<DuplicateDeclSpecifier>;
 def ext_plain_complex : ExtWarn<
   "plain '_Complex' requires a type specifier; assuming '_Complex double'">;
 def ext_imaginary_constant : Extension<
   "imaginary constants are a GNU extension">, InGroup<GNUImaginaryConstant>;
 def ext_integer_complex : Extension<

lib/Parse/ParseDecl.cpp

case tok::less:
       // If a type specifier follows, it will be diagnosed elsewhere.
       continue;
     }
     // If the specifier wasn't legal, issue a diagnostic.
     if (isInvalid) {
       assert(PrevSpec && "Method did not return previous specifier!");
       assert(DiagID);
 
-      if (DiagID == diag::ext_duplicate_declspec)
+      if (DiagID == diag::ext_duplicate_declspec ||
+          DiagID == diag::ext_warn_duplicate_declspec)
         Diag(Tok, DiagID)
           << PrevSpec << FixItHint::CreateRemoval(Tok.getLocation());
       else if (DiagID == diag::err_opencl_unknown_type_specifier) {
         Diag(Tok, DiagID) << getLangOpts().OpenCLCPlusPlus
             << getLangOpts().getOpenCLVersionTuple().getAsString()
             << PrevSpec << isStorageClass;
       } else
         Diag(Tok, DiagID) << PrevSpec;

lib/Sema/DeclSpec.cpp

 template <class T> static bool BadSpecifier(T TNew, T TPrev,
                                             const char *&PrevSpec,
                                             unsigned &DiagID,
                                             bool IsExtension = true) {
   PrevSpec = DeclSpec::getSpecifierName(TPrev);
   if (TNew != TPrev)
     DiagID = diag::err_invalid_decl_spec_combination;
   else
-    DiagID = IsExtension ? diag::ext_duplicate_declspec :
+    DiagID = IsExtension ? diag::ext_warn_duplicate_declspec :
                            diag::warn_duplicate_declspec;
   return true;
 }
 
 const char *DeclSpec::getSpecifierName(DeclSpec::SCS S) {
   switch (S) {
   case DeclSpec::SCS_unspecified: return "unspecified";
   case DeclSpec::SCS_typedef:     return "typedef";
   case DeclSpec::SCS_extern:      return "extern";
   case DeclSpec::SCS_static:      return "static";
bool DeclSpec::SetTypeQual(TQ T, SourceLocation Loc, const char *&PrevSpec,
                            unsigned &DiagID, const LangOptions &Lang) {
   // Duplicates are permitted in C99 onwards, but are not permitted in C89 or
   // C++.  However, since this is likely not what the user intended, we will
   // always warn.  We do not need to set the qualifier's location since we
   // already have it.
   if (TypeQualifiers & T) {
     bool IsExtension = true;
     if (Lang.C99)
       IsExtension = false;
     return BadSpecifier(T, T, PrevSpec, DiagID, IsExtension);
   }
   TypeQualifiers |= T;
 
   switch (T) {
   case TQ_unspecified: break;
   case TQ_const:    TQ_constLoc = Loc; return false;
   case TQ_restrict: TQ_restrictLoc = Loc; return false;
   case TQ_volatile: TQ_volatileLoc = Loc; return false;
bool DeclSpec::SetFriendSpec(SourceLocation Loc, const char *&PrevSpec,
   FriendLoc = Loc;
   return false;
 }
 
 bool DeclSpec::setModulePrivateSpec(SourceLocation Loc, const char *&PrevSpec,
                                     unsigned &DiagID) {
   if (isModulePrivateSpecified()) {
     PrevSpec = "__module_private__";
-    DiagID = diag::ext_duplicate_declspec;
+    DiagID = diag::ext_warn_duplicate_declspec;
     return true;
   }
 
   ModulePrivateLoc = Loc;
   return false;
 }
 
 bool DeclSpec::SetConstexprSpec(SourceLocation Loc, const char *&PrevSpec,

test/Sema/pr32985.c

+/*
+RUN: %clang_cc1 %s -std=gnu89 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU89 %s -allow-empty
+RUN: %clang_cc1 %s -std=gnu89 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU89-PEDANTIC %s
+*/
+
+typedef const int t;
+const t c_i;
+/*
+CHECK-GNU89-NOT: 7:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU89-PEDANTIC: 7:1: warning: duplicate 'const' declaration specifier
+*/
+
+const int c_i2;
+const typeof(c_i2) c_i3;
+/*
+CHECK-GNU89-NOT: 14:7: warning: extension used
+CHECK-GNU89-NOT: 14:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU89-PEDANTIC: 14:7: warning: extension used
+CHECK-GNU89-PEDANTIC: 14:1: warning: duplicate 'const' declaration specifier
+*/