Skip to content

[cxx-interop] Implement foreign reference types. #39605

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

Jump to bottom
Merged
merged 1 commit into from
Dec 8, 2021
Merged

[cxx-interop] Implement foreign reference types. #39605

merged 1 commit into from
Dec 8, 2021

Conversation

zoecarver
Copy link
Contributor

This is an experimental feature to allow an attribute, import_as_ref, to import a C++ record as a non-reference-counted reference type in Swift.

@zoecarver zoecarver added the c++ interop Feature: Interoperability with C++ label Oct 6, 2021
rjmccall
rjmccall requested changes Oct 6, 2021
View reviewed changes
lib/AST/Decl.cpp
@@ -1887,7 +1887,8 @@ static bool isPolymorphic(const AbstractStorageDecl *storage) {
return true;

if (auto *classDecl = dyn_cast<ClassDecl>(storage->getDeclContext())) {
if (storage->isFinal() || classDecl->isFinal())
if (storage->isFinal() || classDecl->isFinal() ||
classDecl->isForeignReferenceType())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here? Imported classes can have polymorphic operations. At the very least, this needs a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 6cf5f8e to dd2f450 Compare October 13, 2021 17:18
@zoecarver zoecarver marked this pull request as ready for review October 13, 2021 17:19
@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from dd2f450 to b693fc7 Compare October 13, 2021 17:40
@zoecarver
Copy link
Contributor Author

@rjmccall this should be ready for re-review now. I think I addressed all your comments. Let me know what you think.

Note: I still need to fix the issue with optionals (unless some macro tells us otherwise, we should be importing foreign reference types as optionals). I'll do that shortly, but I think the rest of the PR can still be reviewed without that change.

@zoecarver zoecarver requested a review from rjmccall October 13, 2021 17:43
@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 5a89572 to 65b4f3c Compare October 22, 2021 00:13
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@rjmccall friendly ping :)

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@rjmccall friendly ping :)

@zoecarver zoecarver mentioned this pull request Oct 28, 2021
Open
@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 65b4f3c to 89fc96a Compare November 19, 2021 22:46
zoecarver
zoecarver commented Nov 20, 2021
View reviewed changes
include/swift/AST/Decl.h
@@ -3841,7 +3842,8 @@ class ClassDecl final : public NominalTypeDecl {
///
/// \see getForeignClassKind
bool isForeign() const {
return getForeignClassKind() != ForeignKind::Normal;
return getForeignClassKind() != ForeignKind::Normal ||
const_cast<ClassDecl *>(this)->isForeignReferenceType();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not ideal, but I think it's OK because we never construct a const decl, right?

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 89fc96a to 191800f Compare November 20, 2021 00:48
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 6e17e51 to ed9dad3 Compare November 29, 2021 20:58
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
Copy link
Contributor Author

@rjmccall friendly ping :)

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from 6feca79 to 298cb75 Compare December 4, 2021 01:23
@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch 2 times, most recently from c6f92e4 to 815b95a Compare December 5, 2021 19:02
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 815b95a to 649b072 Compare December 5, 2021 19:18
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 649b072 to f01913e Compare December 6, 2021 08:21
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from f01913e to 58d5f52 Compare December 6, 2021 16:32
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 58d5f52 to 817a695 Compare December 6, 2021 22:25
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 817a695 to 9f0119b Compare December 6, 2021 22:28
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from 9f0119b to c38d1ed Compare December 8, 2021 10:17
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux.

@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from c38d1ed to ae9b541 Compare December 8, 2021 11:59
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

@zoecarver
[cxx-interop] Implement foreign reference types.
fc3b3a1 
This is an expiremental feature to allow an attribute, `import_as_ref`, to import a C++ record as a non-reference-counted reference type in Swift.
@zoecarver zoecarver force-pushed the cxx-interop-import-as-class branch from ae9b541 to fc3b3a1 Compare December 8, 2021 15:35
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test.

hyp
hyp reviewed Dec 8, 2021
View reviewed changes
lib/AST/Type.cpp
}

bool CanType::isForeignReferenceType() {
if (auto *classDecl = getPointer()->lookThroughAllOptionalTypes()->getClassOrBoundGenericClass())
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you duplicate the logic here. Can you call getPointer()->isForeignReferenceType() instead?

zoecarver
zoecarver commented Dec 8, 2021
View reviewed changes
test/Interop/Cxx/foreign-reference/not-any-object.swift
public func test(_ _: AnyObject) {}

// TODO: make this a better error.
test(Empty.create()) // expected-error {{type of expression is ambiguous without more context}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a test for casting explicitly to any object.

zoecarver
zoecarver commented Dec 8, 2021
View reviewed changes
test/Interop/Cxx/foreign-reference/Inputs/pod.h
IntPair passThroughByValue(IntPair x) { return x; }

struct __attribute__((swift_attr("import_as_ref"))) RefHoldingPair {
// REVIEW-NOTE: I added support for this but then removed it, as this sort of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolve this.

@zoecarver zoecarver merged commit a46a3c5 into swiftlang:main Dec 8, 2021
@bro4all bro4all self-requested a review December 9, 2021 06:06
@finagolfin finagolfin mentioned this pull request Dec 19, 2021
Merged
finagolfin added a commit to finagolfin/swift that referenced this pull request Jun 9, 2022
@finagolfin
[android][test] Fix several tests on the Android CI
3933fc0 
- swiftlang#58975 switched many tests from XFAIL on linux to linux-gnu, so seven
  fail on the Android CI and two natively. They are now explicitly excluded.
- swiftlang#39605 added several C++ Interop tests, 11 of which fail on the Android CI,
  so disable them for now.
- swiftlang#42478 removed the @NoEscape attribute for the non-Android
  SIL/clang-function-types tests, so I remove it for Android too.
- My pull swiftlang#40779 moved the Swift pointer tags to the second byte, so
  SILOptimizer/concat_string_literals.64 will need to be updated for that,
  disabled it for now.
- Compiler-rt moved the directory in which it places those libraries for
  Android, llvm/llvm-project@a68ccba, so lit.cfg is updated for that.
@finagolfin finagolfin mentioned this pull request Jun 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guitard0g guitard0g guitard0g left review comments

@hyp hyp hyp approved these changes

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

@bro4all bro4all Awaiting requested review from bro4all

Assignees
No one assigned
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zoecarver @rjmccall @hyp @guitard0g