Diagnose UnresolvedLookupExprs that resolve to instance members in static methods

[rnk]

Summary

During the initial template parse for this code, 'member' is unresolved
and we don't know anything about it:

struct A { int member };
template <typename T>
struct B : public T {
  using T::member;
  static void f() {
    (void)member; // Could be static or non-static.
  }
};
template class B<A>;

The pattern declaration contains an UnresolvedLookupExpr rather than an
UnresolvedMemberExpr because f is static, and member should never
be a field. However, if the code is invalid, it may become a field, in which
case we need to attempt to form a member expr so that we get a diagnostic.

[rnk]

Richard, do you remember why this was no good? I remember we chatted and decided it was horribly complicated and I gave up and went home.

[rsmith]

I don't remember anything more than what's in my review comments, sorry.

[rnk]

In D6700#131774, @rsmith wrote:

I don't remember anything more than what's in my review comments, sorry.

There they are, they're in my inbox, not Phab. Carry on. :)

[rnk]

Richard suggested that maybe we formed the wrong AST while parsing the template. I'm not sure that's the case. We have this very explicit logic that controls what AST nodes we form in SemaExprMember.cpp ClassifyImplicitMemberAccess:

bool isStaticContext = SemaRef.CXXThisTypeOverride.isNull() &&
  (!isa<CXXMethodDecl>(DC) || cast<CXXMethodDecl>(DC)->isStatic());
if (R.isUnresolvableResult())
  return isStaticContext ? IMA_Unresolved_StaticContext : IMA_Unresolved;

John wrote this code in r90266. The behavior seems pretty intentional, so think we're forming the right AST for the template. Our AST just anticipates that it's going to be instantiated with a base class where the member in question is static.

I think we just need some extra diagnostics during instantiation, which this change implements as-is. What do you think? I'll reupload without the other changes.

[rnk]

• Rebase

[rnk]

lib/Sema/TreeTransform.h

9138 ↗

