[MSVC] Improved lookup into dependent/non-dependent bases of dependent class

[ABataev]

Summary

Patch improves lookup into dependendt bases of dependent class and adds lookup into non-dependent bases.

[ABataev]

Ping!

[rnk]

Looks pretty good.

lib/Sema/SemaDecl.cpp

132

"FoundNonType" would be closer to the standardese terminology of "non-type template parameter"

159–160

You can reduce the indentation here with continue.

test/SemaTemplate/ms-lookup-template-base-classes.cpp

449

Can you add a test for the case where we look through non-dependent bases? So far as I can tell, these are all dependent.

[thakis]

Does this happen to fix http://llvm.org/bugs/show_bug.cgi?id=22066 ?

[rsmith]

lib/AST/Type.cpp

1894–1900 ↗

(On Diff #18746)

What's the purpose of this change? We'll call the const version for a non-const object without this, which will do the same thing.

lib/Sema/SemaDecl.cpp

132

This should either be an enum class or should use a prefix for its names; these identifiers seem too general to drop into global scope here.

139

The name of this function should mention something about dependent bases, since that's what's interesting about it and how it differs from normal name lookup.

153

You're going to lengths to ensure we look up into dependent bases of dependent bases, but you don't support finding names in dependent bases of non-dependent bases of dependent bases: we'll use normal name lookup here in that case. Maybe remove this whole case and do the same thing you do below: look up into the RecordDecl you get here and then recurse to its base classes.

173–187

Switch these two over: first look up in the base class itself, and then look up in its base classes if you find nothing. Otherwise you'll get the wrong result if the name is a type in one place and a non-type in the other.

195–207

If there are multiple such base classes, we should ideally search outer ones if we find nothing in the innermost one, to match normal unqualified lookup.

[ABataev]

Richard, Reid,
Thanks for the review!

lib/AST/Type.cpp

1894–1900 ↗

(On Diff #18746)

Removed.

lib/Sema/SemaDecl.cpp

132

Turned enum into enum class and renamed FoundNotType to FoundNonTypeTemplateParam

139

Renamed to lookupUnqualifiedTypeNameInDependentBase

153

Hmm, maybe I'm missing something, but is it possible at all? How non-dependent class may have dependent base?

159–160

Ok

173–187

Ok, got it.

195–207

Agree, will be fixed

test/SemaTemplate/ms-lookup-template-base-classes.cpp

449

Test in 'namespace type_in_base_of_dependent_base' checks this already. But I'll add another one.

[ABataev]

In D7173#118851, @thakis wrote:

Does this happen to fix http://llvm.org/bugs/show_bug.cgi?id=22066 ?

Nope, unfortunately not. I'll try to fix this one later.

[ABataev]

Update after review

[rnk]

lib/Sema/SemaDecl.cpp

132

Well, we aren't searching for template parameters specifically, so I'd still prefer FoundNonType. :)

[ABataev]

Fixed lookup in non-dependent bases

[ABataev]

Any comments on updated version of the patch?

[ABataev]

Ping!

[rsmith]

Thanks, LGTM

lib/Sema/SemaDecl.cpp

bool Sema::isSimpleTypeSpecifier(tok::TokenKind Kind) const {
 
   default:
     break;
   }
 
   return false;
 }
 
-static ParsedType recoverFromTypeInKnownDependentBase(Sema &S,
-                                                      const IdentifierInfo &II,
-                                                      SourceLocation NameLoc) {
-  // Find the first parent class template context, if any.
-  // FIXME: Perform the lookup in all enclosing class templates.
-  const CXXRecordDecl *RD = nullptr;
-  for (DeclContext *DC = S.CurContext; DC; DC = DC->getParent()) {
-    RD = dyn_cast<CXXRecordDecl>(DC);
-    if (RD && RD->getDescribedClassTemplate())
-      break;
-  }
-  if (!RD)
-    return ParsedType();
+namespace {
+enum class UnqualifiedTypeNameLookupResult {
+  NotFound,
+  FoundNonType,
+  FoundType
+};
+} // namespace
 
+/// \brief Tries to perform unqualified lookup of the type decls in bases for
+/// dependent class.
+/// \return \a NotFound if no any decls is found, \a FoundNotType if found not a
+/// type decl, \a FoundType if only type decls are found.
+static UnqualifiedTypeNameLookupResult
+lookupUnqualifiedTypeNameInBase(Sema &S, const IdentifierInfo &II,
+                                SourceLocation NameLoc,
+                                const CXXRecordDecl *RD) {
+  if (!RD->hasDefinition())
+    return UnqualifiedTypeNameLookupResult::NotFound;
+  // Look for type decls in base classes.
+  UnqualifiedTypeNameLookupResult FoundTypeDecl =
+      UnqualifiedTypeNameLookupResult::NotFound;
+  for (const auto &Base : RD->bases()) {
+    const CXXRecordDecl *BaseRD = nullptr;
+    if (auto *BaseTT = Base.getType()->getAs<TagType>())
+      BaseRD = BaseTT->getAsCXXRecordDecl();
+    else if (auto *TST = Base.getType()->getAs<TemplateSpecializationType>()) {
       // Look for type decls in dependent base classes that have known primary
       // templates.
-  bool FoundTypeDecl = false;
-  for (const auto &Base : RD->bases()) {
-    auto *TST = Base.getType()->getAs<TemplateSpecializationType>();
       if (!TST || !TST->isDependentType())
         continue;
       auto *TD = TST->getTemplateName().getAsTemplateDecl();
       if (!TD)
         continue;
       auto *BasePrimaryTemplate =
           dyn_cast_or_null<CXXRecordDecl>(TD->getTemplatedDecl());
       if (!BasePrimaryTemplate)
         continue;
-    // FIXME: Allow lookup into non-dependent bases of dependent bases, possibly
-    // by calling or integrating with the main LookupQualifiedName mechanism.
-    for (NamedDecl *ND : BasePrimaryTemplate->lookup(&II)) {
-      if (FoundTypeDecl)
-        return ParsedType();
-      FoundTypeDecl = isa<TypeDecl>(ND);
-      if (!FoundTypeDecl)
-        return ParsedType();
+      BaseRD = BasePrimaryTemplate;
+    }
+    if (BaseRD) {
+      for (NamedDecl *ND : BaseRD->lookup(&II)) {
+        if (!isa<TypeDecl>(ND))
+          return UnqualifiedTypeNameLookupResult::FoundNonType;
+        FoundTypeDecl = UnqualifiedTypeNameLookupResult::FoundType;
       }
+      if (FoundTypeDecl == UnqualifiedTypeNameLookupResult::NotFound) {
+        switch (lookupUnqualifiedTypeNameInBase(S, II, NameLoc, BaseRD)) {
+        case UnqualifiedTypeNameLookupResult::FoundNonType:
+          return UnqualifiedTypeNameLookupResult::FoundNonType;
+        case UnqualifiedTypeNameLookupResult::FoundType:
+          FoundTypeDecl = UnqualifiedTypeNameLookupResult::FoundType;
+          break;
+        case UnqualifiedTypeNameLookupResult::NotFound:
+          break;
+        }
+      }
+    }
+  }
+
+  return FoundTypeDecl;
+}
+
+static ParsedType recoverFromTypeInKnownDependentBase(Sema &S,
+                                                      const IdentifierInfo &II,
+                                                      SourceLocation NameLoc) {
+  // Lookup in the parent class template context, if any.
+  const CXXRecordDecl *RD = nullptr;
+  UnqualifiedTypeNameLookupResult FoundTypeDecl =
+      UnqualifiedTypeNameLookupResult::NotFound;
+  for (DeclContext *DC = S.CurContext;
+       DC && FoundTypeDecl == UnqualifiedTypeNameLookupResult::NotFound;
+       DC = DC->getParent()) {
+    // Look for type decls in dependent base classes that have known primary
+    // templates.
+    RD = dyn_cast<CXXRecordDecl>(DC);
+    if (RD && RD->getDescribedClassTemplate())
+      FoundTypeDecl = lookupUnqualifiedTypeNameInBase(S, II, NameLoc, RD);
   }
-  if (!FoundTypeDecl)
+  if (FoundTypeDecl != UnqualifiedTypeNameLookupResult::FoundType)
     return ParsedType();
 
   // We found some types in dependent base classes.  Recover as if the user
   // wrote 'typename MyClass::II' instead of 'II'.  We'll fully resolve the
   // lookup during template instantiation.
   S.Diag(NameLoc, diag::ext_found_via_dependent_bases_lookup) << &II;
 
   ASTContext &Context = S.Context;

test/SemaTemplate/ms-lookup-template-base-classes.cpp

 template <typename T> struct A { enum NameFromBase { X }; };
 template <typename T> struct B : A<T> {
   NameFromBase m; // expected-warning {{found via unqualified lookup into dependent bases}}
 };
 static_assert(sizeof(B<int>) == sizeof(A<int>::NameFromBase), "");
 }
 
 namespace two_types_in_base {
-template <typename T> struct A { typedef T NameFromBase; };
-template <typename T> struct B { struct NameFromBase { T m; }; };
+template <typename T> struct A { typedef T NameFromBase; }; // expected-note {{member found by ambiguous name lookup}}
+template <typename T> struct B { struct NameFromBase { T m; }; }; // expected-note {{member found by ambiguous name lookup}}
 template <typename T> struct C : A<T>, B<T> {
-  NameFromBase m; // expected-error {{unknown type name 'NameFromBase'}}
+  NameFromBase m; // expected-error {{member 'NameFromBase' found in multiple base classes of different types}} expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
 };
-static_assert(sizeof(C<int>) == 4, "");
+static_assert(sizeof(C<int>) == 4, ""); // expected-note {{in instantiation of template class 'two_types_in_base::C<int>' requested here}}
 }
 
 namespace type_and_decl_in_base {
 template <typename T> struct A { typedef T NameFromBase; };
 template <typename T> struct B { static const T NameFromBase = 42; };
 template <typename T> struct C : A<T>, B<T> {
   NameFromBase m; // expected-error {{unknown type name 'NameFromBase'}}
 };

 struct B : A<T> { NameFromBase m; }; // expected-error {{unknown type name 'NameFromBase'}}
 struct C : A<int> { NameFromBase m; }; // expected-error {{unknown type name 'NameFromBase'}}
 }
 
 namespace type_in_base_of_dependent_base {
 struct A { typedef int NameFromBase; };
 template <typename T>
 struct B : A {};
-// FIXME: MSVC accepts this.
 template <typename T>
-struct C : B<T> { NameFromBase m; }; // expected-error {{unknown type name 'NameFromBase'}}
+struct C : B<T> { NameFromBase m; }; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+}
+
+namespace type_in_second_dependent_base {
+template <typename T>
+struct A {};
+template<typename T>
+struct B { typedef T NameFromBase; };
+template <typename T>
+struct D : A<T>, B<T> { NameFromBase m; }; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+}
+
+namespace type_in_second_non_dependent_base {
+struct A {};
+struct B { typedef int NameFromBase; };
+template<typename T>
+struct C : A, B {};
+template <typename T>
+struct D : C<T> { NameFromBase m; }; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+}
+
+namespace type_in_virtual_base_of_dependent_base {
+template <typename T>
+struct A { typedef T NameFromBase; };
+template <typename T>
+struct B : virtual A<T> {};
+template <typename T>
+struct C : B<T>, virtual A<T> { NameFromBase m; }; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+C<int> c;
+}
+
+namespace type_in_base_of_multiple_dependent_bases {
+template <typename T>
+struct A { typedef T NameFromBase; };
+template <typename T>
+struct B : public A<T> {};
+template <typename T>
+struct C : B<T>, public A<T> { NameFromBase m; }; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}} expected-warning {{direct base 'A<int>' is inaccessible due to ambiguity:}}
+C<int> c; // expected-note {{in instantiation of template class 'type_in_base_of_multiple_dependent_bases::C<int>' requested here}}
+}
+
+namespace type_in_dependent_base_of_non_dependent_type {
+template<typename T> struct A { typedef int NameFromBase; };
+template<typename T> struct B : A<T> {
+  struct C;
+  template<typename TT>
+  struct D : C {
+    NameFromBase m; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+  };
+  struct E : C {
+    NameFromBase m; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+  };
+};
+template<typename T> struct B<T>::C : B {
+  NameFromBase m; // expected-warning {{use of identifier 'NameFromBase' found via unqualified lookup into dependent bases of class templates is a Microsoft extension}}
+};
+template<typename T> struct F : B<T>::C {
+  NameFromBase m; // expected-error {{unknown type name 'NameFromBase'}}
+};
 }
 
 namespace lookup_in_function_contexts {
 template <typename T> struct A { typedef T NameFromBase; };
 template <typename T>
 struct B : A<T> {
   // expected-warning@+1 {{lookup into dependent bases}}
   static auto lateSpecifiedFunc() -> decltype(NameFromBase()) {
     return {};