[clang][CodeComplete] Ensure there are no crashes when completing with ParenListExprs as LHS

[sammccall]

clang/lib/Sema/SemaCodeComplete.cpp

5272–5273

I think we should merge the two comments, something like...

/// If LHS is ParenListExpr, assume a chain of comma operators and pick the last expr.
/// We expect other ParenListExprs to be resolved to e.g. constructor calls before here.
/// (So the ParenListExpr should be nonempty, but check just in case)

5272–5273

now this is getting a little more complicated, we could pull out a function Expr* unwrapParenList(Expr*) and avoid duplicating code & comments, but up to you.

5273

I'm not actually sure we should fix this. Nonempty is a subtle invariant that could also be violated by e.g. broken code. And the check is cheap.

5274

I think you want OtherOpBase = null here, not return

[kadircet]

• Pull unwrapping of parenlistexprs into a function
• Merge comments

clang/lib/Sema/SemaCodeComplete.cpp

if (RD && RD->isCompleteDefinition()) {
       for (const auto &Member : RD->lookup(CDSME->getMember()))
         if (const ValueDecl *VD = llvm::dyn_cast<ValueDecl>(Member))
           return VD->getType().getNonReferenceType();
     }
   }
   return Unresolved;
 }
 
+// If \p Base is ParenListExpr, assume a chain of comma operators and pick the
+// last expr. We expect other ParenListExprs to be resolved to e.g. constructor
+// calls before here. (So the ParenListExpr should be nonempty, but check just
+// in case)
+Expr *unwrapParenList(Expr *Base) {
+  if (auto *PLE = llvm::dyn_cast_or_null<ParenListExpr>(Base)) {
+    if (PLE->getNumExprs() == 0)
+      return nullptr;
+    Base = PLE->getExpr(PLE->getNumExprs() - 1);
+  }
+  return Base;
+}
+
 } // namespace
 
 void Sema::CodeCompleteMemberReferenceExpr(Scope *S, Expr *Base,
                                            Expr *OtherOpBase,
                                            SourceLocation OpLoc, bool IsArrow,
                                            bool IsBaseExprStatement,
                                            QualType PreferredType) {
+  Base = unwrapParenList(Base);
+  OtherOpBase = unwrapParenList(OtherOpBase);
   if (!Base || !CodeCompleter)
     return;
 
-  // Peel off the ParenListExpr by chosing the last one, as they don't have a
-  // predefined type.
-  if (auto *PLE = llvm::dyn_cast<ParenListExpr>(Base))
-    Base = PLE->getExpr(PLE->getNumExprs() - 1);
-  if (OtherOpBase) {
-    if (auto *PLE = llvm::dyn_cast<ParenListExpr>(OtherOpBase))
-      OtherOpBase = PLE->getExpr(PLE->getNumExprs() - 1);
-  }
-
   ExprResult ConvertedBase = PerformMemberExprBaseConversion(Base, IsArrow);
   if (ConvertedBase.isInvalid())
     return;
   QualType ConvertedBaseType = getApproximateType(ConvertedBase.get());
 
   enum CodeCompletionContext::Kind contextKind;
 
   if (IsArrow) {
     if (const auto *Ptr = ConvertedBaseType->getAs<PointerType>())
       ConvertedBaseType = Ptr->getPointeeType();
ProduceSignatureHelp(Sema &SemaRef, Scope *S,
   SemaRef.CodeCompleter->ProcessOverloadCandidates(
       SemaRef, CurrentArg, Candidates.data(), Candidates.size(), OpenParLoc);
   return getParamType(SemaRef, Candidates, CurrentArg);
 }
 
 QualType Sema::ProduceCallSignatureHelp(Scope *S, Expr *Fn,
                                         ArrayRef<Expr *> Args,
                                         SourceLocation OpenParLoc) {
+  Fn = unwrapParenList(Fn);
   if (!CodeCompleter || !Fn)
     return QualType();
 
-  // If we have a ParenListExpr for LHS, peel it off by chosing the last expr.
-  // As ParenListExprs don't have a predefined type.
-  if (auto *PLE = llvm::dyn_cast<ParenListExpr>(Fn))
-    Fn = PLE->getExpr(PLE->getNumExprs() - 1);
-
   // FIXME: Provide support for variadic template functions.
   // Ignore type-dependent call expressions entirely.
   if (Fn->isTypeDependent() || anyNullArguments(Args))
     return QualType();
   // In presence of dependent args we surface all possible signatures using the
   // non-dependent args in the prefix. Afterwards we do a post filtering to make
   // sure provided candidates satisfy parameter count restrictions.
   auto ArgsWithoutDependentTypes =