MSVC compat: Allow lookup of friend types in enclosing namespaces

[rnk]

Summary

The relevant portion of C++ standard says [namespace.memdef]p3:

If the name in a friend declaration is neither qualified nor a
template-id and the declaration is a function or an
elaborated-type-specifier, the lookup to determine whether the entity
has been previously declared shall not consider any scopes outside the
innermost enclosing namespace.

MSVC does not implement that rule for types. If there is a type in an
enclosing namespace, they consider an unqualified tag declaration with
the same name to be a redeclaration of the type from another namespace.

Implementing compatibility is a simple matter of disabling our
implementation of this rule for types, which was added in r177473.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

1000–1002

Please add a bit more information here: which NNS should be added? What is the enclosing namespace that we looked in? Which type did we actually find? A fixit adding the NNS would help.

1001

Singular/plural mismatch ("declarations ... is a MS extension"). Please make this singular.

1002

Maybe drop the "to fix"?

lib/Sema/SemaDecl.cpp

11467

This was a prior attempt to get ~half of the same effect. In particular, for:

struct X {};
namespace N {
  struct Y { friend struct X; };
  X x;
}

... we need to not inject X into N, or the lookup for N::x's type will find the wrong struct.

Perhaps we should remove this hack (for MicrosoftExt) now that we have a more faithful hack (for MSVCCompat)? It's not a conforming extension. That is, replace this with just

New->setObjectOfFriendDecl(getLangOpts().MSVCCompat);

[rnk]

include/clang/Basic/DiagnosticSemaKinds.td

1000–1002

Done. It's a very nice diagnostic now, although maybe this is overkill for a fairly pedantic portion of the standard where implementations disagree. :)

1001

done

1002

done

lib/Sema/SemaDecl.cpp

11467

I agree, I'd rather have just one faithful hack, but presumably Doug put this under -fms-extensions for a reason. Hopefully we can raise him. :)

I'll make the change, since I think we both agree this shouldn't be under -fms-extensions.

[rnk]

• add a super awesome fixit

[rsmith]

LGTM

lib/Sema/SemaDecl.cpp

10738

Stray 'something'?

10938

auto?

[rnk]

Closed by commit rL212784 (authored by @rnk).

[rnk]

lib/Sema/SemaDecl.cpp

10738

yeah

10938

It used to be auto, but I had to feed it into addFriendTagNNSFixIt, so I made it explicit. I guess I can flip it back.

[rsmith]

lib/Sema/SemaDecl.cpp

10938

Maybe either make addFriendTagNNSFixIt take a DiagnosticBuilder& so it doesn't need to care about this, or make it return a FixItHint? (A default-constructed FixItHint will be ignored by the diagnostic renderer.)

[rnk]

lib/Sema/SemaDecl.cpp

10938

I considered that, but FixItHint::CreateInsertion takes a StringRef, so I didn't think I could safely return one without UAF bugs. Looks like it uses a std::string for storage, so it all works out. Done in r212786.

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_template_qualified_friend_unsupported : Warning<
   "dependent nested name specifier '%0' for friend class declaration is "
   "not supported; turning off access control for %1">,
   InGroup<UnsupportedFriend>;
 def warn_template_qualified_friend_ignored : Warning<
   "dependent nested name specifier '%0' for friend template declaration is "
   "not supported; ignoring this friend declaration">,
   InGroup<UnsupportedFriend>;
-  
+def ext_friend_tag_redecl_outside_namespace : ExtWarn<
+  "unqualified friend declarations referring to types outside of the nearest "
+  "enclosing namespace is a Microsoft extension; add a nested name specifier to fix">,
+  InGroup<Microsoft>;
 def err_invalid_member_in_interface : Error<
   "%select{data member |non-public member function |static member function |"
           "user-declared constructor|user-declared destructor|operator |"
           "nested class }0%1 is not permitted within an interface type">;
 def err_invalid_base_in_interface : Error<
   "interface type cannot inherit from "
   "%select{'struct|non-public 'interface|'class}0 %1'">;
 

lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                      SourceLocation ModulePrivateLoc,
                      MultiTemplateParamsArg TemplateParameterLists,
                      bool &OwnedDecl, bool &IsDependent,
                      SourceLocation ScopedEnumKWLoc,
                      bool ScopedEnumUsesClassTag,
                      TypeResult UnderlyingType,
                      bool IsTypeSpecifier) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
          "Nameless record must be a definition!");
   assert(TemplateParameterLists.size() == 0 || TUK != TUK_Reference);
 
   OwnedDecl = false;
   TagTypeKind Kind = TypeWithKeyword::getTagTypeKindForTypeSpec(TagSpec);
   bool ScopedEnum = ScopedEnumKWLoc.isValid();
 
