Skip to content

[cxx-interop] Import attributes on inherited C++ methods #59022

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 1, 2022
Merged

[cxx-interop] Import attributes on inherited C++ methods #59022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions include/swift/AST/Attr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,6 +1269,13 @@ class EffectsAttr : public DeclAttribute {
static bool classof(const DeclAttribute *DA) {
return DA->getKind() == DAK_Effects;
}

EffectsAttr *clone(ASTContext &ctx) const {
if (getKind() == EffectsKind::Custom) {
return new (ctx) EffectsAttr(customString);
}
return new (ctx) EffectsAttr(getKind());
}
};


Expand Down Expand Up @@ -2303,6 +2310,13 @@ class DeclAttributes {
DeclAttrs = Attr;
}

/// Add multiple constructed DeclAttributes to this list.
void add(DeclAttributes &Attrs) {
for (auto attr : Attrs) {
add(attr);
}
}

// Iterator interface over DeclAttribute objects.
class iterator : public iterator_base<DeclAttribute, iterator> {
public:
Expand Down
50 changes: 49 additions & 1 deletion lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4851,6 +4851,51 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
return {getterDecl, setterDecl};
}

// Clone attributes that have been imported from Clang.
DeclAttributes cloneImportedAttributes(ValueDecl *decl, ASTContext &context) {
auto attrs = DeclAttributes();
for (auto attr : decl->getAttrs()) {
switch (attr->getKind()) {
case DAK_Available: {
attrs.add(cast<AvailableAttr>(attr)->clone(context, true));
break;
}
case DAK_Custom: {
if (CustomAttr *cAttr = cast<CustomAttr>(attr)) {
attrs.add(CustomAttr::create(context, SourceLoc(), cAttr->getTypeExpr(),
cAttr->getInitContext(), cAttr->getArgs(),
true));
}
break;
}
case DAK_DiscardableResult: {
attrs.add(new (context) DiscardableResultAttr(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking this might not always be true, maybe that's why we're seeing all the attrs in the test below. But I don't know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to always inherit the attributes as implicit because we also inherit methods as implicit. Do you think that makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I see now. Sounds good.

break;
}
case DAK_Effects: {
attrs.add(cast<EffectsAttr>(attr)->clone(context));
break;
}
case DAK_Final: {
attrs.add(new (context) FinalAttr(true));
break;
}
case DAK_Transparent: {
attrs.add(new (context) TransparentAttr(true));
break;
}
case DAK_WarnUnqualifiedAccess: {
attrs.add(new (context) WarnUnqualifiedAccessAttr(true));
break;
}
default:
break;
}
}

return attrs;
}

ValueDecl *cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
if (auto fn = dyn_cast<FuncDecl>(decl)) {
// TODO: function templates are specialized during type checking so to
Expand All @@ -4862,11 +4907,14 @@ ValueDecl *cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
isa<clang::FunctionTemplateDecl>(fn->getClangDecl())))
return nullptr;

ASTContext &context = decl->getASTContext();
auto out = FuncDecl::createImplicit(
fn->getASTContext(), fn->getStaticSpelling(), fn->getName(),
context, fn->getStaticSpelling(), fn->getName(),
fn->getNameLoc(), fn->hasAsync(), fn->hasThrows(),
fn->getGenericParams(), fn->getParameters(),
fn->getResultInterfaceType(), newContext);
auto inheritedAttributes = cloneImportedAttributes(decl, context);
out->getAttrs().add(inheritedAttributes);
out->copyFormalAccessFrom(fn);
out->setBodySynthesizer(synthesizeBaseClassMethodBody, fn);
out->setSelfAccessKind(fn->getSelfAccessKind());
Expand Down
6 changes: 6 additions & 0 deletions test/Interop/Cxx/class/inheritance/Inputs/functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ struct Base {
}

static const char *staticInBase() { return "Base::staticInBase"; }

int renamed(int i) __attribute__((swift_name("swiftRenamed(input:)"))) {
return i * 2;
}

void pure() const __attribute__((pure)) {}
};

