Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reapply "[Clang] Implement resolution for CWG1835 (#92957, #98547)" #100425

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

sdkrystian
Copy link
Member

@sdkrystian sdkrystian commented Jul 24, 2024

This reapplies #92957 and #98547 with the following changes:

  • We only apply the resolution to CWG1835 in C++23 and later, and
  • We issue a diagnostic when its application results in a missing template keyword that breaks whatever construct that follows the intended template name. e.g.
template<int I>
struct A {
  int x;
};

template<int I>
struct B : A<I> {
  void f() {
    this->A<I>::f(); // with -std=c++20 and earlier: no error
                     // with -std=c++23 and later: 
                     //     error: no member named 'f' in the global namespace
                     //     error: missing 'template' keyword prior to dependent template name 'A'
  }
};

Only applying the resolution in C++23 and later conveniently gets around a bug in GCC releases prior to 11.1 that prohibits the use of template prior to the nested-name-specifier in a class member access expression as the affected versions do not support C++23.

@sdkrystian sdkrystian requested review from Endilll and a team as code owners July 24, 2024 16:51
@llvmbot llvmbot added clang Clang issues not falling into any other category libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules coroutines C++20 coroutines clang:openmp OpenMP related changes to Clang labels Jul 24, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 24, 2024

@llvm/pr-subscribers-coroutines

@llvm/pr-subscribers-clang-modules

Author: Krystian Stasiowski (sdkrystian)

Changes

This reapplies #92957 and #98547 with the following changes:

  • We only apply the resolution to CWG1835 in C++23 and later, and
  • We issue a diagnostic when its application results in a missing template keyword that breaks whatever construct that follows the intended template name. e.g.
template&lt;int I&gt;
struct A {
  int x;
};

template&lt;int I&gt;
struct B : A&lt;I&gt; {
  void f() {
    this-&gt;A&lt;I&gt;::f(); // with -std=c++20 and earlier: no error
                     // with -std=c++23 and later: 
                     //     error: no member named 'f' in the global namespace
                     //     error: missing 'template' keyword prior to dependent template name 'A'
  }
};

Only applying the resolution in C++23 and later conveniently gets around a bug in GCC releases prior to 11.1 that prohibits the use of template prior to the nested-name-specifier in a class member access expression as the affected versions do not support C++23.


Patch is 167.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100425.diff

47 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/include/clang/AST/ExprCXX.h (+49-43)
  • (modified) clang/include/clang/AST/Stmt.h (+8-7)
  • (modified) clang/include/clang/AST/UnresolvedSet.h (+4)
  • (modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+1-3)
  • (modified) clang/include/clang/Parse/Parser.h (+7-10)
  • (modified) clang/include/clang/Sema/DeclSpec.h (+8)
  • (modified) clang/include/clang/Sema/Lookup.h (+6-2)
  • (modified) clang/include/clang/Sema/Sema.h (+38-42)
  • (modified) clang/lib/AST/ASTImporter.cpp (+9-3)
  • (modified) clang/lib/AST/ExprCXX.cpp (+39-27)
  • (modified) clang/lib/AST/ItaniumMangle.cpp (+15-24)
  • (modified) clang/lib/Parse/ParseExpr.cpp (+4-6)
  • (modified) clang/lib/Parse/ParseExprCXX.cpp (+54-50)
  • (modified) clang/lib/Parse/ParseOpenMP.cpp (+1-3)
  • (modified) clang/lib/Parse/ParseTemplate.cpp (+40)
  • (modified) clang/lib/Sema/SemaCXXScopeSpec.cpp (+95-104)
  • (modified) clang/lib/Sema/SemaCoroutine.cpp (+2-2)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+23-24)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaExprMember.cpp (+33-42)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+5-7)
  • (modified) clang/lib/Sema/SemaStmtAsm.cpp (+5-3)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+111-114)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+7-10)
  • (modified) clang/lib/Sema/TreeTransform.h (+131-198)
  • (modified) clang/lib/Serialization/ASTReaderStmt.cpp (+32-31)
  • (modified) clang/lib/Serialization/ASTWriterStmt.cpp (+25-18)
  • (modified) clang/test/CXX/basic/basic.lookup/basic.lookup.classref/p1-cxx11.cpp (+9-5)
  • (modified) clang/test/CXX/basic/basic.lookup/basic.lookup.classref/p1.cpp (+9-5)
  • (added) clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p3-example3.cpp (+27)
  • (added) clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p3.cpp (+98)
  • (modified) clang/test/CXX/drs/cwg1xx.cpp (+12-10)
  • (added) clang/test/CXX/temp/temp.names/p3-23.cpp (+237)
  • (modified) clang/test/CXX/temp/temp.res/p3.cpp (+1-1)
  • (modified) clang/test/FixIt/fixit.cpp (+2-2)
  • (modified) clang/test/Misc/warning-flags.c (+1-1)
  • (modified) clang/test/Parser/cxx2a-concepts-requires-expr.cpp (+1-1)
  • (modified) clang/test/SemaCXX/cxx0x-noexcept-expression.cpp (+3-3)
  • (modified) clang/test/SemaCXX/nested-name-spec.cpp (+4-4)
  • (modified) clang/test/SemaCXX/pseudo-destructors.cpp (+16-11)
  • (modified) clang/test/SemaTemplate/dependent-base-classes.cpp (+10-10)
  • (modified) clang/test/SemaTemplate/dependent-template-recover.cpp (+6-6)
  • (modified) clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp (+1-1)
  • (modified) clang/test/SemaTemplate/template-id-expr.cpp (+7-7)
  • (modified) clang/test/SemaTemplate/typename-specifier-3.cpp (+1-1)
  • (modified) libcxx/include/regex (+1-1)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a34f109ba21ce..1ffc759f7e4b8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -313,6 +313,9 @@ Resolutions to C++ Defect Reports
 - Clang now considers ``noexcept(typeid(expr))`` more carefully, instead of always assuming that ``std::bad_typeid`` can be thrown.
   (`CWG2191: Incorrect result for noexcept(typeid(v)) <https://cplusplus.github.io/CWG/issues/2191.html>`_).
 
+- Clang now correctly implements lookup for the terminal name of a member-qualified nested-name-specifier.
+  (`CWG1835: Dependent member lookup before < <https://cplusplus.github.io/CWG/issues/1835.html>`_).
+
 C Language Changes
 ------------------
 
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index c2feac525c1ea..edaea6fe27cc3 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -3676,9 +3676,9 @@ class CXXUnresolvedConstructExpr final
 /// an implicit access if a qualifier is provided.
 class CXXDependentScopeMemberExpr final
     : public Expr,
