[Sema] Don't skip function bodies with 'auto' without trailing return type

[ilya-biryukov]

Summary

Skipping them was clearly not intentional. It's impossible to
guarantee correctness if the bodies are skipped.
Also adds a test case for r327504, now that it does not produce
invalid errors that made the test fail.

[aaron.ballman]

Adding Richard as a reviewer, because this seems somewhat clever to me (so there may be a better change to make).

lib/Sema/SemaDecl.cpp

12668

Please don't use auto here as the type is not spelled out explicitly. Also, the type can be const-qualified.

[ilya-biryukov]

• Replace auto with explicit type

[ilya-biryukov]

I hit that bug in practice a few days ago when playing around with a C++17 codebase. It makes completion totally unusable when functions with auto-deduced return type are used.
@rsmith, any chance you could take a look? Or maybe suggest someone else as a reviewer?

[rwols]

Can we get this merged soon? I'm experiencing a lot of false positive diagnostics with this library at work.

[aaron.ballman]

Expounding on my concerns a bit -- I'm worried about all the other places calling isUndeducedType() and whether they're also at risk and thus a better fix is to change isUndeducedType() to pay attention to the language mode.

lib/Sema/SemaDecl.cpp

12668

This can be marked const.

[rsmith]

Thanks, I think this can be simplified a bit but this looks like the right fix.

lib/Sema/SemaDecl.cpp

12666–12667

I don't think this comment captures the problem. Rather, I think the problem is that in a template we deduce the auto to a dependent type, so it's not considered "undeduced" at the point when we ask this question.

12669–12670

Is there any need to check whether the type has been deduced here? It seems simpler (and equivalent) to turn off skipping for functions whose return type involves a deduced type regardless of whether deduction has been performed already (which it hasn't).

[ilya-biryukov]

• Address review comments.
• Added one more test.

[ilya-biryukov]

Sorry for delays with this patch, I somehow missed the email notification about it.

Expounding on my concerns a bit -- I'm worried about all the other places calling isUndeducedType() and whether they're also at risk and thus a better fix is to change isUndeducedType() to pay attention to the language mode.

I thought semantics of isUndeducedType() is actually what's most callers (e.g. diagnostics) want. That is "the type is undeduced and not dependent". But the name can definitely bring some confusion, so there might be other places that misinterpreted it.

@rsmith, it would be great if you could take another look when you have time.

lib/Sema/SemaDecl.cpp

12666–12667

Thanks! That definitely explains the problem in a proper manner.

12669–12670

Thanks! I misinterpreted the deduced type's semantics. I thought it also appears in the following case:
auto foo() -> int {}
But it doesn't.

[rsmith]

test/CodeCompletion/skip-auto-funcs.cpp

10

is skipped -> is not skipped?

19

is skipped -> is not skipped?

[ilya-biryukov]

• Fix the comments

[nik]

I've stumbled about this bug too and was looking into it and then I saw the mail about this change being submitted :)

I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , instead of FD->getReturnType()->getContainedDeducedType() I have FD->getReturnType()->getAs<AutoType>()). It looks like the tests from this change pass with my change and my tests pass with this change (consider to add decltype(auto) tests, too!). So I'm wondering whether there is a test case that fails for one change but not the other (if there is, the test case should probably be added), or if there are differences in the performance.

I'm not familiar enough with the code base to justify the getAs<AutoType>(), I've just observed that it made my tests pass.

[ilya-biryukov]

In D44480#1116359, @nik wrote:

I've stumbled about this bug too and was looking into it and then I saw the mail about this change being submitted :)

I've ended up with a slightly different change (https://dpaste.de/PSpM/raw , instead of FD->getReturnType()->getContainedDeducedType() I have FD->getReturnType()->getAs<AutoType>()). It looks like the tests from this change pass with my change and my tests pass with this change (consider to add decltype(auto) tests, too!). So I'm wondering whether there is a test case that fails for one change but not the other (if there is, the test case should probably be added), or if there are differences in the performance.

I'm not familiar enough with the code base to justify the getAs<AutoType>(), I've just observed that it made my tests pass.

I'm sure there should be no significant differences in performance. Adding decltype(auto) to the test would certainly be useful.
W.r.t. correctness I think at this point checks for DeducedType and AutoType are equivalent. There is only one other inheritor of DeducedType, the DeducedTemplateSpecializationType, and it should not appear in the function return type IIUC.

When the concepts get into the standard and clangd implements them, this might change, as the comment of DeducedType suggests:

