Fix for Bug 8446. Template instantiation without a 'typename' keyword.

[rnk]

From the changes to the test suite, I can't see why we'd want to do this. This greatly reduces our ability to accept but warn on invalid code.

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

209

All of these new errors are accepted by MSVC. This seems like a step backwards. The most common kind of reference into a dependent base is probably a reference to a field from a base class, i.e. missing this->.

test/SemaTemplate/typename-specifier.cpp

166

This seems bad, we're no longer warning people about non-conforming code.

[rnk]

test/SemaTemplate/lookup-dependent-bases.cpp

16

Making this an error seems like a compatibility regression, here and elsewhere.

[zahiraam]

[erichkeane]

lib/Parse/ParseDecl.cpp

2619 ↗

(On Diff #134934)

I believe we'd only want to do this in MSVCCompat mode, allowing invalid code in all situations is probably a bad idea.

2621 ↗

(On Diff #134934)

Shouldn't SS also be checked to make sure its valid?

2623 ↗

(On Diff #134934)

Does this correctly handle Template-templates?

2624 ↗

(On Diff #134934)

What about RValues?

lib/Parse/ParseTentative.cpp

1427 ↗

(On Diff #134934)

Again, perhaps needs to handle RValues.

[rnk]

lib/Parse/ParseDecl.cpp

2619 ↗

(On Diff #134934)

right

test/SemaTemplate/missing-typename.cpp

13 ↗

(On Diff #134934)

I'm guessing this is where the amp and star token type classification heuristic fires.

We should have a test case that goes the opposite direction, what happens if it's not a type name, but an expression? As in:

int g(int);
template <typename T>
int f(int x) {
  return g((T::InnerName & x) & x);
}
struct A { static const int InnerName = 42; };
int main() {
  f<A>(0);
}

[zahiraam]

Hopefully I have responded to the issues you have raised.
I have added the test case you have submitted, to make sure that an expression is generated.
Some of the error messages in the lit test have to be updated (they do match MSVC error messages).

test/SemaTemplate/missing-typename.cpp

13 ↗

(On Diff #134934)

Added the test case.

[rnk]

lib/Parse/ParseDecl.cpp

2621 ↗

(On Diff #136732)

This doesn't seem like the right spot to inject the MS compat check, shouldn't it be associated with the & and * token lookaheads?

2625 ↗

(On Diff #136732)

Doing lookahead for identifiers doesn't seem like the right approach either. It could be an arbitrary expression, 0, a parenthetical expression, etc.

test/SemaCXX/invalid-member-expr.cpp

56 ↗

(On Diff #136732)

This recovery seems worse, and this isn't an MS compat test, so this shouldn't change at all.

test/SemaCXX/typo-correction.cpp

527 ↗

(On Diff #136732)

ditto

[rnk]

lib/Parse/ParseDecl.cpp

2643 ↗

(On Diff #141611)

The MSVC check should come first so that we short-circuit and so that readers know that this logic is only important for MS compatibility insanity.

Please also break this boolean condition up. Something like:

bool IsTemplateName = getLangOpts().CPlusPlus && NextToken().is(tok::less);
if (getLangOpts().MSVCCompat) {
  // <comments about patterns to match and avoid>
  IsTemplateName |= ...;
}

lib/Sema/SemaDecl.cpp

761

Please make this issue an ext_typename_missing diagnostic in MS compat mode.

[zahiraam]

lib/Parse/ParseDecl.cpp

2621 ↗

(On Diff #136732)

Changing place of the condition about the MSVCCompat had the effect that the lit tests below passed without editing them. Is that acceptable?

2625 ↗

(On Diff #136732)

Not sure about this. Could you provide an example?
Meanwhile I am positing a new patch.

lib/Sema/SemaDecl.cpp

761

Not sure what you mean here?
DiagID is already set to ext_typename_missing for MSVC above. Or do you mean that the the condition to throw a diag should be (!IsTemplateName && MSVCCompat)?
Thanks.

[rnk]

lib/Parse/ParseDecl.cpp

2621 ↗

(On Diff #136732)

Yes, the change is for readability, not functionality.

2625 ↗

(On Diff #136732)

Sure, I'm suggesting something like my previous example:

int g(int);
template <typename T>
int f(int x) {
  return g((T::InnerName & x) & x);
}
struct A { static const int InnerName = 42; };
int main() {
  f<A>(0);
}

First, I don't see this as a test case below anywhere.

Second, I think you can replace x in (T::InnerName & x) with other integer expressions that don't start with an identifier:

return g((T::InnerName & 3) & x);
return g((T::InnerName & (3)));
return g((T::InnerName * 3) & x);

We need to accept these examples and instantiate T::InnerName & 3 as a binary operator expression, not a type name.

lib/Sema/SemaDecl.cpp

761

I'm not sure what the correct condition is, but in the test cases you've added, we should be emitting a warning that we expected typename before the type names in place of the error that we emit without MSVC compat enabled.

[zahiraam]

[zahiraam]

Hello.

Feedback on this please. Thanks.

[zahiraam]

Hello,

Feedback please. Thanks.

[rnk]

Broadly, what I think needs to happen is that we need to lift these compatibility checks before typo correction. See also this bug about typo correction interfering with our compatibility heuristics: https://bugs.llvm.org/show_bug.cgi?id=20249

What I see in this change is that you are setting IsTemplateName to true to arrange to reach the isMicrosoftMissingTypename codepath in DiagnoseUnknownTypeName, but we really should be moving the missing typename check before typo correction, or adding a separate parameter to DiagnoseUnknownTypeName to disambiguate the cases we're recovering from.

Lastly, sorry for the delay in response. My only excuse is that this parser code is hard to understand, so I tend to avoid looking at it. =?

lib/Parse/ParseDecl.cpp

2676 ↗

(On Diff #142418)

I'm concerned that you set IsTemplateName to true above, but this code shouldn't execute, because there won't be any < > tokens after this name. In practice, if recovery succeeds, this code is never executed because we return earlier, but this doesn't seem well-structured.

lib/Parse/ParseTentative.cpp

1439–1440 ↗

(On Diff #142418)

I think I see how this works now, this is some tentative parsing logic that looks for reference or pointer type tokens (*, &, or &&) followed by > or ), which typically indicate that the punctuation is used as part of a type name.

lib/Sema/SemaDecl.cpp

660

I'm concerned that this code is becoming harder to understand. In ParseDecl, you are setting IsTemplateName to true when an unknown type name happens to be followed by tokens that look like they indicate a pointer or reference type is being used. But, it's not really a template name. We are trying to recover in the situation where the user has named a non-template type inside of another dependent type. Then, we use IsTemplateName inside this function in ways that only really make sense if we're naming a template.

test/SemaTemplate/missing-typename.cpp

75 ↗

(On Diff #142418)

Thanks for adding these test cases, it gives me more confidence that we're on the right path.

[zahiraam]

lib/Sema/SemaDecl.cpp

660

I can't remember why I have added that. May be the code base has changed since I made this change.
The setting of IsTemplateName is definitely not needed. So this code is not in the new patch that I am uploading.

[rnk]

lib/Parse/ParseTentative.cpp

1497–1500 ↗

(On Diff #186837)

There are no checks here for -fms-compatibility, so this seems like it affects clang's behavior everywhere. Now we assume that (T::name *) is a type in all modes. Is that a problem? My best guess for what's supposed to happen here is that we're supposed to go down the HasMissingTypename codepath above to warn and suggest, hey, this can't possibly be an expression because of this token peeking heuristic, you should add typename.

Also, the indentation is off in phabricator. Please make sure there aren't any tabs when you upload the diff.

lib/Sema/SemaDecl.cpp

761

The effect of this line is, if we're not in ms compat mode, and it is a template name, then don't emit the error. Is that correct?

[zahiraam]

lib/Parse/ParseTentative.cpp

1497–1500 ↗

(On Diff #186837)

Let's see if this this is acceptable to you. I didn't add a warning here since it will be generated later on. And considering the role of the current function.
Hopefully indentation is correct now.

[rnk]

lgtm

[zahiraam]

In D41950#1404899, @rnk wrote:

lgtm

I don't think I have commit right can you please commit it. Thanks.

[rnk]

In D41950#1409332, @zahiraam wrote:

I don't think I have commit right can you please commit it. Thanks.

Sure, done!

lib/Sema/SemaDecl.cpp

if (!LookupCtx) {
         //   A qualified-id that refers to a type and in which the
         //   nested-name-specifier depends on a template-parameter (14.6.2)
         //   shall be prefixed by the keyword typename to indicate that the
         //   qualified-id denotes a type, forming an
         //   elaborated-type-specifier (7.1.5.3).
         //
         // We therefore do not perform any name lookup if the result would
         // refer to a member of an unknown specialization.
-        if (!isClassName && !IsCtorOrDtorName)
+        if (!isClassName && !IsCtorOrDtorName &&
+            !((CurContext->getDeclKind() == Decl::Function ||
+               CurContext->getDeclKind() == Decl::CXXMethod) &&
+              getLangOpts().CPlusPlus &&
+              getLangOpts().MSVCCompat &&
+              getLangOpts().MicrosoftExt))
           return nullptr;
-
         // We know from the grammar that this name refers to a type,
         // so build a dependent node to describe the type.
         if (WantNontrivialTypeSourceInfo)
           return ActOnTypenameType(S, SourceLocation(), *SS, II, NameLoc).get();
 
         NestedNameSpecifierLoc QualifierLoc = SS->getWithLocInContext(Context);
         QualType T = CheckTypenameType(ETK_None, SourceLocation(), QualifierLoc,
                                        II, NameLoc);
bool Sema::isMicrosoftMissingTypename(const CXXScopeSpec *SS, Scope *S) {
   return CurContext->isFunctionOrMethod() || S->isFunctionPrototypeScope();
 }
 
 void Sema::DiagnoseUnknownTypeName(IdentifierInfo *&II,
                                    SourceLocation IILoc,
                                    Scope *S,
                                    CXXScopeSpec *SS,
                                    ParsedType &SuggestedType,
                                    bool IsTemplateName) {
   // Don't report typename errors for editor placeholders.
   if (II->isEditorPlaceholder())
     return;
   // We don't have anything to suggest (yet).
   SuggestedType = nullptr;
 
   // There may have been a typo in the name of the type. Look up typo
   // results, in case we have something that we can suggest.
void Sema::DiagnoseUnknownTypeName(IdentifierInfo *&II,
   else if (isDependentScopeSpecifier(*SS)) {
     unsigned DiagID = diag::err_typename_missing;
     if (getLangOpts().MSVCCompat && isMicrosoftMissingTypename(SS, S))
       DiagID = diag::ext_typename_missing;
 
     Diag(SS->getRange().getBegin(), DiagID)
       << SS->getScopeRep() << II->getName()
       << SourceRange(SS->getRange().getBegin(), IILoc)
       << FixItHint::CreateInsertion(SS->getRange().getBegin(), "typename ");
     SuggestedType = ActOnTypenameType(S, SourceLocation(),
                                       *SS, *II, IILoc).get();
   } else {
     assert(SS && SS->isInvalid() &&
            "Invalid scope specifier has already been diagnosed");
   }
 }
 

test/SemaCXX/MicrosoftCompatibility.cpp

 class D {
 public:
     typedef int Type;
 };
 
 template <class T>
 void function_missing_typename(const T::Type param)// expected-warning {{missing 'typename' prior to dependent type name}}
 {
-    const T::Type var = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
+    const T::Type var = 2;
 }
 
 template void function_missing_typename<D>(const D::Type param);
 
 }
 
 //MSVC allows forward enum declaration
 enum ENUM; // expected-warning {{forward references to 'enum' types are a Microsoft extension}}

test/SemaTemplate/lookup-dependent-bases.cpp

 // RUN: %clang_cc1 -fms-compatibility -fsyntax-only -verify %s
 
 namespace basic {
 struct C {
   static void foo2() {}
 };
 template <typename T>
 struct A {
   typedef C D;
 };
 
 template <typename T>
 struct B : A<T> {
   void foo() {
-    D::foo2(); // expected-warning {{use of undeclared identifier 'D'; unqualified lookup into dependent bases of class template 'B' is a Microsoft extension}}
+    // expected-warning@+2 {{use of undeclared identifier 'D'}}
+    // expected-error@+1 {{use of undeclared identifier 'D'}}
+    D::foo2();
   }
 };
 
-template struct B<int>; // Instantiation has no warnings.
+template struct B<int>; // expected-note {{in instantiation of member function 'basic::B<int>::foo' requested here}}
 }
 
 namespace nested_nodep_base {
 // There are limits to our hacks, MSVC accepts this, but we don't.
 struct A {
   struct D { static void foo2(); };
 };
 template <typename T>

 // though it isn't a template.
 struct A {
   struct D { static void foo2(); };
 };
 template <typename T>
 struct B {
   struct C : T {
     void foo() {
-      D::foo2(); // expected-warning {{use of undeclared identifier 'D'; unqualified lookup into dependent bases of class template 'C' is a Microsoft extension}}
+            // expected-warning@+2 {{use of undeclared identifier 'D'; unqualified lookup into dependent bases of class template 'C' is a Microsoft extension}}
+      // expected-error@+1 {{use of undeclared identifier 'D'}}
+      D::foo2();
     }
   };
 };
 
-template struct B<A>; // Instantiation has no warnings.
+template struct B<A>; //expected-note {{in instantiation of member function 'nested_dep_base::B<nested_dep_base::A>::C::foo' requested here}}
 }

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

 
 struct A {
   int a;
   static int sa;
 };
 template <typename T> struct B : T {
   int     foo() { return a; }           // expected-warning {{lookup into dependent bases}}
   int    *bar() { return &a; }          // expected-warning {{lookup into dependent bases}}
-  int     baz() { return T::a; }
-  int T::*qux() { return &T::a; }
-  static int T::*stuff() { return &T::a; }
-  static int stuff1() { return T::sa; }
-  static int *stuff2() { return &T::sa; }
+  int     baz() { return T::a; }        //expected-error {{expected '(' for function-style cast or type construction}}
+  int T::*qux() { return &T::a; }       //expected-error {{expected '(' for function-style cast or type construction}}
+  static int T::*stuff() { return &T::a; } //expected-error {{expected '(' for function-style cast or type construction}}
+  static int stuff1() { return T::sa; } //expected-error {{expected '(' for function-style cast or type construction}}
+  static int *stuff2() { return &T::sa; } //expected-error {{expected '(' for function-style cast or type construction}}
   static int stuff3() { return sa; }    // expected-warning {{lookup into dependent bases}}
   static int *stuff4() { return &sa; }  // expected-warning {{lookup into dependent bases}}
 };
 
 template <typename T> struct C : T {
   int     foo() { return b; }      // expected-error {{no member named 'b' in 'PR16014::C<PR16014::A>'}} expected-warning {{lookup into dependent bases}}
   int    *bar() { return &b; }     // expected-error {{no member named 'b' in 'PR16014::C<PR16014::A>'}} expected-warning {{lookup into dependent bases}}
-  int     baz() { return T::b; }   // expected-error {{no member named 'b' in 'PR16014::A'}}
-  int T::*qux() { return &T::b; }  // expected-error {{no member named 'b' in 'PR16014::A'}}
-  int T::*fuz() { return &U::a; }  // expected-error {{use of undeclared identifier 'U'}} \
-  // expected-warning {{unqualified lookup into dependent bases of class template 'C'}}
+  int     baz() { return T::b; }   //expected-error {{expected '(' for function-style cast or type construction}}
+  int T::*qux() { return &T::b; }  //expected-error {{expected '(' for function-style cast or type construction}}
+  // expected-warning@+1 {{use of undeclared identifier 'U'; unqualified lookup into dependent bases of class template 'C' is a Microsoft extension}}
+  int T::*fuz() { return &U::a; }  //expected-error {{expected '(' for function-style cast or type construction}}
 };
 
 template struct B<A>;
 template struct C<A>;  // expected-note-re 1+ {{in instantiation of member function 'PR16014::C<PR16014::A>::{{.*}}' requested here}}
 
 template <typename T> struct D : T {
   struct Inner {
     int foo() {

 struct A : T {
   void foo() {
     ::undef(); // expected-error {{no member named 'undef' in the global namespace}}
   }
   void bar() {
     ::UndefClass::undef(); // expected-error {{no member named 'UndefClass' in the global namespace}}
   }
   void baz() {
-    B::qux(); // expected-error {{use of undeclared identifier 'B'}} \
-    // expected-warning {{unqualified lookup into dependent bases of class template 'A'}}
+    // expected-error@+1 2 {{use of undeclared identifier 'B'}}
+    B::qux(); // expected-warning {{use of undeclared identifier 'B'; unqualified lookup into dependent bases of class template 'A' is a Microsoft extension}}
   }
 };
 
 struct B { void qux(); };
 struct C : B { };
-template struct A<C>; // No error!  B is a base of A<C>, and qux is available.
+template struct A<C>; //expected-note {{in instantiation of member function 'PR19233::A<PR19233::C>::baz' requested here}}
 
 struct D { };
 template struct A<D>; // expected-note {{in instantiation of member function 'PR19233::A<PR19233::D>::baz' requested here}}
 
 }
 
 namespace nonmethod_missing_this {
 template <typename T> struct Base { int y = 42; };

test/SemaTemplate/ms-sizeof-missing-typename.cpp

 // RUN: %clang_cc1 -std=c++11 -fms-compatibility -fsyntax-only -verify %s
 
 // If we were even more clever, we'd tell the user to use one set of parens to
 // get the size of this type, so they don't get errors after inserting typename.
 
 namespace basic {
-template <typename T> int type_f() { return sizeof T::type; }  // expected-error {{missing 'typename' prior to dependent type name 'X::type'}}
-template <typename T> int type_g() { return sizeof(T::type); }  // expected-warning {{missing 'typename' prior to dependent type name 'X::type'}}
-template <typename T> int type_h() { return sizeof((T::type)); }  // expected-error {{missing 'typename' prior to dependent type name 'X::type'}}
-template <typename T> int value_f() { return sizeof T::not_a_type; }
+template <typename T> int type_f() { return sizeof T::type; }  // expected-error {{expected parentheses around type name in sizeof expression}}
+template <typename T> int type_g() { return sizeof(T::type); }
+template <typename T> int type_h() { return sizeof((T::type)); }  //expected-error {{expected expression}}
+template <typename T> int value_f() { return sizeof T::not_a_type; } //expected-error {{expected parentheses around type name in sizeof expression}}
 template <typename T> int value_g() { return sizeof(T::not_a_type); }
-template <typename T> int value_h() { return sizeof((T::not_a_type)); }
+template <typename T> int value_h() { return sizeof((T::not_a_type)); } //expected-error {{expected expression}}
 struct X {
   typedef int type;
   static const int not_a_type;
 };
 int bar() {
   return
-      type_f<X>() + // expected-note-re {{in instantiation {{.*}} requested here}}
-      type_g<X>() + // expected-note-re {{in instantiation {{.*}} requested here}}
-      type_h<X>() + // expected-note-re {{in instantiation {{.*}} requested here}}
+      type_f<X>() +
+      type_g<X>() +
+      type_h<X>() +
       value_f<X>() +
       value_f<X>() +
       value_f<X>();
 }
 }
 
 namespace nested_sizeof {
 template <typename T>

test/SemaTemplate/typename-specifier.cpp

namespace PR10925 {
   template<class ctype, int mydim, int coorddim>
   class MockGeometry : BasicGeometry<mydim, int>{
     using typename BasicGeometry<mydim, int>::operator[]; // expected-error {{typename is allowed for identifiers only}}
   };
 }
 
 
 namespace missing_typename {
-template <class T1, class T2> struct pair {}; // expected-note 7 {{template parameter is declared here}}
+#ifdef MSVC
+// expected-note@+4 5 {{template parameter is declared here}}
+#else
+// expected-note@+2 7 {{template parameter is declared here}}
+#endif
+template <class T1, class T2> struct pair {};
 
 template <class T1, class T2>
 struct map {
   typedef T1* iterator;
 };
 
 template <class T>
 class ExampleClass1 {
   struct ExampleItem;
 
 
   struct ExampleItemSet {
     typedef ExampleItem* iterator;
     ExampleItem* operator[](unsigned);
   };
 
   void foo() {
 #ifdef MSVC
-    // expected-warning@+4 {{omitted 'typename' is a Microsoft extension}}
+    // no error
 #else
     // expected-error@+2 {{template argument for template type parameter must be a type; did you forget 'typename'?}}
 #endif
     pair<ExampleItemSet::iterator, int> i;
     pair<this->ExampleItemSet::iterator, int> i; // expected-error-re {{template argument for template type parameter must be a type{{$}}}}
     pair<ExampleItemSet::operator[], int> i; // expected-error-re {{template argument for template type parameter must be a type{{$}}}}
   }
 #ifdef MSVC
     // expected-warning@+4 {{omitted 'typename' is a Microsoft extension}}
 #else
   // expected-error@+2 {{template argument for template type parameter must be a type; did you forget 'typename'?}}
 #endif
   pair<ExampleItemSet::iterator, int> elt;
 
 
   typedef map<int, ExampleItem*> ExampleItemMap;
 
   static void bar() {
 #ifdef MSVC
-    // expected-warning@+4 {{omitted 'typename' is a Microsoft extension}}
+    // no error
 #else
     // expected-error@+2 {{template argument for template type parameter must be a type; did you forget 'typename'?}}
 #endif
     pair<ExampleItemMap::iterator, int> i;
   }
 #ifdef MSVC
     // expected-warning@+4 {{omitted 'typename' is a Microsoft extension}}
 #else

 Foo<Base> x;
 } // namespace missing_typename_and_base
 
 namespace func_type_vs_construct_tmp {
 template <typename> struct S { typedef int type; };
 template <typename T> void f();
 template <int N> void f();
 
-// expected-error@+1 {{missing 'typename' prior to dependent type name 'S<int>::type'}}
+#ifdef MSVC
+// no error
+#else
+// expected-error@+2 {{missing 'typename' prior to dependent type name 'S<int>::type'}}
+#endif
 template <typename T> void g() { f</*typename*/ S<T>::type(int())>(); }
 
 // Adding typename does fix the diagnostic.
 template <typename T> void h() { f<typename S<T>::type(int())>(); }
 
 void j() {
-  g<int>(); // expected-note-re {{in instantiation {{.*}} requested here}}
+#ifdef MSVC
+  // no note.
+#else
+  // expected-note-re@+2 {{in instantiation {{.*}} requested here}}
+#endif
+  g<int>();
   h<int>();
 }
 } // namespace func_type_vs_construct_tmp
 
 namespace pointer_vs_multiply {
 int x;
-// expected-error@+1 {{missing 'typename' prior to dependent type name 'B::type_or_int'}}
+#ifdef MSVC
+// expected-error@+4 {{typename specifier refers to non-type member 'type_or_int' in 'pointer_vs_multiply::A'}}
+#else
+// expected-error@+2 {{missing 'typename' prior to dependent type name 'B::type_or_int'}}
+#endif
 template <typename T> void g() { T::type_or_int * x; }
 // expected-error@+1 {{typename specifier refers to non-type member 'type_or_int' in 'pointer_vs_multiply::A'}}
 template <typename T> void h() { typename T::type_or_int * x; }
 
+#ifdef MSVC
+// expected-note@+4 {{referenced member 'type_or_int' is declared here}}
+#else
+// no note
+#endif
 struct A { static const int type_or_int = 5; }; // expected-note {{referenced member 'type_or_int' is declared here}}
 struct B { typedef int type_or_int; };
 
 void j() {
+#ifdef MSVC
+  //expected-note@+4 {{in instantiation of function template specialization 'pointer_vs_multiply::g<pointer_vs_multiply::A>' requested here}}
+#else
+  // no note
+#endif
   g<A>();
-  g<B>(); // expected-note-re {{in instantiation {{.*}} requested here}}
+#ifdef MSVC
+  // no note
+#else
+  // expected-note-re@+2 {{in instantiation {{.*}} requested here}}
+#endif
+  g<B>();
   h<A>(); // expected-note-re {{in instantiation {{.*}} requested here}}
   h<B>();
 }
 } // namespace pointer_vs_multiply