[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 ↗

(On Diff #18746)

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

159–160 ↗

(On Diff #18746)

You can reduce the indentation here with continue.

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

411 ↗

(On Diff #18746)

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 ↗

(On Diff #18746)

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 ↗

(On Diff #18746)

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 ↗

(On Diff #18746)

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 ↗

(On Diff #18746)

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.

196–202 ↗

(On Diff #18746)

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 ↗

(On Diff #18746)

Turned enum into enum class and renamed FoundNotType to FoundNonTypeTemplateParam

139 ↗

(On Diff #18746)

Renamed to lookupUnqualifiedTypeNameInDependentBase

153 ↗

(On Diff #18746)

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

159–160 ↗

(On Diff #18746)

Ok

173–187 ↗

(On Diff #18746)

Ok, got it.

196–202 ↗

(On Diff #18746)

Agree, will be fixed

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

411 ↗

(On Diff #18746)

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 ↗

(On Diff #18746)

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

cfe/trunk/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;

cfe/trunk/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()) {