[AST] Make MemberExpr non-dependent according to core issue 224

[mgehre]

Summary

A MemberExpr is non-dependent if

1. it is a member of the current instantiation
2. the member is non-dependent

We check 1) by asserting that the base of the
MemberExpr is a CXXThisExpr. In addition,
the parent of the member needs to be the current
class or a base of it. (It can happen that the member
decl resolves to a member of an outer class, and later
on Sema prints an error about it. In this case, we keep
it type-dependent, and the error will only appear if it is
ODR-used.)

This changes makes clang reject certain source code that it previously accepted,
namely invalid use of members in functions that are never specialized
or ODR-used. The changes to the tests show examples of this.

[rsmith]

lib/AST/Expr.cpp

1425

The language rule for this says nothing about the base being a this expression; there are many other ways in which a MemberExpr could name a member of the current instantiation.

In general, the base expression should have (pointer to) RecordType or InjectedClassNameType as its type (that is, getAsCXXRecordDecl() should succeed) in all cases except when the member access denotes a member of an unknown specialization.

1454

Please assert that ty is not a dependent type here.

[mgehre]

• Properly check for being member of current instantiation.
• Add new testcase test/CXX/temp/temp.res/temp.dep/temp.dep.expr/p5.cpp
• Added assert() according to review comment

[mgehre]

ping

[rsmith]

lib/AST/Expr.cpp

1414

You should presumably be checking isValueDependent here.

I also don't see any test changes covering the handling of this attribute, and it's not completely clear to me why a dependent enable_if attribute would cause a member to be treated as not being a member of the current instantiation. This should probably be grouped with the check for a member with a dependent type down on line 1458, not as part of the determination of whether we have a member of the current instantiation.

1426

The E->isTypeDependent() test should be in isMemberOfCurrentInstantiation -- right now, that function is determining whether we have a member of the current instantiation or a member of a non-dependent type.

1426–1428

Please clang-format this.

test/CXX/temp/temp.res/temp.dep/temp.dep.expr/p5.cpp

14–15 ↗