if (Name && SS.isNotEmpty()) {
 
     // C++11 [namespace.memdef]p3:
     //   If the name in a friend declaration is neither qualified nor
     //   a template-id and the declaration is a function or an
     //   elaborated-type-specifier, the lookup to determine whether
     //   the entity has been previously declared shall not consider
     //   any scopes outside the innermost enclosing namespace.
     //
+    // MSVC doesn't implement the above rule for types, so a friend tag
+    // declaration may be a redeclaration of a type declared in an enclosing
+    // scope.  They do implement this rule for friend functions.
+    //
     // Does it matter that this should be by scope instead of by
     // semantic context?
     if (!Previous.empty() && TUK == TUK_Friend) {
       DeclContext *EnclosingNS = SearchDC->getEnclosingNamespaceContext();
       LookupResult::Filter F = Previous.makeFilter();
       while (F.hasNext()) {
         NamedDecl *ND = F.next();
         DeclContext *DC = ND->getDeclContext()->getRedeclContext();
         if (DC->isFileContext() &&
             !EnclosingNS->Encloses(ND->getDeclContext())) {
+          if (!getLangOpts().MSVCCompat)
             F.erase();
           FriendSawTagOutsideEnclosingNamespace = true;
         }
       }
       F.done();
+
+      if (FriendSawTagOutsideEnclosingNamespace && getLangOpts().MSVCCompat) {
+        Diag(KWLoc, diag::ext_friend_tag_redecl_outside_namespace);
+        // Set this back to false so that we don't inject a forward declaration
+        // into the enclosing namespace.  We already found a forward declaration
+        // in another namespace.
+        FriendSawTagOutsideEnclosingNamespace = false;
+      }
     }
-    
     // Note:  there used to be some attempt at recovery here.
     if (Previous.isAmbiguous())
       return nullptr;
 
     if (!getLangOpts().CPlusPlus && TUK != TUK_Reference) {
       // FIXME: This makes sure that we ignore the contexts associated
       // with C structs, unions, and enums when looking for a matching
       // tag declaration or definition. See the similar lookup tweak
CreateNewDecl:
   // lexical context will be different from the semantic context.
   New->setLexicalDeclContext(CurContext);
 
   // Mark this as a friend decl if applicable.
   // In Microsoft mode, a friend declaration also acts as a forward
   // declaration so we always pass true to setObjectOfFriendDecl to make
   // the tag name visible.
   if (TUK == TUK_Friend)
     New->setObjectOfFriendDecl(!FriendSawTagOutsideEnclosingNamespace &&
                                getLangOpts().MicrosoftExt);
 
   // Set the access specifier.
   if (!Invalid && SearchDC->isRecord())
     SetMemberAccessSpecifier(New, PrevDecl, AS);
 
   if (TUK == TUK_Definition)
     New->startDefinition();

test/SemaCXX/MicrosoftCompatibility.cpp

struct Bar {
     const Foo::MemberFcnPtr pB;
   };
 
   Bar g_bar = { (Foo::MemberFcnPtr)Foo::ZERO };
 
   template<int N> int *get_n() { return N; }   // expected-warning {{expression which evaluates to zero treated as a null pointer constant}}
   int *g_nullptr = get_n<0>();  // expected-note {{in instantiation of function template specialization}}
 }
+
+namespace enclosing_friend_decl {
+struct B;
+namespace ns {
+struct A {
+  friend struct B; // expected-warning-re {{unqualified friend declarations {{.*}} is a Microsoft extension}}
+protected:
+  A();
+};
+}
+struct B {
+  static void f() { ns::A x; }
+};
+}
+
+namespace enclosing_friend_qualified {
+struct B;
+namespace ns {
+struct A {
+  friend struct enclosing_friend_qualified::B; // Adding name specifiers fixes it.
+protected:
+  A();
+};
+}
+struct B {
+  static void f() { ns::A x; }
+};
+}
+
+namespace enclosing_friend_no_tag {
+struct B;
+namespace ns {
+struct A {
+  friend B; // Removing the tag decl fixes it.
+protected:
+  A();
+};
+}
+struct B {
+  static void f() { ns::A x; }
+};
+}
+
+namespace enclosing_friend_func {
+void f();
+namespace ns {
+struct A{
+  // Amusingly, in MSVC, this declares ns::f(), and doesn't find the outer f().
+  friend void f();
+protected:
+  A(); // expected-note {{declared protected here}}
+};
+}
+void f() { ns::A x; } // expected-error {{calling a protected constructor of class 'enclosing_friend_func::ns::A'}}
+}