-      private llvm::TrailingObjects<CXXDependentScopeMemberExpr,
-                                    ASTTemplateKWAndArgsInfo,
-                                    TemplateArgumentLoc, NamedDecl *> {
+      private llvm::TrailingObjects<
+          CXXDependentScopeMemberExpr, NestedNameSpecifierLoc, DeclAccessPair,
+          ASTTemplateKWAndArgsInfo, TemplateArgumentLoc> {
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
   friend TrailingObjects;
@@ -3691,17 +3691,15 @@ class CXXDependentScopeMemberExpr final
   /// implicit accesses.
   QualType BaseType;
 
-  /// The nested-name-specifier that precedes the member name, if any.
-  /// FIXME: This could be in principle store as a trailing object.
-  /// However the performance impact of doing so should be investigated first.
-  NestedNameSpecifierLoc QualifierLoc;
-
   /// The member to which this member expression refers, which
   /// can be name, overloaded operator, or destructor.
   ///
   /// FIXME: could also be a template-id
   DeclarationNameInfo MemberNameInfo;
 
+  /// The location of the '->' or '.' operator.
+  SourceLocation OperatorLoc;
+
   // CXXDependentScopeMemberExpr is followed by several trailing objects,
   // some of which optional. They are in order:
   //
@@ -3721,8 +3719,16 @@ class CXXDependentScopeMemberExpr final
     return CXXDependentScopeMemberExprBits.HasTemplateKWAndArgsInfo;
   }
 
-  bool hasFirstQualifierFoundInScope() const {
-    return CXXDependentScopeMemberExprBits.HasFirstQualifierFoundInScope;
+  unsigned getNumUnqualifiedLookups() const {
+    return CXXDependentScopeMemberExprBits.NumUnqualifiedLookups;
+  }
+
+  unsigned numTrailingObjects(OverloadToken<NestedNameSpecifierLoc>) const {
+    return hasQualifier();
+  }
+
+  unsigned numTrailingObjects(OverloadToken<DeclAccessPair>) const {
+    return getNumUnqualifiedLookups();
   }
 
   unsigned numTrailingObjects(OverloadToken<ASTTemplateKWAndArgsInfo>) const {
@@ -3733,33 +3739,32 @@ class CXXDependentScopeMemberExpr final
     return getNumTemplateArgs();
   }
 
-  unsigned numTrailingObjects(OverloadToken<NamedDecl *>) const {
-    return hasFirstQualifierFoundInScope();
-  }
-
   CXXDependentScopeMemberExpr(const ASTContext &Ctx, Expr *Base,
                               QualType BaseType, bool IsArrow,
                               SourceLocation OperatorLoc,
                               NestedNameSpecifierLoc QualifierLoc,
                               SourceLocation TemplateKWLoc,
-                              NamedDecl *FirstQualifierFoundInScope,
+                              ArrayRef<DeclAccessPair> UnqualifiedLookups,
                               DeclarationNameInfo MemberNameInfo,
                               const TemplateArgumentListInfo *TemplateArgs);
 
-  CXXDependentScopeMemberExpr(EmptyShell Empty, bool HasTemplateKWAndArgsInfo,
-                              bool HasFirstQualifierFoundInScope);
+  CXXDependentScopeMemberExpr(EmptyShell Empty, bool HasQualifier,
+                              unsigned NumUnqualifiedLookups,
+                              bool HasTemplateKWAndArgsInfo);
 
 public:
   static CXXDependentScopeMemberExpr *
   Create(const ASTContext &Ctx, Expr *Base, QualType BaseType, bool IsArrow,
          SourceLocation OperatorLoc, NestedNameSpecifierLoc QualifierLoc,
-         SourceLocation TemplateKWLoc, NamedDecl *FirstQualifierFoundInScope,
+         SourceLocation TemplateKWLoc,
+         ArrayRef<DeclAccessPair> UnqualifiedLookups,
          DeclarationNameInfo MemberNameInfo,
          const TemplateArgumentListInfo *TemplateArgs);
 
   static CXXDependentScopeMemberExpr *
-  CreateEmpty(const ASTContext &Ctx, bool HasTemplateKWAndArgsInfo,
-              unsigned NumTemplateArgs, bool HasFirstQualifierFoundInScope);
+  CreateEmpty(const ASTContext &Ctx, bool HasQualifier,
+              unsigned NumUnqualifiedLookups, bool HasTemplateKWAndArgsInfo,
+              unsigned NumTemplateArgs);
 
   /// True if this is an implicit access, i.e. one in which the
   /// member being accessed was not written in the source.  The source
@@ -3784,34 +3789,35 @@ class CXXDependentScopeMemberExpr final
   bool isArrow() const { return CXXDependentScopeMemberExprBits.IsArrow; }
 
   /// Retrieve the location of the '->' or '.' operator.
-  SourceLocation getOperatorLoc() const {
-    return CXXDependentScopeMemberExprBits.OperatorLoc;
+  SourceLocation getOperatorLoc() const { return OperatorLoc; }
+
+  /// Determines whether this member expression had a nested-name-specifier
+  /// prior to the name of the member, e.g., x->Base::foo.
+  bool hasQualifier() const {
+    return CXXDependentScopeMemberExprBits.HasQualifier;
   }
 
-  /// Retrieve the nested-name-specifier that qualifies the member name.
-  NestedNameSpecifier *getQualifier() const {
-    return QualifierLoc.getNestedNameSpecifier();
+  /// If the member name was qualified, retrieves the nested-name-specifier
+  /// that precedes the member name, with source-location information.
+  NestedNameSpecifierLoc getQualifierLoc() const {
+    if (!hasQualifier())
+      return NestedNameSpecifierLoc();
+    return *getTrailingObjects<NestedNameSpecifierLoc>();
   }
 
-  /// Retrieve the nested-name-specifier that qualifies the member
-  /// name, with source location information.
-  NestedNameSpecifierLoc getQualifierLoc() const { return QualifierLoc; }
+  /// If the member name was qualified, retrieves the
+  /// nested-name-specifier that precedes the member name. Otherwise, returns
+  /// NULL.
+  NestedNameSpecifier *getQualifier() const {
+    return getQualifierLoc().getNestedNameSpecifier();
+  }
 
-  /// Retrieve the first part of the nested-name-specifier that was
-  /// found in the scope of the member access expression when the member access
-  /// was initially parsed.
-  ///
-  /// This function only returns a useful result when member access expression
-  /// uses a qualified member name, e.g., "x.Base::f". Here, the declaration
-  /// returned by this function describes what was found by unqualified name
-  /// lookup for the identifier "Base" within the scope of the member access
-  /// expression itself. At template instantiation time, this information is
-  /// combined with the results of name lookup into the type of the object
-  /// expression itself (the class type of x).
-  NamedDecl *getFirstQualifierFoundInScope() const {
-    if (!hasFirstQualifierFoundInScope())
-      return nullptr;
-    return *getTrailingObjects<NamedDecl *>();
+  /// Retrieve the declarations found by unqualified lookup for the first
+  /// component name of the nested-name-specifier, if any.
+  ArrayRef<DeclAccessPair> unqualified_lookups() const {
+    if (!getNumUnqualifiedLookups())
+      return std::nullopt;
+    return {getTrailingObjects<DeclAccessPair>(), getNumUnqualifiedLookups()};
   }
 
   /// Retrieve the name of the member that this expression refers to.
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 9cd7a364cd3f1..257a61c97c9c6 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1020,18 +1020,19 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsArrow : 1;
 
+    /// True if this member expression used a nested-name-specifier to
+    /// refer to the member, e.g., "x->Base::f".
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasQualifier : 1;
+
     /// Whether this member expression has info for explicit template
     /// keyword and arguments.
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasTemplateKWAndArgsInfo : 1;
 
-    /// See getFirstQualifierFoundInScope() and the comment listing
-    /// the trailing objects.
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned HasFirstQualifierFoundInScope : 1;
-
-    /// The location of the '->' or '.' operator.
-    SourceLocation OperatorLoc;
+    /// Number of declarations found by unqualified lookup for the
+    /// first component name of the nested-name-specifier.
+    unsigned NumUnqualifiedLookups;
   };
 
   class OverloadExprBitfields {
diff --git a/clang/include/clang/AST/UnresolvedSet.h b/clang/include/clang/AST/UnresolvedSet.h
index 1369725ab4e96..ef44499ce5926 100644
--- a/clang/include/clang/AST/UnresolvedSet.h
+++ b/clang/include/clang/AST/UnresolvedSet.h
@@ -97,6 +97,10 @@ class UnresolvedSetImpl {
     decls().push_back(DeclAccessPair::make(D, AS));
   }
 
+  void addAllDecls(ArrayRef<DeclAccessPair> Other) {
+    append(iterator(Other.begin()), iterator(Other.end()));
+  }
+
   /// Replaces the given declaration with the new one, once.
   ///
   /// \return true if the set changed
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 12aab09f28556..0bd2e35bf2e31 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -895,9 +895,7 @@ def missing_template_arg_list_after_template_kw : Extension<
   "keyword">, InGroup<DiagGroup<"missing-template-arg-list-after-template-kw">>,
   DefaultError;
 
-def err_missing_dependent_template_keyword : Error<
-  "use 'template' keyword to treat '%0' as a dependent template name">;
-def warn_missing_dependent_template_keyword : ExtWarn<
+def ext_missing_dependent_template_keyword : ExtWarn<
   "use 'template' keyword to treat '%0' as a dependent template name">;
 
 def ext_extern_template : Extension<
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 126fea0aef2a7..134bb1b3534e7 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1906,6 +1906,7 @@ class Parser : public CodeCompletionHandler {
   }
 
   bool diagnoseUnknownTemplateId(ExprResult TemplateName, SourceLocation Less);
+  bool isMissingTemplateKeywordBeforeScope();
   void checkPotentialAngleBracket(ExprResult &PotentialTemplateName);
   bool checkPotentialAngleBracketDelimiter(const AngleBracketTracker::Loc &,
                                            const Token &OpToken);
@@ -1966,7 +1967,7 @@ class Parser : public CodeCompletionHandler {
   //===--------------------------------------------------------------------===//
   // C++ Expressions
   ExprResult tryParseCXXIdExpression(CXXScopeSpec &SS, bool isAddressOfOperand,
-                                     Token &Replacement);
+                                     Token *Replacement = nullptr);
 
   ExprResult tryParseCXXPackIndexingExpression(ExprResult PackIdExpression);
   ExprResult ParseCXXPackIndexingExpression(ExprResult PackIdExpression);
@@ -3372,15 +3373,11 @@ class Parser : public CodeCompletionHandler {
   BaseResult ParseBaseSpecifier(Decl *ClassDecl);
   AccessSpecifier getAccessSpecifierIfPresent() const;
 
-  bool ParseUnqualifiedIdTemplateId(CXXScopeSpec &SS,
-                                    ParsedType ObjectType,
-                                    bool ObjectHadErrors,
-                                    SourceLocation TemplateKWLoc,
-                                    IdentifierInfo *Name,
-                                    SourceLocation NameLoc,
-                                    bool EnteringContext,
-                                    UnqualifiedId &Id,
-                                    bool AssumeTemplateId);
+  bool ParseUnqualifiedIdTemplateId(
+      CXXScopeSpec &SS, ParsedType ObjectType, bool ObjectHadErrors,
+      SourceLocation TemplateKWLoc, SourceLocation TildeLoc,
+      IdentifierInfo *Name, SourceLocation NameLoc, bool EnteringContext,
+      UnqualifiedId &Id, bool AssumeTemplateId);
   bool ParseUnqualifiedIdOperator(CXXScopeSpec &SS, bool EnteringContext,
                                   ParsedType ObjectType,
                                   UnqualifiedId &Result);
diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 425b6e2a0b30c..9c22c35535ede 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -75,6 +75,7 @@ class CXXScopeSpec {
   SourceRange Range;
   NestedNameSpecifierLocBuilder Builder;
   ArrayRef<TemplateParameterList *> TemplateParamLists;
+  ArrayRef<DeclAccessPair> UnqualifiedLookups;
 
 public:
   SourceRange getRange() const { return Range; }
@@ -91,6 +92,13 @@ class CXXScopeSpec {
     return TemplateParamLists;
   }
 
+  void setUnqualifiedLookups(ArrayRef<DeclAccessPair> Found) {
+    UnqualifiedLookups = Found;
+  }
+  ArrayRef<DeclAccessPair> getUnqualifiedLookups() const {
+    return UnqualifiedLookups;
+  }
+
   /// Retrieve the representation of the nested-name-specifier.
   NestedNameSpecifier *getScopeRep() const {
     return Builder.getRepresentation();
diff --git a/clang/include/clang/Sema/Lookup.h b/clang/include/clang/Sema/Lookup.h
index b0a08a05ac6a0..6b765ef3c980f 100644
--- a/clang/include/clang/Sema/Lookup.h
+++ b/clang/include/clang/Sema/Lookup.h
@@ -483,11 +483,15 @@ class LookupResult {
     ResultKind = Found;
   }
 
+  void addAllDecls(ArrayRef<DeclAccessPair> Other) {
+    Decls.addAllDecls(Other);
+    ResultKind = Found;
+  }
+
   /// Add all the declarations from another set of lookup
   /// results.
   void addAllDecls(const LookupResult &Other) {
-    Decls.append(Other.Decls.begin(), Other.Decls.end());
-    ResultKind = Found;
+    addAllDecls(Other.Decls.pairs());
   }
 
   /// Determine whether no result was found because we could not
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 48dff1b76cc57..a86decb67bca1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2803,7 +2803,8 @@ class Sema final : public SemaBase {
   /// (e.g., Base::), perform name lookup for that identifier as a
   /// nested-name-specifier within the given scope, and return the result of
   /// that name lookup.
-  NamedDecl *FindFirstQualifierInScope(Scope *S, NestedNameSpecifier *NNS);
+  bool LookupFirstQualifierInScope(Scope *S, NestedNameSpecifier *NNS,
+                                   UnresolvedSetImpl &R);
 
   /// Keeps information about an identifier in a nested-name-spec.
   ///
@@ -2843,9 +2844,6 @@ class Sema final : public SemaBase {
   /// \param EnteringContext If true, enter the context specified by the
   ///        nested-name-specifier.
   /// \param SS Optional nested name specifier preceding the identifier.
-  /// \param ScopeLookupResult Provides the result of name lookup within the
-  ///        scope of the nested-name-specifier that was computed at template
-  ///        definition time.
   /// \param ErrorRecoveryLookup Specifies if the method is called to improve
   ///        error recovery and what kind of recovery is performed.
   /// \param IsCorrectedToColon If not null, suggestion of replace '::' -> ':'
@@ -2854,11 +2852,6 @@ class Sema final : public SemaBase {
   ///        not '::'.
   /// \param OnlyNamespace If true, only considers namespaces in lookup.
   ///
-  /// This routine differs only slightly from ActOnCXXNestedNameSpecifier, in
-  /// that it contains an extra parameter \p ScopeLookupResult, which provides
-  /// the result of name lookup within the scope of the nested-name-specifier
-  /// that was computed at template definition time.
-  ///
   /// If ErrorRecoveryLookup is true, then this call is used to improve error
   /// recovery.  This means that it should not emit diagnostics, it should
   /// just return true on failure.  It also means it should only return a valid
@@ -2867,7 +2860,6 @@ class Sema final : public SemaBase {
   /// specifier.
   bool BuildCXXNestedNameSpecifier(Scope *S, NestedNameSpecInfo &IdInfo,
                                    bool EnteringContext, CXXScopeSpec &SS,
-                                   NamedDecl *ScopeLookupResult,
                                    bool ErrorRecoveryLookup,
                                    bool *IsCorrectedToColon = nullptr,
                                    bool OnlyNamespace = false);
@@ -8567,11 +8559,12 @@ class Sema final : public SemaBase {
                           const TemplateArgumentListInfo *TemplateArgs,
                           bool IsDefiniteInstance, const Scope *S);
 
-  ExprResult ActOnDependentMemberExpr(
-      Expr *Base, QualType BaseType, bool IsArrow, SourceLocation OpLoc,
-      const CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
-      NamedDecl *FirstQualifierInScope, const DeclarationNameInfo &NameInfo,
-      const TemplateArgumentListInfo *TemplateArgs);
+  ExprResult
+  ActOnDependentMemberExpr(Expr *Base, QualType BaseType, bool IsArrow,
+                           SourceLocation OpLoc, const CXXScopeSpec &SS,
+                           SourceLocation TemplateKWLoc,
+                           const DeclarationNameInfo &NameInfo,
+                           const TemplateArgumentListInfo *TemplateArgs);
 
   /// The main callback when the parser finds something like
   ///   expression . [nested-name-specifier] identifier
@@ -8627,15 +8620,14 @@ class Sema final : public SemaBase {
   ExprResult BuildMemberReferenceExpr(
       Expr *Base, QualType BaseType, SourceLocation OpLoc, bool IsArrow,
       CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
-      NamedDecl *FirstQualifierInScope, const DeclarationNameInfo &NameInfo,
+      const DeclarationNameInfo &NameInfo,
       const TemplateArgumentListInfo *TemplateArgs, const Scope *S,
       ActOnMemberAccessExtraArgs *ExtraArgs = nullptr);
 
   ExprResult
   BuildMemberReferenceExpr(Expr *Base, QualType BaseType, SourceLocation OpLoc,
                            bool IsArrow, const CXXScopeSpec &SS,
-                           SourceLocation TemplateKWLoc,
-                           NamedDecl *FirstQualifierInScope, LookupResult &R,
+                           SourceLocation TemplateKWLoc, LookupResult &R,
                            const TemplateArgumentListInfo *TemplateArgs,
                            const Scope *S, bool SuppressQualifierCheck = false,
                            ActOnMemberAccessExtraArgs *ExtraArgs = nullptr);
@@ -11123,15 +11115,14 @@ class Sema final : public SemaBase {
                      QualType ObjectType, bool EnteringContext,
                      RequiredTemplateKind RequiredTemplate = SourceLocation(),
                      AssumedTemplateKind *ATK = nullptr,
-                     bool AllowTypoCorrection = true);
+                     bool AllowTypoCorrection = true, bool MayBeNNS = false);
 
-  TemplateNameKind isTemplateName(Scope *S, CXXScopeSpec &SS,
-                                  bool hasTemplateKeyword,
-                                  const UnqualifiedId &Name,
-                                  ParsedType ObjectType, bool EnteringContext,
-                                  TemplateTy &Template,
-                                  bool &MemberOfUnknownSpecialization,
-                                  bool Disambiguation = false);
+  TemplateNameKind
+  isTemplateName(Scope *S, CXXScopeSpec &SS, bool hasTemplateKeyword,
+                 const UnqualifiedId &Name, ParsedType ObjectType,
+                 bool EnteringContext, TemplateTy &Template,
+                 bool &MemberOfUnknownSpecialization,
+                 bool Disambiguation = false, bool MayBeNNS = false);
 
   /// Try to resolve an undeclared templa...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 24, 2024

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

This reapplies #92957 and #98547 with the following changes:

  • We only apply the resolution to CWG1835 in C++23 and later, and
  • We issue a diagnostic when its application results in a missing template keyword that breaks whatever construct that follows the intended template name. e.g.
template&lt;int I&gt;
struct A {
  int x;
};

template&lt;int I&gt;
struct B : A&lt;I&gt; {
  void f() {
    this-&gt;A&lt;I&gt;::f(); // with -std=c++20 and earlier: no error
                     // with -std=c++23 and later: 
                     //     error: no member named 'f' in the global namespace
                     //     error: missing 'template' keyword prior to dependent template name 'A'
  }
};

Only applying the resolution in C++23 and later conveniently gets around a bug in GCC releases prior to 11.1 that prohibits the use of template prior to the nested-name-specifier in a class member access expression as the affected versions do not support C++23.


Patch is 167.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100425.diff

47 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+3)
  • (modified) clang/include/clang/AST/ExprCXX.h (+49-43)
  • (modified) clang/include/clang/AST/Stmt.h (+8-7)
  • (modified) clang/include/clang/AST/UnresolvedSet.h (+4)
  • (modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+1-3)
  • (modified) clang/include/clang/Parse/Parser.h (+7-10)
  • (modified) clang/include/clang/Sema/DeclSpec.h (+8)
  • (modified) clang/include/clang/Sema/Lookup.h (+6-2)
  • (modified) clang/include/clang/Sema/Sema.h (+38-42)
  • (modified) clang/lib/AST/ASTImporter.cpp (+9-3)
  • (modified) clang/lib/AST/ExprCXX.cpp (+39-27)
  • (modified) clang/lib/AST/ItaniumMangle.cpp (+15-24)
  • (modified) clang/lib/Parse/ParseExpr.cpp (+4-6)
  • (modified) clang/lib/Parse/ParseExprCXX.cpp (+54-50)
  • (modified) clang/lib/Parse/ParseOpenMP.cpp (+1-3)
  • (modified) clang/lib/Parse/ParseTemplate.cpp (+40)
  • (modified) clang/lib/Sema/SemaCXXScopeSpec.cpp (+95-104)
  • (modified) clang/lib/Sema/SemaCoroutine.cpp (+2-2)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+23-24)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaExprMember.cpp (+33-42)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+5-7)
  • (modified) clang/lib/Sema/SemaStmtAsm.cpp (+5-3)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+111-114)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+7-10)
  • (modified) clang/lib/Sema/TreeTransform.h (+131-198)
  • (modified) clang/lib/Serialization/ASTReaderStmt.cpp (+32-31)
  • (modified) clang/lib/Serialization/ASTWriterStmt.cpp (+25-18)
  • (modified) clang/test/CXX/basic/basic.lookup/basic.lookup.classref/p1-cxx11.cpp (+9-5)
  • (modified) clang/test/CXX/basic/basic.lookup/basic.lookup.classref/p1.cpp (+9-5)
  • (added) clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p3-example3.cpp (+27)
  • (added) clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p3.cpp (+98)
  • (modified) clang/test/CXX/drs/cwg1xx.cpp (+12-10)
  • (added) clang/test/CXX/temp/temp.names/p3-23.cpp (+237)
  • (modified) clang/test/CXX/temp/temp.res/p3.cpp (+1-1)
  • (modified) clang/test/FixIt/fixit.cpp (+2-2)
  • (modified) clang/test/Misc/warning-flags.c (+1-1)
  • (modified) clang/test/Parser/cxx2a-concepts-requires-expr.cpp (+1-1)
  • (modified) clang/test/SemaCXX/cxx0x-noexcept-expression.cpp (+3-3)
  • (modified) clang/test/SemaCXX/nested-name-spec.cpp (+4-4)
  • (modified) clang/test/SemaCXX/pseudo-destructors.cpp (+16-11)
  • (modified) clang/test/SemaTemplate/dependent-base-classes.cpp (+10-10)
  • (modified) clang/test/SemaTemplate/dependent-template-recover.cpp (+6-6)
  • (modified) clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp (+1-1)
  • (modified) clang/test/SemaTemplate/template-id-expr.cpp (+7-7)
  • (modified) clang/test/SemaTemplate/typename-specifier-3.cpp (+1-1)
  • (modified) libcxx/include/regex (+1-1)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a34f109ba21ce..1ffc759f7e4b8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -313,6 +313,9 @@ Resolutions to C++ Defect Reports
 - Clang now considers ``noexcept(typeid(expr))`` more carefully, instead of always assuming that ``std::bad_typeid`` can be thrown.
   (`CWG2191: Incorrect result for noexcept(typeid(v)) <https://cplusplus.github.io/CWG/issues/2191.html>`_).
 
+- Clang now correctly implements lookup for the terminal name of a member-qualified nested-name-specifier.
+  (`CWG1835: Dependent member lookup before < <https://cplusplus.github.io/CWG/issues/1835.html>`_).
+
 C Language Changes
 ------------------
 
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index c2feac525c1ea..edaea6fe27cc3 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -3676,9 +3676,9 @@ class CXXUnresolvedConstructExpr final
 /// an implicit access if a qualifier is provided.
 class CXXDependentScopeMemberExpr final
     : public Expr,
-      private llvm::TrailingObjects<CXXDependentScopeMemberExpr,
-                                    ASTTemplateKWAndArgsInfo,
-                                    TemplateArgumentLoc, NamedDecl *> {
+      private llvm::TrailingObjects<
+          CXXDependentScopeMemberExpr, NestedNameSpecifierLoc, DeclAccessPair,
+          ASTTemplateKWAndArgsInfo, TemplateArgumentLoc> {
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
   friend TrailingObjects;
@@ -3691,17 +3691,15 @@ class CXXDependentScopeMemberExpr final
   /// implicit accesses.
   QualType BaseType;
 
-  /// The nested-name-specifier that precedes the member name, if any.
-  /// FIXME: This could be in principle store as a trailing object.
-  /// However the performance impact of doing so should be investigated first.
-  NestedNameSpecifierLoc QualifierLoc;
-
   /// The member to which this member expression refers, which
   /// can be name, overloaded operator, or destructor.
   ///
   /// FIXME: could also be a template-id
   DeclarationNameInfo MemberNameInfo;
 
+  /// The location of the '->' or '.' operator.
+  SourceLocation OperatorLoc;
+
   // CXXDependentScopeMemberExpr is followed by several trailing objects,
   // some of which optional. They are in order:
   //
@@ -3721,8 +3719,16 @@ class CXXDependentScopeMemberExpr final
     return CXXDependentScopeMemberExprBits.HasTemplateKWAndArgsInfo;
   }
 
-  bool hasFirstQualifierFoundInScope() const {
-    return CXXDependentScopeMemberExprBits.HasFirstQualifierFoundInScope;
+  unsigned getNumUnqualifiedLookups() const {
+    return CXXDependentScopeMemberExprBits.NumUnqualifiedLookups;
+  }
+
+  unsigned numTrailingObjects(OverloadToken<NestedNameSpecifierLoc>) const {
+    return hasQualifier();
+  }
+
+  unsigned numTrailingObjects(OverloadToken<DeclAccessPair>) const {
+    return getNumUnqualifiedLookups();
   }
 
   unsigned numTrailingObjects(OverloadToken<ASTTemplateKWAndArgsInfo>) const {
@@ -3733,33 +3739,32 @@ class CXXDependentScopeMemberExpr final
     return getNumTemplateArgs();
   }
 
-  unsigned numTrailingObjects(OverloadToken<NamedDecl *>) const {
-    return hasFirstQualifierFoundInScope();
-  }
-
   CXXDependentScopeMemberExpr(const ASTContext &Ctx, Expr *Base,
                               QualType BaseType, bool IsArrow,
                               SourceLocation OperatorLoc,
                               NestedNameSpecifierLoc QualifierLoc,
                               SourceLocation TemplateKWLoc,
-                              NamedDecl *FirstQualifierFoundInScope,
+                              ArrayRef<DeclAccessPair> UnqualifiedLookups,
                               DeclarationNameInfo MemberNameInfo,
                               const TemplateArgumentListInfo *TemplateArgs);
 
-  CXXDependentScopeMemberExpr(EmptyShell Empty, bool HasTemplateKWAndArgsInfo,
-                              bool HasFirstQualifierFoundInScope);
+  CXXDependentScopeMemberExpr(EmptyShell Empty, bool HasQualifier,
+                              unsigned NumUnqualifiedLookups,
+                              bool HasTemplateKWAndArgsInfo);
 
 public:
   static CXXDependentScopeMemberExpr *
   Create(const ASTContext &Ctx, Expr *Base, QualType BaseType, bool IsArrow,
          SourceLocation OperatorLoc, NestedNameSpecifierLoc QualifierLoc,
-         SourceLocation TemplateKWLoc, NamedDecl *FirstQualifierFoundInScope,
+         SourceLocation TemplateKWLoc,
+         ArrayRef<DeclAccessPair> UnqualifiedLookups,
          DeclarationNameInfo MemberNameInfo,
          const TemplateArgumentListInfo *TemplateArgs);
 
   static CXXDependentScopeMemberExpr *
-  CreateEmpty(const ASTContext &Ctx, bool HasTemplateKWAndArgsInfo,
-              unsigned NumTemplateArgs, bool HasFirstQualifierFoundInScope);
+  CreateEmpty(const ASTContext &Ctx, bool HasQualifier,
+              unsigned NumUnqualifiedLookups, bool HasTemplateKWAndArgsInfo,
+              unsigned NumTemplateArgs);
 
   /// True if this is an implicit access, i.e. one in which the
   /// member being accessed was not written in the source.  The source
@@ -3784,34 +3789,35 @@ class CXXDependentScopeMemberExpr final
   bool isArrow() const { return CXXDependentScopeMemberExprBits.IsArrow; }
 
   /// Retrieve the location of the '->' or '.' operator.
-  SourceLocation getOperatorLoc() const {
-    return CXXDependentScopeMemberExprBits.OperatorLoc;
+  SourceLocation getOperatorLoc() const { return OperatorLoc; }
+
+  /// Determines whether this member expression had a nested-name-specifier
+  /// prior to the name of the member, e.g., x->Base::foo.
+  bool hasQualifier() const {
+    return CXXDependentScopeMemberExprBits.HasQualifier;
   }
 
-  /// Retrieve the nested-name-specifier that qualifies the member name.
-  NestedNameSpecifier *getQualifier() const {
-    return QualifierLoc.getNestedNameSpecifier();
+  /// If the member name was qualified, retrieves the nested-name-specifier
+  /// that precedes the member name, with source-location information.
+  NestedNameSpecifierLoc getQualifierLoc() const {
+    if (!hasQualifier())
+      return NestedNameSpecifierLoc();
+    return *getTrailingObjects<NestedNameSpecifierLoc>();
   }
 
-  /// Retrieve the nested-name-specifier that qualifies the member
-  /// name, with source location information.
-  NestedNameSpecifierLoc getQualifierLoc() const { return QualifierLoc; }
+  /// If the member name was qualified, retrieves the
+  /// nested-name-specifier that precedes the member name. Otherwise, returns
+  /// NULL.
+  NestedNameSpecifier *getQualifier() const {
+    return getQualifierLoc().getNestedNameSpecifier();
+  }
 
-  /// Retrieve the first part of the nested-name-specifier that was
-  /// found in the scope of the member access expression when the member access
-  /// was initially parsed.
-  ///
-  /// This function only returns a useful result when member access expression
-  /// uses a qualified member name, e.g., "x.Base::f". Here, the declaration
-  /// returned by this function describes what was found by unqualified name
-  /// lookup for the identifier "Base" within the scope of the member access
-  /// expression itself. At template instantiation time, this information is
-  /// combined with the results of name lookup into the type of the object
-  /// expression itself (the class type of x).
-  NamedDecl *getFirstQualifierFoundInScope() const {
-    if (!hasFirstQualifierFoundInScope())
-      return nullptr;
-    return *getTrailingObjects<NamedDecl *>();
+  /// Retrieve the declarations found by unqualified lookup for the first
+  /// component name of the nested-name-specifier, if any.
+  ArrayRef<DeclAccessPair> unqualified_lookups() const {
+    if (!getNumUnqualifiedLookups())
+      return std::nullopt;
+    return {getTrailingObjects<DeclAccessPair>(), getNumUnqualifiedLookups()};
   }
 
   /// Retrieve the name of the member that this expression refers to.
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 9cd7a364cd3f1..257a61c97c9c6 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -1020,18 +1020,19 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsArrow : 1;
 
+    /// True if this member expression used a nested-name-specifier to
+    /// refer to the member, e.g., "x->Base::f".
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned HasQualifier : 1;
+
     /// Whether this member expression has info for explicit template
     /// keyword and arguments.
     LLVM_PREFERRED_TYPE(bool)
     unsigned HasTemplateKWAndArgsInfo : 1;
 
-    /// See getFirstQualifierFoundInScope() and the comment listing
-    /// the trailing objects.
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned HasFirstQualifierFoundInScope : 1;
-
-    /// The location of the '->' or '.' operator.
-    SourceLocation OperatorLoc;
+    /// Number of declarations found by unqualified lookup for the
+    /// first component name of the nested-name-specifier.
+    unsigned NumUnqualifiedLookups;
   };
 
   class OverloadExprBitfields {
diff --git a/clang/include/clang/AST/UnresolvedSet.h b/clang/include/clang/AST/UnresolvedSet.h
index 1369725ab4e96..ef44499ce5926 100644
--- a/clang/include/clang/AST/UnresolvedSet.h
+++ b/clang/include/clang/AST/UnresolvedSet.h
@@ -97,6 +97,10 @@ class UnresolvedSetImpl {
     decls().push_back(DeclAccessPair::make(D, AS));
   }
 
+  void addAllDecls(ArrayRef<DeclAccessPair> Other) {
+    append(iterator(Other.begin()), iterator(Other.end()));
+  }
+
   /// Replaces the given declaration with the new one, once.
   ///
   /// \return true if the set changed
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 12aab09f28556..0bd2e35bf2e31 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -895,9 +895,7 @@ def missing_template_arg_list_after_template_kw : Extension<
   "keyword">, InGroup<DiagGroup<"missing-template-arg-list-after-template-kw">>,
   DefaultError;
 
-def err_missing_dependent_template_keyword : Error<
-  "use 'template' keyword to treat '%0' as a dependent template name">;
-def warn_missing_dependent_template_keyword : ExtWarn<
+def ext_missing_dependent_template_keyword : ExtWarn<
   "use 'template' keyword to treat '%0' as a dependent template name">;
 
 def ext_extern_template : Extension<
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 126fea0aef2a7..134bb1b3534e7 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1906,6 +1906,7 @@ class Parser : public CodeCompletionHandler {
   }
 
   bool diagnoseUnknownTemplateId(ExprResult TemplateName, SourceLocation Less);
+  bool isMissingTemplateKeywordBeforeScope();
   void checkPotentialAngleBracket(ExprResult &PotentialTemplateName);
   bool checkPotentialAngleBracketDelimiter(const AngleBracketTracker::Loc &,
                                            const Token &OpToken);
@@ -1966,7 +1967,7 @@ class Parser : public CodeCompletionHandler {
   //===--------------------------------------------------------------------===//
   // C++ Expressions
   ExprResult tryParseCXXIdExpression(CXXScopeSpec &SS, bool isAddressOfOperand,
-                                     Token &Replacement);
+                                     Token *Replacement = nullptr);
 
   ExprResult tryParseCXXPackIndexingExpression(ExprResult PackIdExpression);
   ExprResult ParseCXXPackIndexingExpression(ExprResult PackIdExpression);
@@ -3372,15 +3373,11 @@ class Parser : public CodeCompletionHandler {
   BaseResult ParseBaseSpecifier(Decl *ClassDecl);
   AccessSpecifier getAccessSpecifierIfPresent() const;
 
-  bool ParseUnqualifiedIdTemplateId(CXXScopeSpec &SS,
-                                    ParsedType ObjectType,
-                                    bool ObjectHadErrors,
-                                    SourceLocation TemplateKWLoc,
-                                    IdentifierInfo *Name,
-                                    SourceLocation NameLoc,
-                                    bool EnteringContext,
-                                    UnqualifiedId &Id,
-                                    bool AssumeTemplateId);
+  bool ParseUnqualifiedIdTemplateId(
+      CXXScopeSpec &SS, ParsedType ObjectType, bool ObjectHadErrors,
+      SourceLocation TemplateKWLoc, SourceLocation TildeLoc,
+      IdentifierInfo *Name, SourceLocation NameLoc, bool EnteringContext,
+      UnqualifiedId &Id, bool AssumeTemplateId);
   bool ParseUnqualifiedIdOperator(CXXScopeSpec &SS, bool EnteringContext,
                                   ParsedType ObjectType,
                                   UnqualifiedId &Result);
diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 425b6e2a0b30c..9c22c35535ede 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -75,6 +75,7 @@ class CXXScopeSpec {
   SourceRange Range;
   NestedNameSpecifierLocBuilder Builder;
   ArrayRef<TemplateParameterList *> TemplateParamLists;
+  ArrayRef<DeclAccessPair> UnqualifiedLookups;
 
 public:
   SourceRange getRange() const { return Range; }
@@ -91,6 +92,13 @@ class CXXScopeSpec {
     return TemplateParamLists;
   }
 
+  void setUnqualifiedLookups(ArrayRef<DeclAccessPair> Found) {
+    UnqualifiedLookups = Found;
+  }
+  ArrayRef<DeclAccessPair> getUnqualifiedLookups() const {
+    return UnqualifiedLookups;
+  }
+
   /// Retrieve the representation of the nested-name-specifier.
   NestedNameSpecifier *getScopeRep() const {
     return Builder.getRepresentation();
diff --git a/clang/include/clang/Sema/Lookup.h b/clang/include/clang/Sema/Lookup.h
index b0a08a05ac6a0..6b765ef3c980f 100644
--- a/clang/include/clang/Sema/Lookup.h
+++ b/clang/include/clang/Sema/Lookup.h
@@ -483,11 +483,15 @@ class LookupResult {
     ResultKind = Found;
   }
 
+  void addAllDecls(ArrayRef<DeclAccessPair> Other) {
+    Decls.addAllDecls(Other);
+    ResultKind = Found;
+  }
+
   /// Add all the declarations from another set of lookup
   /// results.
   void addAllDecls(const LookupResult &Other) {
-    Decls.append(Other.Decls.begin(), Other.Decls.end());
-    ResultKind = Found;
+    addAllDecls(Other.Decls.pairs());
   }
 
   /// Determine whether no result was found because we could not
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 48dff1b76cc57..a86decb67bca1 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2803,7 +2803,8 @@ class Sema final : public SemaBase {
   /// (e.g., Base::), perform name lookup for that identifier as a
   /// nested-name-specifier within the given scope, and return the result of
   /// that name lookup.
-  NamedDecl *FindFirstQualifierInScope(Scope *S, NestedNameSpecifier *NNS);
+  bool LookupFirstQualifierInScope(Scope *S, NestedNameSpecifier *NNS,
+                                   UnresolvedSetImpl &R);
 
   /// Keeps information about an identifier in a nested-name-spec.
   ///
@@ -2843,9 +2844,6 @@ class Sema final : public SemaBase {
   /// \param EnteringContext If true, enter the context specified by the
   ///        nested-name-specifier.
   /// \param SS Optional nested name specifier preceding the identifier.
-  /// \param ScopeLookupResult Provides the result of name lookup within the
-  ///        scope of the nested-name-specifier that was computed at template
-  ///        definition time.
   /// \param ErrorRecoveryLookup Specifies if the method is called to improve
   ///        error recovery and what kind of recovery is performed.
   /// \param IsCorrectedToColon If not null, suggestion of replace '::' -> ':'
@@ -2854,11 +2852,6 @@ class Sema final : public SemaBase {
   ///        not '::'.
   /// \param OnlyNamespace If true, only considers namespaces in lookup.
   ///
-  /// This routine differs only slightly from ActOnCXXNestedNameSpecifier, in
-  /// that it contains an extra parameter \p ScopeLookupResult, which provides
-  /// the result of name lookup within the scope of the nested-name-specifier
-  /// that was computed at template definition time.
-  ///
   /// If ErrorRecoveryLookup is true, then this call is used to improve error
   /// recovery.  This means that it should not emit diagnostics, it should
   /// just return true on failure.  It also means it should only return a valid
@@ -2867,7 +2860,6 @@ class Sema final : public SemaBase {
   /// specifier.
   bool BuildCXXNestedNameSpecifier(Scope *S, NestedNameSpecInfo &IdInfo,
                                    bool EnteringContext, CXXScopeSpec &SS,
-                                   NamedDecl *ScopeLookupResult,
                                    bool ErrorRecoveryLookup,
                                    bool *IsCorrectedToColon = nullptr,
                                    bool OnlyNamespace = false);
@@ -8567,11 +8559,12 @@ class Sema final : public SemaBase {
                           const TemplateArgumentListInfo *TemplateArgs,
                           bool IsDefiniteInstance, const Scope *S);
 
-  ExprResult ActOnDependentMemberExpr(
-      Expr *Base, QualType BaseType, bool IsArrow, SourceLocation OpLoc,
-      const CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
-      NamedDecl *FirstQualifierInScope, const DeclarationNameInfo &NameInfo,
-      const TemplateArgumentListInfo *TemplateArgs);
+  ExprResult
+  ActOnDependentMemberExpr(Expr *Base, QualType BaseType, bool IsArrow,
+                           SourceLocation OpLoc, const CXXScopeSpec &SS,
+                           SourceLocation TemplateKWLoc,
+                           const DeclarationNameInfo &NameInfo,
+                           const TemplateArgumentListInfo *TemplateArgs);
 
   /// The main callback when the parser finds something like
   ///   expression . [nested-name-specifier] identifier
@@ -8627,15 +8620,14 @@ class Sema final : public SemaBase {
   ExprResult BuildMemberReferenceExpr(
       Expr *Base, QualType BaseType, SourceLocation OpLoc, bool IsArrow,
       CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
-      NamedDecl *FirstQualifierInScope, const DeclarationNameInfo &NameInfo,
+      const DeclarationNameInfo &NameInfo,
       const TemplateArgumentListInfo *TemplateArgs, const Scope *S,
       ActOnMemberAccessExtraArgs *ExtraArgs = nullptr);
 
   ExprResult
   BuildMemberReferenceExpr(Expr *Base, QualType BaseType, SourceLocation OpLoc,
                            bool IsArrow, const CXXScopeSpec &SS,
-                           SourceLocation TemplateKWLoc,
-                           NamedDecl *FirstQualifierInScope, LookupResult &R,
+                           SourceLocation TemplateKWLoc, LookupResult &R,
                            const TemplateArgumentListInfo *TemplateArgs,
                            const Scope *S, bool SuppressQualifierCheck = false,
                            ActOnMemberAccessExtraArgs *ExtraArgs = nullptr);
@@ -11123,15 +11115,14 @@ class Sema final : public SemaBase {
                      QualType ObjectType, bool EnteringContext,
                      RequiredTemplateKind RequiredTemplate = SourceLocation(),
                      AssumedTemplateKind *ATK = nullptr,
-                     bool AllowTypoCorrection = true);
+                     bool AllowTypoCorrection = true, bool MayBeNNS = false);
 
-  TemplateNameKind isTemplateName(Scope *S, CXXScopeSpec &SS,
-                                  bool hasTemplateKeyword,
-                                  const UnqualifiedId &Name,
-                                  ParsedType ObjectType, bool EnteringContext,
-                                  TemplateTy &Template,
-                                  bool &MemberOfUnknownSpecialization,
-                                  bool Disambiguation = false);
+  TemplateNameKind
+  isTemplateName(Scope *S, CXXScopeSpec &SS, bool hasTemplateKeyword,
+                 const UnqualifiedId &Name, ParsedType ObjectType,
+                 bool EnteringContext, TemplateTy &Template,
+                 bool &MemberOfUnknownSpecialization,
+                 bool Disambiguation = false, bool MayBeNNS = false);
 
   /// Try to resolve an undeclared templa...
[truncated]

@sdkrystian
Copy link
Member Author

I'm considering whether to add a compatibility warning to ease the transition to the new behavior, e.g.

template<int I>
struct A {
  int x;
};

template<int I>
struct B : A<I> {
  void f() {
    this->A<I>::f(); // warning `template` is required after '->' in C++23 and later
  }
};

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libc++ changes LGTM.

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the comment I left, Sema.h and DR test changes look good.

clang/test/CXX/drs/cwg1xx.cpp Show resolved Hide resolved
@sdkrystian
Copy link
Member Author

@KyunLFA Could you check if ares compiles with the patch applied using C++23 please?

@AaronBallman
Copy link
Collaborator

There is discussion on the Core reflectors about this DR (https://lists.isocpp.org/core/2024/07/16028.php), so given the amount of difficulty we've had with this DR so far, I would recommend we hold off on landing any changes until Core has finished deliberation.

@sdkrystian
Copy link
Member Author

sdkrystian commented Jul 26, 2024

@AaronBallman @cor3ntin @mizvekov So, I've made improvements to our implementation of isTemplateArgumentList (not on this branch) that allow us to issue a warning instead of an error for most of the cases where applying the DR breaks existing code:

template<int I>
struct A { int x; };

template<int I, bool J>
struct B { int x; };

template<int I, typename T, int J>
struct C { int x; };

template<int I>
constexpr inline int V = I;

int y;

template<typename T>
void f(T t)
{
    t.A<0>::x;               // warning: use 'template' keyword to treat 'A' as a dependent template name
    t.B<1, true>::x;         // warning: use 'template' keyword to treat 'B' as a dependent template name
    t.C<2, int, 4>::x;       // warning: use 'template' keyword to treat 'C' as a dependent template name
    t.A<V<0>>::x;            // warning: use 'template' keyword to treat 'A' as a dependent template name
    t.B<1, V<1>>::x;         // warning: use 'template' keyword to treat 'B' as a dependent template name
    t.C<V<2>, int, V<3>>::x; // warning: use 'template' keyword to treat 'C' as a dependent template name

    t.A<(1 > 2)>::x;         // warning: use 'template' keyword to treat 'A' as a dependent template name
    t.A<(1 < 3)>::x;         // warning: use 'template' keyword to treat 'A' as a dependent template name

    t.A<1 < 4>::x;           // error: no member named 'x' in the global namespace
                             // error: missing 'template' keyword prior to dependent template name 'A'

    t.A<1 > 4>::x;           // error: no member named 'x' in the global namespace

    t.A<0>::y;               // ok, parsed as '((t.A) < 0) > ::y'
}

This is accomplished by (attempting to) look past the end of the potential template-argument-list, and if the token following the > is ::, we try to parse it as an id-expression/type. If it ends up being invalid, then we consider it as being an intended template name and we issue the warning.

Since we can look past the potential template-argument-list in most cases, we could probably apply CWG1835 in all language modes with minimal impact on existing code.

@mizvekov
Copy link
Contributor

@AaronBallman @cor3ntin @mizvekov So, I've made improvements to our implementation of isTemplateArgumentList (not on this branch) that allow us to issue a warning instead of an error for most of the cases where applying the DR breaks existing code:

That's great, thanks for following up on this!

I am still concerned about the double error on the t.A<1 < 4>::x example.
The ideal outcome would be to turn the second error into a note.
How feasible to fix you think that is?
Otherwise, I don't think this is a blocker for merging this, we can improve it on a follow up patch.

Since we can look past the potential template-argument-list in most cases, we could probably apply CWG1835 in all language modes with minimal impact on existing code.

Yes, I would certainly support that!

@sdkrystian
Copy link
Member Author

I am still concerned about the double error on the t.A<1 < 4>::x example.
The ideal outcome would be to turn the second error into a note.
How feasible to fix you think that is?

That's trivial to fix, just need to add a note variant of the "missing template" diagnostic.

@@ -767,6 +757,7 @@ bool Sema::BuildCXXNestedNameSpecifier(Scope *S, NestedNameSpecInfo &IdInfo,
else
Diag(IdInfo.IdentifierLoc, diag::err_undeclared_var_use)
<< IdInfo.Identifier;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably remove these before merging :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these changes shouldn't really be part of this PR. Just something I noticed when refactoring BuildCXXNestedNameSpecifier that needs to go. I'll address it in another PR.

// If we didn't find anything during our lookup, try again with
// ordinary name lookup, which can help us produce better error
// messages.
if (Found.empty()) {
Found.clear(LookupOrdinaryName);
LookupName(Found, S);
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@sdkrystian sdkrystian force-pushed the reapply-cwg-1835-again branch 2 times, most recently from a3bea01 to e3829cd Compare July 30, 2024 15:06
Copy link

github-actions bot commented Jul 30, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@sdkrystian
Copy link
Member Author

I've further improved isTemplateArgumentList so we only consider < as the start of a potential template-argument-list if it follows an identifier or the name in an operator-function-id/literal-operator-id when trying to find the closing >. Also, explicit casts (a la static_cast) are handled, as are constructs which may either be a type-id or an explicit type conversion (functional notation).

@sdkrystian
Copy link
Member Author

Also added support for names prefixed with the template keyword.

@mizvekov
Copy link
Contributor

Nice!

Please keep track and let us know which parts we haven't seen and have important changes we need to pay more attention to during re-review.

Otherwise the patch is getting quite big already :)

@sdkrystian
Copy link
Member Author

With these changes, we can now issue warnings for the missing template keyword in the vast majority of cases:

template<int I>
struct A { int x; };

template<int I, bool J>
struct B { int x; };

template<int I, typename T, int J>
struct C { int x; };

template<typename T>
struct D { int x; };

template<int I>
constexpr inline int V = I;

int y;

template<typename T, int I>
void f(T t) {
  t.A<0>::x;                            // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.A<int()>::x;                        // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.A<int(0)>::x;                       // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.A<int*()>::x;                       // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.A<static_cast<D<int>>(0)>::x;       // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.A<static_cast<const D<int>>(0)>::x; // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.B<1, true>::x;                      // expected-warning {{use 'template' keyword to treat 'B' as a dependent template name}}
  t.C<2, int, 4>::x;                    // expected-warning {{use 'template' keyword to treat 'C' as a dependent template name}}
  t.A<V<0>>::x;                         // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.B<1, V<1>>::x;                      // expected-warning {{use 'template' keyword to treat 'B' as a dependent template name}}
  t.C<V<2>, int, V<3>>::x;              // expected-warning {{use 'template' keyword to treat 'C' as a dependent template name}}
  t.A<1 && T::template f<0>()>::x;      // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}

  t.A<(1 > 2)>::x;           // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}
  t.A<(1 < 3)>::x;           // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}

  t.B<A<I>::x < int, 1>::x;  // expected-warning {{use 'template' keyword to treat 'x' as a dependent template name}}

  t.B<A<I>::x < 0, 1>::x;    // expected-error   {{no member named 'x' in the global namespace}}
                             // expected-error   {{missing 'template' keyword prior to dependent template name 'B'}}

  t.A<1 < 4>::x;             // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}

  t.A<I < 4>::x;             // expected-error   {{no member named 'x' in the global namespace}}
                             // expected-error   {{missing 'template' keyword prior to dependent template name 'A'}}

  t.A<I ? I < 4 : false>::x; // expected-warning {{use 'template' keyword to treat 'A' as a dependent template name}}

  t.A<1 > 4>::x;             // expected-error   {{no member named 'x' in the global namespace}}

  t.A<0>::y;                 // ok, parsed as '((t.A) < 0) > ::y'
}

Would like to get feedback from @AaronBallman, @cor3ntin, @hokein, and perhaps @zygoloid on this :).

I highly doubt the discussion on the reflector will result in any mitigation mechanisms being added (see my reply here). If any action is taken, the most I foresee would be applying it in C++23 & later.

@cor3ntin
Copy link
Contributor

cor3ntin commented Aug 4, 2024

@sdkrystian
Copy link
Member Author

CWG reviewed CWG2920 during the 08-16 teleconference:

The resolution for issue 1835 in P1787 has addressed real concerns. CWG recognizes that real-world code now no longer compiles, although the fix for the affected source code is trivial. A limited exception to support some of the existing code might be feasible. CWG solicits a paper with specification and analysis.

The meeting minutes portray a general feeling of apathy from CWG for code broken by the resolution. With the enhanced detection and recovery from missing template keywords implemented in this PR (see my comment for examples; all cases reported by users are addressed), I feel that we can go ahead with relanding this patch. WDYT @AaronBallman, @cor3ntin, and @mizvekov?

@cor3ntin
Copy link
Contributor

In a follow up, the following suggestion was made

my compromise suggestion for [CWG2020] was that we could, without changing name lookup itself, say that this->BaseT< (wherein we are interpreting this-> as referring to the current instantiation, missing wording notwithstanding) was interpreted as beginning a template argument list if lookup for BaseT finds nothing in the current class and there is a direct dependent base class that (resolving only trivial alias templates, if any) is a specialization of a class template named BaseT. (Note that we might reject during instantiation thanks to ambiguity.)

This seems worth exploring to me

@sdkrystian
Copy link
Member Author

sdkrystian commented Aug 19, 2024

@cor3ntin This wouldn't fix the case that abseil users complained about (example reduced from the source):

template <typename... Ts>
class CompressedTuple
    : private internal_compressed_tuple::CompressedTupleImpl<
          CompressedTuple<Ts...>, absl::index_sequence_for<Ts...>,
          internal_compressed_tuple::ShouldAnyUseBase<Ts...>()> {

  template <int I>
  using StorageT = internal_compressed_tuple::Storage<ElemT<I>, I>;

 public:
  template <int I>
  constexpr ElemT<I>&& get() && {
    return std::move(*this).StorageT<I>::get();
  }
};

Moreover, this fails to address cases where the base class declares a non-static data member with the same name as the class template:

template<typename T>
struct A
{
    int A;
};

template<typename T>
struct B : A<T>
{
    bool f()
    {
        return this->A < 1; // '<' would be interpreted as the start of a template argument list
    }
};

I think that implementing CWG1835's resolution as it stands is our best option. Doing so results in the most consistent behavior (e.g. 'use template when the object expression is dependent'), the most obvious fixes to achieve the intended interpretation (i.e. 'add template prior to the intended template name' [as opposed to surrounding the class member access expression in parentheses]). Given that we can now identify almost all cases where template is missing with this patch and issue a warning, and that these cases wouldn't be resolved by any of the proposed compromises, I think our best option is to simply implement the resolution with enhanced recovery and call it a day.

@AaronBallman
Copy link
Collaborator

CWG reviewed CWG2920 during the 08-16 teleconference:

The resolution for issue 1835 in P1787 has addressed real concerns. CWG recognizes that real-world code now no longer compiles, although the fix for the affected source code is trivial. A limited exception to support some of the existing code might be feasible. CWG solicits a paper with specification and analysis.

The meeting minutes portray a general feeling of apathy from CWG for code broken by the resolution. With the enhanced detection and recovery from missing template keywords implemented in this PR (see my comment for examples; all cases reported by users are addressed), I feel that we can go ahead with relanding this patch. WDYT @AaronBallman, @cor3ntin, and @mizvekov?

My reading of Core's minutes isn't that there's apathy but more that there's not an obvious solution that resolves all situations, and therefore there's a call for a paper to be written. I don't agree with the logic that there's a need to identify a fix before deciding whether something is or is not a DR -- if it breaks existing code and we don't know how to correct it, making it not a DR has the least fallout. So if we move forward, I think the best approach is to apply it to C++23 and up. However, it's not clear to me why we need to implement this right now instead of trying to determine what the best possible compromise is for our needs and writing the paper CWG is asking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category coroutines C++20 coroutines libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants