[Sema] Pop function scope when instantiating a func with skipped body

[ilya-biryukov]

Summary

By calling ActOnFinishFunctionBody(). Previously we were only calling
ActOnSkippedFunctionBody, which didn't pop the function scope.
This causes a crash when running on our internal code. No test-case,
though, since I couldn't come up with a small example in reasonable
time.

The bug was introduced in r321174.

[sammccall]

lib/Sema/SemaTemplateInstantiateDecl.cpp

3942

(hmm, not sure whether it's better to duplicate or not-duplicate this comment. up to you)

[aaron.ballman]

Test case for this change?

lib/Sema/SemaTemplateInstantiateDecl.cpp

3944

Rather than having two paths that call the same function, why not hoist the call out to the common path?

3967

This change looks unrelated to the patch.

[ilya-biryukov]

I also came up with a test case, but it breaks because of invalid errors when function bodies are skipped. I'll make sure to include it along with a fix for the errors in a follow-up patch.

lib/Sema/SemaTemplateInstantiateDecl.cpp

3942

It looks relevant to both sites, so I'd probably keep it. It adds some noise, but it's better than having a chance of confusing someone by not including it.

[ilya-biryukov]

• Addressed review comments

[aaron.ballman]

LGTM, thank you for the explanation about the tests.

[ilya-biryukov]

@aaron.ballman , here's a test-case I came up with. I'll fix the other issue (invalid error, that forces the test to fail) and submit it for review along with the fix for the error. Does that SG?

template <class T>
auto make_func() {
  struct impl {
    impl* func() {
      // body of this function is skipped during completion.
      // but will be instantiated anyway.
      return this;
    }
  };

  return impl();
}

void foo() {
  []() {
    make_func<int>();    // currently also produces invalid error `function with auto-deduced type 'make_func<int>' is used before defined
    m
    // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:17:6 %s -o - | FileCheck %s
    // CHECK: make_func : [#auto#]make_func<<#class T#>>()

  };
}

lib/Sema/SemaTemplateInstantiateDecl.cpp

3967

It was a formatting change.

[aaron.ballman]

In D44439#1036868, @ilya-biryukov wrote:

@aaron.ballman , here's a test-case I came up with. I'll fix the other issue (invalid error, that forces the test to fail) and submit it for review along with the fix for the error. Does that SG?

Yes, that sounds good to me. Thank you!

lib/Sema/SemaTemplateInstantiateDecl.cpp

else {
     Sema::ContextRAII savedContext(*this, Function);
 
     if (addInstantiatedParametersToScope(*this, Function, PatternDecl, Scope,
                                          TemplateArgs))
       return;
 
     if (PatternDecl->hasSkippedBody()) {
       ActOnSkippedFunctionBody(Function);
+      // FIXME: finishing the function body while in an expression evaluation
+      // context seems wrong. Investigate more.
+      ActOnFinishFunctionBody(Function, nullptr, /*IsInstantiation=*/true);
     } else {
       if (CXXConstructorDecl *Ctor = dyn_cast<CXXConstructorDecl>(Function)) {
         // If this is a constructor, instantiate the member initializers.
         InstantiateMemInitializers(Ctor, cast<CXXConstructorDecl>(PatternDecl),
                                    TemplateArgs);
 
         // If this is an MS ABI dllexport default constructor, instantiate any
         // default arguments.
         if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
             Ctor->isDefaultConstructor()) {
           InstantiateDefaultCtorDefaultArgs(*this, Ctor);
         }
       }
 
       // Instantiate the function body.
       StmtResult Body = SubstStmt(Pattern, TemplateArgs);
 
       if (Body.isInvalid())
         Function->setInvalidDecl();
 
       // FIXME: finishing the function body while in an expression evaluation
       // context seems wrong. Investigate more.
-      ActOnFinishFunctionBody(Function, Body.get(),
-                              /*IsInstantiation=*/true);
+      ActOnFinishFunctionBody(Function, Body.get(), /*IsInstantiation=*/true);
     }
 
     PerformDependentDiagnostics(PatternDecl, TemplateArgs);
 
     if (auto *Listener = getASTMutationListener())
       Listener->FunctionDefinitionInstantiated(Function);
 
     savedContext.pop();