This is an archive of the discontinued LLVM Phabricator instance.

Diagnose UnresolvedLookupExprs that resolve to instance members in static methods
ClosedPublic

Authored by rnk on Dec 16 2014, 4:07 PM.

Details

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.

Diff Detail

Event Timeline

rnk updated this revision to Diff 17374.Dec 16 2014, 4:07 PM
rnk retitled this revision from to Instantiate UnresolvedLookupExpr to MemberExpr when appropriate.
rnk updated this object.
rnk added a reviewer: rsmith.
rnk added a subscriber: Unknown Object (MLST).
rnk added a comment.Feb 27 2015, 4:22 PM

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 edited edge metadata.Feb 27 2015, 4:59 PM

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

rnk added a comment.Feb 28 2015, 1:14 PM

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 added a comment.Oct 13 2015, 1:08 PM

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 updated this revision to Diff 37283.Oct 13 2015, 1:31 PM
  • Rebase
rnk retitled this revision from Instantiate UnresolvedLookupExpr to MemberExpr when appropriate to Diagnose UnresolvedLookupExprs that resolve to instance members in static methods.Oct 13 2015, 1:34 PM
rnk updated this object.
rnk added inline comments.
lib/Sema/TreeTransform.h
9138

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 added inline comments.Oct 13 2015, 2:33 PM
lib/Sema/TreeTransform.h
9135

getAsSingle already looks through to the underlying decl.

9138

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 edited edge metadata.Oct 13 2015, 2:44 PM

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 updated this revision to Diff 37302.Oct 13 2015, 5:05 PM
rnk edited edge metadata.
  • Diagnose instance members rather than pretending to build an expr
rnk updated this revision to Diff 37303.Oct 13 2015, 5:16 PM
  • Remove duplicate tests, beef them up
rsmith accepted this revision.Oct 13 2015, 5:59 PM
rsmith edited edge metadata.

LGTM

lib/Sema/TreeTransform.h
9131

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

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

This revision is now accepted and ready to land.Oct 13 2015, 5:59 PM
This revision was automatically updated to reflect the committed changes.
rnk marked an inline comment as done.