[cxx-interop] Refactor ClangTypeEscapability and CxxValueSemantics requests #85485

susmonteiro · 2025-11-13T12:04:31Z

The ClangTypeEscapability and CxxValueSemantics requests are similar and both relied on recursion, which could cause cycles when the type being analyzed depended on itself. This patch refactors these two requests to remember which types have been seen, preventing these cycles from happening.

susmonteiro · 2025-11-13T12:05:10Z

@swift-ci please smoke test

lib/ClangImporter/ClangImporter.cpp

susmonteiro · 2025-11-17T11:38:33Z

@swift-ci please smoke test

susmonteiro · 2025-11-17T16:27:51Z

@swift-ci please smoke test windows

j-hui

Thanks for this patch Susana! I do have quite a few questions and suggestions for improvement, but I'm happy for you to merge patch mostly as is—this is a big refactor that happened to touch a lot of logic I hadn't closely reviewed before, which is probably orthogonal to what you're trying to do here. Feel free to address my feedback incrementally in multiple follow-up patches, at your discretion.

Aside from what I've already commented on, my main ask is that you check in at least one test case that involves circularity, which would have triggered a compiler error prior to your refactor. I'd like to make sure we have those cases accounted for in our test suite, so that we don't accidentally regress.

Another orthogonal issue here is of diagnostics and how it interacts with cacheing. I'm a little worried that by doing the diagnostics in the request, and mixing and matching it with the short-circuiting, we might encounter weird cases like this, where we should emit a diagnostic, but don't:

Outer<MoveOnlyType, MalformedAnnotations>

where Outer<T, U> is COPYABLE_IF both its parameters are. If we request the copyability of this type, we will consider it move-only due to the MoveOnlyType template parameter without ever encountering the MalformedAnnotations type in our traversal.

If my instincts are right about how the cacheing will work out here, I think we'll also get duplicate diagnostics for MalformedAnnotations for a pair of types like this:

Outer<MalformedAnnotations, int>;
Outer<MalformedAnnotations, float>;

since these two types are cached separately.

I think the right way to deal with this is to do a separate pass for diagnostics, which never short-circuits, though that might lead to more code duplication.

Then again, I believe these may all be short-comings of the existing implementation, and in any case COPYABLE_IF is an opt-in annotation that probably doesn't have much adoption yet, so maybe we're ok on that front.

lib/ClangImporter/ClangImporter.cpp

j-hui · 2025-11-18T18:32:10Z

lib/ClangImporter/ClangImporter.cpp

+      auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);
+      if (recordDecl->getDefinition() &&
+          (!cxxRecordDecl || cxxRecordDecl->isAggregate())) {
+        if (cxxRecordDecl) {
+          for (auto base : cxxRecordDecl->bases())
+            maybePushToStack(base.getType()->getUnqualifiedDesugaredType());
+        }
+        for (auto field : recordDecl->fields())
+          maybePushToStack(field->getType()->getUnqualifiedDesugaredType());


I'm a bit confused by why !cxxRecordDecl || cxxRecordDecl->isAggregate() here. Can you please leave a comment to explain why we're not checking non-aggregate C++ records?

nit: it might make things a little clearer to refactor it like this + rebind recordDecl to its definition if distinct:

Suggested change

auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl);