(On Diff #37283)

I believe this will always fail, so one alternative way to do this would be to add a stripped down entry point into Sema just for template instantiation that diagnoses UnresolvedLookupExprs finding instance members.

[rjmccall]

lib/Sema/TreeTransform.h

9135 ↗

(On Diff #37283)

getAsSingle already looks through to the underlying decl.

9138 ↗

(On Diff #37283)

At the very least, we should leave a comment explaining that we don't expect this to succeed, and that we're just doing this for diagnostic purposes. Might want to assert that as well.

[rjmccall]

As a more general comment, I believe the rule is that we try to always make a MemberExpr/UnresolvedMemberExpr whenever there *might* be a base, but that the resulting distinction between an implicit-base UnresolvedMemberExpr and an UnresolvedLookupExpr is not actually used for anything in Sema. When I wrote this code originally, I wanted to make sure we made the "right" decision early to leave plenty of room to handle language requirements that drove a stronger wedge between them, and that's probably still a good idea; but I'm pretty sure it's true that those requirements don't exist right now. It does give you an invariant that you only see implicit-base MemberExprs and UnresolvedMemberExprs in contexts that actually have a 'this'.

[rnk]

• Diagnose instance members rather than pretending to build an expr

[rnk]

• Remove duplicate tests, beef them up

[rsmith]

LGTM

lib/Sema/TreeTransform.h

9131 ↗

(On Diff #37302)

Please also add a test for the case with explicit template arguments, something like:

struct X {
  template<typename> void f();
  template<typename T, typename T::type = 0> static void f();
  template<typename T> void g() { (void)f<T>; }
};
void h(X x) { x.g<int>(); }

... should do the trick. (This already seems to work.)

test/SemaCXX/using-decl-1.cpp

340–349 ↗

(On Diff #37302)

Please also add tests that we don't reject the corresponding address-of-member cases.

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

public:
   BuildAnonymousStructUnionMemberReference(
       const CXXScopeSpec &SS,
       SourceLocation nameLoc,
       IndirectFieldDecl *indirectField,
       DeclAccessPair FoundDecl = DeclAccessPair::make(nullptr, AS_none),
       Expr *baseObjectExpr = nullptr,
       SourceLocation opLoc = SourceLocation());
 
+  void DiagnoseInstanceReference(const CXXScopeSpec &SS, NamedDecl *Rep,
+                                 const DeclarationNameInfo &nameInfo);
+
   ExprResult BuildPossibleImplicitMemberExpr(const CXXScopeSpec &SS,
                                              SourceLocation TemplateKWLoc,
                                              LookupResult &R,
                                 const TemplateArgumentListInfo *TemplateArgs,
                                              const Scope *S);
   ExprResult BuildImplicitMemberExpr(const CXXScopeSpec &SS,
                                      SourceLocation TemplateKWLoc,
                                      LookupResult &R,

cfe/trunk/lib/Sema/SemaExprMember.cpp

if (isProvablyNotDerivedFrom(SemaRef, contextClass, Classes))
     return hasNonInstance ? IMA_Mixed_Unrelated :
            AbstractInstanceResult ? AbstractInstanceResult :
                                     IMA_Error_Unrelated;
 
   return (hasNonInstance ? IMA_Mixed : IMA_Instance);
 }
 
 /// Diagnose a reference to a field with no object available.
-static void diagnoseInstanceReference(Sema &SemaRef,
-                                      const CXXScopeSpec &SS,
-                                      NamedDecl *Rep,
-                                      const DeclarationNameInfo &nameInfo) {
+void Sema::DiagnoseInstanceReference(const CXXScopeSpec &SS, NamedDecl *Rep,
   SourceLocation Loc = nameInfo.getLoc();
   SourceRange Range(Loc);
   if (SS.isSet()) Range.setBegin(SS.getRange().getBegin());
 
   // Look through using shadow decls and aliases.
   Rep = Rep->getUnderlyingDecl();
 
-  DeclContext *FunctionLevelDC = SemaRef.getFunctionLevelDeclContext();
+  DeclContext *FunctionLevelDC = getFunctionLevelDeclContext();
   CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(FunctionLevelDC);
   CXXRecordDecl *ContextClass = Method ? Method->getParent() : nullptr;
   CXXRecordDecl *RepClass = dyn_cast<CXXRecordDecl>(Rep->getDeclContext());
 
   bool InStaticMethod = Method && Method->isStatic();
   bool IsField = isa<FieldDecl>(Rep) || isa<IndirectFieldDecl>(Rep);
 
   if (IsField && InStaticMethod)
     // "invalid use of member 'x' in static member function"
-    SemaRef.Diag(Loc, diag::err_invalid_member_use_in_static_method)
+    Diag(Loc, diag::err_invalid_member_use_in_static_method)
         << Range << nameInfo.getName();
   else if (ContextClass && RepClass && SS.isEmpty() && !InStaticMethod &&
            !RepClass->Equals(ContextClass) && RepClass->Encloses(ContextClass))
     // Unqualified lookup in a non-static member function found a member of an
     // enclosing class.
-    SemaRef.Diag(Loc, diag::err_nested_non_static_member_use)
+    Diag(Loc, diag::err_nested_non_static_member_use)
         << IsField << RepClass << nameInfo.getName() << ContextClass << Range;
   else if (IsField)
-    SemaRef.Diag(Loc, diag::err_invalid_non_static_member_use)
-      << nameInfo.getName() << Range;
-  else
-    SemaRef.Diag(Loc, diag::err_member_call_without_object)
+    Diag(Loc, diag::err_invalid_non_static_member_use) << nameInfo.getName()
                                                        << Range;
+  else
+    Diag(Loc, diag::err_member_call_without_object) << Range;
 }
 
 /// Builds an expression which might be an implicit member expression.
 ExprResult
 Sema::BuildPossibleImplicitMemberExpr(const CXXScopeSpec &SS,
                                       SourceLocation TemplateKWLoc,
                                       LookupResult &R,
                                 const TemplateArgumentListInfo *TemplateArgs,
Sema::BuildPossibleImplicitMemberExpr(const CXXScopeSpec &SS,
   case IMA_Mixed_StaticContext:
   case IMA_Unresolved_StaticContext:
     if (TemplateArgs || TemplateKWLoc.isValid())
       return BuildTemplateIdExpr(SS, TemplateKWLoc, R, false, TemplateArgs);
     return BuildDeclarationNameExpr(SS, R, false);
 
   case IMA_Error_StaticContext:
   case IMA_Error_Unrelated:
-    diagnoseInstanceReference(*this, SS, R.getRepresentativeDecl(),
+    DiagnoseInstanceReference(SS, R.getRepresentativeDecl(),
                               R.getLookupNameInfo());
     return ExprError();
   }
 
   llvm_unreachable("unexpected instance member access kind");
 }
 
 /// Check an ext-vector component access expression.
static void DiagnoseQualifiedMemberReference(Sema &SemaRef,
                                              Expr *BaseExpr,
                                              QualType BaseType,
                                              const CXXScopeSpec &SS,
                                              NamedDecl *rep,
                                        const DeclarationNameInfo &nameInfo) {
   // If this is an implicit member access, use a different set of
   // diagnostics.
   if (!BaseExpr)
-    return diagnoseInstanceReference(SemaRef, SS, rep, nameInfo);
+    return SemaRef.DiagnoseInstanceReference(SS, rep, nameInfo);
 
   SemaRef.Diag(nameInfo.getLoc(), diag::err_qualified_member_of_unrelated)
     << SS.getRange() << rep << BaseType;
 }
 
 // Check whether the declarations we found through a nested-name
 // specifier in a member expression are actually members of the base
 // type.  The restriction here is:

cfe/trunk/lib/Sema/TreeTransform.h

if (Old->getNamingClass()) {
 
     R.setNamingClass(NamingClass);
   }
 
   SourceLocation TemplateKWLoc = Old->getTemplateKeywordLoc();
 
   // If we have neither explicit template arguments, nor the template keyword,
   // it's a normal declaration name.
-  if (!Old->hasExplicitTemplateArgs() && !TemplateKWLoc.isValid())
+  if (!Old->hasExplicitTemplateArgs() && !TemplateKWLoc.isValid()) {
+    // If an UnresolvedLookupExpr resolved to an instance member, that's an
+    // error.
+    NamedDecl *D = R.getAsSingle<NamedDecl>();
+    if (D && D->isCXXInstanceMember()) {
+      SemaRef.DiagnoseInstanceReference(SS, D, Old->getNameInfo());
+      R.clear();
+      return ExprError();
+    }
+
     return getDerived().RebuildDeclarationNameExpr(SS, R, Old->requiresADL());
+  }
 
   // If we have template arguments, rebuild them, then rebuild the
   // templateid expression.
   TemplateArgumentListInfo TransArgs(Old->getLAngleLoc(), Old->getRAngleLoc());
   if (Old->hasExplicitTemplateArgs() &&
       getDerived().TransformTemplateArguments(Old->getTemplateArgs(),
                                               Old->getNumTemplateArgs(),
                                               TransArgs)) {

cfe/trunk/test/SemaCXX/using-decl-1.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++98 %s
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -DCXX11 -fsyntax-only -verify -std=c++11 %s
 
 extern "C" { void f(bool); }
 
 namespace std {
   using ::f;
   inline void f() { return f(true); }
 }
 
namespace Y {
     using PR24033::s; // expected-error {{target of using declaration conflicts with declaration already in scope}}
     using PR24033::e; // expected-error {{target of using declaration conflicts with declaration already in scope}}
 
     using PR24033::vt; // expected-error {{target of using declaration conflicts with declaration already in scope}}
     using PR24033::ft; // expected-error {{target of using declaration conflicts with declaration already in scope}}
     using PR24033::st; // expected-error {{target of using declaration conflicts with declaration already in scope}}
   }
 }
+
+namespace pr21923 {
+template <typename> struct Base {
+  int field;
+  void method();
+};
+template <typename Scalar> struct Derived : Base<Scalar> {
+  using Base<Scalar>::field;
+  using Base<Scalar>::method;
+  static void m_fn1() {
+    // expected-error@+1 {{invalid use of member 'field' in static member function}}
+    (void)field;
+    // expected-error@+1 {{invalid use of member 'field' in static member function}}
+    (void)&field;
+    // expected-error@+1 {{call to non-static member function without an object argument}}
+    (void)method;
+    // expected-error@+1 {{call to non-static member function without an object argument}}
+    (void)&method;
+    // expected-error@+1 {{call to non-static member function without an object argument}}
+    method();
+    (void)&Base<Scalar>::field;
+    (void)&Base<Scalar>::method;
+  }
+};
+// expected-note@+1 {{in instantiation of member function 'pr21923::Derived<int>::m_fn1' requested here}}
+template class Derived<int>;
+
+#ifdef CXX11
+// This is interesting because we form an UnresolvedLookupExpr in the static
+// function template and an UnresolvedMemberExpr in the instance function
+// template. As a result, we get slightly different behavior.
+struct UnresolvedTemplateNames {
+  template <typename> void maybe_static();
+  template <typename T, typename T::type = 0> static void maybe_static();
+
+  template <typename T>
+  void instance_method() { (void)maybe_static<T>(); }
+  template <typename T>
+  static void static_method() {
+    // expected-error@+1 {{call to non-static member function without an object argument}}
+    (void)maybe_static<T>();
+  }
+};
+void force_instantiation(UnresolvedTemplateNames x) {
+  x.instance_method<int>();
+  UnresolvedTemplateNames::static_method<int>(); // expected-note {{requested here}}
+}
+#endif // CXX11
+}