Skip to content
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][Sema] Skip the RequiresExprBodyDecls for lambda dependencies #83997

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Mar 5, 2024

The dependency of a lambda inside of a RequiresExprBodyDecl was previously affected by its parent, e.g., ClassTemplateSpecializationDecl. This made the lambda always dependent regardless of the template arguments we had, which caused some crashes on the constraint evaluation later.

This fixes #56556, fixes #82849 and a case demonstrated by #49570 (comment).

@zyn0217 zyn0217 changed the title [Sema] gh56556 [clang][Sema] Skip the RequiresExprBodyDecls for lambda dependencies Mar 6, 2024
@zyn0217 zyn0217 added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Mar 6, 2024
@zyn0217 zyn0217 requested a review from cor3ntin March 6, 2024 06:02
@zyn0217 zyn0217 marked this pull request as ready for review March 6, 2024 06:02
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Mar 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 6, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

The dependency of a lambda inside of a RequiresExprBodyDecl was previously affected by its parent, e.g., ClassTemplateSpecializationDecl. This made the lambda always dependent regardless of the template arguments we had, which caused some crashes on the constraint evaluation later.

This fixes #56556, #82849 and a case demonstrated by #49570 (comment).


Full diff: https://github.com/llvm/llvm-project/pull/83997.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/TreeTransform.h (+21-2)
  • (modified) clang/test/SemaTemplate/concepts-lambda.cpp (+52)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5e0352a7eaf6cd..9c64cd2b5f6a10 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -311,6 +311,8 @@ Bug Fixes to C++ Support
   Fixes (#GH80630)
 - Fix a crash when an explicit template argument list is used with a name for which lookup
   finds a non-template function and a dependent using declarator.
+- Fixed an issue where the ``RequiresExprBody`` was involved in the lambda dependency
+  calculation. (#GH56556), (#GH82849).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 7389a48fe56fcc..84348e13567e71 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -13649,10 +13649,29 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
   // use evaluation contexts to distinguish the function parameter case.
   CXXRecordDecl::LambdaDependencyKind DependencyKind =
       CXXRecordDecl::LDK_Unknown;
+  DeclContext *DC = getSema().CurContext;
+  // A RequiresExprBodyDecl is not interesting for dependencies.
+  // For the following case,
+  //
+  // template <typename>
+  // concept C = requires { [] {}; };
+  //
+  // template <class F>
+  // struct Widget;
+  //
+  // template <C F>
+  // struct Widget<F> {};
+  //
+  // While we are here in substitution for Widget<F>, the parent of DC would be
+  // the template specialization itself. Thus, the lambda expression
+  // will be deemed as dependent even if we have non-dependent template
+  // arguments.
+  // (A ClassTemplateSpecializationDecl is always a dependent context.)
+  if (DC->getDeclKind() == Decl::Kind::RequiresExprBody)
+    DC = DC->getParent();
   if ((getSema().isUnevaluatedContext() ||
        getSema().isConstantEvaluatedContext()) &&
-      (getSema().CurContext->isFileContext() ||
-       !getSema().CurContext->getParent()->isDependentContext()))
+      (DC->isFileContext() || !DC->getParent()->isDependentContext()))
     DependencyKind = CXXRecordDecl::LDK_NeverDependent;
 
   CXXRecordDecl *OldClass = E->getLambdaClass();
diff --git a/clang/test/SemaTemplate/concepts-lambda.cpp b/clang/test/SemaTemplate/concepts-lambda.cpp
index 0b7580f91043c7..ef04cad4eef98b 100644
--- a/clang/test/SemaTemplate/concepts-lambda.cpp
+++ b/clang/test/SemaTemplate/concepts-lambda.cpp
@@ -90,6 +90,58 @@ struct Foo {
 
 static_assert(ConstructibleWithN<Foo>);
 
+namespace GH56556 {
+
+template <typename It>
+inline constexpr It declare ();
+
+template <typename It, template <typename> typename Template>
+concept D = requires {
+	{ [] <typename T1> (Template<T1> &) {}(declare<It &>()) };
+};
+
+template <typename T>
+struct B {};
+
+template <typename T>
+struct Adapter;
+
+template <D<B> T>
+struct Adapter<T> {};
+
+template struct Adapter<B<int>>;
+
+} // namespace GH56556
+
+namespace GH82849 {
+
+template <class T>
+concept C = requires(T t) {
+  [](T) {}(t);
+};
+
+template <class From>
+struct Widget;
+
+template <C F>
+struct Widget<F> {
+  static F create(F from) {
+    return from;
+  }
+};
+
+template <class>
+bool foo() {
+  return C<int>;
+}
+
+void bar() {
+  // https://github.com/llvm/llvm-project/issues/49570#issuecomment-1664966972
+  Widget<char>::create(0);
+}
+
+} // namespace GH82849
+
 }
 
 // GH60642 reported an assert being hit, make sure we don't assert.

Comment on lines 13670 to 13671
if (DC->getDeclKind() == Decl::Kind::RequiresExprBody)
DC = DC->getParent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get into the same issue if there are multiple nested requires?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this should be a while loop, I think

clang/lib/Sema/TreeTransform.h Outdated Show resolved Hide resolved
clang/lib/Sema/TreeTransform.h Outdated Show resolved Hide resolved
@zyn0217
Copy link
Contributor Author

zyn0217 commented Mar 6, 2024

@cor3ntin Thanks! I have updated the test to reflect the nested requires cases. PTAL?

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@yuxuanchen1997 yuxuanchen1997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! This looks good.

@alexfh
Copy link
Contributor

alexfh commented Mar 13, 2024

Hi folks, we've started seeing "implicit instantiation of undefined template" compilation errors after this commit. In all cases code used to compile fine before this commit. I haven't spotted any particular issues in the code (though the examples are looked at are rather convoluted, thus, I'm not 100% sure). I'll try to prepare a self-sufficient test case.

Was this sort of a behavior change expected?

@zyn0217
Copy link
Contributor Author

zyn0217 commented Mar 13, 2024

Was this sort of a behavior change expected?

I don't think so; a reproducer would be really appreciated.

@alexfh
Copy link
Contributor

alexfh commented Mar 13, 2024

I'm reducing the test case. In the meantime, should this be reverted?

@zyn0217
Copy link
Contributor Author

zyn0217 commented Mar 13, 2024

I'm reducing the test case. In the meantime, should this be reverted?

I think this depends on the case. Note the issues addressed by this patch may not only cause some crashes in debug build, but probably lead to incorrect constraint evaluation in release build, which means there's possibility that your case was an accept-invalid before.

(Sorry for making confusion earlier; my office hour just begins and I'm sitting here for your case. If there is really something wrong caused by this PR, I'll revert it in time. Don't worry.)

@alexfh
Copy link
Contributor

alexfh commented Mar 13, 2024

I have a reduced test case, which is accepted by clang 18, but fails with this patch: https://gcc.godbolt.org/z/zMbKvsf7K

@alexfh
Copy link
Contributor

alexfh commented Mar 13, 2024

A bit more compact one: https://gcc.godbolt.org/z/4rzYPKaze

@jyknight
Copy link
Member

Can reduce further to:

template <typename T>
concept h = requires(T i) { [] {}(i); };

template <typename T> struct k;
template <h m> struct k<m> {
  struct n;
};
using o = k<int>::n;

But, is requires(T i) { [] {}(i); }; actually valid? I think that should fail the requirement, since you cannot call that lambda with an argument? Maybe that's just an artifact of the reduction?

@dwblaikie
Copy link
Collaborator

FWIW, @jyknight's example fails to compile with GCC - succeeds with clang 18 release but assert-fails with clang 18 +Asserts build. (not sure about the original/full internal reproduction - we have a way to run compile with assertions enaled, but I'm not sure I'm holding it right... )

@zyn0217
Copy link
Contributor Author

zyn0217 commented Mar 14, 2024

I have a reduced test case, which is accepted by clang 18, but fails with this patch: https://gcc.godbolt.org/z/zMbKvsf7K
A bit more compact one: https://gcc.godbolt.org/z/4rzYPKaze

Your case is rejected by all mainstream compilers as of now: https://gcc.godbolt.org/z/nrMf3zvfY
Again, it was an accept-invalid before because of the constant evaluation on dependent expressions, which is an assertion failure if you compile it with debug mode. https://gcc.godbolt.org/z/eascd7j1G

To clarify why the requirement should be evaluated to false:

The enclosing requires-expression will evaluate to false if substitution of template arguments into the expression fails.

https://eel.is/c++draft/expr.prim.req#simple-1.sentence-2

@alexfh
Copy link
Contributor

alexfh commented Mar 14, 2024

I guess the reduction could have dropped some important parts of this. Let me try the original code with assertions-enabled clang build...

@alexfh
Copy link
Contributor

alexfh commented Mar 14, 2024

It turns out debug build of clang before this patch generated an assertion on the original code as well:

assert.h assertion failed at llvm-project/clang/lib/AST/ExprConstant.cpp:15739 in bool clang::Expr::EvaluateAsConstantExpr(EvalResult &, const ASTContext &, ConstantExprKind) const: !isValueDependent() && "Expression evaluator can't be called on a dependent expression."
...
    @     0x5643bcdac9e4  __assert_fail
    @     0x5643b9734df7  clang::Expr::EvaluateAsConstantExpr()
    @     0x5643b89f7fea  calculateConstraintSatisfaction<>()
    @     0x5643b89f1bcf  CheckConstraintSatisfaction()
    @     0x5643b89f193e  clang::Sema::CheckConstraintSatisfaction()
    @     0x5643b90d9f23  clang::Sema::CheckConceptTemplateId()
    @     0x5643b9259656  clang::TreeTransform<>::TransformConceptSpecializationExpr()
    @     0x5643b925ce00  clang::TreeTransform<>::TransformCXXFoldExpr()
    @     0x5643b924b6e0  clang::Sema::SubstConstraintExpr()
    @     0x5643b89f9097  calculateConstraintSatisfaction()::$_0::operator()()
    @     0x5643b89f7f50  calculateConstraintSatisfaction<>()
    @     0x5643b89f1bcf  CheckConstraintSatisfaction()
    @     0x5643b89f193e  clang::Sema::CheckConstraintSatisfaction()
    @     0x5643b9237c6d  FinishTemplateArgumentDeduction<>()
    @     0x5643b9236cd2  llvm::function_ref<>::callback_fn<>()
    @     0x5643b888fa2f  clang::Sema::runWithSufficientStackSpace()
    @     0x5643b91c2e17  clang::Sema::DeduceTemplateArguments()
    @     0x5643b9248e19  clang::Sema::InstantiateClassTemplateSpecialization()
    @     0x5643b9334e73  llvm::function_ref<>::callback_fn<>()
    @     0x5643b888fa2f  clang::Sema::runWithSufficientStackSpace()
    @     0x5643b931fdc4  clang::Sema::RequireCompleteTypeImpl()
    @     0x5643b931f535  clang::Sema::RequireCompleteType()
    @     0x5643b891bad7  clang::Sema::RequireCompleteDeclContext()
    @     0x5643b90eda4d  clang::Sema::CheckTypenameType()
    @     0x5643b926edd2  clang::TreeTransform<>::TransformDependentNameType()
    @     0x5643b9243d1c  clang::TreeTransform<>::TransformType()
    @     0x5643b92436ff  clang::TreeTransform<>::TransformType()
    @     0x5643b9244293  clang::Sema::SubstType()
    @     0x5643b90d0e8a  clang::Sema::CheckTemplateIdType()
    @     0x5643b90d5015  clang::Sema::ActOnTemplateIdType()
    @     0x5643b8674084  clang::Parser::AnnotateTemplateIdTokenAsType()
    @     0x5643b86536cf  clang::Parser::ParseDeclarationSpecifiers()
    @     0x5643b864fffa  clang::Parser::ParseSpecifierQualifierList()
    @     0x5643b863da31  clang::Parser::ParseTypeName()
    @     0x5643b86262bd  clang::Parser::ParseAliasDeclarationAfterDeclarator()
    @     0x5643b8624ea0  clang::Parser::ParseUsingDeclaration()
    @     0x5643b8623e70  clang::Parser::ParseUsingDirectiveOrDeclaration()
    @     0x5643b864b6ef  clang::Parser::ParseDeclaration()
    @     0x5643b868a9a4  clang::Parser::ParseStatementOrDeclarationAfterAttributes()
    @     0x5643b8689d78  clang::Parser::ParseStatementOrDeclaration()
    @     0x5643b8693bc0  clang::Parser::ParseCompoundStatementBody()
    @     0x5643b8694bce  clang::Parser::ParseFunctionStatementBody()
    @     0x5643b85da8ad  clang::Parser::ParseFunctionDefinition()
    @     0x5643b864df89  clang::Parser::ParseDeclGroup()
    @     0x5643b85d8ec9  clang::Parser::ParseDeclOrFunctionDefInternal()
    @     0x5643b85d868e  clang::Parser::ParseDeclarationOrFunctionDefinition()
    @     0x5643b85d7434  clang::Parser::ParseExternalDeclaration()
    @     0x5643b8622693  clang::Parser::ParseInnerNamespace()
    @     0x5643b8621893  clang::Parser::ParseNamespace()
    @     0x5643b864b7de  clang::Parser::ParseDeclaration()
    @     0x5643b85d6fb6  clang::Parser::ParseExternalDeclaration()
    @     0x5643b85d516b  clang::Parser::ParseTopLevelDecl()
    @     0x5643b85ceebe  clang::ParseAST()
    @     0x5643b831a4c3  clang::FrontendAction::Execute()
    @     0x5643b8294efd  clang::CompilerInstance::ExecuteAction()
    @     0x5643b717f84e  clang::ExecuteCompilerInvocation()
    @     0x5643b71737c6  cc1_main()
    @     0x5643b7171066  ExecuteCC1Tool()
    @     0x5643b843fe3e  llvm::function_ref<>::callback_fn<>()
    @     0x5643bcc44415  llvm::CrashRecoveryContext::RunSafely()
    @     0x5643b843f5bb  clang::driver::CC1Command::Execute()
    @     0x5643b8400494  clang::driver::Compilation::ExecuteCommand()
    @     0x5643b84007af  clang::driver::Compilation::ExecuteJobs()
    @     0x5643b841ea80  clang::driver::Driver::ExecuteCompilation()
    @     0x5643b717061b  clang_main()
    @     0x5643b716d834  main

I'll try to figure out what's wrong with the original code.

@alexfh
Copy link
Contributor

alexfh commented Mar 15, 2024

The internal code was fixed. Thanks everyone for helping to figure this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
7 participants