/// Common base class for placeholders for types that get replaced by
/// placeholder type deduction: C++11 auto, C++14 decltype(auto), C++17 deduced
/// class template types, and (eventually) constrained type names from the C++
/// Concepts TS.

I don't know the exact details of the latest Concepts proposal, but would assume usages of constrained template parameters from Concepts TS shouldn't block skipping of function bodies. In which case, your fix seems to do the right thing and this one doesn't.
@rsmith, WDYT? Should we check for AutoType instead?

[cpplearner]

Does getAs<AutoType>() work correctly with function returning auto&?

[nik]

In D44480#1117147, @cpplearner wrote:

Does getAs<AutoType>() work correctly with function returning auto&?

No. For

class Foo {} foo;
auto& return_auto_ref() { return foo; }
auto r6 = return_auto_ref();

the "getAs<AutoType>()" version will skip the function body and generate an error message on use, but "FD->getReturnType()->getContainedDeducedType()" works fine (will not skip the body, as it should here). OK, so now I have a rough idea what the "Contained" was referring too :)

[ilya-biryukov]

In D44480#1117230, @nik wrote:

In D44480#1117147, @cpplearner wrote:

Does getAs<AutoType>() work correctly with function returning auto&?

the "getAs<AutoType>()" version will skip the function body and generate an error message on use, but "FD->getReturnType()->getContainedDeducedType()" works fine (will not skip the body, as it should here). OK, so now I have a rough idea what the "Contained" was referring too :)

Yep, getContainedDeducedType() call is definitely necessary. Added the tests for discussed cases in rL333735.

lib/Sema/SemaDecl.cpp

 }
 
 bool Sema::canSkipFunctionBody(Decl *D) {
   // We cannot skip the body of a function (or function template) which is
   // constexpr, since we may need to evaluate its body in order to parse the
   // rest of the file.
   // We cannot skip the body of a function with an undeduced return type,
   // because any callers of that function need to know the type.
-  if (const FunctionDecl *FD = D->getAsFunction())
-    if (FD->isConstexpr() || FD->getReturnType()->isUndeducedType())
+  if (const FunctionDecl *FD = D->getAsFunction()) {
+    if (FD->isConstexpr())
       return false;
+    // We can't simply call Type::isUndeducedType here, because inside template
+    // auto can be deduced to a dependent type, which is not considered
+    // "undeduced".
+    if (FD->getReturnType()->getContainedDeducedType())
+      return false;
+  }
   return Consumer.shouldSkipFunctionBody(D);
 }
 
 Decl *Sema::ActOnSkippedFunctionBody(Decl *Decl) {
   if (!Decl)
     return nullptr;
   if (FunctionDecl *FD = Decl->getAsFunction())
     FD->setHasSkippedBody();

test/CodeCompletion/crash-skipped-bodies-template-inst.cpp

+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:24:5 %s -o - 2>&1 | FileCheck %s
+template <class T>
+auto make_func() {
+  struct impl {
+    impl* func() {
+      int x;
+      if (x = 10) {}
+      // Check that body of this function is actually skipped.
+      // CHECK-NOT: crash-skipped-bodies-template-inst.cpp:7:{{[0-9]+}}: warning: using the result of an assignment as a condition without parentheses
+      return this;
+    }
+  };
+
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: crash-skipped-bodies-template-inst.cpp:15:9: warning: using the result of an assignment as a condition without parentheses
+  return impl();
+}
+
+void foo() {
+  []() {
+    make_func<int>();
+    m
+    // CHECK: COMPLETION: make_func : [#auto#]make_func<<#class T#>>()
+  };
+}

test/CodeCompletion/skip-auto-funcs.cpp

+// We run clang in completion mode to force skipping of function bodies and
+// check if the function bodies were skipped by observing the warnings that
+// clang produces.
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:42:1 %s -o - 2>&1 | FileCheck %s
+template <class T>
+auto not_skipped() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: 8:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template <class T>
+auto lambda_not_skipped = []() {
+  int x;
+  if (x = 10) {}
+  // Check that this function is not skipped.
+  // CHECK: 17:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+}
+
+template <class T>
+auto skipped() -> T {
+  int x;
+  if (x = 10) {}
+  // Check that this function is skipped.
+  // CHECK-NOT: 26:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+auto lambda_skipped = []() -> int {
+  int x;
+  if (x = 10) {}
+  // This could potentially be skipped, but it isn't at the moment.
+  // CHECK: 34:9: warning: using the result of an assignment as a condition without parentheses
+  return 1;
+};
+
+int test() {
+  int complete_in_this_function;
+  // CHECK: COMPLETION: complete_in_this_function
+}