[clang/AST] Make it possible to use SwiftAttr in type context #108631
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Pavel Yaskevich (xedin) ChangesSwift ClangImporter now supports concurrency annotations on imported declarations and their parameters/results, to make it possible to use imported APIs in Swift safely there has to be a way to annotate individual parameters and result types with relevant attributes that indicate that e.g. a block is called on a particular actor or it accepts a To faciliate that Patch is 29.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/108631.diff 22 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 168bdca3c880b2..f49bb7afce2336 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1677,8 +1677,15 @@ class ASTContext : public RefCountedBase<ASTContext> {
QualType getInjectedClassNameType(CXXRecordDecl *Decl, QualType TST) const;
QualType getAttributedType(attr::Kind attrKind, QualType modifiedType,
+ QualType equivalentType,
+ const Attr *attr = nullptr) const;
+
+ QualType getAttributedType(const Attr *attr, QualType modifiedType,
QualType equivalentType) const;
+ QualType getAttributedType(NullabilityKind nullability, QualType modifiedType,
+ QualType equivalentType);
+
QualType getBTFTagAttributedType(const BTFTypeTagAttr *BTFAttr,
QualType Wrapped);
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index 9b934b20cf2559..0d1712a1fe15fd 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -76,6 +76,7 @@ def APValue : PropertyType { let PassByReference = 1; }
def APValueKind : EnumPropertyType<"APValue::ValueKind">;
def ArraySizeModifier : EnumPropertyType<"ArraySizeModifier">;
def AttrKind : EnumPropertyType<"attr::Kind">;
+def Attr : PropertyType<"const Attr *">;
def AutoTypeKeyword : EnumPropertyType;
def Bool : PropertyType<"bool">;
def BuiltinTypeKind : EnumPropertyType<"BuiltinType::Kind">;
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index ef36a73716454f..7bb8aea05b8fa4 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -68,6 +68,7 @@ class ValueDecl;
class TagDecl;
class TemplateParameterList;
class Type;
+class Attr;
enum {
TypeAlignmentInBits = 4,
@@ -6037,13 +6038,24 @@ class AttributedType : public Type, public llvm::FoldingSetNode {
private:
friend class ASTContext; // ASTContext creates these
+ const Attr *Attribute;
+
QualType ModifiedType;
QualType EquivalentType;
AttributedType(QualType canon, attr::Kind attrKind, QualType modified,
QualType equivalent)
+ : AttributedType(canon, attrKind, nullptr, modified, equivalent) {}
+
+ AttributedType(QualType canon, const Attr *attr, QualType modified,
+ QualType equivalent);
+
+private:
+ AttributedType(QualType canon, attr::Kind attrKind, const Attr *attr,
+ QualType modified, QualType equivalent)
: Type(Attributed, canon, equivalent->getDependence()),
- ModifiedType(modified), EquivalentType(equivalent) {
+ Attribute(attr), ModifiedType(modified),
+ EquivalentType(equivalent) {
AttributedTypeBits.AttrKind = attrKind;
}
@@ -6052,6 +6064,8 @@ class AttributedType : public Type, public llvm::FoldingSetNode {
return static_cast<Kind>(AttributedTypeBits.AttrKind);
}
+ const Attr *getAttr() const { return Attribute; }
+
QualType getModifiedType() const { return ModifiedType; }
QualType getEquivalentType() const { return EquivalentType; }
@@ -6083,25 +6097,6 @@ class AttributedType : public Type, public llvm::FoldingSetNode {
std::optional<NullabilityKind> getImmediateNullability() const;
- /// Retrieve the attribute kind corresponding to the given
- /// nullability kind.
- static Kind getNullabilityAttrKind(NullabilityKind kind) {
- switch (kind) {
- case NullabilityKind::NonNull:
- return attr::TypeNonNull;
-
- case NullabilityKind::Nullable:
- return attr::TypeNullable;
-
- case NullabilityKind::NullableResult:
- return attr::TypeNullableResult;
-
- case NullabilityKind::Unspecified:
- return attr::TypeNullUnspecified;
- }
- llvm_unreachable("Unknown nullability kind.");
- }
-
/// Strip off the top-level nullability annotation on the given
/// type, if it's there.
///
diff --git a/clang/include/clang/AST/TypeProperties.td b/clang/include/clang/AST/TypeProperties.td
index 539a344cb0b690..82605b18fbde6c 100644
--- a/clang/include/clang/AST/TypeProperties.td
+++ b/clang/include/clang/AST/TypeProperties.td
@@ -668,12 +668,16 @@ let Class = AttributedType in {
def : Property<"equivalentType", QualType> {
let Read = [{ node->getEquivalentType() }];
}
- def : Property<"attribute", AttrKind> {
+ def : Property<"attrKind", AttrKind> {
let Read = [{ node->getAttrKind() }];
}
+ def : Property<"attribute", Attr> {
+ let Read = [{ node->getAttr() }];
+ }
def : Creator<[{
- return ctx.getAttributedType(attribute, modifiedType, equivalentType);
+ return ctx.getAttributedType(attrKind, modifiedType,
+ equivalentType, attribute);
}]>;
}
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 70fad60d4edbb5..71f626845b0108 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2834,7 +2834,7 @@ def SwiftAsyncName : InheritableAttr {
let Documentation = [SwiftAsyncNameDocs];
}
-def SwiftAttr : InheritableAttr {
+def SwiftAttr : DeclOrTypeAttr {
let Spellings = [GNU<"swift_attr">];
let Args = [StringArgument<"Attribute">];
let Documentation = [SwiftAttrDocs];
diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td
index 546e5100b79dd9..0bb51585d407bf 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4489,8 +4489,8 @@ def SwiftAttrDocs : Documentation {
let Heading = "swift_attr";
let Content = [{
The ``swift_attr`` provides a Swift-specific annotation for the declaration
-to which the attribute appertains to. It can be used on any declaration
-in Clang. This kind of annotation is ignored by Clang as it doesn't have any
+or type to which the attribute appertains to. It can be used on any declaration
+or type in Clang. This kind of annotation is ignored by Clang as it doesn't have any
semantic meaning in languages supported by Clang. The Swift compiler can
interpret these annotations according to its own rules when importing C or
Objective-C declarations.
diff --git a/clang/include/clang/Serialization/ASTRecordWriter.h b/clang/include/clang/Serialization/ASTRecordWriter.h
index 0c8ac75fc40f40..d6090ba1a6c690 100644
--- a/clang/include/clang/Serialization/ASTRecordWriter.h
+++ b/clang/include/clang/Serialization/ASTRecordWriter.h
@@ -128,6 +128,8 @@ class ASTRecordWriter
AddStmt(const_cast<Stmt*>(S));
}
+ void writeAttr(const Attr *A) { AddAttr(A); }
+
/// Write an BTFTypeTagAttr object.
void writeBTFTypeTagAttr(const BTFTypeTagAttr *A) { AddAttr(A); }
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 7cc69ca4a8a814..afaa77d3014be2 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -3566,11 +3566,13 @@ QualType ASTContext::getFunctionTypeWithExceptionSpec(
MQT->getMacroIdentifier());
// Might have a calling-convention attribute.
- if (const auto *AT = dyn_cast<AttributedType>(Orig))
+ if (const auto *AT = dyn_cast<AttributedType>(Orig)) {
return getAttributedType(
AT->getAttrKind(),
getFunctionTypeWithExceptionSpec(AT->getModifiedType(), ESI),
- getFunctionTypeWithExceptionSpec(AT->getEquivalentType(), ESI));
+ getFunctionTypeWithExceptionSpec(AT->getEquivalentType(), ESI),
+ AT->getAttr());
+ }
// Anything else must be a function type. Rebuild it with the new exception
// specification.
@@ -5125,7 +5127,8 @@ QualType ASTContext::getUnresolvedUsingType(
QualType ASTContext::getAttributedType(attr::Kind attrKind,
QualType modifiedType,
- QualType equivalentType) const {
+ QualType equivalentType,
+ const Attr *attr) const {
llvm::FoldingSetNodeID id;
AttributedType::Profile(id, attrKind, modifiedType, equivalentType);
@@ -5133,9 +5136,11 @@ QualType ASTContext::getAttributedType(attr::Kind attrKind,
AttributedType *type = AttributedTypes.FindNodeOrInsertPos(id, insertPos);
if (type) return QualType(type, 0);
+ assert(!attr || attr->getKind() == attrKind);
+
QualType canon = getCanonicalType(equivalentType);
- type = new (*this, alignof(AttributedType))
- AttributedType(canon, attrKind, modifiedType, equivalentType);
+ type = new (*this, alignof(AttributedType))
+ AttributedType(canon, attrKind, attr, modifiedType, equivalentType);
Types.push_back(type);
AttributedTypes.InsertNode(type, insertPos);
@@ -5143,6 +5148,33 @@ QualType ASTContext::getAttributedType(attr::Kind attrKind,
return QualType(type, 0);
}
+QualType ASTContext::getAttributedType(const Attr *attr, QualType modifiedType,
+ QualType equivalentType) const {
+ return getAttributedType(attr->getKind(), modifiedType, equivalentType, attr);
+}
+
+QualType ASTContext::getAttributedType(NullabilityKind nullability,
+ QualType modifiedType,
+ QualType equivalentType) {
+ switch (nullability) {
+ case NullabilityKind::NonNull:
+ return getAttributedType(attr::TypeNonNull, modifiedType, equivalentType);
+
+ case NullabilityKind::Nullable:
+ return getAttributedType(attr::TypeNullable, modifiedType, equivalentType);
+
+ case NullabilityKind::NullableResult:
+ return getAttributedType(attr::TypeNullableResult, modifiedType,
+ equivalentType);
+
+ case NullabilityKind::Unspecified:
+ return getAttributedType(attr::TypeNullUnspecified, modifiedType,
+ equivalentType);
+ }
+
+ llvm_unreachable("Unknown nullability kind");
+}
+
QualType ASTContext::getBTFTagAttributedType(const BTFTypeTagAttr *BTFAttr,
QualType Wrapped) {
llvm::FoldingSetNodeID ID;
@@ -7474,8 +7506,8 @@ QualType ASTContext::getArrayDecayedType(QualType Ty) const {
// int x[_Nullable] -> int * _Nullable
if (auto Nullability = Ty->getNullability()) {
- Result = const_cast<ASTContext *>(this)->getAttributedType(
- AttributedType::getNullabilityAttrKind(*Nullability), Result, Result);
+ Result = const_cast<ASTContext *>(this)->getAttributedType(*Nullability,
+ Result, Result);
}
return Result;
}
@@ -13693,7 +13725,8 @@ static QualType getCommonSugarTypeNode(ASTContext &Ctx, const Type *X,
return QualType();
// FIXME: It's inefficient to have to unify the modified types.
return Ctx.getAttributedType(Kind, Ctx.getCommonSugaredType(MX, MY),
- Ctx.getQualifiedType(Underlying));
+ Ctx.getQualifiedType(Underlying),
+ AX->getAttr());
}
case Type::BTFTagAttributed: {
const auto *BX = cast<BTFTagAttributedType>(X);
diff --git a/clang/lib/AST/ASTDiagnostic.cpp b/clang/lib/AST/ASTDiagnostic.cpp
index 15c3efe4212719..4f677b60e60dae 100644
--- a/clang/lib/AST/ASTDiagnostic.cpp
+++ b/clang/lib/AST/ASTDiagnostic.cpp
@@ -85,8 +85,7 @@ QualType clang::desugarForDiagnostic(ASTContext &Context, QualType QT,
QualType SugarRT = FT->getReturnType();
QualType RT = desugarForDiagnostic(Context, SugarRT, DesugarReturn);
if (auto nullability = AttributedType::stripOuterNullability(SugarRT)) {
- RT = Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*nullability), RT, RT);
+ RT = Context.getAttributedType(*nullability, RT, RT);
}
bool DesugarArgument = false;
@@ -97,8 +96,7 @@ QualType clang::desugarForDiagnostic(ASTContext &Context, QualType QT,
QualType PT = desugarForDiagnostic(Context, SugarPT, DesugarArgument);
if (auto nullability =
AttributedType::stripOuterNullability(SugarPT)) {
- PT = Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*nullability), PT, PT);
+ PT = Context.getAttributedType(*nullability, PT, PT);
}
Args.push_back(PT);
}
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index c2fb7dddcfc637..1be755e97f716d 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -1580,8 +1580,9 @@ ExpectedType ASTNodeImporter::VisitAttributedType(const AttributedType *T) {
if (!ToEquivalentTypeOrErr)
return ToEquivalentTypeOrErr.takeError();
- return Importer.getToContext().getAttributedType(T->getAttrKind(),
- *ToModifiedTypeOrErr, *ToEquivalentTypeOrErr);
+ return Importer.getToContext().getAttributedType(
+ T->getAttrKind(), *ToModifiedTypeOrErr, *ToEquivalentTypeOrErr,
+ T->getAttr());
}
ExpectedType
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index a55e6c8bf02611..51e67e71751bc0 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -1241,8 +1241,8 @@ struct SimpleTransformVisitor : public TypeVisitor<Derived, QualType> {
== T->getEquivalentType().getAsOpaquePtr())
return QualType(T, 0);
- return Ctx.getAttributedType(T->getAttrKind(), modifiedType,
- equivalentType);
+ return Ctx.getAttributedType(T->getAttrKind(), modifiedType, equivalentType,
+ T->getAttr());
}
QualType VisitSubstTemplateTypeParmType(const SubstTemplateTypeParmType *T) {
@@ -1545,7 +1545,8 @@ struct SubstObjCTypeArgsVisitor
// Rebuild the attributed type.
return Ctx.getAttributedType(newAttrType->getAttrKind(),
- newAttrType->getModifiedType(), newEquivType);
+ newAttrType->getModifiedType(), newEquivType,
+ newAttrType->getAttr());
}
};
@@ -4104,6 +4105,10 @@ bool RecordType::hasConstFields() const {
return false;
}
+AttributedType::AttributedType(QualType canon, const Attr *attr,
+ QualType modified, QualType equivalent)
+ : AttributedType(canon, attr->getKind(), attr, modified, equivalent) {}
+
bool AttributedType::isQualifier() const {
// FIXME: Generate this with TableGen.
switch (getAttrKind()) {
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index be627a6242eb40..8cc4c39f7b80e2 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -1932,6 +1932,14 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
return;
}
+ if (T->getAttrKind() == attr::SwiftAttr) {
+ if (auto *swiftAttr = dyn_cast_or_null<SwiftAttrAttr>(T->getAttr())) {
+ OS << " __attribute__((swift_attr(\"" << swiftAttr->getAttribute()
+ << "\")))";
+ }
+ return;
+ }
+
OS << " __attribute__((";
switch (T->getAttrKind()) {
#define TYPE_ATTR(NAME)
@@ -1990,6 +1998,7 @@ void TypePrinter::printAttributedAfter(const AttributedType *T,
case attr::NonAllocating:
case attr::Blocking:
case attr::Allocating:
+ case attr::SwiftAttr:
llvm_unreachable("This attribute should have been handled already");
case attr::NSReturnsRetained:
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 8557c25b93a8da..4bb3f2a0f302cb 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -3329,9 +3329,7 @@ static void mergeParamDeclTypes(ParmVarDecl *NewParam,
}
} else {
QualType NewT = NewParam->getType();
- NewT = S.Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*Oldnullability),
- NewT, NewT);
+ NewT = S.Context.getAttributedType(*Oldnullability, NewT, NewT);
NewParam->setType(NewT);
}
}
diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp
index 807453400abdd0..229cf87de52f12 100644
--- a/clang/lib/Sema/SemaDeclObjC.cpp
+++ b/clang/lib/Sema/SemaDeclObjC.cpp
@@ -4577,9 +4577,7 @@ static QualType mergeTypeNullabilityForRedecl(Sema &S, SourceLocation loc,
return type;
// Otherwise, provide the result with the same nullability.
- return S.Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*prevNullability),
- type, type);
+ return S.Context.getAttributedType(*prevNullability, type, type);
}
/// Merge information from the declaration of a method in the \@interface
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 8f3e15cc9a9bb7..958ef034d598c0 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8738,8 +8738,7 @@ static QualType computeConditionalNullability(QualType ResTy, bool IsBin,
ResTy = ResTy.getSingleStepDesugaredType(Ctx);
// Create a new AttributedType with the new nullability kind.
- auto NewAttr = AttributedType::getNullabilityAttrKind(MergedKind);
- return Ctx.getAttributedType(NewAttr, ResTy, ResTy);
+ return Ctx.getAttributedType(MergedKind, ResTy, ResTy);
}
ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
diff --git a/clang/lib/Sema/SemaExprObjC.cpp b/clang/lib/Sema/SemaExprObjC.cpp
index 2f914ddc22a38b..5a2e1fe4f16f6e 100644
--- a/clang/lib/Sema/SemaExprObjC.cpp
+++ b/clang/lib/Sema/SemaExprObjC.cpp
@@ -551,9 +551,7 @@ ExprResult SemaObjC::BuildObjCBoxedExpr(SourceRange SR, Expr *ValueExpr) {
const llvm::UTF8 *StrEnd = Str.bytes_end();
// Check that this is a valid UTF-8 string.
if (llvm::isLegalUTF8String(&StrBegin, StrEnd)) {
- BoxedType = Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(
- NullabilityKind::NonNull),
+ BoxedType = Context.getAttributedType(NullabilityKind::NonNull,
NSStringPointer, NSStringPointer);
return new (Context) ObjCBoxedExpr(CE, BoxedType, nullptr, SR);
}
@@ -605,9 +603,8 @@ ExprResult SemaObjC::BuildObjCBoxedExpr(SourceRange SR, Expr *ValueExpr) {
std::optional<NullabilityKind> Nullability =
BoxingMethod->getReturnType()->getNullability();
if (Nullability)
- BoxedType = Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*Nullability), BoxedType,
- BoxedType);
+ BoxedType =
+ Context.getAttributedType(*Nullability, BoxedType, BoxedType);
}
} else if (ValueType->isBuiltinType()) {
// The other types we support are numeric, char and BOOL/bool. We could also
@@ -1444,10 +1441,8 @@ static QualType stripObjCInstanceType(ASTContext &Context, QualType T) {
QualType origType = T;
if (auto nullability = AttributedType::stripOuterNullability(T)) {
if (T == Context.getObjCInstanceType()) {
- return Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*nullability),
- Context.getObjCIdType(),
- Context.getObjCIdType());
+ return Context.getAttributedType(*nullability, Context.getObjCIdType(),
+ Context.getObjCIdType());
}
return origType;
@@ -1485,10 +1480,7 @@ static QualType getBaseMessageSendResultType(Sema &S,
(void)AttributedType::stripOuterNullability(type);
// Form a new attributed type using the method result type's nullability.
- return Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*nullability),
- type,
- type);
+ return Context.getAttributedType(*nullability, type, type);
}
return type;
@@ -1559,9 +1551,8 @@ QualType SemaObjC::getMessageSendResultType(const Expr *Receiver,
QualType NewResultType = Context.getObjCObjectPointerType(
Context.getObjCInterfaceType(MD->getClassInterface()));
if (auto Nullability = resultType->getNullability())
- NewResultType = Context.getAttributedType(
- AttributedType::getNullabilityAttrKind(*Nullability),
- ...
[truncated]
|
Xazax-hun
requested review from
Xazax-hun,
AaronBallman and
erichkeane
and removed request for
AaronBallman
There was a problem hiding this comment.
Overall looks good to me, I have one question and a couple nits inline. I think having access to Attr from AttributedType makes sense, I would not be surprised if this helped some other use cases like Lifetime annotations in Crubit. But please wait for a review from @erichkeane as well.
clang/include/clang/AST/Type.h
Outdated
| : Type(Attributed, canon, equivalent->getDependence()), | ||
| ModifiedType(modified), EquivalentType(equivalent) { | ||
| Attribute(attr), ModifiedType(modified), | ||
| EquivalentType(equivalent) { | ||
| AttributedTypeBits.AttrKind = attrKind; |
There was a problem hiding this comment.
Do we need both attrKind and attr? It removed an indirection, but wondering if we should prefer that or memory. No strong feelings here. @erichkeane any opinions?
There was a problem hiding this comment.
I wouldn't expect that both are necessary here, and would rather we don't have the additional field, it is important that the two of these match, so at minimum I'd want an assert that they match (or a good example/explaination of why they WOULDN'T match).
There was a problem hiding this comment.
If I remember correct the Attr * is not always there (nullable or something) that's why we need both Attr * and attrKind
There was a problem hiding this comment.
I see, thanks! In that case it makes sense to me to encode this invariant in the constructor as an assertion (either attr is null, or it has the same kind as attrKind).
There was a problem hiding this comment.
In fact I added that assert to getAttribute(...) method on ASTContext already.
|
|
||
| // If the attribute as attached to a paren move it closer to | ||
| // the declarator. This can happen in block declarations when | ||
| // an attribute is placed before `^` i.e. `(__attribute__((...)) ^)`. |
There was a problem hiding this comment.
I don't see a test case covering this scenario, should we add one?
There was a problem hiding this comment.
This actually applies more on the Swift side, I have multiple tests for this in ClangImporter but I'm not sure how to test this here since it doesn't really matter for clang AST where the attribute ends up being located.
There was a problem hiding this comment.
Would this not matter for the AST dump? Or could you match on that?
There was a problem hiding this comment.
Done! I remembered why I didn't add that - it required -fblocks.
There was a problem hiding this comment.
So I think this patch is doing too much, I'd like to see the changes to AttributedType split into a separate patch so we can talk about the swift_attr alone.
As far as moving this attribute to be a Type Attribute, that comes with a ton of concerns around templates. I don't see anything set to instantiate teh arguments, so any value you give to it will not be instantiated (the attribute will as far as I can tell, be instantiated, but with no modifications to its arguments).
I would like to see a some tests (AST and not!) that validate the behavior of this on a template, and as a type being instantiated, to ensure it doesn't get lost in cases where you'd want it to stay.
Also, we need a release note.
clang/include/clang/AST/Type.h
Outdated
| : Type(Attributed, canon, equivalent->getDependence()), | ||
| ModifiedType(modified), EquivalentType(equivalent) { | ||
| Attribute(attr), ModifiedType(modified), | ||
| EquivalentType(equivalent) { | ||
| AttributedTypeBits.AttrKind = attrKind; |
There was a problem hiding this comment.
I wouldn't expect that both are necessary here, and would rather we don't have the additional field, it is important that the two of these match, so at minimum I'd want an assert that they match (or a good example/explaination of why they WOULDN'T match).
clang/lib/AST/ASTContext.cpp
Outdated
| @@ -5125,24 +5127,54 @@ QualType ASTContext::getUnresolvedUsingType( | |||
|
|
|||
| QualType ASTContext::getAttributedType(attr::Kind attrKind, | |||
| QualType modifiedType, | |||
| QualType equivalentType) const { | |||
| QualType equivalentType, | |||
| const Attr *attr) const { | |||
| llvm::FoldingSetNodeID id; | |||
| AttributedType::Profile(id, attrKind, modifiedType, equivalentType); | |||
There was a problem hiding this comment.
AttributedType is uniqued based on attrKind, but not attr. So calling this function may return a type with a pointer to a different Attr (although of the same kind). Is this a problem? Do we want to fully unique AttributedType based on it's Attr*?
The other option I believe is to fetch the Attr* from AttributedTypeLoc (which you can get through the TypeSourceInfo from Decl or ExplicitCastExpr) rather than the AttributedType, but doing that for generic expressions gets quite hairy.
There was a problem hiding this comment.
I think adding attr to profile is a better approach, I'll look into that.
xedin
force-pushed
the
swift-attr-in-type-context
branch
from
e3c3d34 to
bd44742
Compare
|
I addressed most of the comments. A few points to add:
|
|
I think the interesting template cases are like: The question is, whether we actually have the type attributes where we expect to have them, or do we lose them? That being said, one question is whether we actually need type attributes to be propagated in this case for Swift's interop. For the release notes, you'd want to mention that |
You're correct, the problem with attributed type is that it is going to be lost A LOT every time we canonicalize the type, else they need to be different types, and thus those would be different instantiations. This is SOMETIMES not problematic (and sometimes even wanted, see vector types), but it IS problematic if you want your attribute to not cause problems being used in containers/etc, since |
I don't think we do, we need swift_attr to help us augment Objective-C APIs to make existing frameworks play well with Swift concurrency. I think C++ Interoperability only uses them in limited set of cases which don't involve templates. |
xedin
force-pushed
the
swift-attr-in-type-context
branch
from
bd44742 to
c8784f8
Compare
|
Updated release notes and added block tests requested by @Xazax-hun. |
|
@erichkeane friendly ping about this, I replied to your last question wondering if it would be okay to land this or should I close since it has been awhile now. |
What about ObjC++? We support that as well, and I'd like for us to not lose that ability too. |
|
@erichkeane I don't think swift_attr is intended in that context but I don't see how these changes regress existing uses. |
| let Read = [{ node->getAttrKind() }]; | ||
| } | ||
| def : Property<"attribute", Attr> { |
There was a problem hiding this comment.
Is there reason to store both attribute and attrKind here? Should we just implement attrKind as attribute.getKind ?
There was a problem hiding this comment.
Yes, Attr is not always available (nullability related attributes only have a kind and don't allocate Attr).
| QualType equivalent) | ||
| : Type(Attributed, canon, equivalent->getDependence()), Attribute(attr), | ||
| ModifiedType(modified), EquivalentType(equivalent) { | ||
| AttributedTypeBits.AttrKind = attrKind; |
There was a problem hiding this comment.
I think it'd be lovely to get these bits back. Perhaps just remove attrKind from this ctor and just implement the above one instead.
There was a problem hiding this comment.
I don't think we can do that, see https://github.com/llvm/llvm-project/pull/108631/files#r1823004595. @Xazax-hun had a comment to that effect already.
|
@erichkeane Do I need to re-run CI? I'd need some help with that since I don't have any permissions :) |
IDK how to do that other than push another 'small' patch with a merge commit or something. You don't HAVE to, but there is of course increased risk of revert if you don't. I might suggest just committing 1st thing in your morning, then dealing with the build bots during the day as they come up |
|
No worries, I can just rebase and push force, that should do it :) |
Swift ClangImporter now supports concurrency annotations on imported declarations and their parameters/results, to make it possible to use imported APIs in Swift safely there has to be a way to annotate individual parameters and result types with relevant attributes that indicate that e.g. a block is called on a particular actor or it accepts a `Sendable` parameter. To faciliate that `SwiftAttr` is switched from `InheritableAttr` which is a declaration attribute to `DeclOrTypeAttr`. To support this attribute in type context we need access to its "Attribute" argument which requires `AttributedType` to be extended to include `Attr *` when available instead of just `attr::Kind` otherwise it won't be possible to determine what attribute should be imported.
xedin
force-pushed
the
swift-attr-in-type-context
branch
from
c8784f8 to
a66d820
Compare
xedin
force-pushed
the
swift-attr-in-type-context
branch
from
f0e050c to
a66d820
Compare
|
@erichkeane CI are green, could you please merge since I don't have permissions?... |
|
@xedin Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/9268 Here is the relevant piece of the build log for the reference |
…08631) Swift ClangImporter now supports concurrency annotations on imported declarations and their parameters/results, to make it possible to use imported APIs in Swift safely there has to be a way to annotate individual parameters and result types with relevant attributes that indicate that e.g. a block is called on a particular actor or it accepts a `Sendable` parameter. To faciliate that `SwiftAttr` is switched from `InheritableAttr` which is a declaration attribute to `DeclOrTypeAttr`. To support this attribute in type context we need access to its "Attribute" argument which requires `AttributedType` to be extended to include `Attr *` when available instead of just `attr::Kind` otherwise it won't be possible to determine what attribute should be imported.
…08631) Swift ClangImporter now supports concurrency annotations on imported declarations and their parameters/results, to make it possible to use imported APIs in Swift safely there has to be a way to annotate individual parameters and result types with relevant attributes that indicate that e.g. a block is called on a particular actor or it accepts a `Sendable` parameter. To faciliate that `SwiftAttr` is switched from `InheritableAttr` which is a declaration attribute to `DeclOrTypeAttr`. To support this attribute in type context we need access to its "Attribute" argument which requires `AttributedType` to be extended to include `Attr *` when available instead of just `attr::Kind` otherwise it won't be possible to determine what attribute should be imported.
|
Hi @xedin! We've observed a difference downstream due to this patch and are curious whether this was intentional. It seems that the changes to how AttributedType is keyed (including the attribute) causes some type duplication when attributes are involved. For example, building this (reduced) program with (Ignore that there are dereferenced pointers with Before this patch, there would only be a single type-info struct/string in the resulting assembly, but with the patch, there are now two identical ones: This is possibly happening for the sanitizer emission due to the code in CodeGenFunction::EmitCheckTypeDescriptor: The two types are different for map purposes and type creation (since they have different syntactical Attrs) but the actual types are really the same. I guess this is pretty rare, but it could cause some hefty duplication depending on what types are used and how. There might be other effects I don't know of either, but this was the noticeable one for us. |
|
I think if |
|
Well, as far as I know Attrs are created on the spot and aren't uniqued. Perhaps that should happen for these kinds of Attrs, if that's possible. |
Swift ClangImporter now supports concurrency annotations on imported declarations and their parameters/results, to make it possible to use imported APIs in Swift safely there has to be a way to annotate individual parameters and result types with relevant attributes that indicate that e.g. a block is called on a particular actor or it accepts a
Sendableparameter.To faciliate that
SwiftAttris switched fromInheritableAttrwhich is a declaration attribute toDeclOrTypeAttr. To support this attribute in type context we need access to its "Attribute" argument which requiresAttributedTypeto be extended to includeAttr *when available instead of justattr::Kindotherwise it won't be possible to determine what attribute should be imported.