Enhance diagnostics for friend function from global namespace

[sepavloff]

Summary

A user may reference a function from global namespace in friend
declaration, which returns enum or class, as in example:

enum E { A, B };
E func();
namespace N {
    class C {
        friend E ::func();
    };
}

This is wrong as the identifier following '::' must be considered
as a part of nested-name-specifier.

This change implements additional note in such case with a hint to put
parenthesis around the function qualified name.

It fixes PR28422 (https://llvm.org/bugs/show_bug.cgi?id=28422).

[sepavloff]

Reworked patch

include/clang/Basic/DiagnosticParseKinds.td

def err_fold_operator_mismatch : Error<
   "operators in fold expression must be the same">;
 
 def err_ctor_init_missing_comma : Error<
   "missing ',' between base or member initializers">;
 
 // C++ declarations
 def err_friend_decl_defines_type : Error<
   "cannot define a type in a friend declaration">;
+def note_possible_global_friend : Note<"if friend function is in global scope, "
+  "put parentheses around its qualified name">;
 def err_missing_whitespace_digraph : Error<
   "found '<::' after a "
   "%select{template name|const_cast|dynamic_cast|reinterpret_cast|static_cast}0"
   " which forms the digraph '<:' (aka '[') and a ':', did you mean '< ::'?">;
 
 def ext_defaulted_deleted_function : ExtWarn<
   "%select{defaulted|deleted}0 function definitions are a C++11 extension">,
   InGroup<CXX11>;

include/clang/Parse/Parser.h

class Parser : public CodeCompletionHandler {
 
   /// \brief When true, we are directly inside an Objective-C message
   /// send expression.
   ///
   /// This is managed by the \c InMessageExpressionRAIIObject class, and
   /// should not be set directly.
   bool InMessageExpression;
 
+  /// \brief When parser processes decl-spec, this field points to the object
+  /// that captures information about it.
+  DeclSpec *CurrentDeclSpec;
+
   /// The "depth" of the template parameters currently being parsed.
   unsigned TemplateParameterDepth;
 
   /// \brief RAII class that manages the template parameter depth.
   class TemplateParameterDepthRAII {
     unsigned &Depth;
     unsigned AddedLevels;
   public:

include/clang/Sema/Sema.h

struct NestedNameSpecInfo {
     IdentifierInfo *Identifier;
 
     /// \brief The location of the identifier.
     SourceLocation IdentifierLoc;
 
     /// \brief The location of the '::'.
     SourceLocation CCLoc;
 
-    /// \brief Creates info object for the most typical case.
+    /// \brief If not null, the identifier following the '::'.
+    IdentifierInfo *NextIdentifier;
+
+    /// \brief The location of the identifier following '::'.
+    SourceLocation NextIdentifierLoc;
+
+    /// \brief True if the declaration seems to be a function declaration.
+    bool FunctionExpected;
+
     NestedNameSpecInfo(IdentifierInfo *II, SourceLocation IdLoc,
              SourceLocation ColonColonLoc, ParsedType ObjectType = ParsedType())
       : ObjectType(ObjectType), Identifier(II), IdentifierLoc(IdLoc),
-        CCLoc(ColonColonLoc) {
+        CCLoc(ColonColonLoc), NextIdentifier(nullptr), FunctionExpected(false) {
     }
 
     NestedNameSpecInfo(IdentifierInfo *II, SourceLocation IdLoc,
                        SourceLocation ColonColonLoc, QualType ObjectType)
       : ObjectType(ParsedType::make(ObjectType)), Identifier(II),
-        IdentifierLoc(IdLoc), CCLoc(ColonColonLoc) {
-    }
+        IdentifierLoc(IdLoc), CCLoc(ColonColonLoc), NextIdentifier(nullptr),
+        FunctionExpected(false) { }
   };
 
   bool isNonTypeNestedNameSpecifier(Scope *S, CXXScopeSpec &SS,
                                     NestedNameSpecInfo &IdInfo);
 
+  struct NNSRecoveryMode {
+    /// Treat '::' as mistyped ':'
+    bool CorrectColon;
+
+    /// Check if the identifier following '::' is found in respective context.
+    bool CheckForMember;
+
+    /// If set, '::' was replaced by ':'.
+    bool ColonCorrected;
+
+    /// If set, the identifier following '::' is not a member of the context
+    /// defined by the current identifier but can be found in global namespace.
+    bool NextIdIsGlobal;
+
+    NNSRecoveryMode(bool CC = false, bool CM = false) : CorrectColon(CC),
+      CheckForMember(CM), ColonCorrected(false), NextIdIsGlobal(false) {}
+  };
+
   bool BuildCXXNestedNameSpecifier(Scope *S,
                                    NestedNameSpecInfo &IdInfo,
                                    bool EnteringContext,
                                    CXXScopeSpec &SS,
                                    NamedDecl *ScopeLookupResult,
                                    bool ErrorRecoveryLookup,
-                                   bool *IsCorrectedToColon = nullptr);
+                                   NNSRecoveryMode *RecoveryMode = nullptr);
 
   /// \brief The parser has parsed a nested-name-specifier 'identifier::'.
   ///
   /// \param S The scope in which this nested-name-specifier occurs.
   ///
   /// \param IdInfo Parser information about an identifier in the
   /// nested-name-spec.
   ///
public:
   /// if the identifier is treated as if it was followed by ':', not '::'.
   ///
   /// \returns true if an error occurred, false otherwise.
   bool ActOnCXXNestedNameSpecifier(Scope *S,
                                    NestedNameSpecInfo &IdInfo,
                                    bool EnteringContext,
                                    CXXScopeSpec &SS,
                                    bool ErrorRecoveryLookup = false,
-                                   bool *IsCorrectedToColon = nullptr);
+                                   NNSRecoveryMode *RecoveryMode = nullptr);
 
   ExprResult ActOnDecltypeExpression(Expr *E);
 
   bool ActOnCXXNestedNameSpecifierDecltype(CXXScopeSpec &SS,
                                            const DeclSpec &DS,
                                            SourceLocation ColonColonLoc);
 
   bool IsInvalidUnlessNestedName(Scope *S, CXXScopeSpec &SS,

lib/Parse/ParseDecl.cpp

 /// [OpenCL] '__kernel'
 ///       'friend': [C++ dcl.friend]
 ///       'constexpr': [C++0x dcl.constexpr]
 void Parser::ParseDeclarationSpecifiers(DeclSpec &DS,
                                         const ParsedTemplateInfo &TemplateInfo,
                                         AccessSpecifier AS,
                                         DeclSpecContext DSContext,
                                         LateParsedAttrList *LateAttrs) {
+  struct DeclSpecCleaner {
+    Parser &P;
+    DeclSpec *Prev;
+  public:
+    DeclSpecCleaner(Parser &APaprser, DeclSpec &DS)
+      : P(APaprser), Prev(APaprser.CurrentDeclSpec) {
+      P.CurrentDeclSpec = &DS;
+    }
+    ~DeclSpecCleaner() { P.CurrentDeclSpec = Prev; }
+  } DSC(*this, DS);
+
   if (DS.getSourceRange().isInvalid()) {
     // Start the range at the current token but make the end of the range
     // invalid.  This will make the entire range invalid unless we successfully
     // consume a token.
     DS.SetRangeStart(Tok.getLocation());
     DS.SetRangeEnd(SourceLocation());
   }
 

lib/Parse/ParseExprCXX.cpp

if (Next.is(tok::coloncolon) && GetLookAheadToken(2).is(tok::l_brace)) {
       ConsumeToken();         // Eat the identifier, current token is now '::'.
       Diag(PP.getLocForEndOfToken(ConsumeToken()), diag::err_expected)
           << tok::identifier;
       UnconsumeToken(Identifier); // Stick the identifier back.
       Next = NextToken();         // Point Next at the '{' token.
     }
 
     if (Next.is(tok::coloncolon)) {
-      if (CheckForDestructor && GetLookAheadToken(2).is(tok::tilde) &&
+      Token TokenAfterCC = GetLookAheadToken(2);
+      if (CheckForDestructor && TokenAfterCC.is(tok::tilde) &&
           !Actions.isNonTypeNestedNameSpecifier(getCurScope(), SS, IdInfo)) {
         *MayBePseudoDestructor = true;
         return false;
       }
 
       if (ColonIsSacred) {
-        const Token &Next2 = GetLookAheadToken(2);
-        if (Next2.is(tok::kw_private) || Next2.is(tok::kw_protected) ||
-            Next2.is(tok::kw_public) || Next2.is(tok::kw_virtual)) {
-          Diag(Next2, diag::err_unexpected_token_in_nested_name_spec)
-              << Next2.getName()
+        if (TokenAfterCC.isOneOf(tok::kw_private, tok::kw_protected,
+                                 tok::kw_public, tok::kw_virtual)) {
+          Diag(TokenAfterCC, diag::err_unexpected_token_in_nested_name_spec)
+              << TokenAfterCC.getName()
               << FixItHint::CreateReplacement(Next.getLocation(), ":");
           Token ColonColon;
           PP.Lex(ColonColon);
           ColonColon.setKind(tok::colon);
           PP.EnterToken(ColonColon);
           break;
         }
       }
 
       if (LastII)
         *LastII = &II;
 
       // We have an identifier followed by a '::'. Lookup this name
       // as the name in a nested-name-specifier.
       Token Identifier = Tok;
       SourceLocation IdLoc = ConsumeToken();
       assert(Tok.isOneOf(tok::coloncolon, tok::colon) &&
              "NextToken() not working properly!");
       Token ColonColon = Tok;
       SourceLocation CCLoc = ConsumeToken();
 
       CheckForLParenAfterColonColon();
 
-      bool IsCorrectedToColon = false;
-      bool *CorrectionFlagPtr = ColonIsSacred ? &IsCorrectedToColon : nullptr;
+      bool CheckForGlobalFriend = false;
+      SourceLocation LParenLoc;
+      if (Tok.is(tok::identifier) && CurrentDeclSpec &&
+          CurrentDeclSpec->isFriendSpecified() && !ObjectType) {
+        CheckForGlobalFriend = true;
+        IdInfo.NextIdentifier = Tok.getIdentifierInfo();
+        IdInfo.NextIdentifierLoc = Tok.getLocation();
+        // Look ahead to see if '::' can start a function declarator.
+        unsigned Displ = 0;
+        while (true) {
+          const Token &CurTok = GetLookAheadToken(Displ);
+          if (CurTok.is(tok::identifier)) {
+            const Token &NextTok = GetLookAheadToken(Displ + 1);
+            if (NextTok.is(tok::coloncolon)) {
+              Displ += 2;
+              continue;
+            }
+            if (NextTok.is(tok::l_paren)) {
+              LParenLoc = NextTok.getLocation();
+              IdInfo.FunctionExpected = true;
+              break;
+            }
+          }
+          break;
+        }
+      }
+      Sema::NNSRecoveryMode RecoveryMode(ColonIsSacred, CheckForGlobalFriend);
+
       if (Actions.ActOnCXXNestedNameSpecifier(getCurScope(), IdInfo,
                                               EnteringContext, SS,
-                                              false, CorrectionFlagPtr)) {
+                                              false, &RecoveryMode)) {
         // Identifier is not recognized as a nested name, but we can have
         // mistyped '::' instead of ':'.
-        if (CorrectionFlagPtr && IsCorrectedToColon) {
+        if (RecoveryMode.ColonCorrected) {
           ColonColon.setKind(tok::colon);
           PP.EnterToken(Tok);
           PP.EnterToken(ColonColon);
           Tok = Identifier;
           break;
         }
         SS.SetInvalid(SourceRange(IdLoc, CCLoc));
+
+        // If we are parsing a decl-spec, improve error diagnostics by
+        // recognizing the case when user refers to a global scope function in
+        // a friend declaration:
+        //
+        //     friend A ::B();
+        //
+        // where 'A' is a class or enum.
+        if (RecoveryMode.NextIdIsGlobal && !LParenLoc.isInvalid()) {
+          Diag(CCLoc, diag::note_possible_global_friend)
+            << FixItHint::CreateInsertion(CCLoc, "(")
+            << FixItHint::CreateInsertion(LParenLoc, ")");
+        }
       }
       HasScopeSpecifier = true;
       continue;
     }
 
     CheckForTemplateAndDigraph(Next, ObjectType, EnteringContext, II, SS);
 
     // nested-name-specifier:

lib/Parse/Parser.cpp

IdentifierInfo *Parser::getSEHExceptKeyword() {
   if (!Ident__except && (getLangOpts().MicrosoftExt || getLangOpts().Borland))
     Ident__except = PP.getIdentifierInfo("__except");
 
   return Ident__except;
 }
 
 Parser::Parser(Preprocessor &pp, Sema &actions, bool skipFunctionBodies)
   : PP(pp), Actions(actions), Diags(PP.getDiagnostics()),
-    GreaterThanIsOperator(true), ColonIsSacred(false), 
-    InMessageExpression(false), TemplateParameterDepth(0),
-    ParsingInObjCContainer(false) {
+    GreaterThanIsOperator(true), ColonIsSacred(false),
+    InMessageExpression(false), CurrentDeclSpec(nullptr),
+    TemplateParameterDepth(0), ParsingInObjCContainer(false) {
   SkipFunctionBodies = pp.isCodeCompletionEnabled() || skipFunctionBodies;
   Tok.startToken();
   Tok.setKind(tok::eof);
   Actions.CurScope = nullptr;
   NumCachedScopes = 0;
   ParenCount = BracketCount = BraceCount = 0;
   CurParsedObjCImpl = nullptr;
 

lib/Sema/SemaCXXScopeSpec.cpp

 /// dependent context, for example. Nor will it extend \p SS with the scope
 /// specifier.
 bool Sema::BuildCXXNestedNameSpecifier(Scope *S,
                                        NestedNameSpecInfo &IdInfo,
                                        bool EnteringContext,
                                        CXXScopeSpec &SS,
                                        NamedDecl *ScopeLookupResult,
                                        bool ErrorRecoveryLookup,
-                                       bool *IsCorrectedToColon) {
+                                       NNSRecoveryMode *RecoveryMode) {
   LookupResult Found(*this, IdInfo.Identifier, IdInfo.IdentifierLoc,
                      LookupNestedNameSpecifierName);
   QualType ObjectType = GetTypeFromParser(IdInfo.ObjectType);
 
   // Determine where to perform name lookup
   DeclContext *LookupCtx = nullptr;
   bool isDependent = false;
-  if (IsCorrectedToColon)
-    *IsCorrectedToColon = false;
+  if (RecoveryMode)
+    RecoveryMode->ColonCorrected = RecoveryMode->NextIdIsGlobal = false;
   if (!ObjectType.isNull()) {
     // This nested-name-specifier occurs in a member access expression, e.g.,
     // x->B::f, and we are looking into the type of the object.
     assert(!SS.isSet() && "ObjectType and scope specifier cannot coexist");
     LookupCtx = computeDeclContext(ObjectType);
     isDependent = ObjectType->isDependentType();
   } else if (SS.isSet()) {
     // This nested-name-specifier occurs after another nested-name-specifier,
if (LookupCtx)
       LookupQualifiedName(R, LookupCtx);
     else if (S && !isDependent)
       LookupName(R, S);
     if (!R.empty()) {
       // Don't diagnose problems with this speculative lookup.
       R.suppressDiagnostics();
       // The identifier is found in ordinary lookup. If correction to colon is
       // allowed, suggest replacement to ':'.
-      if (IsCorrectedToColon) {
-        *IsCorrectedToColon = true;
+      if (RecoveryMode && RecoveryMode->CorrectColon) {
+        RecoveryMode->ColonCorrected = true;
         Diag(IdInfo.CCLoc, diag::err_nested_name_spec_is_not_class)
             << IdInfo.Identifier << getLangOpts().CPlusPlus
             << FixItHint::CreateReplacement(IdInfo.CCLoc, ":");
         if (NamedDecl *ND = R.getAsSingle<NamedDecl>())
           Diag(ND->getLocation(), diag::note_declared_at);
         return true;
       }
       // Replacement '::' -> ':' is not allowed, just issue respective error.
bool Sema::BuildCXXNestedNameSpecifier(Scope *S,
   NamedDecl *SD =
       Found.isSingleResult() ? Found.getRepresentativeDecl() : nullptr;
   bool IsExtension = false;
   bool AcceptSpec = isAcceptableNestedNameSpecifier(SD, &IsExtension);
   if (!AcceptSpec && IsExtension) {
     AcceptSpec = true;
     Diag(IdInfo.IdentifierLoc, diag::ext_nested_name_spec_is_enum);
   }
+  if (AcceptSpec && RecoveryMode && RecoveryMode->CheckForMember) {
+    // Check if the identifier following '::' represents an entity in the
+    // context of found declaration.
+    assert(IdInfo.NextIdentifier != nullptr);
+    NamedDecl *FoundDecl = SD;
+    if (TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(FoundDecl)) {
+      QualType QT = TD->getUnderlyingType();
+      if (const TagType *TT = QT->getAs<TagType>())
+        if (TagDecl *TD = TT->getDecl())
+          FoundDecl = TD;
+    }
+
+    if (TagDecl *TD = dyn_cast<TagDecl>(FoundDecl)) {
+      if (TagDecl *Def = TD->getDefinition()) {
+        LookupResult Member(*this, IdInfo.NextIdentifier,
+                            IdInfo.NextIdentifierLoc, LookupMemberName);
+        LookupQualifiedName(Member, Def);
+        LookupResult::LookupResultKind Res = Member.getResultKind();
+        Member.clear();
+        if (Res == LookupResult::NotFound && IdInfo.FunctionExpected) {
+          Diag(IdInfo.NextIdentifierLoc, diag::err_typename_nested_not_found)
+            << IdInfo.NextIdentifier << Def << SS.getRange();
+          LookupResult Global(*this, IdInfo.NextIdentifier,
+                              IdInfo.NextIdentifierLoc, LookupOrdinaryName);
+          LookupQualifiedName(Global, Context.getTranslationUnitDecl());
+          for (NamedDecl *D : Global) {
+            if (isa<FunctionDecl>(D) || isa<FunctionTemplateDecl>(D))
+              RecoveryMode->NextIdIsGlobal = true;
+          }
+          Global.clear();
+          return true;
+        }
+      }
+    }
+  }
+
   if (AcceptSpec) {
     if (!ObjectType.isNull() && !ObjectTypeSearchedInScope &&
         !getLangOpts().CPlusPlus11) {
       // C++03 [basic.lookup.classref]p4:
       //   [...] If the name is found in both contexts, the
       //   class-name-or-namespace-name shall refer to the same entity.
       //
       // We already found the name in the scope of the object. Now, look
bool Sema::BuildCXXNestedNameSpecifier(Scope *S,
   return true;
 }
 
 bool Sema::ActOnCXXNestedNameSpecifier(Scope *S,
                                        NestedNameSpecInfo &IdInfo,
                                        bool EnteringContext,
                                        CXXScopeSpec &SS,
                                        bool ErrorRecoveryLookup,
-                                       bool *IsCorrectedToColon) {
+                                       NNSRecoveryMode *RecoveryMode) {
   if (SS.isInvalid())
     return true;
 
   return BuildCXXNestedNameSpecifier(S, IdInfo,
                                      EnteringContext, SS,
                                      /*ScopeLookupResult=*/nullptr, false,
-                                     IsCorrectedToColon);
+                                     RecoveryMode);
 }
 
 bool Sema::ActOnCXXNestedNameSpecifierDecltype(CXXScopeSpec &SS,
                                                const DeclSpec &DS,
                                                SourceLocation ColonColonLoc) {
   if (SS.isInvalid() || DS.getTypeSpecType() == DeclSpec::TST_error)
     return true;
 

test/CXX/drs/dr1xx.cpp

namespace dr122 { // dr122: yes
   template<typename T> void f();
   void g() { f<int>(); }
 }
 
 // dr123: na
 // dr124: dup 201
 
 // dr125: yes
-struct dr125_A { struct dr125_B {}; }; // expected-note {{here}}
+struct dr125_A { struct dr125_B {}; };
 dr125_A::dr125_B dr125_C();
 namespace dr125_B { dr125_A dr125_C(); }
 namespace dr125 {
   struct X {
     friend dr125_A::dr125_B (::dr125_C)(); // ok
     friend dr125_A (::dr125_B::dr125_C)(); // ok
-    friend dr125_A::dr125_B::dr125_C(); // expected-error {{did you mean the constructor name 'dr125_B'?}}
-    // expected-error@-1 {{missing exception specification}}
-#if __cplusplus >= 201103L
-    // expected-error@-3 {{follows constexpr declaration}} expected-note@-10 {{here}}
-#endif
+    friend dr125_A::dr125_B::dr125_C(); // expected-error {{no type named 'dr125_C' in 'dr125_B'}}
+                                        // expected-note@-1 {{if friend function is in global scope, put parentheses}}
   };
 }
 
 namespace dr126 { // dr126: no
   struct C {};
   struct D : C {};
   struct E : private C { friend class A; friend class B; };
   struct F : protected C {};

test/SemaCXX/nested-name-spec2.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
+enum E1 { ABCD };
+
+struct C1 {
+  struct C2 {};
+  enum E2 { XYZ };
+};
+
+E1     func_01();
+C1     func_02();
+C1::C2 func_03();
+C1::E2 func_04();
+
+int value_01;
+typedef int type_01;
+
+
+class C3 {
+  friend E1     ::func_01();    // expected-error{{no type named 'func_01' in 'E1'}}
+                                // expected-note@-1{{if friend function is in global scope, put parentheses}}
+  friend C1     ::func_02();    // expected-error{{no type named 'func_02' in 'C1'}}
+                                // expected-note@-1{{if friend function is in global scope, put parentheses}}
+  friend C1::C2 ::func_03();    // expected-error{{no type named 'func_03' in 'C2'}}
+                                // expected-note@-1{{if friend function is in global scope, put parentheses}}
+  friend C1::E2 ::func_04();    // expected-error{{no type named 'func_04' in 'E2'}}
+                                // expected-note@-1{{if friend function is in global scope, put parentheses}}
+  friend E1     ::nonexistent(); // expected-error{{no type named 'nonexistent' in 'E1'}}
+  friend E1     ::value_01();   // expected-error{{no type named 'value_01' in 'E1'}}
+  friend E1     ::type_01();    // expected-error{{no type named 'type_01' in 'E1'}}
+};
+
+
+namespace N1 {
+  class C3 {
+    friend ::E1     ::func_01();  // expected-error{{no type named 'func_01' in 'E1'}}
+                                  // expected-note@-1{{if friend function is in global scope, put parentheses}}
+    friend ::C1     ::func_02();  // expected-error{{no type named 'func_02' in 'C1'}}
+                                  // expected-note@-1{{if friend function is in global scope, put parentheses}}
+    friend ::C1::C2 ::func_03();  // expected-error{{no type named 'func_03' in 'C2'}}
+                                  // expected-note@-1{{if friend function is in global scope, put parentheses}}
+    friend ::C1::E2 ::func_04();  // expected-error{{no type named 'func_04' in 'E2'}}
+                                  // expected-note@-1{{if friend function is in global scope, put parentheses}}
+  };
+}
+
+
+namespace N2 {
+  enum E1 { ABCD };
+
+  struct C1 {
+    struct C2 {};
+    enum E2 { XYZ };
+  };
+
+  E1     func_01();
+  C1     func_02();
+  C1::C2 func_03();
+  C1::E2 func_04();
+}
+
+namespace N3 {
+  class C4 {
+    friend ::N2::E1     ::N2::func_01();  // expected-error{{no type named 'N2' in 'E1'}}
+    friend ::N2::C1     ::N2::func_02();  // expected-error{{no type named 'N2' in 'C1'}}
+    friend ::N2::C1::C2 ::N2::func_03();  // expected-error{{no type named 'N2' in 'C2'}}
+    friend ::N2::C1::E2 ::N2::func_04();  // expected-error{{no type named 'N2' in 'E2'}}
+  };
+}