[cfe-commits] [PATCH] Member reference fixit (bug 6800)

[Douglas Gregor]

On Jan 27, 2012, at 4:18 AM, Aaron Ballman wrote:

> On Tue, Jan 24, 2012 at 6:54 PM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> On Jan 21, 2012, at 11:42 PM, Aaron Ballman wrote:
>> 
>>> I've attached a patch to fix bug 6800 (and test cases) so that a
>>> proper fixit is output when dereferencing using -> instead of .  We
>>> were missing the fixit when looking up overloaded operator -> and
>>> emitting a diagnostic.
>> 
>> This goes in the right direction, but there's a bit more we need. We only emit a Fix-It when the compiler can recover as if you had typed when we're suggesting. In this case, we'll still end up returning an error expression, which suppresses a lot of important type checking. In this particular case, we'd need to communicate up to the caller that the user meant to type a "." but typed an "->" instead.
> 
> I've been trying various ways to implement this, but what I'm finding
> is that by attempting to recover, the error gets re-generated
> elsewhere (SemaMemberExpr.cpp's LookupMemberExpr to be exact).  So we
> end up emitting the diagnostic twice in that scenario.

This is almost certainly because you aren't communicating the change from '->' to '.' from ActOnStartCXXMemberReference to its caller, so it's caller barges ahead with '->'. Remember, once we've made a suggestion, we want the parser and semantic analysis to behave as if the user had typed the suggested code.

>> Moreover, you should customize the error message in this case. Perhaps something like "type %0 is not a pointer and does not have an overloaded 'operator->'; did you mean to use '.'?".
> 
> Good call!
> 
> I've attached a second attempt, but because I am leaving out the
> diagnostic for operator-> (so as not to get duplicate diagnostics), I
> don't think I've quite got it right.  If you have suggestions, I'd
> love to hear them.

Index: lib/Sema/SemaOverload.cpp
===================================================================
--- lib/Sema/SemaOverload.cpp	(revision 148604)
+++ lib/Sema/SemaOverload.cpp	(working copy)
@@ -10509,7 +10509,8 @@

 ///  (if one exists), where @c Base is an expression of class type and
 /// @c Member is the name of the member we're trying to find.
 ExprResult
-Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc) {
+Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc, 
+                                bool &retryAsPeriod) {
   assert(Base->getType()->isRecordType() &&
          "left-hand side must have class type");
 
@@ -10554,12 +10555,15 @@

     break;
 
   case OR_No_Viable_Function:
-    if (CandidateSet.empty())
-      Diag(OpLoc, diag::err_typecheck_member_reference_arrow)
-        << Base->getType() << Base->getSourceRange();
-    else
+    if (CandidateSet.empty()) {
+//      Diag(OpLoc, diag::err_typecheck_member_reference_suggestion)
+//        << Base->getType() << 1 /*Is Arrow*/ << Base->getSourceRange()
+//        << FixItHint::CreateReplacement(OpLoc, ".");
+        retryAsPeriod = true;
+    } else {

I suggest having the caller of BuildOverloadedArrowExpr indicate when it is able to recover and only suppress the diagnostic in that case. That way, TreeTransform won't allow the recovery (and nothing will change) but the parser path can provide nice recovery.

	- Doug