[Sema] Fix a regression introduced in "[AST][Sema] Remove CallExpr::setNumArgs"

[riccibruno]

Summary

D54902 removed CallExpr::setNumArgs in preparation of tail-allocating the arguments of CallExpr. It did this by allocating storage for max(number of arguments, number of parameters in the prototype). The temporarily nulled arguments however causes issues in BuildResolvedCallExpr when typo correction is done just after the creation of the call expression.

This was unfortunately missed by the tests /:

To fix this, delay setting the number of arguments to max(number of arguments, number of parameters in the prototype) until we are ready for it. It would be nice to have this encapsulated in CallExpr but this is the best I can come up with under the constraint that we cannot add anything the CallExpr.

Fixes PR40286. This should probably be cherry-picked into the release branch.

[aaron.ballman]

lib/AST/Expr.cpp

1272–1274 ↗

(On Diff #185954)

Could use std::fill() here to remove the for-loop. Your call whether that looks cleaner or not.

lib/Sema/SemaExpr.cpp

5813

case rebuild -> case, rebuild

lib/Sema/SemaOverload.cpp

12909–12910 ↗

(On Diff #185954)

Oye. When we had to call this hack in one place, I could hold my nose, but having to call it randomly any time something like a call expression is created is a bit too much for me.

How awful is the alternative fix, such as making things resilient to null parameters in BuildResolvedCallExpr()?

[riccibruno]

Rewrote the patch to make it local to BuildResolvedCallExpr. It is still a hack though.

[aaron.ballman]

LGTM!

[riccibruno]

(Following up on a discussion on IRC) I have looked at what would be needed to revert the set of patches which introduced this change, but this results in a >1k lines diff which do not apply cleanly. I think it might be safer to just merge this fix. Unless @aaron.ballman disagree, @hans could you please merge this into the release branch ?

[aaron.ballman]

In D57948#1399403, @riccibruno wrote:

(Following up on a discussion on IRC) I have looked at what would be needed to revert the set of patches which introduced this change, but this results in a >1k lines diff which do not apply cleanly. I think it might be safer to just merge this fix. Unless @aaron.ballman disagree, @hans could you please merge this into the release branch ?

Thank you for looking into the 8.0 revert approach, @riccibruno! I'm okay with putting this patch on 8.0 instead -- you should file a new bug, mark it as blocking https://bugs.llvm.org/show_bug.cgi?id=40331, and make sure @hansw is CCed on the bug.

[riccibruno]

Done, thanks!

[hans]

In D57948#1399683, @riccibruno wrote:

Done, thanks!

That was https://bugs.llvm.org/show_bug.cgi?id=40742 and it's now been merged.

include/clang/AST/Expr.h

public:
   /// the argument array, something we cannot support since the arguments are
   /// stored in a trailing array.
   void shrinkNumArgs(unsigned NewNumArgs) {
     assert((NewNumArgs <= getNumArgs()) &&
            "shrinkNumArgs cannot increase the number of arguments!");
     NumArgs = NewNumArgs;
   }
 
+  /// Bluntly set a new number of arguments without doing any checks whatsoever.
+  /// Only used during construction of a CallExpr in a few places in Sema.
+  /// FIXME: Find a way to remove it.
+  void setNumArgsUnsafe(unsigned NewNumArgs) { NumArgs = NewNumArgs; }
+
   typedef ExprIterator arg_iterator;
   typedef ConstExprIterator const_arg_iterator;
   typedef llvm::iterator_range<arg_iterator> arg_range;
   typedef llvm::iterator_range<const_arg_iterator> const_arg_range;
 
   arg_range arguments() { return arg_range(arg_begin(), arg_end()); }
   const_arg_range arguments() const {
     return const_arg_range(arg_begin(), arg_end());

lib/Sema/SemaExpr.cpp

TheCall =
         CUDAKernelCallExpr::Create(Context, Fn, cast<CallExpr>(Config), Args,
                                    ResultTy, VK_RValue, RParenLoc, NumParams);
   } else {
     TheCall = CallExpr::Create(Context, Fn, Args, ResultTy, VK_RValue,
                                RParenLoc, NumParams, UsesADL);
   }
 
   if (!getLangOpts().CPlusPlus) {
+    // Forget about the nulled arguments since typo correction
+    // do not handle them well.
+    TheCall->shrinkNumArgs(Args.size());
     // C cannot always handle TypoExpr nodes in builtin calls and direct
     // function calls as their argument checking don't necessarily handle
     // dependent types properly, so make sure any TypoExprs have been
     // dealt with.
     ExprResult Result = CorrectDelayedTyposInExpr(TheCall);
     if (!Result.isUsable()) return ExprError();
+    CallExpr *TheOldCall = TheCall;
     TheCall = dyn_cast<CallExpr>(Result.get());
+    bool CorrectedTypos = TheCall != TheOldCall;
     if (!TheCall) return Result;
-    // TheCall at this point has max(Args.size(), NumParams) arguments,
-    // with extra arguments nulled. We don't want to introduce nulled
-    // arguments in Args and so we only take the first Args.size() arguments.
-    Args = llvm::makeArrayRef(TheCall->getArgs(), Args.size());
+    Args = llvm::makeArrayRef(TheCall->getArgs(), TheCall->getNumArgs());
+
+    // A new call expression node was created if some typos were corrected.
+    // However it may not have been constructed with enough storage. In this
+    // case, rebuild the node with enough storage. The waste of space is
+    // immaterial since this only happens when some typos were corrected.
+    if (CorrectedTypos && Args.size() < NumParams) {
+      if (Config)
+        TheCall = CUDAKernelCallExpr::Create(
+            Context, Fn, cast<CallExpr>(Config), Args, ResultTy, VK_RValue,
+            RParenLoc, NumParams);
+      else
+        TheCall = CallExpr::Create(Context, Fn, Args, ResultTy, VK_RValue,
+                                   RParenLoc, NumParams, UsesADL);
+    }
+    // We can now handle the nulled arguments for the default arguments.
+    TheCall->setNumArgsUnsafe(std::max<unsigned>(Args.size(), NumParams));
   }
 
   // Bail out early if calling a builtin with custom type checking.
   if (BuiltinID && Context.BuiltinInfo.hasCustomTypechecking(BuiltinID))
     return CheckBuiltinFunctionCall(FDecl, BuiltinID, TheCall);
 
   if (getLangOpts().CUDA) {
     if (Config) {

test/Sema/typo-correction.c

 
 void rdar38642201_callee(int x, int y);
 void rdar38642201_caller() {
   struct rdar38642201 structVar;
   rdar38642201_callee(
       structVar1.fieldName1.member1, //expected-error{{use of undeclared identifier 'structVar1'}}
       structVar2.fieldName2.member2); //expected-error{{use of undeclared identifier 'structVar2'}}
 }
+
+void PR40286_g(int x, int y);
+void PR40286_h(int x, int y, int z);
+void PR40286_1(int the_value) {
+  PR40286_g(the_walue); // expected-error {{use of undeclared identifier 'the_walue'}}
+}
+void PR40286_2(int the_value) {
+  PR40286_h(the_value, the_walue); // expected-error {{use of undeclared identifier 'the_walue'}}
+}
+void PR40286_3(int the_value) {
+  PR40286_h(the_walue); // expected-error {{use of undeclared identifier 'the_walue'}}
+}
+void PR40286_4(int the_value) { // expected-note {{'the_value' declared here}}
+  PR40286_h(the_value, the_value, the_walue); // expected-error {{use of undeclared identifier 'the_walue'; did you mean 'the_value'?}}
+}