(On Diff #64777)

Do you know why we don't diagnose this?

[mgehre]

Thanks for your comments! I'm very sorry for the huge delay. I have now more time to work on this.
I added all your comments to the commit
and fixed a crash in SemaChecking::RefersToMemberWithReducedAlignment,
when the base is an InjectedClassNameType (because it assumed that it can only be a RecordType).

lib/AST/Expr.cpp

1414

Thanks, I will move the check down.

A test for this is in test/SemaCXX/enable_if.cpp:

template <typename T> class C {
  void f() __attribute__((enable_if(T::expr == 0, ""))) {}
  void g() { f(); }
};

If I would not check for value dependent attributes, then the call to f would be considered non-type dependent and produce the warnings

error: 'error' diagnostics seen but not expected: 
  File test/SemaCXX/enable_if.cpp Line 116: no matching member function for call to 'f'
error: 'note' diagnostics seen but not expected: 
  File test/SemaCXX/enable_if.cpp Line 115: candidate disabled: <no message provided>
2 errors generated.

We need to keep f() type-dependent until the template is instantiated.

1426

I will move it there.

1426–1428

Sorry, done.

test/CXX/temp/temp.res/temp.dep/temp.dep.expr/p5.cpp

14–15 ↗

(On Diff #64777)

This patch adds support for the MemberExpr, e.g.

i = j; // expected-error {{incompatible type}}

which before this patch had the AST

|   |   |-BinaryOperator 0x3449f50 <line:10:5, col:9> '<dependent type>' '='
|   |   | |-MemberExpr 0x3449ef0 <col:5> 'int *' lvalue ->i 0x3449950
|   |   | | `-CXXThisExpr 0x3449ed8 <col:5> 'A<T, I> *' this
|   |   | `-DeclRefExpr 0x3449f28 <col:9> 'char *' lvalue Var 0x3449e30 'j' 'char *'

Not yet fixed are CXXDependentScopeMemberExpr, i.e.

(*this).i = j;               // FIXME {{incompatible type}}
  |   |   | - BinaryOperator 0x8cfbd0 <line:12:5, col:17> '<dependent type>' '='
  |   |   | |-CXXDependentScopeMemberExpr 0x8cfb50 <col:5, col:13> '<dependent type>' lvalue .i
  |   |   | | `-ParenExpr 0x8cfb30 <col:5, col:11> '<dependent type>'
  |   |   | |   `-UnaryOperator 0x8cfb10 <col:6, col:7> '<dependent type>' prefix '*'
  |   |   | |     `-CXXThisExpr 0x8cfaf8 <col:7> 'A<T, I> *' this
  |   |   | `-DeclRefExpr 0x8cfba8 <col:17> 'char *' lvalue Var 0x8a5a08 'j' 'char *'

this->i = j;                 // FIXME {{incompatible type}}
  |   |   |-BinaryOperator 0x8cfc90 <line:13:5, col:15> '<dependent type>' '='
  |   |   | |-CXXDependentScopeMemberExpr 0x8cfc10 <col:5, col:11> '<dependent type>' lvalue ->i
  |   |   | | `-CXXThisExpr 0x8cfbf8 <col:5> 'A<T, I> *' this
  |   |   | `-DeclRefExpr 0x8cfc68 <col:15> 'char *' lvalue Var 0x8a5a08 'j' 'char *'

[mgehre]

Updated fix for the crash in SemaChecking::RefersToMemberWithReducedAlignment,
so it also works for C code.

[mgehre]

Friendly ping

lib/AST/Expr.cpp

MemberExpr *MemberExpr::Create(
     DeclarationNameInfo nameinfo, const TemplateArgumentListInfo *targs,
     QualType ty, ExprValueKind vk, ExprObjectKind ok) {
 
   bool hasQualOrFound = (QualifierLoc ||
                          founddecl.getDecl() != memberdecl ||
                          founddecl.getAccess() != memberdecl->getAccess());
 
   bool HasTemplateKWAndArgsInfo = targs || TemplateKWLoc.isValid();
   std::size_t Size =
       totalSizeToAlloc<MemberExprNameQualifier, ASTTemplateKWAndArgsInfo,
                        TemplateArgumentLoc>(hasQualOrFound ? 1 : 0,
                                             HasTemplateKWAndArgsInfo ? 1 : 0,
                                             targs ? targs->size() : 0);
 
   void *Mem = C.Allocate(Size, llvm::alignOf<MemberExpr>());
   MemberExpr *E = new (Mem)
       MemberExpr(base, isarrow, OperatorLoc, memberdecl, nameinfo, ty, vk, ok);
 
-  if (hasQualOrFound) {
-    // FIXME: Wrong. We should be looking at the member declaration we found.
-    if (QualifierLoc && QualifierLoc.getNestedNameSpecifier()->isDependent()) {
-      E->setValueDependent(true);
-      E->setTypeDependent(true);
-      E->setInstantiationDependent(true);
-    } 
-    else if (QualifierLoc && 
-             QualifierLoc.getNestedNameSpecifier()->isInstantiationDependent()) 
-      E->setInstantiationDependent(true);
-    
+
+  if (E->isTypeDependent() && isa<CXXThisExpr>(base)) {
+    bool MemberOfCurrentInstantiation = true;
+
+    auto *BaseClass = base->getType()->getPointeeType()->getAsCXXRecordDecl();
+    assert(BaseClass && "Type of CXXThisExpr must be a pointer to CXXRecordDecl");
+
+    if (auto *Method = dyn_cast<CXXMethodDecl>(memberdecl)) {
+      for (auto *EnableIf : Method->specific_attrs<EnableIfAttr>()) {
+        if (EnableIf->getCond()->isTypeDependent()) {
+          MemberOfCurrentInstantiation = false;
+          break;
+        }
+      }
+      const auto *Parent = Method->getParent();
+      if (BaseClass != Parent && !BaseClass->isDerivedFrom(Parent))
+        MemberOfCurrentInstantiation = false;
+
+    } else if (auto *Field = dyn_cast<FieldDecl>(memberdecl)) {
+      const auto *Parent = dyn_cast<CXXRecordDecl>(Field->getParent());
+      if (!Parent || (BaseClass != Parent && !BaseClass->isDerivedFrom(Parent)))
+        MemberOfCurrentInstantiation = false;
+    } else {
+      MemberOfCurrentInstantiation = false;
+    }
+
+    // A member of the current instantiation is only type dependent if
+    // the field is type dependent. (14.6.2.1 [temp.dep.type])
+    if (MemberOfCurrentInstantiation && !memberdecl->getType()->isDependentType())
+      E->setTypeDependent(false);
+  }
+
+  if (hasQualOrFound) {
     E->HasQualifierOrFoundDecl = true;
 
     MemberExprNameQualifier *NQ =
         E->getTrailingObjects<MemberExprNameQualifier>();
     NQ->QualifierLoc = QualifierLoc;
     NQ->FoundDecl = founddecl;
   }
 

lib/Sema/SemaOverload.cpp

TryObjectArgumentInitialization(Sema &S, SourceLocation Loc, QualType FromType,
   if (const PointerType *PT = FromType->getAs<PointerType>()) {
     FromType = PT->getPointeeType();
 
     // When we had a pointer, it's implicitly dereferenced, so we
     // better have an lvalue.
     assert(FromClassification.isLValue());
   }
 
-  assert(FromType->isRecordType());
+  assert(FromType->isRecordType() || FromType->getTypeClass() == Type::InjectedClassName);
 
   // C++0x [over.match.funcs]p4:
   //   For non-static member functions, the type of the implicit object
   //   parameter is
   //
   //     - "lvalue reference to cv X" for functions declared without a
   //        ref-qualifier or with the & ref-qualifier
   //     - "rvalue reference to cv X" for functions declared with the &&

test/Analysis/stack-addr-ps.cpp

void *lf() {
     void *const &x = &&label; // expected-note {{binding reference variable 'x' here}}
     return x; // expected-warning {{returning address of label, which is local}}
 }
 
 template <typename T>
 struct TS {
   int *get();
   int *m() {
-    int *&x = get();
-    return x;
+    int * const &x = get(); // expected-note {{binding reference variable 'x' here}}
+    return x; // expected-warning {{returning address of local temporary object}}
   }
 };
 
 // rdar://11345441
 int* f5() {
   int& i = i; // expected-warning {{Assigned value is garbage or undefined}} expected-note {{binding reference variable 'i' here}} expected-warning{{reference 'i' is not yet bound to a value when used within its own initialization}}
   return &i; // expected-warning {{address of stack memory associated with local variable 'i' returned}}
 }

test/CXX/class.access/class.access.dcl/p1.cpp

 #if __cplusplus <= 199711L
     // expected-warning@-2 {{access declarations are deprecated; use using declarations instead}}
 #else
     // expected-error@-4 {{ISO C++11 does not allow access declarations; use using declarations instead}}
 #endif
   };
 }
 
-struct Opaque0 {};
+struct Opaque0 {}; // expected-note {{candidate constructor}}
+#if __cplusplus >= 201103L
+// expected-note@-2 {{candidate constructor}}
+#endif
 
 namespace test1 {
   struct A {
     typedef int type;
     struct hiding {}; // expected-note {{previous use is here}}
     Opaque0 hiding;
     union { double union_member; };
     enum tagname { enumerator };
void test3() {
       char array[sizeof(union_member) == sizeof(double) ? 1 : -1];
     }
 
     void test4() {
       enum tagname _ = enumerator;
     }
 
     void test5() {
-      Opaque0 _ = hiding;
+      Opaque0 _ = hiding; // expected-error {{no viable conversion from 'int' to 'Opaque0'}}
     }
   };
 }
 
 namespace test3 {
   struct hiding {};
 
   template <class T> struct A {

test/CXX/dcl.dcl/basic.namespace/namespace.udecl/p4.cpp

namespace test0 {
   class Test0 {
     using NonClass::type; // expected-error {{not a class}}
     using NonClass::hiding; // expected-error {{not a class}}
     using NonClass::union_member; // expected-error {{not a class}}
     using NonClass::enumerator; // expected-error {{not a class}}
   };
 }
 
-struct Opaque0 {};
+struct Opaque0 {}; // expected-note {{no known conversion}}
+#if __cplusplus >= 201103L
+//  expected-note@-2 {{no known conversion}}
+#endif
 
 namespace test1 {
   struct A {
     typedef int type;
     struct hiding {}; // expected-note {{previous use is here}}
     Opaque0 hiding;
     union { double union_member; };
     enum tagname { enumerator };
void test3() {
       char array[sizeof(union_member) == sizeof(double) ? 1 : -1];
     }
 
     void test4() {
       enum tagname _ = enumerator;
     }
 
     void test5() {
-      Opaque0 _ = hiding;
+      Opaque0 _ = hiding; // expected-error {{no viable conversion from 'int' to 'Opaque0'}}
     }
   };
 }
 
 namespace test3 {
   struct hiding {};
 
   template <class T> struct A {

test/CXX/drs/dr3xx.cpp

void local() {
     // FIXME: This is ill-formed.
     extern WithoutLinkage1 withoutLinkageLocal;
   }
 }
 
 namespace dr390 { // dr390: yes
   template<typename T>
   struct A {
-    A() { f(); } // expected-warning {{call to pure virt}}
-    virtual void f() = 0; // expected-note {{here}}
+    A() { f(); } // expected-warning {{call to pure virt}} expected-warning {{call to pure virt}}
+    virtual void f() = 0; // expected-note {{here}} expected-note {{here}}
     virtual ~A() = 0;
   };
   template<typename T> A<T>::~A() { T::error; } // expected-error {{cannot be used prior to}}
   template<typename T> void A<T>::f() { T::error; } // ok, not odr-used
   struct B : A<int> { // expected-note 2{{in instantiation of}}
     void f() {}
   } b;
 }

test/SemaTemplate/dependent-names.cpp

namespace A1 {
     struct S {
       void f() {
         for (auto &a : e)
           __range(a); // expected-error {{undeclared identifier '__range'}}
       }
       int e[10];
     };
     void g() {
-      S<int>().f(); // expected-note {{here}}
+      S<int>().f();
     }
   }
 
   namespace A2 {
     template<typename T>
     struct S {
       void f() {
         for (auto &a : e)

test/SemaTemplate/enum-argument.cpp

 namespace rdar8020920 {
   template<typename T>
   struct X {
     enum { e0 = 32 };
 
     unsigned long long bitfield : e0;
 
     void f(int j) {
-      bitfield + j;
+      (void)(bitfield + j);
     }
   };
 }

test/SemaTemplate/member-access-expr.cpp

 // PR8739
 namespace test6 {
   struct A {};
   struct B {};
   template <class T> class Base;
   template <class T> class Derived : public Base<T> {
     A *field;
     void get(B **ptr) {
-      // It's okay if at some point we figure out how to diagnose this
-      // at instantiation time.
-      *ptr = field;
+      *ptr = field; // expected-error{{assigning to 'test6::B *' from incompatible type 'test6::A *'}}
     }
   };
 }