[ms] Allow more unqualified lookup of types in dependent base classes

[rnk]

Summary

In dependent contexts where we know a type name is required, such as a
new expression, we can recover by forming a DependentNameType.

Works towards parsing atlctrlw.h, which is PR26748.

[rsmith]

lib/Parse/ParseDecl.cpp

2282 ↗

(On Diff #58015)

Maybe put the check here instead. That way, you can apply it for any DSContext.

lib/Sema/SemaDecl.cpp

507 ↗

(On Diff #58015)

Missing loop termination condition

[rnk]

lib/Parse/ParseDecl.cpp

2282 ↗

(On Diff #58015)

Done. With that change, we ended up over-accepting invalid C++ like this:

template <typename T>
struct A : T {
  C c; // MSVC rejects, C is undeclared. My patch accepted invalid.
};
struct B { typedef int C; };
template struct A<B>;

This recovery mode should only be firing inside *methods* of classes with dependent bases, so I tweaked ActOnMSVCUnknownType to do that.

[rnk]

• Recover in ParseImplicitInt instead

[rsmith]

include/clang/Sema/Sema.h

1536–1537 ↗

(On Diff #58119)

Can you remove this now, along with the corresponding recovery path in the parser? The new recovery path seems like it should be able to cover a superset of cases.

lib/Sema/SemaDecl.cpp

524–527 ↗

(On Diff #58119)

Instead of doing this, could you synthesizeCurrentNestedNameSpecifier as ActOnDelayedDefaultTemplateArg does, and just check for the existence of a dependent base class here? That way you should do the right thing even if there are multiple dependent base classes.

[rnk]

• Share the MSVC compatibility hack between the inside a method case and the default type template argument case

[rsmith]

include/clang/Sema/Sema.h

1534 ↗

(On Diff #58169)

form -> form a

lib/Sema/SemaDecl.cpp

500 ↗

(On Diff #58169)

refering -> referring

520–522 ↗

(On Diff #58169)

Maybe make this an else for the above if.

559–562 ↗

(On Diff #58169)

Doesn't seem to be done? I can easily believe there's a good reason why we want two different paths for building a recovery nested name specifier, but I'd like to know what it is at least.

[rnk]

lib/Sema/SemaDecl.cpp

559–562 ↗

(On Diff #58169)

We should already be handling the case you are thinking of. We don't recover by using a dependent base in the NNS. We recover by using the class *deriving* from those bases in the NNS, like this:

template <typename T, typename U>
struct A : T, U {
  void *p() { return new /*A::*/ X; } // We hallucinate the commented out specifier
};

synthesizeCurrentNestedNameSpecifier is doing something different. It's just looking for the nearest parent scope that can legitimately form an NNS, which means global scope, namespace scope, or record scope, mainly by skipping function scopes. This is sort of what it looks like:

template <typename T = /*::*/ X> struct A { ... };
namespace ns {
template <typename T = /*ns::*/ X> struct B { ... };
}

[rnk]

• nits

[rnk]

• fix the diff

cfe/trunk/include/clang/Sema/Sema.h

public:
   bool isMicrosoftMissingTypename(const CXXScopeSpec *SS, Scope *S);
   void DiagnoseUnknownTypeName(IdentifierInfo *&II,
                                SourceLocation IILoc,
                                Scope *S,
                                CXXScopeSpec *SS,
                                ParsedType &SuggestedType,
                                bool AllowClassTemplates = false);
 
-  /// \brief For compatibility with MSVC, we delay parsing of some default
-  /// template type arguments until instantiation time.  Emits a warning and
-  /// returns a synthesized DependentNameType that isn't really dependent on any
-  /// other template arguments.
-  ParsedType ActOnDelayedDefaultTemplateArg(const IdentifierInfo &II,
-                                            SourceLocation NameLoc);
+  /// Attempt to behave like MSVC in situations where lookup of an unqualified
+  /// type name has failed in a dependent context. In these situations, we
+  /// automatically form a DependentTypeName that will retry lookup in a related
+  /// scope during instantiation.
+  ParsedType ActOnMSVCUnknownTypeName(const IdentifierInfo &II,
+                                      SourceLocation NameLoc,
+                                      bool IsTemplateTypeArg);
 
   /// \brief Describes the result of the name lookup and resolution performed
   /// by \c ClassifyName().
   enum NameClassificationKind {
     NC_Unknown,
     NC_Error,
     NC_Keyword,
     NC_Type,

cfe/trunk/lib/Parse/ParseDecl.cpp

if (getLangOpts().CPlusPlus &&
       DS.getStorageClassSpec() == DeclSpec::SCS_auto) {
     // Don't require a type specifier if we have the 'auto' storage class
     // specifier in C++98 -- we'll promote it to a type specifier.
     if (SS)
       AnnotateScopeToken(*SS, /*IsNewAnnotation*/false);
     return false;
   }
 
+  if (getLangOpts().CPlusPlus && (!SS || SS->isEmpty()) &&
+      getLangOpts().MSVCCompat) {
+    // Lookup of an unqualified type name has failed in MSVC compatibility mode.
+    // Give Sema a chance to recover if we are in a template with dependent base
+    // classes.
+    if (ParsedType T = Actions.ActOnMSVCUnknownTypeName(
+            *Tok.getIdentifierInfo(), Tok.getLocation(),
+            DSC == DSC_template_type_arg)) {
+      const char *PrevSpec;
+      unsigned DiagID;
+      DS.SetTypeSpecType(DeclSpec::TST_typename, Loc, PrevSpec, DiagID, T,
+                         Actions.getASTContext().getPrintingPolicy());
+      DS.SetRangeEnd(Tok.getLocation());
+      ConsumeToken();
+      return false;
+    }
+  }
+
   // Otherwise, if we don't consume this token, we are going to emit an
   // error anyway.  Try to recover from various common problems.  Check
   // to see if this was a reference to a tag name without a tag specified.
   // This is a common problem in C (saying 'foo' instead of 'struct foo').
   //
   // C++ doesn't need this, and isTagName doesn't take SS.
   if (SS == nullptr) {
     const char *TagName = nullptr, *FixitTagName = nullptr;
case tok::identifier: {
         ConsumeToken();
         continue;
       }
 
       ParsedType TypeRep =
         Actions.getTypeName(*Tok.getIdentifierInfo(),
                             Tok.getLocation(), getCurScope());
 
-      // MSVC: If we weren't able to parse a default template argument, and it's
-      // just a simple identifier, create a DependentNameType.  This will allow
-      // us to defer the name lookup to template instantiation time, as long we
-      // forge a NestedNameSpecifier for the current context.
-      if (!TypeRep && DSContext == DSC_template_type_arg &&
-          getLangOpts().MSVCCompat && getCurScope()->isTemplateParamScope()) {
-        TypeRep = Actions.ActOnDelayedDefaultTemplateArg(
-            *Tok.getIdentifierInfo(), Tok.getLocation());
-      }
-
       // If this is not a typedef name, don't parse it as part of the declspec,
       // it must be an implicit int or an error.
       if (!TypeRep) {
         ParsedAttributesWithRange Attrs(AttrFactory);
         if (ParseImplicitInt(DS, nullptr, TemplateInfo, AS, DSContext, Attrs)) {
           if (!Attrs.empty()) {
             AttrsLastTime = true;
             attrs.takeAllFrom(Attrs);

cfe/trunk/lib/Sema/SemaDecl.cpp

else if (auto *RD = dyn_cast<CXXRecordDecl>(DC))
       return NestedNameSpecifier::Create(Context, nullptr, RD->isTemplateDecl(),
                                          RD->getTypeForDecl());
     else if (isa<TranslationUnitDecl>(DC))
       return NestedNameSpecifier::GlobalSpecifier(Context);
   }
   llvm_unreachable("something isn't in TU scope?");
 }
 
-ParsedType Sema::ActOnDelayedDefaultTemplateArg(const IdentifierInfo &II,
-                                                SourceLocation NameLoc) {
-  // Accepting an undeclared identifier as a default argument for a template
-  // type parameter is a Microsoft extension.
+/// Find the parent class with dependent bases of the innermost enclosing method
+/// context. Do not look for enclosing CXXRecordDecls directly, or we will end
+/// up allowing unqualified dependent type names at class-level, which MSVC
+/// correctly rejects.
+static const CXXRecordDecl *
+findRecordWithDependentBasesOfEnclosingMethod(const DeclContext *DC) {
+  for (; DC && DC->isDependentContext(); DC = DC->getLookupParent()) {
+    DC = DC->getPrimaryContext();
+    if (const auto *MD = dyn_cast<CXXMethodDecl>(DC))
+      if (MD->getParent()->hasAnyDependentBases())
+        return MD->getParent();
+  }
+  return nullptr;
+}
+
+ParsedType Sema::ActOnMSVCUnknownTypeName(const IdentifierInfo &II,
+                                          SourceLocation NameLoc,
+                                          bool IsTemplateTypeArg) {
+  assert(getLangOpts().MSVCCompat && "shouldn't be called in non-MSVC mode");
+
+  NestedNameSpecifier *NNS = nullptr;
+  if (IsTemplateTypeArg && getCurScope()->isTemplateParamScope()) {
+    // If we weren't able to parse a default template argument, delay lookup
+    // until instantiation time by making a non-dependent DependentTypeName. We
+    // pretend we saw a NestedNameSpecifier referring to the current scope, and
+    // lookup is retried.
+    // FIXME: This hurts our diagnostic quality, since we get errors like "no
+    // type named 'Foo' in 'current_namespace'" when the user didn't write any
+    // name specifiers.
+    NNS = synthesizeCurrentNestedNameSpecifier(Context, CurContext);
     Diag(NameLoc, diag::ext_ms_delayed_template_argument) << &II;
+  } else if (const CXXRecordDecl *RD =
+                 findRecordWithDependentBasesOfEnclosingMethod(CurContext)) {
+    // Build a DependentNameType that will perform lookup into RD at
+    // instantiation time.
+    NNS = NestedNameSpecifier::Create(Context, nullptr, RD->isTemplateDecl(),
+                                      RD->getTypeForDecl());
+
+    // Diagnose that this identifier was undeclared, and retry the lookup during
+    // template instantiation.
+    Diag(NameLoc, diag::ext_undeclared_unqual_id_with_dependent_base) << &II
+                                                                      << RD;
+  } else {
+    // This is not a situation that we should recover from.
+    return ParsedType();
+  }
 
-  // Build a fake DependentNameType that will perform lookup into CurContext at
-  // instantiation time.  The name specifier isn't dependent, so template
-  // instantiation won't transform it.  It will retry the lookup, however.
-  NestedNameSpecifier *NNS =
-      synthesizeCurrentNestedNameSpecifier(Context, CurContext);
   QualType T = Context.getDependentNameType(ETK_None, NNS, &II);
 
   // Build type location information.  We synthesized the qualifier, so we have
   // to build a fake NestedNameSpecifierLoc.
   NestedNameSpecifierLocBuilder NNSLocBuilder;
   NNSLocBuilder.MakeTrivial(Context, NNS, SourceRange(NameLoc));
   NestedNameSpecifierLoc QualifierLoc = NNSLocBuilder.getWithLocInContext(Context);
 

cfe/trunk/test/SemaTemplate/ms-delayed-default-template-args.cpp

 struct Foo {
   static_assert(sizeof(T) == 4, "Bar should have gotten int");
   // FIXME: These diagnostics are bad.
 }; // expected-error {{expected ',' or '>' in template-parameter-list}}
 // expected-warning@-1 {{does not declare anything}}
 typedef int Weber;
 }
 
+// MSVC accepts this, but Clang doesn't.
+namespace test_scope_spec {
+template <typename T = ns::Bar>  // expected-error {{use of undeclared identifier 'ns'}}
+struct Foo {
+  static_assert(sizeof(T) == 4, "Bar should have gotten int");
+};
+namespace ns { typedef int Bar; }
+}
+
 #ifdef __clang__
 // These are negative test cases that MSVC doesn't compile either.  Try to use
 // unique undeclared identifiers so typo correction doesn't find types declared
 // above.
 
 namespace test_undeclared_nontype_parm_type {
 template <Zargon N> // expected-error {{unknown type name 'Zargon'}}
 struct Foo { int x[N]; };

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

-// RUN: %clang_cc1 -std=c++1y -fms-compatibility -fno-spell-checking -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -std=c++1y -fms-compatibility -fno-spell-checking -fsyntax-only -verify %s
 
 
 template <class T>
 class A {
 public:
    void f(T a) { }// expected-note 2{{must qualify identifier to find this declaration in dependent base class}}
    void g();// expected-note 2{{must qualify identifier to find this declaration in dependent base class}}
 };

 // Don't delay lookup in SFINAE context.
 template <typename T> decltype(g(T())) check(); // expected-note{{candidate template ignored: substitution failure [with T = int]: use of undeclared identifier 'g'}}
 decltype(check<int>()) x; // expected-error{{no matching function for call to 'check'}}
 
 void h();
 template <typename T> decltype(h(T())) check2(); // expected-note{{candidate template ignored: substitution failure [with T = int]: no matching function for call to 'h'}}
 decltype(check2<int>()) y; // expected-error{{no matching function for call to 'check2'}}
 }
+
+// We also allow unqualified lookup into bases in contexts where the we know the
+// undeclared identifier *must* be a type, such as a new expression or catch
+// parameter type.
+template <typename T>
+struct UseUnqualifiedTypeNames : T {
+  void foo() {
+    void *P = new TheType; // expected-warning {{unqualified lookup}} expected-error {{no type}}
+    size_t x = __builtin_offsetof(TheType, f2); // expected-warning {{unqualified lookup}} expected-error {{no type}}
+    try {
+    } catch (TheType) { // expected-warning {{unqualified lookup}} expected-error {{no type}}
+    }
+    enum E : IntegerType { E0 = 42 }; // expected-warning {{unqualified lookup}} expected-error {{no type}}
+    _Atomic(TheType) a; // expected-warning {{unqualified lookup}} expected-error {{no type}}
+  }
+  void out_of_line();
+};
+template <typename T>
+void UseUnqualifiedTypeNames<T>::out_of_line() {
+  void *p = new TheType; // expected-warning {{unqualified lookup}} expected-error {{no type}}
+}
+struct Base {
+  typedef int IntegerType;
+  struct TheType {
+    int f1, f2;
+  };
+};
+template struct UseUnqualifiedTypeNames<Base>;
+struct BadBase { };
+template struct UseUnqualifiedTypeNames<BadBase>; // expected-note-re 2 {{in instantiation {{.*}} requested here}}