Clang c++ compiles error with struct in template class and < operator

[wangbo15]

Please consider the following code:

template<class T>
class confused{};

template<class T>
class test
{
    void foo() {
        if(this->b.confused < 1);
    };

    struct bar
    {
        int confused;
    } b;
};

Clang rejects it by giving such considerably puzzling output:

<source>:8:32: error: expected '>'
    8 |         if(this->b.confused < 1);
      |                                ^
<source>:8:29: note: to match this '<'
    8 |         if(this->b.confused < 1);
      |                             ^
<source>:8:32: error: expected unqualified-id
    8 |         if(this->b.confused < 1);
      |                                ^
<source>:8:33: warning: if statement has empty body [-Wempty-body]
    8 |         if(this->b.confused < 1);
      |                                 ^
<source>:8:33: note: put the semicolon on a separate line to silence this warning
1 warning and 2 errors generated.
Compiler returned: 1

We suppose that's because the compiler has found incorrect local variable name of confused by confusing the global template class declared above with the integer member variable declared in the nested class bar inside test

Please check https://godbolt.org/z/7afqsvEvo.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Bo Wang (wangbo15)

Please consider the following code:

template<class T>
class confused{};

template<class T>
class test
{
    void foo() {
        if(this->b.confused < 1);
    };

    struct bar
    {
        int confused;
    } b;
};

Clang rejects it by giving such considerably puzzling output:

<source>:8:32: error: expected '>'
    8 |         if(this->b.confused < 1);
      |                                ^
<source>:8:29: note: to match this '<'
    8 |         if(this->b.confused < 1);
      |                             ^
<source>:8:32: error: expected unqualified-id
    8 |         if(this->b.confused < 1);
      |                                ^
<source>:8:33: warning: if statement has empty body [-Wempty-body]
    8 |         if(this->b.confused < 1);
      |                                 ^
<source>:8:33: note: put the semicolon on a separate line to silence this warning
1 warning and 2 errors generated.
Compiler returned: 1

We suppose that's because the compiler has found incorrect local variable name of confused by confusing the global template class declared above with the integer member variable declared in the nested class bar inside test

Please check https://godbolt.org/z/7afqsvEvo.

[shafik]

This goes all the way back to clang-3.0: https://godbolt.org/z/srY7rsW94

I am surprised we have not seen this before. Could be related to: #78213

Maybe related to: #16855

[shafik]

The main difference between https://godbolt.org/z/d8dY8e779 and https://godbolt.org/z/ovqssd9Y8 is here:

llvm-project/clang/lib/Sema/SemaTemplate.cpp

Lines 459 to 468 in 5a82daa

	// In a class member access expression (5.2.5), if the . or -> token is
	// immediately followed by an identifier followed by a <, the
	// identifier must be looked up to determine whether the < is the
	// beginning of a template argument list (14.2) or a less-than operator.
	// The identifier is first looked up in the class of the object
	// expression. If the identifier is not found, it is then looked up in
	// the context of the entire postfix-expression and shall name a class
	// template.
	if (S)
	LookupName(Found, S);

In the bad case LookupName name finds class confused and nothing else and in the good case it finds nothing. This in and of itself is not the whole issue. It is when it comes back to here:

llvm-project/clang/lib/Sema/SemaTemplate.cpp

Lines 227 to 228 in 5a82daa

	if (R.empty())
	return TNK_Non_template;

In the working case R.empty() is true and it returns TNK_Non_template and in the bad case we end up trying to process the template, which fails:

llvm-project/clang/lib/Parse/ParseExprCXX.cpp

