Skip to content

[cxx-interop] Only mark projections of self-contained types as unsafe. #67296

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

Conversation

zoecarver
Copy link
Contributor

Projections of trivial types and view types aren't unsafe. This matches what was described in the vision document.

To elaborate: a view type like StringRef is already referencing memory it does not own, so if it projects its data again (via the data method for example), there's no additional unsafety. The really unsafe thing is when a self contained (owned) type projects its owned memory because there are subtle difference in C++ and Swift lifetime models (along with different safety expectations).

This is a relatively breaking change. Expect turbulence.

@zoecarver zoecarver requested review from hyp and egorzhdan as code owners July 14, 2023 01:43
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@hyp
Copy link
Contributor

hyp commented Jul 14, 2023

@swift-ci please smoke test

@zoecarver zoecarver force-pushed the only-methods-on-self-contained-types-are-unsafe branch from 42db562 to 5bc6abd Compare July 15, 2023 02:54
@zoecarver
Copy link
Contributor Author

Feature added. @hyp wdyt?

@zoecarver
Copy link
Contributor Author

@swift-ci please test

2 similar comments
@zoecarver
Copy link
Contributor Author

@swift-ci please test

@zoecarver
Copy link
Contributor Author

@swift-ci please test

@@ -6795,10 +6826,24 @@ bool IsSafeUseOfCxxDecl::evaluate(Evaluator &evaluator,
if (isForeignReferenceType(method->getReturnType()))
return true;

// If it returns a pointer or reference, that's a projection.
// begin and end methods likely return an interator, so they're unsafe. This
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: iterator


// Mark this as safe to help our diganostics down the road.
if (!cxxRecordReturnType->getDefinition()) {
return true;
}

if (!hasCustomCopyOrMoveConstructor(cxxRecordReturnType) &&
// A projection of a view type (such as a string_view) from a self
// contained parent is a proejction (unsafe).
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: projection

@egorzhdan
Copy link
Contributor

@swift-ci please smoke test Linux

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test Linux platform

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test linux

1 similar comment
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test linux

@zoecarver zoecarver force-pushed the only-methods-on-self-contained-types-are-unsafe branch from 6c037d9 to 806956d Compare July 19, 2023 00:43
@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test

@zoecarver
Copy link
Contributor Author

(Doing a smoke test as we just saw a full test succeed.)

@zoecarver
Copy link
Contributor Author

@swift-ci please smoke test

@zoecarver zoecarver merged commit 7db5fe1 into swiftlang:main Jul 19, 2023
eeckstein added a commit to eeckstein/swift that referenced this pull request Jul 20, 2023
egorzhdan added a commit to swiftlang/swift-llvm-bindings that referenced this pull request Aug 4, 2023
This makes sure that the bindings can be built with a 5.9+ snapshot compiler.

See swiftlang/swift#67296
egorzhdan added a commit to swiftlang/swift-llvm-bindings that referenced this pull request Sep 6, 2023
This makes sure that the bindings can be built with a 5.9+ snapshot compiler.

See swiftlang/swift#67296

(cherry picked from commit 470e7d8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants