[Clang] Do not create dependent CallExpr having UnresolvedLookupExpr inside non-dependent context #124609
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: TilakChad (TilakChad) ChangesThe We attempt to determine if the enclosing function is templated before moving on with the substitution introduced in the 20a0567. This fixes #122892. Full diff: https://github.com/llvm/llvm-project/pull/124609.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 6ae9c51c06b315..b6cb358eb71c8b 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14228,9 +14228,17 @@ ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,
const FunctionDecl *FDecl = Best->Function;
if (FDecl && FDecl->isTemplateInstantiation() &&
FDecl->getReturnType()->isUndeducedType()) {
+
+ // UnresolvedLookupExpr will not be resolved again inside non-dependent
+ // function (i.e non-templated function in this case).
+ const FunctionDecl *EnclosingFn = getCurFunctionDecl();
+ const bool Resolvable =
+ EnclosingFn && EnclosingFn->getTemplatedKind() ==
+ FunctionDecl::TemplatedKind::TK_FunctionTemplate;
+
if (const auto *TP =
FDecl->getTemplateInstantiationPattern(/*ForDefinition=*/false);
- TP && TP->willHaveBody()) {
+ TP && TP->willHaveBody() && Resolvable) {
return CallExpr::Create(Context, Fn, Args, Context.DependentTy,
VK_PRValue, RParenLoc, CurFPFeatureOverrides());
}
diff --git a/clang/test/SemaCXX/deduced-return-type-cxx14.cpp b/clang/test/SemaCXX/deduced-return-type-cxx14.cpp
index c33e07088ba32f..aa62c4a57a6366 100644
--- a/clang/test/SemaCXX/deduced-return-type-cxx14.cpp
+++ b/clang/test/SemaCXX/deduced-return-type-cxx14.cpp
@@ -703,6 +703,48 @@ auto f(auto x) { // cxx14-error {{'auto' not allowed in function prototype}}
return f(1) + 1;
}
+namespace GH122892 {
+ struct NonTemplate {
+ void caller() {
+ c1(int{}); // since-cxx20-error {{cannot be used before it is defined}}
+ c2(int{}); // since-cxx14-error {{cannot be used before it is defined}}
+ }
+
+ static auto c1(auto x) { // since-cxx20-note {{declared here}} // cxx14-error {{'auto' not allowed in function prototype}}
+ }
+
+ template <typename T>
+ static auto c2(T x) { // since-cxx14-note {{declared here}}
+ return x;
+ }
+ };
+
+ struct FunctionTemplateSpecialized {
+ template <typename T>
+ void specialized(){}
+
+ template <>
+ void specialized<int>() {
+ c1(int{}); // since-cxx20-error {{cannot be used before it is defined}}
+ c2(int{}); // since-cxx14-error {{cannot be used before it is defined}}
+ }
+
+ static auto c1(auto x) { // since-cxx20-note {{declared here}} // cxx14-error {{'auto' not allowed in function prototype}}
+ }
+
+ template <typename T>
+ static auto c2(T x) { // since-cxx14-note {{declared here}}
+ return x;
+ }
+ };
+
+ struct MemberInit {
+ int x1 = c1(int{}); // since-cxx20-error {{cannot be used before it is defined}}
+
+ static auto c1(auto x) { return x; } // since-cxx20-note {{declared here}} // cxx14-error {{'auto' not allowed in function prototype}}
+ };
+
+}
}
#if __cplusplus >= 202002L
|
|
This doesn't quite seem right to me... First, I would expect us to check the declaration context here rather than whether it is in a function template. Second, I find myself thinking the |
|
Yeah, it seems more appropriate to check for the dependence of current DeclContext. As for the second part, it seems to happen only when a non-dependent function calls the later defined overloaded member function. Should it compile (I guess not) or be diagnosed before reaching that point? |
|
Hi @erichkeane |
I apparently didn't see your last change since your last comment. Looking now. |
clang/lib/Sema/SemaOverload.cpp
Outdated
| // As there'll be no attempt to resolve UnresolvedLookupExpr again inside | ||
| // non-dependent context, skip considering it as type-dependent. | ||
| const DeclContext *DC = CurContext; | ||
| const bool Resolvable = DC && DC->isDependentContext(); |
There was a problem hiding this comment.
Resolvable isn't a good name here, it sorta implies the opposite of what we mean, but not to the point of NotResolvable being a good idea. I think just putting && CurContext->isDependentContext() below is actually more readable. Also, you don't really have to check the DeclContext, having a null one I think requires that you be outside of a TranslationUnitDecl, which isn't possible.
I see that we are just going through FinishOverloadedCallExpr in the event that we are going to instantiate this again, which I guess makes sense, since this is creating a call with a DependentTy, but this comment doesn't make it clear what i I think I'd be happy with a better commit message explaining the whole situation and why this works. I've debugged a while and think I have a good hold on it though, so just a better description I think would help
…d inside non-dependent context
TilakChad
force-pushed
the
clang-18-regression-unexpected-placeholder
branch
from
2204b25 to
c865ba5
Compare
|
I've incorporated your feedback and updated the PR. Let me know if I have to change anything. |
TilakChad
changed the title
|
This is still missing a release note, but I can merge it for you once you add one. |
@Sirraide |
|
I’ll do that once CI is done |
Co-authored-by: Sirraide <[email protected]>
|
Huh, I wonder why CI’s upset all of a sudden |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/18066 Here is the relevant piece of the build log for the reference |
There was a problem hiding this comment.
I have a few concerns w/ this PR.
Number one we do not cover the original test case which crashes, as far as I can tell none of the tests crash before this fix. This is large oversight.
It looks like currently gcc is the only one that rejects this code as we will now do as well: https://godbolt.org/z/Pqnb4eq6K
I don't see anyone justifying this change in behavior w/ wording from the standard. I would have expected to see such wording references in the summary and also added to a comment in the code itself to document the justification for this change in behavior for future readers of the code.
|
Clang does crash with one of the included test cases, if you try to actually use the function: https://godbolt.org/z/e6e6Ehjoj My reading is that using a static member function with placeholder type before it has been deduced is ill-formed, per https://eel.is/c++draft/dcl.spec.auto#general-13 |
That is not a regression test, at minimum when fixing a bug we should include a test that directly reproduces the bug to ensure that we don't reintroduce the bug again in the future in some subtle different way. |
The
UnresolvedLookupExprdoesn't get looked up and resolved again while it is inside the non-dependent context. It propagates into the codegen phase, causing the assertion failure.We attempt to determine if the current context is dependent before moving on with the substitution introduced in the 20a0567.
This fixes #122892.