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 ↗

(On Diff #11226)

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 ↗

(On Diff #11226)

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

1002 ↗

(On Diff #11226)

Maybe drop the "to fix"?

lib/Sema/SemaDecl.cpp

11467 ↗

(On Diff #11226)

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 ↗

(On Diff #11226)

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 ↗

(On Diff #11226)

done

1002 ↗

(On Diff #11226)

done

lib/Sema/SemaDecl.cpp

11467 ↗

(On Diff #11226)

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

10740 ↗

(On Diff #11296)

Stray 'something'?

10986 ↗

(On Diff #11296)

auto?

[rnk]

Closed by commit rL212784 (authored by @rnk).

[rnk]

lib/Sema/SemaDecl.cpp

10740 ↗

(On Diff #11296)

yeah

10986 ↗

(On Diff #11296)

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

10986 ↗

(On Diff #11296)

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

10986 ↗

(On Diff #11296)

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.

cfe/trunk/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 declaration referring to type outside of the nearest "
+  "enclosing namespace is a Microsoft extension; add a nested name specifier">,
+  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'">;
 

cfe/trunk/lib/Sema/SemaDecl.cpp

if (Previous->getDefinition()) {
                TypeWithKeyword::getTagTypeKindName(Redecl->getTagKind()));
     }
 
     return true;
   }
   return false;
 }
 
+/// Add a minimal nested name specifier fixit hint to allow lookup of a tag name
+/// from an outer enclosing namespace or file scope inside a friend declaration.
+/// This should provide the commented out code in the following snippet:
+///   namespace N {
+///     struct X;
+///     namespace M {
+///       struct Y { friend struct /*N::*/ X; };
+///     }
+///   }
+static void addFriendTagNNSFixIt(Sema &SemaRef, Sema::SemaDiagnosticBuilder &D,
+                                 NamedDecl *ND, Scope *S,
+                                 SourceLocation NameLoc) {
+  // While the decl is in a namespace, do repeated lookup of that name and see
+  // if we get the same namespace back.  If we do not, continue until
+  // translation unit scope, at which point we have a fully qualified NNS.
+  SmallVector<IdentifierInfo *, 4> Namespaces;
+  DeclContext *DC = ND->getDeclContext()->getRedeclContext();
+  for (; !DC->isTranslationUnit(); DC = DC->getParent()) {
+    // This tag should be declared in a namespace, which can only be enclosed by
+    // other namespaces.  Bail if there's an anonymous namespace in the chain.
+    NamespaceDecl *Namespace = dyn_cast<NamespaceDecl>(DC);
+    if (!Namespace || Namespace->isAnonymousNamespace())
+      return;
+    IdentifierInfo *II = Namespace->getIdentifier();
+    Namespaces.push_back(II);
+    NamedDecl *Lookup = SemaRef.LookupSingleName(
+        S, II, NameLoc, Sema::LookupNestedNameSpecifierName);
+    if (Lookup == Namespace)
+      break;
+  }
+
+  // Once we have all the namespaces, reverse them to go outermost first, and
+  // build an NNS.
+  SmallString<64> Insertion;
+  llvm::raw_svector_ostream OS(Insertion);
+  if (DC->isTranslationUnit())
+    OS << "::";
+  std::reverse(Namespaces.begin(), Namespaces.end());
+  for (auto *II : Namespaces)
+    OS << II->getName() << "::";
+  OS.flush();
+  D << FixItHint::CreateInsertion(NameLoc, Insertion);
+}
+
 /// ActOnTag - This is invoked when we see 'struct foo' or 'struct {'.  In the
 /// former case, Name will be non-null.  In the later case, Name will be null.
 /// TagSpec indicates what kind of tag this is. TUK indicates whether this is a
 /// reference/declaration/definition of a tag.
 ///
 /// IsTypeSpecifier is true if this is a type-specifier (or
 /// trailing-type-specifier) other than one in an alias-declaration.
 Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
   DeclContext *DC = CurContext;
   bool isStdBadAlloc = false;
 
   RedeclarationKind Redecl = ForRedeclaration;
   if (TUK == TUK_Friend || TUK == TUK_Reference)
     Redecl = NotForRedeclaration;
 
   LookupResult Previous(*this, Name, NameLoc, LookupTagName, Redecl);
-  bool FriendSawTagOutsideEnclosingNamespace = false;
   if (Name && SS.isNotEmpty()) {
     // We have a nested-name tag ('struct foo::bar').
 
     // Check for invalid 'foo::'.
     if (SS.isInvalid()) {
       Name = nullptr;
       goto CreateNewDecl;
     }
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();
+      bool FriendSawTagOutsideEnclosingNamespace = false;
       while (F.hasNext()) {
         NamedDecl *ND = F.next();
         DeclContext *DC = ND->getDeclContext()->getRedeclContext();
         if (DC->isFileContext() &&
             !EnclosingNS->Encloses(ND->getDeclContext())) {
-          F.erase();
+          if (getLangOpts().MSVCCompat)
             FriendSawTagOutsideEnclosingNamespace = true;
+          else
+            F.erase();
         }
       }
       F.done();
+
+      // Diagnose this MSVC extension in the easy case where lookup would have
+      // unambiguously found something outside the enclosing namespace.
+      if (Previous.isSingleResult() && FriendSawTagOutsideEnclosingNamespace) {
+        NamedDecl *ND = Previous.getFoundDecl();
+        auto D = Diag(NameLoc, diag::ext_friend_tag_redecl_outside_namespace);
+        addFriendTagNNSFixIt(*this, D, ND, S, NameLoc);
+      }
     }
-    
     // 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);
+    New->setObjectOfFriendDecl(getLangOpts().MSVCCompat);
 
   // Set the access specifier.
   if (!Invalid && SearchDC->isRecord())
     SetMemberAccessSpecifier(New, PrevDecl, AS);
 
   if (TUK == TUK_Definition)
     New->startDefinition();
 

cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp

void pointer_to_integral_type_conv(char* ptr) {
    // These are valid C++.
    bool b = (bool)ptr;
    b = static_cast<bool>(ptr);
 
    // This is bad.
    b = reinterpret_cast<bool>(ptr); // expected-error {{cast from pointer to smaller type 'bool' loses information}}
 }
 
-namespace friend_as_a_forward_decl {
-
-class A {
-  class Nested {
-    friend class B;
-    B* b;
-  };
-  B* b;
-};
-B* global_b;
-
-
-void f()
-{
-  class Local {
-    friend class Z;
-    Z* b;
-  };
-  Z* b;
-}
-
-}
-
 struct PR11150 {
   class X {
     virtual void f() = 0;
   };
 
   int array[__is_abstract(X)? 1 : -1];
 };
 

cfe/trunk/test/SemaCXX/ms-friend-lookup.cpp

+// RUN: %clang_cc1 %s -triple i686-pc-win32 -std=c++11 -Wmicrosoft -fms-compatibility -verify
+// RUN: not %clang_cc1 %s -triple i686-pc-win32 -std=c++11 -Wmicrosoft -fms-compatibility -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s
+
+struct X;
+namespace name_at_tu_scope {
+struct Y {
+  friend struct X; // expected-warning-re {{unqualified friend declaration {{.*}} is a Microsoft extension}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:"::"
+};
+}
+
+namespace enclosing_friend_decl {
+struct B;
+namespace ns {
+struct A {
+  friend struct B; // expected-warning-re {{unqualified friend declaration {{.*}} is a Microsoft extension}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:"enclosing_friend_decl::"
+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'}}
+}
+
+namespace test_nns_fixit_hint {
+namespace name1 {
+namespace name2 {
+struct X;
+struct name2;
+namespace name3 {
+struct Y {
+  friend struct X; // expected-warning-re {{unqualified friend declaration {{.*}} is a Microsoft extension}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:17-[[@LINE-1]]:17}:"name1::name2::"
+};
+}
+}
+}
+}
+
+// A friend declaration injects a forward declaration into the nearest enclosing
+// non-member scope.
+namespace friend_as_a_forward_decl {
+
+class A {
+  class Nested {
+    friend class B;
+    B *b;
+  };
+  B *b;
+};
+B *global_b;
+
+void f() {
+  class Local {
+    friend class Z;
+    Z *b;
+  };
+  Z *b;
+}
+
+}