From 2a3b86c157166f3b15f718443334ab0e27b40592 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 17 Jun 2020 14:06:11 -0700 Subject: [PATCH] Fix rejects-valid when referencing an implicit operator== from within a templated class. When a defaulted operator<=> results in the injection of a defaulted operator==, that operator== can be named by unqualified name within the same class, even if the class is templated. To make this work, perform the transform from defaulted operator<=> to defaulted operator== in the template definition context instead of the template instantiation context. This results in our substituting into a declaration from a context where we don't have a full list of template arguments (or indeed any), for which we are now more careful to not spuriously instantiate declarations that are not dependent on the arguments we're substituting. --- clang/include/clang/AST/DeclBase.h | 13 +++ clang/include/clang/Sema/Template.h | 7 ++ clang/lib/AST/DeclBase.cpp | 34 ++++++- clang/lib/Sema/SemaDecl.cpp | 6 +- clang/lib/Sema/SemaDeclCXX.cpp | 135 +++++++++++++------------ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp | 31 ++++-- clang/test/PCH/cxx2a-defaulted-comparison.cpp | 2 +- clang/test/SemaTemplate/defaulted.cpp | 10 ++ 8 files changed, 164 insertions(+), 74 deletions(-) create mode 100644 clang/test/SemaTemplate/defaulted.cpp diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index ba84f27..5e00be3 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -66,6 +66,7 @@ class SourceManager; class Stmt; class StoredDeclsMap; class TemplateDecl; +class TemplateParameterList; class TranslationUnitDecl; class UsingDirectiveDecl; @@ -862,6 +863,10 @@ public: // within the scope of a template parameter). bool isTemplated() const; + /// Determine the number of levels of template parameter surrounding this + /// declaration. + unsigned getTemplateDepth() const; + /// isDefinedOutsideFunctionOrMethod - This predicate returns true if this /// scoped decl is defined outside the current function or method. This is /// roughly global variables and functions, but also handles enums (which @@ -1038,8 +1043,16 @@ public: /// If this is a declaration that describes some template, this /// method returns that template declaration. + /// + /// Note that this returns nullptr for partial specializations, because they + /// are not modeled as TemplateDecls. Use getDescribedTemplateParams to handle + /// those cases. TemplateDecl *getDescribedTemplate() const; + /// If this is a declaration that describes some template or partial + /// specialization, this returns the corresponding template parameter list. + const TemplateParameterList *getDescribedTemplateParams() const; + /// Returns the function itself, or the templated function if this is a /// function template. FunctionDecl *getAsFunction() LLVM_READONLY; diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h index 741de48..91d175f 100644 --- a/clang/include/clang/Sema/Template.h +++ b/clang/include/clang/Sema/Template.h @@ -121,6 +121,10 @@ enum class TemplateSubstitutionKind : char { return TemplateArgumentLists.size(); } + unsigned getNumRetainedOuterLevels() const { + return NumRetainedOuterLevels; + } + /// Determine how many of the \p OldDepth outermost template parameter /// lists would be removed by substituting these arguments. unsigned getNewDepth(unsigned OldDepth) const { @@ -185,6 +189,9 @@ enum class TemplateSubstitutionKind : char { void addOuterRetainedLevel() { ++NumRetainedOuterLevels; } + void addOuterRetainedLevels(unsigned Num) { + NumRetainedOuterLevels += Num; + } /// Retrieve the innermost template argument list. const ArgList &getInnermost() const { diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp index 2aab53f..a9cd791 100644 --- a/clang/lib/AST/DeclBase.cpp +++ b/clang/lib/AST/DeclBase.cpp @@ -240,6 +240,16 @@ TemplateDecl *Decl::getDescribedTemplate() const { return nullptr; } +const TemplateParameterList *Decl::getDescribedTemplateParams() const { + if (auto *TD = getDescribedTemplate()) + return TD->getTemplateParameters(); + if (auto *CTPSD = dyn_cast(this)) + return CTPSD->getTemplateParameters(); + if (auto *VTPSD = dyn_cast(this)) + return VTPSD->getTemplateParameters(); + return nullptr; +} + bool Decl::isTemplated() const { // A declaration is dependent if it is a template or a template pattern, or // is within (lexcially for a friend, semantically otherwise) a dependent @@ -248,7 +258,29 @@ bool Decl::isTemplated() const { if (auto *AsDC = dyn_cast(this)) return AsDC->isDependentContext(); auto *DC = getFriendObjectKind() ? getLexicalDeclContext() : getDeclContext(); - return DC->isDependentContext() || isTemplateDecl() || getDescribedTemplate(); + return DC->isDependentContext() || isTemplateDecl() || + getDescribedTemplateParams(); +} + +unsigned Decl::getTemplateDepth() const { + if (auto *DC = dyn_cast(this)) + if (DC->isFileContext()) + return 0; + + if (auto *TPL = getDescribedTemplateParams()) + return TPL->getDepth() + 1; + + // If this is a dependent lambda, there might be an enclosing variable + // template. In this case, the next step is not the parent DeclContext (or + // even a DeclContext at all). + auto *RD = dyn_cast(this); + if (RD && RD->isDependentLambda()) + if (Decl *Context = RD->getLambdaContextDecl()) + return Context->getTemplateDepth(); + + const DeclContext *DC = + getFriendObjectKind() ? getLexicalDeclContext() : getDeclContext(); + return cast(DC)->getTemplateDepth(); } const DeclContext *Decl::getParentFunctionOrMethod() const { diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 5eebfb7..30d2e5b 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -17171,10 +17171,10 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, I.setAccess((*I)->getAccess()); } - if (!CXXRecord->isDependentType()) { - // Add any implicitly-declared members to this class. - AddImplicitlyDeclaredMembersToClass(CXXRecord); + // Add any implicitly-declared members to this class. + AddImplicitlyDeclaredMembersToClass(CXXRecord); + if (!CXXRecord->isDependentType()) { if (!CXXRecord->isInvalidDecl()) { // If we have virtual base classes, we may end up finding multiple // final overriders for a given virtual function. Check for this diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index a2b669c..e38ea6d 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -9839,86 +9839,95 @@ static void findImplicitlyDeclaredEqualityComparisons( /// [special]p1). This routine can only be executed just before the /// definition of the class is complete. void Sema::AddImplicitlyDeclaredMembersToClass(CXXRecordDecl *ClassDecl) { - if (ClassDecl->needsImplicitDefaultConstructor()) { - ++getASTContext().NumImplicitDefaultConstructors; + // Don't add implicit special members to templated classes. + // FIXME: This means unqualified lookups for 'operator=' within a class + // template don't work properly. + if (!ClassDecl->isDependentType()) { + if (ClassDecl->needsImplicitDefaultConstructor()) { + ++getASTContext().NumImplicitDefaultConstructors; - if (ClassDecl->hasInheritedConstructor()) - DeclareImplicitDefaultConstructor(ClassDecl); - } + if (ClassDecl->hasInheritedConstructor()) + DeclareImplicitDefaultConstructor(ClassDecl); + } - if (ClassDecl->needsImplicitCopyConstructor()) { - ++getASTContext().NumImplicitCopyConstructors; + if (ClassDecl->needsImplicitCopyConstructor()) { + ++getASTContext().NumImplicitCopyConstructors; - // If the properties or semantics of the copy constructor couldn't be - // determined while the class was being declared, force a declaration - // of it now. - if (ClassDecl->needsOverloadResolutionForCopyConstructor() || - ClassDecl->hasInheritedConstructor()) - DeclareImplicitCopyConstructor(ClassDecl); - // For the MS ABI we need to know whether the copy ctor is deleted. A - // prerequisite for deleting the implicit copy ctor is that the class has a - // move ctor or move assignment that is either user-declared or whose - // semantics are inherited from a subobject. FIXME: We should provide a more - // direct way for CodeGen to ask whether the constructor was deleted. - else if (Context.getTargetInfo().getCXXABI().isMicrosoft() && - (ClassDecl->hasUserDeclaredMoveConstructor() || - ClassDecl->needsOverloadResolutionForMoveConstructor() || - ClassDecl->hasUserDeclaredMoveAssignment() || - ClassDecl->needsOverloadResolutionForMoveAssignment())) - DeclareImplicitCopyConstructor(ClassDecl); - } + // If the properties or semantics of the copy constructor couldn't be + // determined while the class was being declared, force a declaration + // of it now. + if (ClassDecl->needsOverloadResolutionForCopyConstructor() || + ClassDecl->hasInheritedConstructor()) + DeclareImplicitCopyConstructor(ClassDecl); + // For the MS ABI we need to know whether the copy ctor is deleted. A + // prerequisite for deleting the implicit copy ctor is that the class has + // a move ctor or move assignment that is either user-declared or whose + // semantics are inherited from a subobject. FIXME: We should provide a + // more direct way for CodeGen to ask whether the constructor was deleted. + else if (Context.getTargetInfo().getCXXABI().isMicrosoft() && + (ClassDecl->hasUserDeclaredMoveConstructor() || + ClassDecl->needsOverloadResolutionForMoveConstructor() || + ClassDecl->hasUserDeclaredMoveAssignment() || + ClassDecl->needsOverloadResolutionForMoveAssignment())) + DeclareImplicitCopyConstructor(ClassDecl); + } - if (getLangOpts().CPlusPlus11 && ClassDecl->needsImplicitMoveConstructor()) { - ++getASTContext().NumImplicitMoveConstructors; + if (getLangOpts().CPlusPlus11 && + ClassDecl->needsImplicitMoveConstructor()) { + ++getASTContext().NumImplicitMoveConstructors; - if (ClassDecl->needsOverloadResolutionForMoveConstructor() || - ClassDecl->hasInheritedConstructor()) - DeclareImplicitMoveConstructor(ClassDecl); - } + if (ClassDecl->needsOverloadResolutionForMoveConstructor() || + ClassDecl->hasInheritedConstructor()) + DeclareImplicitMoveConstructor(ClassDecl); + } - if (ClassDecl->needsImplicitCopyAssignment()) { - ++getASTContext().NumImplicitCopyAssignmentOperators; + if (ClassDecl->needsImplicitCopyAssignment()) { + ++getASTContext().NumImplicitCopyAssignmentOperators; - // If we have a dynamic class, then the copy assignment operator may be - // virtual, so we have to declare it immediately. This ensures that, e.g., - // it shows up in the right place in the vtable and that we diagnose - // problems with the implicit exception specification. - if (ClassDecl->isDynamicClass() || - ClassDecl->needsOverloadResolutionForCopyAssignment() || - ClassDecl->hasInheritedAssignment()) - DeclareImplicitCopyAssignment(ClassDecl); - } + // If we have a dynamic class, then the copy assignment operator may be + // virtual, so we have to declare it immediately. This ensures that, e.g., + // it shows up in the right place in the vtable and that we diagnose + // problems with the implicit exception specification. + if (ClassDecl->isDynamicClass() || + ClassDecl->needsOverloadResolutionForCopyAssignment() || + ClassDecl->hasInheritedAssignment()) + DeclareImplicitCopyAssignment(ClassDecl); + } - if (getLangOpts().CPlusPlus11 && ClassDecl->needsImplicitMoveAssignment()) { - ++getASTContext().NumImplicitMoveAssignmentOperators; + if (getLangOpts().CPlusPlus11 && ClassDecl->needsImplicitMoveAssignment()) { + ++getASTContext().NumImplicitMoveAssignmentOperators; - // Likewise for the move assignment operator. - if (ClassDecl->isDynamicClass() || - ClassDecl->needsOverloadResolutionForMoveAssignment() || - ClassDecl->hasInheritedAssignment()) - DeclareImplicitMoveAssignment(ClassDecl); - } + // Likewise for the move assignment operator. + if (ClassDecl->isDynamicClass() || + ClassDecl->needsOverloadResolutionForMoveAssignment() || + ClassDecl->hasInheritedAssignment()) + DeclareImplicitMoveAssignment(ClassDecl); + } - if (ClassDecl->needsImplicitDestructor()) { - ++getASTContext().NumImplicitDestructors; + if (ClassDecl->needsImplicitDestructor()) { + ++getASTContext().NumImplicitDestructors; - // If we have a dynamic class, then the destructor may be virtual, so we - // have to declare the destructor immediately. This ensures that, e.g., it - // shows up in the right place in the vtable and that we diagnose problems - // with the implicit exception specification. - if (ClassDecl->isDynamicClass() || - ClassDecl->needsOverloadResolutionForDestructor()) - DeclareImplicitDestructor(ClassDecl); + // If we have a dynamic class, then the destructor may be virtual, so we + // have to declare the destructor immediately. This ensures that, e.g., it + // shows up in the right place in the vtable and that we diagnose problems + // with the implicit exception specification. + if (ClassDecl->isDynamicClass() || + ClassDecl->needsOverloadResolutionForDestructor()) + DeclareImplicitDestructor(ClassDecl); + } } // C++2a [class.compare.default]p3: // If the member-specification does not explicitly declare any member or // friend named operator==, an == operator function is declared implicitly - // for each defaulted three-way comparison operator function defined in the - // member-specification + // for each defaulted three-way comparison operator function defined in + // the member-specification // FIXME: Consider doing this lazily. - if (getLangOpts().CPlusPlus20) { - llvm::SmallVector DefaultedSpaceships; + // We do this during the initial parse for a class template, not during + // instantiation, so that we can handle unqualified lookups for 'operator==' + // when parsing the template. + if (getLangOpts().CPlusPlus20 && !inTemplateInstantiation()) { + llvm::SmallVector DefaultedSpaceships; findImplicitlyDeclaredEqualityComparisons(Context, ClassDecl, DefaultedSpaceships); for (auto *FD : DefaultedSpaceships) diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp index 1fff6b8..444fb20 100644 --- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp @@ -3719,6 +3719,8 @@ FunctionDecl *Sema::SubstSpaceshipAsEqualEqual(CXXRecordDecl *RD, // access and function-definition and in the same class scope as the // three-way comparison operator function MultiLevelTemplateArgumentList NoTemplateArgs; + NoTemplateArgs.setKind(TemplateSubstitutionKind::Rewrite); + NoTemplateArgs.addOuterRetainedLevels(RD->getTemplateDepth()); TemplateDeclInstantiator Instantiator(*this, RD, NoTemplateArgs); Decl *R; if (auto *MD = dyn_cast(Spaceship)) { @@ -5605,6 +5607,20 @@ DeclContext *Sema::FindInstantiatedContext(SourceLocation Loc, DeclContext* DC, } else return DC; } +/// Determine whether the given context is dependent on template parameters at +/// level \p Level or below. +/// +/// Sometimes we only substitute an inner set of template arguments and leave +/// the outer templates alone. In such cases, contexts dependent only on the +/// outer levels are not effectively dependent. +static bool isDependentContextAtLevel(DeclContext *DC, unsigned Level) { + if (!DC->isDependentContext()) + return false; + if (!Level) + return true; + return cast(DC)->getTemplateDepth() > Level; +} + /// Find the instantiation of the given declaration within the /// current instantiation. /// @@ -5635,6 +5651,10 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D, const MultiLevelTemplateArgumentList &TemplateArgs, bool FindingInstantiatedContext) { DeclContext *ParentDC = D->getDeclContext(); + // Determine whether our parent context depends on any of the tempalte + // arguments we're currently substituting. + bool ParentDependsOnArgs = isDependentContextAtLevel( + ParentDC, TemplateArgs.getNumRetainedOuterLevels()); // FIXME: Parmeters of pointer to functions (y below) that are themselves // parameters (p below) can have their ParentDC set to the translation-unit // - thus we can not consistently check if the ParentDC of such a parameter @@ -5651,15 +5671,14 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D, // - as long as we have a ParmVarDecl whose parent is non-dependent and // whose type is not instantiation dependent, do nothing to the decl // - otherwise find its instantiated decl. - if (isa(D) && !ParentDC->isDependentContext() && + if (isa(D) && !ParentDependsOnArgs && !cast(D)->getType()->isInstantiationDependentType()) return D; if (isa(D) || isa(D) || isa(D) || isa(D) || - ((ParentDC->isFunctionOrMethod() || - isa(ParentDC) || - isa(ParentDC)) && - ParentDC->isDependentContext()) || + (ParentDependsOnArgs && (ParentDC->isFunctionOrMethod() || + isa(ParentDC) || + isa(ParentDC))) || (isa(D) && cast(D)->isLambda())) { // D is a local of some kind. Look into the map of local // declarations to their instantiations. @@ -5813,7 +5832,7 @@ NamedDecl *Sema::FindInstantiatedDecl(SourceLocation Loc, NamedDecl *D, // anonymous unions in class templates). } - if (!ParentDC->isDependentContext()) + if (!ParentDependsOnArgs) return D; ParentDC = FindInstantiatedContext(Loc, ParentDC, TemplateArgs); diff --git a/clang/test/PCH/cxx2a-defaulted-comparison.cpp b/clang/test/PCH/cxx2a-defaulted-comparison.cpp index 8fea0fb..8aeb168 100644 --- a/clang/test/PCH/cxx2a-defaulted-comparison.cpp +++ b/clang/test/PCH/cxx2a-defaulted-comparison.cpp @@ -1,4 +1,4 @@ -// RxN: %clang_cc1 -std=c++2a -verify -Wno-defaulted-function-deleted -include %s %s +// RUN: %clang_cc1 -std=c++2a -verify -Wno-defaulted-function-deleted -include %s %s // // RUN: %clang_cc1 -std=c++2a -emit-pch %s -o %t.pch // RUN: %clang_cc1 -std=c++2a -include-pch %t.pch %s -verify diff --git a/clang/test/SemaTemplate/defaulted.cpp b/clang/test/SemaTemplate/defaulted.cpp new file mode 100644 index 0000000..15111f7 --- /dev/null +++ b/clang/test/SemaTemplate/defaulted.cpp @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -std=c++20 -verify %s +// expected-no-diagnostics + +namespace SpaceshipImpliesEq { + template struct A { + int operator<=>(const A&) const = default; + constexpr bool f() { return operator==(*this); } + }; + static_assert(A().f()); +} -- 2.7.4