if (recordDecl->getDefinition() &&

(!cxxRecordDecl || cxxRecordDecl->isAggregate())) {

if (cxxRecordDecl) {

for (auto base : cxxRecordDecl->bases())

maybePushToStack(base.getType()->getUnqualifiedDesugaredType());

}

for (auto field : recordDecl->fields())

maybePushToStack(field->getType()->getUnqualifiedDesugaredType());

if ((recordDecl = recordDecl->getDefinition())) {

auto checkFields = true;

if (auto cxxRecDecl = dyn_cast<clang::CXXRecordDecl>(recordDecl)) {

checkFields = cxxRecDec->isAggregate();

for (auto base : cxxRecDecl->Bases())

// etc.

}

if (checkFields) {

for (auto field : recordDecl->fields())

// etc.

}

For escapability, we do inference and checking separately. The inference is done in this request, while the checking is part of VisitRecordDecl. We only attempt to infer escapability in simple cases (trying to do so in more complex cases caused a few breakages), and that's why this inference is behind the aggregate check.

Having said that, since annotationOnly is always set to true, we never actually get to infer escapability at the moment. This needs some refactoring, which will be done in a follow-up patch.

j-hui · 2025-11-18T18:34:25Z

lib/ClangImporter/ClangImporter.cpp

+        }
+        for (auto field : recordDecl->fields())
+          maybePushToStack(field->getType()->getUnqualifiedDesugaredType());
+        continue;


How are we handling cases of RecordDecls for which we lack a definition, or for non-aggregate CxxRecordDecls?

Shouldn't those raise hasUnknown?

In any case, I suggest handling that case explicitly (or at least leave a comment if there's nothing to do), rather than just let it "fall off the end" like would happen with the current version.

That's a great point. Once again, because annotationOnly is always true, we actually always set hasUnknown if the record doesn't have an escapability annotation. But I agree we should make it explicit! I'll add a comment for now, but will take this into account in the follow-up patch to get rid of annotationOnly

lib/ClangImporter/ClangImporter.cpp

j-hui · 2025-11-18T19:04:16Z

lib/ClangImporter/ClangImporter.cpp

-  const auto *type = desc.type;
+  // A C++ type can be imported to Swift as Copyable or ~Copyable.
+  // We assume a type can be imported as Copyable unless:
+  // - There's no copy constructor


How are we handling cases for (1) CxxRecordDecl vs plain C RecordDecls; (2) and any kind of RecordDecl that lacks a definition?

We currently assume that a RecordDecl that is lacking definition or a RecordDecl that is not a CxxRecordDecl are copyable (unless explicit annotated with SWIFT_NONCOPYABLE). I'll add a comment to the code as well :)

On a second thought, I don't think this is something that needs a comment. The comment you highlighted correctly describes these situations (Copyable unless there's a SWIFT_NONCOPYABLE annotation) and the part of the code where we deal with these is quite explicit, imo:

if (!cxxRecordDecl || !recordDecl->isCompleteDefinition()) { if (hasNonCopyableAttr(recordDecl)) return CxxValueSemanticsKind::MoveOnly; continue; }

What do you think?

susmonteiro · 2025-11-19T13:27:53Z

Thanks for the thorough review @j-hui

Aside from what I've already commented on, my main ask is that you check in at least one test case that involves circularity, which would have triggered a compiler error prior to your refactor. I'd like to make sure we have those cases accounted for in our test suite, so that we don't accidentally regress.

I agree that it's important to add these tests. The thing is the patch that created the cycle was reverted and has not been merged yet - I actually have it on draft here #85559 as I'm waiting for this one to be merged. I don't see a case where the current patch creates a circular reference, but the future one already contains some tests that would trigger a cycle before.

Another orthogonal issue here is of diagnostics and how it interacts with cacheing. I'm a little worried that by doing the diagnostics in the request, and mixing and matching it with the short-circuiting, we might encounter weird cases like this, where we should emit a diagnostic, but don't:

Outer<MoveOnlyType, MalformedAnnotations>
where Outer<T, U> is COPYABLE_IF both its parameters are. If we request the copyability of this type, we will consider it move-only due to the MoveOnlyType template parameter without ever encountering the MalformedAnnotations type in our traversal.

I also agree with your point regarding diagnostics. However, since this would involve some changes and some new diagnostics, I opted to add them on a different patch, which I'm already working on. The diagnostics are important, but if you're fine with that, we'll add them later on.

I'll explore your suggestion to do a separate pass for diagnostics. Perhaps we could check them during import, instead of during the copyability request, and these way we would avoid cycles. Anyways, since I think this should be added in a future PR, we can discuss this offline.

lib/ClangImporter/ClangImporter.cpp

Xazax-hun

Overall looks good, some nits inline.

lib/ClangImporter/ClangImporter.cpp

Xazax-hun · 2025-11-20T10:27:55Z

lib/ClangImporter/ClangImporter.cpp

-
+  bool hasUnknown = false;
  auto desugared = desc.type->getUnqualifiedDesugaredType();
  if (const auto *recordType = desugared->getAs<clang::RecordType>()) {


Do we actually need this code path? Shouldn't this be handled inside the loop already?

Inside of the loop, we don't return from the request when we encounter an escapable type, because there might still be a nonescapable type, which takes precedence. However, if the root type has a SWIFT_ESCAPABLE annotation, then it should be imported as escapable, without further analysis needed.

lib/ClangImporter/ClangImporter.cpp

Xazax-hun · 2025-11-20T10:38:22Z

lib/ClangImporter/ClangImporter.cpp

-        return std::nullopt;
-      };
+  std::function maybePushToStack = [&](const clang::Type *type) {
+    const auto *recordType = type->getAs<clang::RecordType>();


Nit: if we are only interested in the decl we can skip this intermediate step and use the getAsRecordDecl API.

Unfortunately using the getAsRecordDecl API triggers an assertion failure while casting the type in Linux. I'll investigate this further, but for now I'll leave this as is

susmonteiro · 2025-11-20T12:36:20Z

@swift-ci please smoke test

susmonteiro · 2025-11-20T15:37:34Z

@swift-ci please smoke test

susmonteiro · 2025-11-20T18:47:09Z

@swift-ci please smoke test

susmonteiro · 2025-12-01T09:53:11Z

@swift-ci please smoke test

susmonteiro · 2025-12-01T16:17:43Z

@swift-ci please smoke test windows

susmonteiro requested a review from egorzhdan as a code owner November 13, 2025 12:04

susmonteiro added the c++ interop Feature: Interoperability with C++ label Nov 13, 2025

susmonteiro requested review from Xazax-hun, beccadax, hnrklssn, ian-twilightcoder and j-hui as code owners November 13, 2025 12:04

hnrklssn reviewed Nov 13, 2025

View reviewed changes

lib/ClangImporter/ClangImporter.cpp Show resolved Hide resolved

susmonteiro force-pushed the susmonteiro/remove-request-cycles branch from a56eeb1 to ae20b9f Compare November 17, 2025 11:38

susmonteiro mentioned this pull request Nov 18, 2025

[cxx-interop] Reapply implicitly defined copy and move constructors #85559

Closed

j-hui approved these changes Nov 18, 2025

View reviewed changes

Xazax-hun reviewed Nov 19, 2025

View reviewed changes

lib/ClangImporter/ClangImporter.cpp Outdated Show resolved Hide resolved

lib/ClangImporter/ClangImporter.cpp Show resolved Hide resolved

Xazax-hun reviewed Nov 20, 2025

View reviewed changes

susmonteiro force-pushed the susmonteiro/remove-request-cycles branch from ae20b9f to f4337c4 Compare November 20, 2025 12:36

susmonteiro force-pushed the susmonteiro/remove-request-cycles branch from f4337c4 to 1962da2 Compare November 20, 2025 15:35

susmonteiro added 4 commits November 20, 2025 18:41

Make CxxValueSemantics request non-recursive

3516448

Make ClangTypeEscapability request non-recursive

5dfb766

Refactor checkConditionalParams

de52134

Simplify CxxValueSemanticsKind

791194b

susmonteiro force-pushed the susmonteiro/remove-request-cycles branch from 1962da2 to 791194b Compare November 20, 2025 18:43

This was referenced Nov 21, 2025

[cxx-interop] Reapply implicitly defined copy and move constructors #85642

Merged

[cxx-interop] Remove annotationOnly flag from Escapability request #85643

Merged

susmonteiro enabled auto-merge December 1, 2025 09:53

susmonteiro merged commit 92cbcb6 into swiftlang:main Dec 1, 2025
3 checks passed

[cxx-interop] Refactor ClangTypeEscapability and CxxValueSemantics requests #85485

[cxx-interop] Refactor ClangTypeEscapability and CxxValueSemantics requests #85485

Uh oh!

Conversation

susmonteiro commented Nov 13, 2025

Uh oh!

susmonteiro commented Nov 13, 2025

Uh oh!

Uh oh!

susmonteiro commented Nov 17, 2025

Uh oh!

susmonteiro commented Nov 17, 2025

Uh oh!

j-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susmonteiro Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susmonteiro Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susmonteiro commented Nov 19, 2025

Uh oh!

Uh oh!

Uh oh!

Xazax-hun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susmonteiro commented Nov 20, 2025

Uh oh!

susmonteiro commented Nov 20, 2025

Uh oh!

susmonteiro commented Nov 20, 2025

Uh oh!

susmonteiro commented Dec 1, 2025

Uh oh!

susmonteiro commented Dec 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

susmonteiro Nov 19, 2025 •

edited

Loading

susmonteiro Nov 19, 2025 •

edited

Loading