-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang] Accept recursive non-dependent calls to functions with deduced return type #75456
Conversation
…d return type Treat such calls as dependent since it is much easier to implement. Fixes llvm#71015
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesTreat such calls as dependent since it is much easier to implement. Fixes #71015 Full diff: https://github.com/llvm/llvm-project/pull/75456.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 05d59d0da264f3..9ffc7500414981 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -688,6 +688,9 @@ Bug Fixes in This Version
- Fixed false positive error emitted when templated alias inside a class
used private members of the same class.
Fixes (`#41693 <https://github.com/llvm/llvm-project/issues/41693>`_)
+- Clang now accepts recursive non-dependent calls to functions with deduced return
+ type.
+ Fixes (`#71015 <https://github.com/llvm/llvm-project/issues/71015>`_)
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/ComputeDependence.cpp b/clang/lib/AST/ComputeDependence.cpp
index 097753fd3267b5..584b58473294be 100644
--- a/clang/lib/AST/ComputeDependence.cpp
+++ b/clang/lib/AST/ComputeDependence.cpp
@@ -603,6 +603,8 @@ ExprDependence clang::computeDependence(PredefinedExpr *E) {
ExprDependence clang::computeDependence(CallExpr *E,
llvm::ArrayRef<Expr *> PreArgs) {
auto D = E->getCallee()->getDependence();
+ if (E->getType()->isDependentType())
+ D |= ExprDependence::Type;
for (auto *A : llvm::ArrayRef(E->getArgs(), E->getNumArgs())) {
if (A)
D |= A->getDependence();
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 5026e1d603e5ee..9fb767101e1eb7 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -13994,6 +13994,24 @@ ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,
OverloadCandidateSet::iterator Best;
OverloadingResult OverloadResult =
CandidateSet.BestViableFunction(*this, Fn->getBeginLoc(), Best);
+ FunctionDecl *FDecl = Best->Function;
+
+ // Model the case with a call to a templated function whose definition
+ // encloses the call and whose return type contains a placeholder type as if
+ // the UnresolvedLookupExpr was type-dependent.
+ if (OverloadResult == OR_Success && FDecl &&
+ FDecl->isTemplateInstantiation() &&
+ FDecl->getReturnType()->isUndeducedType()) {
+ if (auto TP = FDecl->getTemplateInstantiationPattern(false)) {
+ if (TP->willHaveBody()) {
+ CallExpr *CE =
+ CallExpr::Create(Context, Fn, Args, Context.DependentTy, VK_PRValue,
+ RParenLoc, CurFPFeatureOverrides());
+ result = CE;
+ return result;
+ }
+ }
+ }
return FinishOverloadedCallExpr(*this, S, Fn, ULE, LParenLoc, Args, RParenLoc,
ExecConfig, &CandidateSet, &Best,
diff --git a/clang/test/SemaCXX/deduced-return-type-cxx14.cpp b/clang/test/SemaCXX/deduced-return-type-cxx14.cpp
index 6344d1df3fbaeb..1da597499d34f5 100644
--- a/clang/test/SemaCXX/deduced-return-type-cxx14.cpp
+++ b/clang/test/SemaCXX/deduced-return-type-cxx14.cpp
@@ -640,3 +640,36 @@ namespace PR46637 {
template<typename T> struct Y { T x; };
Y<auto() -> auto> y; // expected-error {{'auto' not allowed in template argument}}
}
+
+namespace GH71015 {
+
+// Check that there is no error in case a templated function is recursive and
+// has a placeholder return type.
+struct Node {
+ int value;
+ Node* left;
+ Node* right;
+};
+
+bool parse(const char*);
+Node* parsePrimaryExpr();
+
+auto parseMulExpr(auto node) { // cxx14-error {{'auto' not allowed in function prototype}}
+ if (node == nullptr) node = parsePrimaryExpr();
+ if (!parse("*")) return node;
+ return parseMulExpr(new Node{.left = node, .right = parsePrimaryExpr()});
+}
+
+template <typename T>
+auto parseMulExpr2(T node) {
+ if (node == nullptr) node = parsePrimaryExpr();
+ if (!parse("*")) return node;
+ return parseMulExpr2(new Node{.left = node, .right = parsePrimaryExpr()});
+}
+
+auto f(auto x) { // cxx14-error {{'auto' not allowed in function prototype}}
+ if (x == 0) return 0;
+ return f(1) + 1;
+}
+
+}
|
This attempts to implement the approach described by @zygoloid in #71015 (comment) . |
Thanks @erichkeane for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
auto parseMulExpr(auto node) { // cxx14-error {{'auto' not allowed in function prototype}} | ||
if (node == nullptr) node = parsePrimaryExpr(); | ||
if (!parse("*")) return node; | ||
return parseMulExpr(new Node{.left = node, .right = parsePrimaryExpr()}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to add a test that this is rejected without the previous line retuen node;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the test.
A note here is that It will only be rejected if the function instantiated because clang doesn't take into account return statements if the function is templated due to this part:
llvm-project/clang/lib/Sema/SemaStmt.cpp
Line 3806 in 1127656
// C++1y [dcl.spec.auto]p12: |
gcc rejects even if the function is not instantiated. msvc doesn't. https://compiler-explorer.com/z/hGc8M1esT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine, and it's what I expected. While we could try to reject in the template definition if we see a recursive call prior to the end of the first return
statement, I don't think it's worth the added complexity.
I think what GCC's doing here is that it's treating the recursive call as dependent in much the same way that Clang does with this patch, but then performing an instantiation of the function template specialization parseMulExpr<Node*>
after it finishes processing the template definition -- so it's also not eagerly diagnosing the problem. (Clang suppresses performing recursive non-dependent function template instantiations until the template has a point of instantiation outside itself, which GCC doesn't seem to do.) But I could be wrong.
Treat such calls as dependent since it is much easier to implement.
Fixes #71015