[Sema] Fix parsing of anonymous union in language linkage specification

[jkorous]

Summary

C++17 [dcl.link]p4:
A linkage specification does not establish a scope.

C++17 [class.union.anon]p2:
Namespace level anonymous unions shall be declared static.

The test produces a crash on master.

[vsapsai]

For the record, DR154: Anonymous unions in unnamed namespaces is relevant here.

Sema/SemaDecl.cpp

4654–4656 ↗

(On Diff #143317)

Looks like DeclContext::isTransparentContext() might be relevant here. At least I was able to get the assertion failure

Assertion failed: (II && "Attempt to mangle unnamed decl."), function getMangledNameImpl, file llvm-project/clang/lib/CodeGen/CodeGenModule.cpp, line 913.

for

// clang -std=c++17 -fmodules-ts test-modules.cpp

export module M;
export {
    union {
        int int_val;
        float float_val;
    };
}

Also based on isTransparentContext() usage, inline namespaces can cause problems. Currently, there are no problems, we have the error

error: anonymous unions at namespace or global scope must be declared 'static'

and there are no negative consequences (as far as I can tell). According to my bad standard knowledge that should be OK (haven't found non-static anonymous unions to be allowed in this case).

4659 ↗

(On Diff #143317)

Checked if we need to do the same change s/Owner/OwnerScope/ elsewhere in this method and looks like it is not required. We care if the owner is a Record and we don't allow linkage specification in classes, so skipping linkage scopes doesn't give us anything.

[rsmith]

Sema/SemaDecl.cpp

4654–4656 ↗

(On Diff #143317)

Right, the correct way to spell this is OwnerScope = Owner->getRedeclContext();.

4659–4661 ↗

(On Diff #143317)

While you're here, OwnerScope->isFileContext() && !OwnerScope->isInlineNamespace() might be clearer. Or at least replace the getDeclName() with !isInlineNamespace().

[jkorous]

Addressed comments + proposal for refactoring of the namespace-related logic.

[vsapsai]

Sema/SemaDecl.cpp

4651–4653 ↗

(On Diff #143736)

I think the code style favours avoiding excessive vertical whitespace and I don't feel like this statement is semantically far enough from the surrounding code to be separated from it. I haven't found a specific LLVM Coding Standards rule, so in this case it is my personal opinion and actual formatting decision is up to you.

Other than that I have no other comments.

[jkorous]

Volodymyr, could you please confirm that the non-anonymous vs non-inline logic makes sense to you?

Sema/SemaDecl.cpp

4659–4661 ↗

(On Diff #143317)

I'd like to explicitly flag this - we are interested in non-anonymous namespaces not non-inline namespaces, right?

[jkorous]

Sema/SemaDecl.cpp

4651–4653 ↗

(On Diff #143736)

Strange, looks like phab lost my comment which basically was: "No problem, I see what you mean. Will remove those blank lines."

[vsapsai]

In D45884#1084060, @jkorous wrote:

Volodymyr, could you please confirm that the non-anonymous vs non-inline logic makes sense to you?

Looks correct to me that we are checking non-anonymous namespaces. Inline namespaces are somewhat similar to anonymous namespaces but my testing shows they are working correctly and don't require any change. By "working correctly" I mean

error: anonymous unions at namespace or global scope must be declared 'static'

[rsmith]

Sema/SemaDecl.cpp

4659–4661 ↗

(On Diff #143317)

Sorry, yes, thinko on my part.

lib/Sema/SemaDecl.cpp

Decl *Sema::BuildAnonymousStructOrUnion(Scope *S, DeclSpec &DS,
   // C and C++ require different kinds of checks for anonymous
   // structs/unions.
   bool Invalid = false;
   if (getLangOpts().CPlusPlus) {
     const char *PrevSpec = nullptr;
     unsigned DiagID;
     if (Record->isUnion()) {
       // C++ [class.union]p6:
+      // C++17 [class.union.anon]p2:
       //   Anonymous unions declared in a named namespace or in the
       //   global namespace shall be declared static.
+      DeclContext *OwnerScope = Owner->getRedeclContext();
       if (DS.getStorageClassSpec() != DeclSpec::SCS_static &&
-          (isa<TranslationUnitDecl>(Owner) ||
-           (isa<NamespaceDecl>(Owner) &&
-            cast<NamespaceDecl>(Owner)->getDeclName()))) {
+          (OwnerScope->isTranslationUnit() ||
+           (OwnerScope->isNamespace() &&
+            !cast<NamespaceDecl>(OwnerScope)->isAnonymousNamespace()))) {
         Diag(Record->getLocation(), diag::err_anonymous_union_not_static)
           << FixItHint::CreateInsertion(Record->getLocation(), "static ");
 
         // Recover by adding 'static'.
         DS.SetStorageClassSpec(*this, DeclSpec::SCS_static, SourceLocation(),
                                PrevSpec, DiagID, Policy);
       }
       // C++ [class.union]p6:

test/SemaCXX/anonymous-union-export.cpp

+// RUN: %clang_cc1 -std=c++17 -fmodules-ts -emit-obj -verify %s
+
+export module M;
+export {
+    union { bool a; }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
+}

test/SemaCXX/anonymous-union.cpp

struct Redecl {
   void zz(); // expected-error{{redefinition of 'zz' as different kind of symbol}}
 };
 
 union { // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
   int int_val;
   float float_val;
 };
 
+extern "C++" {
+union { }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
+}
+
 static union {
   int int_val2; // expected-note{{previous definition is here}}
   float float_val2;
 };
 
 void PR21858() {
   void int_val2(); // expected-error{{redefinition of 'int_val2' as different kind of symbol}}
 }