struct OtherBase {
Expand Down
44 changes: 43 additions & 1 deletion test/Interop/Cxx/class/inheritance/functions-module-interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,94 @@
// RUN: %target-swift-ide-test -print-module -module-to-print=Functions -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s
// RUN: %target-swift-ide-test -print-module -print-implicit-attrs -module-to-print=Functions -I %S/Inputs -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s

// CHECK: struct NonTrivial {
// CHECK-NEXT: init()
// CHECK-NEXT: @discardableResult
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we're getting a lot of @discardableResult attrs when the base class' member didn't have that attribute. Is it possible that the base class had the attribute but it was false (so it didn't get printed)? Or do you have any idea what's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think clang importer attaches @discardableResult to all non-void methods that aren't annotated with __attribute__((warn_unused_result)) (see lib/ClangImporter/ImportDecl.cpp:9247). The reason they suddenly showed up in the test, is because I added the -print-implicit-attrs to check for attributes that are imported as implicit by the clang importer (like @available).

In order to make the test less cluttered, we could switch to using CHECK so we can ignore the lines with @discardableResult. Or I could add a new test for inherited attributes. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now. Thanks for explaining. Make sense. This LGTM.

// CHECK-NEXT: func inNonTrivial() -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inNonTrivialWithArgs(_ a: Int32, _ b: Int32) -> UnsafePointer<CChar>!
// CHECK-NEXT: }

// CHECK-NEXT: struct Base {
// CHECK-NEXT: init()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: mutating func mutatingInBase() -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func constInBase() -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func takesArgsInBase(_ a: Int32, _ b: Int32, _ c: Int32) -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func takesNonTrivialInBase(_ a: NonTrivial) -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func returnsNonTrivialInBase() -> NonTrivial
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func templateInBase<T>(_ t: T) -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: static func staticInBase() -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: mutating func swiftRenamed(input i: Int32) -> Int32
// CHECK-NEXT: @available(swift, obsoleted: 3, renamed: "swiftRenamed(input:)")
// CHECK-NEXT: mutating func renamed(_ i: Int32) -> Int32
// CHECK-NEXT: @_effects(readonly) func pure()
// CHECK-NEXT: }

// CHECK-NEXT: struct OtherBase {
// CHECK-NEXT: init()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inOtherBase() -> UnsafePointer<CChar>!
// CHECK-NEXT: }

// CHECK-NEXT: struct Derived {
// CHECK-NEXT: init()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inDerived() -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: mutating func mutatingInBase() -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func constInBase() -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func takesArgsInBase(_ a: Int32, _ b: Int32, _ c: Int32) -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func takesNonTrivialInBase(_ a: NonTrivial) -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func returnsNonTrivialInBase() -> NonTrivial
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: mutating func swiftRenamed(input i: Int32) -> Int32
// CHECK-NEXT: @available(swift, obsoleted: 3, renamed: "swiftRenamed(input:)")
// CHECK-NEXT: mutating func renamed(_ i: Int32) -> Int32
// CHECK-NEXT: @_effects(readonly) func pure()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inOtherBase() -> UnsafePointer<CChar>?
// CHECK-NEXT: }

// CHECK-NEXT: struct DerivedFromDerived {
// CHECK-NEXT: init()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func topLevel() -> UnsafePointer<CChar>!
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inDerived() -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: mutating func mutatingInBase() -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func constInBase() -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func takesArgsInBase(_ a: Int32, _ b: Int32, _ c: Int32) -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func takesNonTrivialInBase(_ a: NonTrivial) -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func returnsNonTrivialInBase() -> NonTrivial
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: mutating func swiftRenamed(input i: Int32) -> Int32
// CHECK-NEXT: @available(swift, obsoleted: 3, renamed: "swiftRenamed(input:)")
// CHECK-NEXT: mutating func renamed(_ i: Int32) -> Int32
// CHECK-NEXT: @_effects(readonly) func pure()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inOtherBase() -> UnsafePointer<CChar>?
// CHECK-NEXT: }

// CHECK-NEXT: struct DerivedFromNonTrivial {
// CHECK-NEXT: init()
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inNonTrivial() -> UnsafePointer<CChar>?
// CHECK-NEXT: @discardableResult
// CHECK-NEXT: func inNonTrivialWithArgs(_ a: Int32, _ b: Int32) -> UnsafePointer<CChar>?
// CHECK-NEXT: }