[Sema] Correct typos in LHS, RHS before building a binop expression.

[vsapsai]

Summary

Specifically, typo correction should be done before dispatching between
different kinds of binary operations like pseudo-object assignment,
overloaded binary operation, etc.

Without this change we hit an assertion

Assertion failed: (!LHSExpr->hasPlaceholderType(BuiltinType::PseudoObject)), function CheckAssignmentOperands

when in Objective-C we reference a property without self and there are
2 equally good typo correction candidates: ivar and a class name. In
this case LHS expression in BuildBinOp is

CXXDependentScopeMemberExpr
`-TypoExpr

and instead of handling Obj-C property assignment as pseudo-object
assignment, we call CreateBuiltinBinOp which corrects typo to

ObjCPropertyRefExpr '<pseudo-object type>'
`-ImplicitCastExpr
  `-ObjCIvarRefExpr
    `-ImplicitCastExpr
      `-DeclRefExpr

but cannot handle pseudo-objects and asserts about it (indirectly,
through CheckAssignmentOperands).

There is no crash in Objective-C++ but I've added a test case because in
the fix we execute different paths for C and C++. I've also considered
removing typo correction from CreateBuiltinBinOp and it doesn't cause
any test failures. But I decided to keep it because CreateBuiltinBinOp
sometimes is called directly, not through BuildBinOp.

rdar://problem/33102722

[ahatanak]

Is it possible to avoid creating CXXDependentScopeMemberExpr in the first place? It seems to me that we shouldn't be creating a CXXDependentScopeMemberExpr for an ObjC property access.

Or perhaps there are reasons to type-correct it later?

[vsapsai]

In D37322#859065, @ahatanak wrote:

Is it possible to avoid creating CXXDependentScopeMemberExpr in the first place? It seems to me that we shouldn't be creating a CXXDependentScopeMemberExpr for an ObjC property access.

Or perhaps there are reasons to type-correct it later?

In Sema::ActOnMemberAccessExpr we handle dependent member expressions before other member references

if (Base->getType()->isDependentType() || Name.isDependentName() ||
    isDependentScopeSpecifier(SS)) {
  return ActOnDependentMemberExpr(Base, Base->getType(), IsArrow, OpLoc, SS,
                                  TemplateKWLoc, FirstQualifierInScope,
                                  NameInfo, TemplateArgs);
}

ActOnMemberAccessExtraArgs ExtraArgs = {S, Id, ObjCImpDecl};
return BuildMemberReferenceExpr(Base, Base->getType(), OpLoc, IsArrow, SS,
                                TemplateKWLoc, FirstQualifierInScope,
                                NameInfo, TemplateArgs, S, &ExtraArgs);

In this case variable Base is a TypoExpr and it is created as type-dependent regardless of the current language.

TypoExpr(QualType T)
    : Expr(TypoExprClass, T, VK_LValue, OK_Ordinary,
           /*isTypeDependent*/ true,
           /*isValueDependent*/ true,
           /*isInstantiationDependent*/ true,
           /*containsUnexpandedParameterPack*/ false) {
  assert(T->isDependentType() && "TypoExpr given a non-dependent type");
}

As a result we end up with CXXDependentScopeMemberExpr when reference a member of a TypoExpr. I'm not entirely sure but it looks like it was designed to work this way.

[ahatanak]

I was thinking about correcting the typo in Sema::ActOnMemberAccessExpr before checking whether Base is dependent. Doing so would cause BuildMemberReferenceExpr to be called instead of ActOnDependentMemberExpr.

I thought that would be a simpler approach, but I'm actually not sure which approach is better. I think this patch should be fine too as long as creating CXXDependentScopeMemberExpr for an ObjC property access doesn't cause a problem before typo correction.

What do you think?

[vsapsai]

Correcting typo in Sema::ActOnMemberAccessExpr is an interesting idea and it almost works. After such change there is a single failure in clang/test/SemaCXX/typo-correction-cxx11.cpp

void run(A *annotations) {
  map new_annotations;

  auto &annotation = *annotations;
  auto new_it = new_annotations.find(5);
  auto &new_anotation = new_it.second;  // expected-note {{'new_anotation' declared here}}
  new_annotation->Swap(&annotation);  // expected-error {{use of undeclared identifier 'new_annotation'; did you mean 'new_anotation'?}}
}

because we don't mention did you mean 'new_anotation'? anymore. I haven't investigated in depth what's causing it but my guess is that general typo correction doesn't suggest replacement due to ambiguity between new_anotation and new_annotations, and prevents member-specific typo correction from handling it.

I think having CXXDependentScopeMemberExpr for an ObjC property should be fine as we are dealing with invalid code and all typos are already type-dependent and instantiation-dependent though it doesn't apply to ObjC.

I agree that my fix isn't specific to the code triggering the assertion. But broader fix can be useful as it covers more cases. After checking SemaExpr.cpp and its multiple CorrectDelayedTyposInExpr, CorrectTypo, CorrectTypoDelayed I have an impression that typo correction absent in Sema::BuildBinOp is an omission worth fixing.

[ahatanak]

I see. It looks like there is an ambiguity between 'new_anotation' and 'new_annotations' that can't be resolved in ActOnMemberAccessExpr because they have the same distance from 'new_annotation'. This doesn't happen if the full expression (new_annotation->Swap) is typo-corrected because 'new_anotation' will be the only valid choice in that case.

I don't think there is a problem with creating CXXDependentScopeMemberExpr for ObjC property access, so this looks fine.

[vsapsai]

Thanks for the review.

clang/lib/Sema/SemaExpr.cpp

if (auto *DRE = dyn_cast<DeclRefExpr>(E))
     return DRE->getDecl();
   if (auto *ME = dyn_cast<MemberExpr>(E))
     return ME->getMemberDecl();
   if (auto *IRE = dyn_cast<ObjCIvarRefExpr>(E))
     return IRE->getDecl();
   return nullptr;
 }
 
+static std::pair<ExprResult, ExprResult>
+CorrectDelayedTyposInBinOp(Sema &S, BinaryOperatorKind Opc, Expr *LHSExpr,
+                           Expr *RHSExpr) {
+  ExprResult LHS = LHSExpr, RHS = RHSExpr;
+  if (!S.getLangOpts().CPlusPlus) {
+    // C cannot handle TypoExpr nodes on either side of a binop because it
+    // doesn't handle dependent types properly, so make sure any TypoExprs have
+    // been dealt with before checking the operands.
+    LHS = S.CorrectDelayedTyposInExpr(LHS);
+    RHS = S.CorrectDelayedTyposInExpr(RHS, [Opc, LHS](Expr *E) {
+      if (Opc != BO_Assign)
+        return ExprResult(E);
+      // Avoid correcting the RHS to the same Expr as the LHS.
+      Decl *D = getDeclFromExpr(E);
+      return (D && D == getDeclFromExpr(LHS.get())) ? ExprError() : E;
+    });
+  }
+  return std::make_pair(LHS, RHS);
+}
+
 /// CreateBuiltinBinOp - Creates a new built-in binary operation with
 /// operator @p Opc at location @c TokLoc. This routine only supports
 /// built-in operations; ActOnBinOp handles overloaded operators.
 ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
                                     BinaryOperatorKind Opc,
                                     Expr *LHSExpr, Expr *RHSExpr) {
   if (getLangOpts().CPlusPlus11 && isa<InitListExpr>(RHSExpr)) {
     // The syntax only allows initializer lists on the RHS of assignment,
ExprResult Sema::CreateBuiltinBinOp(SourceLocation OpLoc,
   ExprResult LHS = LHSExpr, RHS = RHSExpr;
   QualType ResultTy;     // Result type of the binary operator.
   // The following two variables are used for compound assignment operators
   QualType CompLHSTy;    // Type of LHS after promotions for computation
   QualType CompResultTy; // Type of computation result
   ExprValueKind VK = VK_RValue;
   ExprObjectKind OK = OK_Ordinary;
 
-  if (!getLangOpts().CPlusPlus) {
-    // C cannot handle TypoExpr nodes on either side of a binop because it
-    // doesn't handle dependent types properly, so make sure any TypoExprs have
-    // been dealt with before checking the operands.
-    LHS = CorrectDelayedTyposInExpr(LHSExpr);
-    RHS = CorrectDelayedTyposInExpr(RHSExpr, [Opc, LHS](Expr *E) {
-      if (Opc != BO_Assign)
-        return ExprResult(E);
-      // Avoid correcting the RHS to the same Expr as the LHS.
-      Decl *D = getDeclFromExpr(E);
-      return (D && D == getDeclFromExpr(LHS.get())) ? ExprError() : E;
-    });
-    if (!LHS.isUsable() || !RHS.isUsable())
-      return ExprError();
-  }
+  std::tie(LHS, RHS) = CorrectDelayedTyposInBinOp(*this, Opc, LHSExpr, RHSExpr);
 
   if (getLangOpts().OpenCL) {
     QualType LHSTy = LHSExpr->getType();
     QualType RHSTy = RHSExpr->getType();
     // OpenCLC v2.0 s6.13.11.1 allows atomic variables to be initialized by
     // the ATOMIC_VAR_INIT macro.
     if (LHSTy->isAtomicType() || RHSTy->isAtomicType()) {
       SourceRange SR(LHSExpr->getLocStart(), RHSExpr->getLocEnd());
static ExprResult BuildOverloadedBinOp(Sema &S, Scope *Sc, SourceLocation OpLoc,
   // Build the (potentially-overloaded, potentially-dependent)
   // binary operation.
   return S.CreateOverloadedBinOp(OpLoc, Opc, Functions, LHS, RHS);
 }
 
 ExprResult Sema::BuildBinOp(Scope *S, SourceLocation OpLoc,
                             BinaryOperatorKind Opc,
                             Expr *LHSExpr, Expr *RHSExpr) {
+  ExprResult LHS, RHS;
+  std::tie(LHS, RHS) = CorrectDelayedTyposInBinOp(*this, Opc, LHSExpr, RHSExpr);
+  if (!LHS.isUsable() || !RHS.isUsable())
+    return ExprError();
+  LHSExpr = LHS.get();
+  RHSExpr = RHS.get();
+
   // We want to end up calling one of checkPseudoObjectAssignment
   // (if the LHS is a pseudo-object), BuildOverloadedBinOp (if
   // both expressions are overloadable or either is type-dependent),
   // or CreateBuiltinBinOp (in any other case).  We also want to get
   // any placeholder types out of the way.
 
   // Handle pseudo-objects in the LHS.
   if (const BuiltinType *pty = LHSExpr->getType()->getAsPlaceholderType()) {

clang/test/SemaObjC/typo-correction.m

 }
 -(void)method {
   (void)self->implementation; // expected-error {{'I' does not have a member named 'implementation'; did you mean '_implementation'?}}
   (void)self->interface; // expected-error {{'I' does not have a member named 'interface'; did you mean '_interface'?}}
   (void)self->extension; // expected-error {{'I' does not have a member named 'extension'; did you mean '_extension'?}}
 }
 @end
 
+// rdar://problem/33102722
+// Typo correction for a property when it has as correction candidates
+// synthesized ivar and a class name, both at the same edit distance.
+@class TypoCandidate;
+
+__attribute__ (( __objc_root_class__ ))
+@interface PropertyType
+@property int x;
+@end
+
+__attribute__ (( __objc_root_class__ ))
+@interface InterfaceC
+@property(assign) PropertyType *typoCandidate; // expected-note {{'_typoCandidate' declared here}}
+@end
+
+@implementation InterfaceC
+-(void)method {
+  typoCandidate.x = 0; // expected-error {{use of undeclared identifier 'typoCandidate'; did you mean '_typoCandidate'?}}
+}
+@end

clang/test/SemaObjCXX/typo-correction.mm

 }
 @property float _b;
 @end
 
 void invalidNameInIvarAndPropertyBase() {
   float a = ((InvalidNameInIvarAndPropertyBase*)node)->_a; // expected-error {{use of undeclared identifier 'node'}}
   float b = ((InvalidNameInIvarAndPropertyBase*)node)._b; // expected-error {{use of undeclared identifier 'node'}}
 }
+
+// rdar://problem/33102722
+// Typo correction for a property when it has as correction candidates
+// synthesized ivar and a class name, both at the same edit distance.
+@class TypoCandidate;
+
+@interface PropertyType : NSObject
+@property int x;
+@end
+
+@interface InterfaceC : NSObject
+@property(assign) PropertyType *typoCandidate; // expected-note {{'_typoCandidate' declared here}}
+@end
+
+@implementation InterfaceC
+-(void)method {
+  typoCandidate.x = 0; // expected-error {{use of undeclared identifier 'typoCandidate'; did you mean '_typoCandidate'?}}
+}
+@end