[Clang] Bugfixes and improved support for AttributedTypes in lambdas
#85325
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
--------- Co-authored-by: Yuxuan Chen <[email protected]>
--------- Co-authored-by: Doug Wyatt <[email protected]>
Sirraide
added
clang:frontend
|
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesSummaryThis pr fixes a crash when we attempt to instantiate a lambda with an The background for this is that, while working on #84983, the author observed that CrashThe cause of the crash was the following: When instantiating the As a fix, we now only instantiate the The reason for this is that the former creates a new Instead of doing any of that, Note: There already is a pr (#78801) that makes changes similar to the ones in this pr wrt refactoring tree transform only, but that pr hasn’t received any updates for over a month, and though the author has reached out to me saying that they’d be happy to help (which I definitely appreciate), I’ve still opted to instead add (some of) their changes to this pr instead—simply because this pr also does more than just address this issue, but trying to split the fixes up into separate prs doesn’t really work too well imo (for instance, there is no testing if Source FidelityWhen rebuilding function types in Side note: I know this doesn’t really belong in this pr, but I’ve also added a This fixes #85120 and fixes #85154. Patch is 20.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85325.diff 9 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index ff6b64c7f72d57..46165f4d4fd547 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1295,6 +1295,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
const FunctionType *adjustFunctionType(const FunctionType *Fn,
FunctionType::ExtInfo EInfo);
+ /// Change the result type of a function type, preserving sugar such as
+ /// attributed types.
+ QualType adjustFunctionResultType(QualType FunctionType,
+ QualType NewResultType);
+
/// Adjust the given function result type.
CanQualType getCanonicalFunctionResultType(QualType ResultType) const;
diff --git a/clang/include/clang/AST/Attr.h b/clang/include/clang/AST/Attr.h
index 8e9b7ad8b46826..6400023947863f 100644
--- a/clang/include/clang/AST/Attr.h
+++ b/clang/include/clang/AST/Attr.h
@@ -112,6 +112,8 @@ class Attr : public AttributeCommonInfo {
// Pretty print this attribute.
void printPretty(raw_ostream &OS, const PrintingPolicy &Policy) const;
+ void dump() const;
+
static StringRef getDocumentation(attr::Kind);
};
diff --git a/clang/include/clang/Sema/Template.h b/clang/include/clang/Sema/Template.h
index ce44aca797b0fb..8c379f51ca3d5d 100644
--- a/clang/include/clang/Sema/Template.h
+++ b/clang/include/clang/Sema/Template.h
@@ -411,6 +411,11 @@ enum class TemplateSubstitutionKind : char {
/// lookup will search our outer scope.
bool CombineWithOuterScope;
+ /// Whether this scope is being used to instantiate a lambda expression,
+ /// in which case it should be reused for instantiating the lambda's
+ /// FunctionProtoType.
+ bool InstantiatingLambda = false;
+
/// If non-NULL, the template parameter pack that has been
/// partially substituted per C++0x [temp.arg.explicit]p9.
NamedDecl *PartiallySubstitutedPack = nullptr;
@@ -425,9 +430,11 @@ enum class TemplateSubstitutionKind : char {
unsigned NumArgsInPartiallySubstitutedPack;
public:
- LocalInstantiationScope(Sema &SemaRef, bool CombineWithOuterScope = false)
+ LocalInstantiationScope(Sema &SemaRef, bool CombineWithOuterScope = false,
+ bool InstantiatingLambda = false)
: SemaRef(SemaRef), Outer(SemaRef.CurrentInstantiationScope),
- CombineWithOuterScope(CombineWithOuterScope) {
+ CombineWithOuterScope(CombineWithOuterScope),
+ InstantiatingLambda(InstantiatingLambda) {
SemaRef.CurrentInstantiationScope = this;
}
@@ -553,6 +560,9 @@ enum class TemplateSubstitutionKind : char {
/// Determine whether D is a pack expansion created in this scope.
bool isLocalPackExpansion(const Decl *D);
+
+ /// Determine whether this scope is for instantiating a lambda.
+ bool isLambda() const { return InstantiatingLambda; }
};
class TemplateDeclInstantiator
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 5a8fae76a43a4d..e9f31047c9caef 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -3140,13 +3140,35 @@ const FunctionType *ASTContext::adjustFunctionType(const FunctionType *T,
return cast<FunctionType>(Result.getTypePtr());
}
+QualType ASTContext::adjustFunctionResultType(QualType FunctionType,
+ QualType ResultType) {
+ // Might be wrapped in a macro qualified type.
+ if (const auto *MQT = dyn_cast<MacroQualifiedType>(FunctionType)) {
+ return getMacroQualifiedType(
+ adjustFunctionResultType(MQT->getUnderlyingType(), ResultType),
+ MQT->getMacroIdentifier());
+ }
+
+ // Might have a calling-convention attribute.
+ if (const auto *AT = dyn_cast<AttributedType>(FunctionType)) {
+ return getAttributedType(
+ AT->getAttrKind(),
+ adjustFunctionResultType(AT->getModifiedType(), ResultType),
+ adjustFunctionResultType(AT->getEquivalentType(), ResultType));
+ }
+
+ // Anything else must be a function type. Rebuild it with the new return
+ // value.
+ const auto *FPT = FunctionType->castAs<FunctionProtoType>();
+ return getFunctionType(ResultType, FPT->getParamTypes(),
+ FPT->getExtProtoInfo());
+}
+
void ASTContext::adjustDeducedFunctionResultType(FunctionDecl *FD,
QualType ResultType) {
FD = FD->getMostRecentDecl();
while (true) {
- const auto *FPT = FD->getType()->castAs<FunctionProtoType>();
- FunctionProtoType::ExtProtoInfo EPI = FPT->getExtProtoInfo();
- FD->setType(getFunctionType(ResultType, FPT->getParamTypes(), EPI));
+ FD->setType(adjustFunctionResultType(FD->getType(), ResultType));
if (FunctionDecl *Next = FD->getPreviousDecl())
FD = Next;
else
diff --git a/clang/lib/AST/ASTDumper.cpp b/clang/lib/AST/ASTDumper.cpp
index 6efc5bb92e28d2..8d8b8621341ef7 100644
--- a/clang/lib/AST/ASTDumper.cpp
+++ b/clang/lib/AST/ASTDumper.cpp
@@ -361,3 +361,11 @@ LLVM_DUMP_METHOD void ConceptReference::dump(raw_ostream &OS) const {
ASTDumper P(OS, Ctx, Ctx.getDiagnostics().getShowColors());
P.Visit(this);
}
+
+//===----------------------------------------------------------------------===//
+// Attr method implementations
+//===----------------------------------------------------------------------===//
+LLVM_DUMP_METHOD void Attr::dump() const {
+ ASTDumper P(llvm::errs(), /*ShowColors=*/false);
+ P.Visit(this);
+}
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 1a0c88703aca01..6c753c9956bddb 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1535,7 +1535,8 @@ namespace {
bool SuppressObjCLifetime);
ExprResult TransformLambdaExpr(LambdaExpr *E) {
- LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
+ LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true,
+ /*InstantiatingLambda=*/true);
Sema::ConstraintEvalRAII<TemplateInstantiator> RAII(*this);
ExprResult Result = inherited::TransformLambdaExpr(E);
@@ -2276,8 +2277,14 @@ QualType TemplateInstantiator::TransformFunctionProtoType(TypeLocBuilder &TLB,
CXXRecordDecl *ThisContext,
Qualifiers ThisTypeQuals,
Fn TransformExceptionSpec) {
- // We need a local instantiation scope for this function prototype.
- LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
+ // If this is a lambda, then TemplateInstantiator::TransformLambdaExpr will
+ // have already pushed a scope for this prototype, so don't create a second
+ // one. Otherwise, push a new instantiation scope.
+ LocalInstantiationScope *Current = getSema().CurrentInstantiationScope;
+ std::optional<LocalInstantiationScope> Scope;
+ if (!Current || !Current->isLambda())
+ Scope.emplace(SemaRef, /*CombineWithOuterScope=*/true);
+
return inherited::TransformFunctionProtoType(
TLB, TL, ThisContext, ThisTypeQuals, TransformExceptionSpec);
}
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 2d22692f3ab750..b1a8fd75a90712 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -675,10 +675,6 @@ class TreeTransform {
Qualifiers ThisTypeQuals,
Fn TransformExceptionSpec);
- template <typename Fn>
- QualType TransformAttributedType(TypeLocBuilder &TLB, AttributedTypeLoc TL,
- Fn TransformModifiedType);
-
bool TransformExceptionSpec(SourceLocation Loc,
FunctionProtoType::ExceptionSpecInfo &ESI,
SmallVectorImpl<QualType> &Exceptions,
@@ -7212,11 +7208,10 @@ TreeTransform<Derived>::TransformElaboratedType(TypeLocBuilder &TLB,
}
template <typename Derived>
-template <typename Fn>
-QualType TreeTransform<Derived>::TransformAttributedType(
- TypeLocBuilder &TLB, AttributedTypeLoc TL, Fn TransformModifiedTypeFn) {
+QualType TreeTransform<Derived>::TransformAttributedType(TypeLocBuilder &TLB,
+ AttributedTypeLoc TL) {
const AttributedType *oldType = TL.getTypePtr();
- QualType modifiedType = TransformModifiedTypeFn(TLB, TL.getModifiedLoc());
+ QualType modifiedType = getDerived().TransformType(TLB, TL.getModifiedLoc());
if (modifiedType.isNull())
return QualType();
@@ -7231,12 +7226,16 @@ QualType TreeTransform<Derived>::TransformAttributedType(
// FIXME: dependent operand expressions?
if (getDerived().AlwaysRebuild() ||
modifiedType != oldType->getModifiedType()) {
- TypeLocBuilder AuxiliaryTLB;
- AuxiliaryTLB.reserve(TL.getFullDataSize());
- QualType equivalentType =
- getDerived().TransformType(AuxiliaryTLB, TL.getEquivalentTypeLoc());
- if (equivalentType.isNull())
- return QualType();
+ // Do not transform the equivalent type if it is equal to the modified type.
+ QualType equivalentType = modifiedType;
+ if (TL.getModifiedLoc().getType() != TL.getEquivalentTypeLoc().getType()) {
+ TypeLocBuilder AuxiliaryTLB;
+ AuxiliaryTLB.reserve(TL.getFullDataSize());
+ equivalentType =
+ getDerived().TransformType(AuxiliaryTLB, TL.getEquivalentTypeLoc());
+ if (equivalentType.isNull())
+ return QualType();
+ }
// Check whether we can add nullability; it is only represented as
// type sugar, and therefore cannot be diagnosed in any other way.
@@ -7260,15 +7259,6 @@ QualType TreeTransform<Derived>::TransformAttributedType(
return result;
}
-template <typename Derived>
-QualType TreeTransform<Derived>::TransformAttributedType(TypeLocBuilder &TLB,
- AttributedTypeLoc TL) {
- return getDerived().TransformAttributedType(
- TLB, TL, [&](TypeLocBuilder &TLB, TypeLoc ModifiedLoc) -> QualType {
- return getDerived().TransformType(TLB, ModifiedLoc);
- });
-}
-
template <typename Derived>
QualType TreeTransform<Derived>::TransformBTFTagAttributedType(
TypeLocBuilder &TLB, BTFTagAttributedTypeLoc TL) {
@@ -13830,62 +13820,40 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
getSema().AddTemplateParametersToLambdaCallOperator(NewCallOperator, Class,
TPL);
- // Transform the type of the original lambda's call operator.
- // The transformation MUST be done in the CurrentInstantiationScope since
- // it introduces a mapping of the original to the newly created
- // transformed parameters.
TypeSourceInfo *NewCallOpTSI = nullptr;
- {
- auto OldCallOpTypeLoc =
- E->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
-
- auto TransformFunctionProtoTypeLoc =
- [this](TypeLocBuilder &TLB, FunctionProtoTypeLoc FPTL) -> QualType {
- SmallVector<QualType, 4> ExceptionStorage;
- return this->TransformFunctionProtoType(
- TLB, FPTL, nullptr, Qualifiers(),
- [&](FunctionProtoType::ExceptionSpecInfo &ESI, bool &Changed) {
- return TransformExceptionSpec(FPTL.getBeginLoc(), ESI,
- ExceptionStorage, Changed);
- });
- };
-
- QualType NewCallOpType;
- TypeLocBuilder NewCallOpTLBuilder;
-
- if (auto ATL = OldCallOpTypeLoc.getAs<AttributedTypeLoc>()) {
- NewCallOpType = this->TransformAttributedType(
- NewCallOpTLBuilder, ATL,
- [&](TypeLocBuilder &TLB, TypeLoc TL) -> QualType {
- return TransformFunctionProtoTypeLoc(
- TLB, TL.castAs<FunctionProtoTypeLoc>());
- });
- } else {
- auto FPTL = OldCallOpTypeLoc.castAs<FunctionProtoTypeLoc>();
- NewCallOpType = TransformFunctionProtoTypeLoc(NewCallOpTLBuilder, FPTL);
- }
+ auto OldCallOpTypeLoc =
+ E->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
- if (NewCallOpType.isNull())
- return ExprError();
- NewCallOpTSI =
- NewCallOpTLBuilder.getTypeSourceInfo(getSema().Context, NewCallOpType);
- }
+ QualType NewCallOpType;
+ TypeLocBuilder NewCallOpTLBuilder;
- ArrayRef<ParmVarDecl *> Params;
- if (auto ATL = NewCallOpTSI->getTypeLoc().getAs<AttributedTypeLoc>()) {
- Params = ATL.getModifiedLoc().castAs<FunctionProtoTypeLoc>().getParams();
- } else {
- auto FPTL = NewCallOpTSI->getTypeLoc().castAs<FunctionProtoTypeLoc>();
- Params = FPTL.getParams();
- }
+ NewCallOpType =
+ getDerived().TransformType(NewCallOpTLBuilder, OldCallOpTypeLoc);
+ if (NewCallOpType.isNull())
+ return ExprError();
+ NewCallOpTSI =
+ NewCallOpTLBuilder.getTypeSourceInfo(getSema().Context, NewCallOpType);
+
+ auto ExtractParams = [](TypeLoc TL) {
+ auto Impl = [](auto Self, TypeLoc TL) -> ArrayRef<ParmVarDecl *> {
+ if (auto FPTL = TL.getAs<FunctionProtoTypeLoc>())
+ return FPTL.getParams();
+ if (auto ATL = TL.getAs<AttributedTypeLoc>())
+ return Self(Self, ATL.getModifiedLoc());
+ if (auto MQTL = TL.getAs<MacroQualifiedTypeLoc>())
+ return Self(Self, MQTL.getInnerLoc());
+ llvm_unreachable("Unhandled TypeLoc");
+ };
+ return Impl(Impl, TL);
+ };
getSema().CompleteLambdaCallOperator(
NewCallOperator, E->getCallOperator()->getLocation(),
E->getCallOperator()->getInnerLocStart(),
E->getCallOperator()->getTrailingRequiresClause(), NewCallOpTSI,
E->getCallOperator()->getConstexprKind(),
- E->getCallOperator()->getStorageClass(), Params,
- E->hasExplicitResultType());
+ E->getCallOperator()->getStorageClass(),
+ ExtractParams(NewCallOpTSI->getTypeLoc()), E->hasExplicitResultType());
getDerived().transformAttrs(E->getCallOperator(), NewCallOperator);
getDerived().transformedLocalDecl(E->getCallOperator(), {NewCallOperator});
diff --git a/clang/test/SemaCXX/lambda-attributes.cpp b/clang/test/SemaCXX/lambda-attributes.cpp
new file mode 100644
index 00000000000000..799649719cf42b
--- /dev/null
+++ b/clang/test/SemaCXX/lambda-attributes.cpp
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -std=c++23 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++23 -fsyntax-only -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++23 -triple x86_64-pc-linux -emit-pch -o %t %s
+// RUN: %clang_cc1 -x c++ -std=c++23 -triple x86_64-pc-linux -include-pch %t -ast-dump-all /dev/null | FileCheck %s
+// expected-no-diagnostics
+
+// Check that we both don't crash on transforming FunctionProtoType's
+// wrapped in type sugar and that we don't drop it when performing
+// instantiations either.
+
+#define PRESERVE __attribute__((preserve_most))
+
+// Skip to the instantiation of f().
+// CHECK: FunctionDecl {{.*}} f 'void ()' implicit_instantiation
+template <typename T>
+void f() {
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const __attribute__((preserve_most))':'void (int) __attribute__((preserve_most)) const' implicit_instantiation
+ (void) [] (T) __attribute__((preserve_most)) { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const {{\[}}[clang::annotate_type(...)]]':'void (int) const' implicit_instantiation
+ (void) [] (T) [[clang::annotate_type("foo")]] { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const {{\[}}[clang::annotate_type(...)]] {{\[}}[clang::annotate_type(...)]] {{\[}}[clang::annotate_type(...)]]':'void (int) const' implicit_instantiation
+ (void) [] (T) [[clang::annotate_type("foo")]]
+ [[clang::annotate_type("foo")]]
+ [[clang::annotate_type("foo")]] { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const __attribute__((preserve_most)) {{\[}}[clang::annotate_type(...)]]':'void (int) __attribute__((preserve_most)) const' implicit_instantiation
+ (void) [] (T) __attribute__((preserve_most))
+ [[clang::annotate_type("foo")]] { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const __attribute__((cdecl)) {{\[}}[clang::annotate_type(...)]]':'void (int) const' implicit_instantiation
+ (void) [] (T) __attribute__((cdecl))
+ [[clang::annotate_type("foo")]] { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const {{\[}}[clang::annotate_type(...)]]':'void (int) const' implicit_instantiation
+ (void) [] (T t) [[clang::annotate_type("foo", t)]] { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const __attribute__((preserve_most)) {{\[}}[clang::annotate_type(...)]]':'void (int) __attribute__((preserve_most)) const' implicit_instantiation
+ (void) [] (T t) __attribute__((preserve_most))
+ [[clang::annotate_type("foo", t, t, t, t)]] { };
+
+ // Check that the MacroQualifiedType is preserved.
+ // CHECK: CXXMethodDecl {{.*}} operator() 'PRESERVE void (int) __attribute__((preserve_most)) const':'void (int) __attribute__((preserve_most)) const' implicit_instantiation
+ (void) [] (T) PRESERVE { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'PRESERVE void (int) __attribute__((preserve_most)) const {{\[}}[clang::annotate_type(...)]]':'void (int) __attribute__((preserve_most)) const' implicit_instantiation
+ (void) [] (T) PRESERVE [[clang::annotate_type("foo")]] { };
+
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int) const {{\[}}[clang::annotate_type(...)]]':'void (int) const' implicit_instantiation
+ (void) [] (T) [[clang::annotate_type("foo")]] {
+ // CHECK: CXXMethodDecl {{.*}} operator() 'PRESERVE void (int) __attribute__((preserve_most)) const {{\[}}[clang::annotate_type(...)]]':'void (int) __attribute__((preserve_most)) const' implicit_instantiation
+ auto l = []<typename U = T> (U u = {}) PRESERVE [[clang::annotate_type("foo", u)]] { };
+
+ // CHECK: DeclRefExpr {{.*}} 'PRESERVE void (int) __attribute__((preserve_most)) const {{\[}}[clang::annotate_type(...)]]':'void (int) __attribute__((preserve_most)) const' lvalue CXXMethod
+ l();
+ };
+}
+
+void g() {
+ f<int>();
+}
diff --git a/clang/test/SemaCXX/lambda-conversion-op-cc.cpp b/clang/test/SemaCXX/lambda-conversion-op-cc.cpp
index 16ca5535019dff..1a6d197af302fd 100644
--- a/clang/test/SemaCXX/lambda-conversion-op-cc.cpp
+++ b/clang/test/SemaCXX/lambda-conversion-op-cc.cpp
@@ -44,19 +44,19 @@ void useage() {
// CHECK: VarDecl {{.*}} vectorcall '
// CHECK: LambdaExpr
- // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((vectorcall)) const'
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const __attribute__((vectorcall))':'void (int, float, double) __attribute__((vectorcall)) const'
// CHECK: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
// CHECK: CXXMethodDecl {{.*}} __invoke 'void (int, float, double) __attribute__((vectorcall))' static inline
// WIN32: VarDecl {{.*}} thiscall '
// WIN32: LambdaExpr
- // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) __attribute__((thiscall)) const'
+ // WIN32: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const __attribute__((thiscall))':'void (int, float, double) __attribute__((thiscall)) const'
// WIN32: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
// WIN32: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
// CHECK: VarDecl {{.*}} cdecl '
// CHECK: LambdaExpr
- // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const'
+ // CHECK: CXXMethodDecl {{.*}} operator() 'void (int, float, double) const __attribute__((cdecl))':'void (int, float, double) const'
// NODEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) 'void
// NODEF: CXXMethodDecl {{.*}} __invoke 'void (int, float, double)' static inline
// VECTDEF: CXXConversionDecl {{.*}} operator void (*)(int, float, double) __attribute__((vectorcall)) 'void
@@ -108,8 +108,8 @@ void useage() {
// CHECK: LambdaExpr
// CHECK: FunctionTemplateDecl {{.*}} operator()
// CHECK: CXXMethodDecl {{.*}} operator() 'auto (auto) __attribute__((vectorcall)) const' inline
-...
[truncated]
|
|
I’ll add a release note once I’ve figured out how long this bug has been in Clang. |
|
Great. Thanks! |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Seeing as there are now two places where we want to adjust function types but also preserve sugar, I’ve gone ahead and added a generic The intent here is that, in the future, functions like |
|
ping |
1 similar comment
|
ping |
zyn0217
reviewed
Aug 7, 2024
There was a problem hiding this comment.
Thanks for the patch. IIUC this patch does 2~3 things:
- Add a
dump()method toAttr - Refactor the pertaining transformation logic of
TransformLambdaExpr() - Refactor some other logic in
ASTContext::AdjustType()
They're really great works, but I suggest we split up the patch to make the code review easier, WDYT?
The NFC change for adding |
Though actually, I suppose it would be possible do add |
|
ping |
erichkeane
approved these changes
Sep 25, 2024
|
Alright, I’ll merge main and then merge this once CI is done. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/8001 Here is the relevant piece of the build log for the reference |
|
Forgot to set the target again it seems; will be fixing that in a moment |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/5052 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/5553 Here is the relevant piece of the build log for the reference |
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Sep 27, 2024
llvm#85325) This fixes a crash when we attempt to instantiate a lambda with an `AnnotatedType`, refactors the code that handles transforming the function type of a lambda, and improves source fidelity for lambda function types. This fixes llvm#85120 and fixes llvm#85154. --------- Co-authored-by: Yuxuan Chen <[email protected]> Co-authored-by: Doug Wyatt <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pr fixes a crash when we attempt to instantiate a lambda with an
AnnotatedType, refactors the code that handles transforming the function type of a lambda, and improves source fidelity for lambda function types.The background for this is that, while working on #84983, the author observed that
AnnotatedTypes were not being propagated correctly when instantiating lambdas, which is a problem for anyone who actually wants to use this information for anything.Crash
The cause of the crash was the following: When instantiating the
FunctionProtoTypeLocof a lambda, we were callingTransformAttributedType; however, anAttributedTypestores twoQualTypes: aModifiedTypeand anEquivalentType, both of which were being transformed in that function. The problem is that instantiating aFunctionProtoTypeLocalso instantiates theParmVarDecls associated with that type loc, which causes a link to be established between the original, dependentParmVarDecls and their instantiations in the currentLocalInstantiationScope. If there already is a link (which was the case when instantiating theEquivalentTypeafter instantiating theModifiedType, if they were the same type), we assert.As a fix, we now only instantiate the
EquivalentTypeiff it is not equal to theModifiedType. Unfortunately, this doesn’t solve everything, as there is another problem here: the code that instantiates the function type of a lambda did account for the case of it being anAttributedTypeLoc, but it was also bending over backwards to avoid callinggetDerived().TransformFunctionProtoType(which ends up beingTemplateInstantiator’s override) and instead callTreeTransform’sTransformFunctionProtoType.The reason for this is that the former creates a new
LocalInstantiationScope, whereas the latter does not—but when we begin instantiating a lambda (inTemplateInstantiator::TransformLambdaExpr), we already push aLocalInstantiationScopefor the lambda, and having two scopes for the same lambda would cause things to go horribly wrong, which is whyTreeTransform’sTransformLambdaExprends up being horribly messy because it has to somehow avoid calling intoTemplateInstantiator(andTransformAttributedTypelikewise suffered from this since it also had to transform theFunctionProtoTypein question).Instead of doing any of that,
TemplateInstantiatornow indicates that the firstLocalInstantiationScopeit creates is meant for instantiating a lambda (this is tracked in theLocalInstantiationScope), and itsTransformFunctionProtoTypeonly creates a new scope, iff the current scope is not a lambda. As a result, a lot of the tree transform code for this is now a lot simpler.Note: There already is a pr (#78801) that makes changes similar to the ones in this pr wrt refactoring tree transform only, but that pr hasn’t received any updates for over a month, and though the author has reached out to me saying that they’d be happy to help (which I definitely appreciate), I’ve still opted to instead add (some of) their changes to this pr instead—simply because this pr also does more than just address this issue, and trying to split the fixes up into separate prs doesn’t really work too well imo (for instance, there is no testing if
AttributedTypes are being propagated correctly if we crash trying to instantiate them), and I found it easier to just combine everything into one pr. I’ve added @yuxuanchen1997 as a co-author to the commit that deals with the tree transform refactor. I hope they’re fine with my doing so.Source Fidelity
When rebuilding function types in
adjustDeducedFunctionResultType, we now also rebuildAttributedTypes andMacroQualfiedTypes. This means that it should now be possible to access any type attributes attached to a lambda’s function type. This code was largely taken from #85147; I’ve also added @dougsonos as a co-author to the relevant commit.Side note: I know this doesn’t really belong in this pr, but I’ve also added a
dump()function toAttrsince it was lacking one, which was a bit annoying when I was debugging this earlier; I can open a separate pr for that if it’s too unrelated to the other changes here.This fixes #85120 and fixes #85154.