Lines 514 to 520 in 5a82daa

	if (TemplateNameKind TNK = Actions.isTemplateName(getCurScope(), SS,
	/*hasTemplateKeyword=*/false,
	TemplateName,
	ObjectType,
	EnteringContext,
	Template,
	MemberOfUnknownSpecialization)) {

It is not clear to me what we need to do differently here. It is not till after the call here:

llvm-project/clang/lib/Parse/ParseExpr.cpp

Lines 2231 to 2233 in bfbd0da

	ParseOptionalCXXScopeSpecifier(
	SS, ObjectType, LHS.get() && LHS.get()->containsErrors(),
	/*EnteringContext=*/false, &MayBePseudoDestructor);

that we finally make an attempt to call ActOnMemberAccessExpr(...).

CC @zygoloid maybe you have an idea

[shafik]

If you look at a bunch of the examples in basic.lookup.qual.general there does not seem to be a nice simple heuristic to determining if I should consider the name found valid or not.

Apparently MSVC is the only once to get all these correct: https://godbolt.org/z/14W8KETz8

int f();
struct A {
  int B, C;
  template<int> using D = void;
  using T = void;
  void f();
};
using B = A;
template<int> using C = A;
template<int> using D = A;
template<int> using X = A;

template<class T>
void g(T *p) {                  // as instantiated for g<A>:
  p->X<0>::f();                 // error: A​::​X not found in ((p->X) < 0) > ​::​f()
  p->template X<0>::f();        // OK, ​::​X found in definition context
  p->B::f();                    // OK, non-type A​::​B ignored
  p->template C<0>::f();        // error: A​::​C is not a template
  p->template D<0>::f();        // error: A​::​D<0> is not a class type
  p->T::f();                    // error: A​::​T is not a class type
}
template void g(A*);

I think the reason clang fails on these cases is the same underlying issue here.

[shafik]

I attempted to do extra filtering in Sema::isTemplateName(...)

if (R.empty())
    return TNK_Non_template;
  else if (!ObjectType.isNull() && R.isSingleResult())
   if (CXXRecordDecl *RD = ObjectType->getAsCXXRecordDecl()) {
    if (NamedDecl *ND = R.getFoundDecl()) {
      if (ClassTemplateDecl *CTD = dyn_cast<ClassTemplateDecl>(ND))
        if (CXXRecordDecl *RDF = dyn_cast<CXXRecordDecl>(CTD->getTemplatedDecl())) {
          bool AnyEqual = false;
          if (RD->getMostRecentDecl() == RDF->getMostRecentDecl())
            AnyEqual = true;

          for ( auto base : RD->getMostRecentDecl()->bases() ) {
            if (!base.getType().isNull() &&
                base.getType()->getAsCXXRecordDecl() &&
                base.getType()->getAsCXXRecordDecl()->getMostRecentDecl() == RDF->getMostRecentDecl())
              AnyEqual = true;
            
            if (!base.getType().isNull())
              if (const TemplateSpecializationType *TST = base.getType()->getAs<TemplateSpecializationType>()) {
                TemplateName TN = TST->getTemplateName();
                if (ClassTemplateDecl *CTD = dyn_cast<ClassTemplateDecl>(TN.getAsTemplateDecl()))
                  if (CXXRecordDecl *RDB = dyn_cast<CXXRecordDecl>(CTD->getTemplatedDecl()))
                    if (RDF->getMostRecentDecl() == RDB->getMostRecentDecl())
                      AnyEqual = true;
              }
          }

          if (!AnyEqual)
            return TNK_Non_template;
        }
    }
  }

It kind of works but I end up w/ a lot of duplicate diagnostics in several tests. It definitely feels hacky but there wasn't a more obvious place to do filtering.

[zygoloid]

Clang is interpreting the confused< as starting a nested name specifier, as in:

template<class T>
class confused{ int x; };

template<class T>
class test
{
    void foo() {
        if(this->b.confused<int>::x);
    };

    struct bar
    {
        int confused;
    } b;
};

template<> class test<int>::bar : confused<int> { };
// now test<int>::foo() accesses the confused<int>::x member in b

The rules here were changed by P1787R6. It used to be that if a member lookup was dependent and the name was followed by <, we were supposed to perform unqualified lookup to see if the name is a template or not. But after P1787, the rules are simpler: if the qualified member lookup is dependent, we don't perform unqualified lookup at all, and then we never treat the < as starting a template argument list. If you mean to name a template here, you need to use the template keyword.

It looks like Clang doesn't implement this part of P1787R6 yet.

[zygoloid]

If you look at a bunch of the examples in basic.lookup.qual.general there does not seem to be a nice simple heuristic to determining if I should consider the name found valid or not.

I'll try to give a bit more context about what's happening in each case:

  p->X<0>::f();                 // error: A​::​X not found in ((p->X) < 0) > ​::​f()

Here, the lookup for X is dependent, so it's treated as a non-template, and the < is a less-than. Instantiation will fail with T = A because there's no member X in A.

  p->template X<0>::f();        // OK, ​::​X found in definition context

Here, the template keyword says to treat the name X as a template name. The actual meaning of X is not determined until instantiation, but we should perform an unqualified lookup during template parsing ("in the definition context") so we have a fallback to use if there's no member named X within T during instantiation.

If there were a member template named X in A, this would name that member template instead. See the D<0> case below for an example of that.

  p->B::f();                    // OK, non-type A​::​B ignored

Right, this lookup for B ignores results that can't be used as nested-name-specifiers.

  p->template C<0>::f();        // error: A​::​C is not a template

This is fine when parsing the template, and parses as a template-id not as a less-than comparison, but fails during instantiation because C resolves to A::C which is not a template but is preceded by the template keyword.

  p->template D<0>::f();        // error: A​::​D<0> is not a class type

Like the previous example, this is fine when parsing the template, but fails in instantiation because A::D<0> is void, which can't be used as a nested name specifier. This D resolves to the member of the class found in the instantiation context, because notionally unqualified lookup is not performed here. (But we still need to perform unqualified lookup in the template definition context so we have a lookup result for D to fall back onto if it turns out that there is no member of A named D.)

  p->T::f();                    // error: A​::​T is not a class type

This names A::T (found in the instantiation context), not the template parameter T (found in the definition context). If A had no member named T, this would instead name the template parameter T during the instantiation with T = A.