Page MenuHomePhabricator

PR43080: Do not build context-sensitive expressions during name classification.
ClosedPublic

Authored by rsmith on Oct 11 2019, 4:14 PM.

Details

Summary

We don't know what context to use until the classification result is
consumed by the parser, which could happen in a different semantic
context.

This covers everything except C++ implicit class member access, which
is a little awkward to handle properly in the face of the protected
member access check. But it at least fixes all the currently-filed
instances of PR43080.

Diff Detail

Event Timeline

rsmith created this revision.Oct 11 2019, 4:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 4:14 PM

Hmm, the ObjC changes were simpler than I expected. And you managed to avoid making changes to overload sets.

The changes related to IsAddressOfOperand are a nice simplification.

Would it make sense to always use ClassifyName from the parser, instead of using ActOnIdExpression?

include/clang/Sema/Sema.h
1865 ↗(On Diff #224705)

This enum could probably use brief comments explaining what the different results mean.

lib/Sema/SemaDecl.cpp
1191 ↗(On Diff #224705)

This doesn't depend on the context... because we're going to throw away the expression later anyway? I guess that makes sense.

rsmith marked an inline comment as done.Oct 11 2019, 7:05 PM

Would it make sense to always use ClassifyName from the parser, instead of using ActOnIdExpression?

I like that idea, at least in principle. We'd need to generalize ClassifyName to operate on an arbitrary UnqualifiedId instead of only on an identifier, and we'd still need something to cover the various current calls to ActOnIdExpression from within Sema, so I'm not sure whether it'll actually work out cleaner in practice. I'm inclined to defer doing that for now, if that's OK :)

lib/Sema/SemaDecl.cpp
1191 ↗(On Diff #224705)

Yes, basically; we don't do anything context-dependent right now when building the UnresolvedLookupExpr, it's all delayed until we resolve the overload set. I suppose I could make this more explicit by directly creating the UnresolvedLookupExpr here, at the cost of duplicating a little of the work done by BuildDeclarationNameExpr. WDYT?

rsmith updated this revision to Diff 224765.Oct 12 2019, 6:38 PM

Add comments for various NC_ classifications. Remove the unused and broken
NC_NestedNameSpecifier classification and the code in ClassifyName that
tries to classify names as nested name specifiers.

efriedma accepted this revision.Oct 14 2019, 12:17 PM

LGTM

I'm inclined to defer doing that for now, if that's OK :)

Sure.

lib/Sema/SemaDecl.cpp
1191 ↗(On Diff #224705)

I don't think duplicating the code really helps; the comment makes it clear enough what's happening here.

This revision is now accepted and ready to land.Oct 14 2019, 12:17 PM
This revision was automatically updated to reflect the committed changes.
kianm added a subscriber: kianm.EditedOct 30 2019, 11:35 AM

Hi,

There is a problem with this commit because it asserts after errors have been recognized. I have attached a reduced test case. The assertion looks to occur here:

clang: /home/kianm/llvm/dev/llvm-project/clang/lib/Parse/ParseExprCXX.cpp:585: clang::ExprResult clang::Parser::tryParseCXXIdExpression(clang::CXXScopeSpec &, bool, clang::Token &): Assertion `SS.isEmpty() && "undeclared non-type annotation should be unqualified"' failed.

The reproducible command is:

clang -cc1 -x c++ reduced.c

kianm added a comment.Dec 10 2019, 1:20 PM

Hi, I am still seeing problems with this assertion. Could we please get a fix? I've posted the reduced test case and reproducible command on this Phabricator patch.

Thanks.

rsmith added a comment.Wed, Feb 5, 5:30 PM

Hi, I am still seeing problems with this assertion. Could we please get a fix? I've posted the reduced test case and reproducible command on this Phabricator patch.

Are you still seeing problems? If so, please can you file a bug report? I can't reproduce this failure with a recent Clang; I get

<stdin>:14:3: error: ambiguous partial specializations of 'Base<A, '\x01'>'
  Base<A, C>::f();
  ^
<stdin>:7:7: note: partial specialization matches [with T = A]
class Base<T, 1> { };
      ^
<stdin>:10:7: note: partial specialization matches [with T = A, I = 1]
class Base<T, I> { };
      ^

but no assertion failure for the posted testcase.

rsmith added a comment.Wed, Feb 5, 5:34 PM

Hi, I am still seeing problems with this assertion. Could we please get a fix? I've posted the reduced test case and reproducible command on this Phabricator patch.

Are you still seeing problems?

For future Phabricator visitors, this was fixed in rG2e48be09b.

kianm added a comment.EditedThu, Feb 6, 7:22 AM

Hi, I am still seeing problems with this assertion. Could we please get a fix? I've posted the reduced test case and reproducible command on this Phabricator patch.

Are you still seeing problems?

For future Phabricator visitors, this was fixed in rG2e48be09b.

No I am not seeing the problem anymore, it was fixed in rG2e48be09b. Thanks!