[c++ interop] Swift should allow multiple SWIFT_CONFORMS_TO_PROTOCOL attributes on a C++ class #79425

CrazyFanFan · 2025-02-17T01:54:37Z

Resolves #75831

j-hui

Thanks for your contribution @CrazyFanFan !

What happens when multiple base classes conform to the same protocol? I'm guessing (hoping) the redundant protocol conformance error would catch that, but I'm not actually sure what would happen without it.

lib/ClangImporter/ImportDecl.cpp

test/Interop/Cxx/class/conforms-to-errors.swift

j-hui · 2025-02-17T04:57:31Z

@swift-ci please smoke test

CrazyFanFan · 2025-02-17T05:12:40Z

@j-hui Thank you for the review. CCC tests cases where a single class redundantly implements the same protocol. CCD was intended to test a correct combination, but since the combination issue was already tested in another file, the CCD here is indeed redundant. As for the multiple base classes, it was indeed overlooked, and I'll add it later.

Xazax-hun · 2025-02-17T11:52:12Z

@swift-ci please smoke test

j-hui · 2025-03-04T06:32:35Z

@swift-ci please smoke test

egorzhdan

Awesome, thanks @CrazyFanFan!

I'm not convinced that we need to emit an error for redundant conformances – declaring duplicate conformances seems harmless and could be convenient with some class hierarchies. Do you think we could extract the diagnostic out of this pull request?

egorzhdan · 2025-03-10T15:20:55Z

include/swift/AST/DiagnosticsClangImporter.def

+ERROR(redundant_conformance_protocol,none,
+     "redundant conformance of %0 to protocol '%1'", (Type, StringRef))


I'm not sure if redundant conformances should trigger an error. There are legitimate cases where you would end up with redundant conformances, for instance:

struct Base1 __attribute__((swift_attr("conforms_to:Mod.Proto"))) {}; struct Base2 __attribute__((swift_attr("conforms_to:Mod.Proto"))) {}; struct Derived : Base1, Base2 {};

Would the compiler emit an error for this code?

Thank you for your review. I have added a flag to skip the checks for protocol conformance originating from bases. With this change, redundant conformance errors will only occur when a type itself declares conformance to the same protocol multiple times.

@Xazax-hun @j-hui what's your take on this diagnostic?

Sorry for missing the ping!

I'll say first that I share Egor's reservations here: I don't see what is semantically wrong with declaring duplicate conformances.

Insofar as consistency with the language conventions, I'm also unsure. Swift throws an error when you write something like struct S : P, P {}, which is inline with @CrazyFanFan's design here. Meanwhile, C/C++ are fairly lenient with respect to redundant specifiers: something like const const unsigned unsigned int * __nullable __nullable x; only raises warnings.

From a strategic standpoint, it's more difficult to convert an error into a warning (or remove it altogether), so unless there's a compelling use case for specifying redundant conformances, perhaps we should treat that as an error for now (i.e., keep this code as it is).

it's more difficult to convert an error into a warning

Could you elaborate on this? I'm not quite following what this means

Oops, I meant to say, it's less difficult to convert an error into a warning (than the other way around).

Aha! Agreed

egorzhdan

Thanks, I have a small comment, but this look exciting!

egorzhdan · 2025-03-28T13:02:36Z

lib/ClangImporter/ImportDecl.cpp

-          });
-      if (conformsToAttr == clangDecl->getAttrs().end())
-        return;
+      DeclAttribute *baseAttrsSentry = *decl->getAttrs().begin();


Having a long-lived iterator like this makes me worried about possible iterator invalidation. Can we call getAttrs().add(...) to add the attributes instead?

@egorzhdan Do you mean insert a sentry into attrs list?

Yeah, I'm just thinking if we can avoid having a long-lived iterator like this. If we can have a list instead of it, that sounds great!

The baseAttrsSentry is used only within the scope of addExplicitProtocolConformance, and it appears to be safe in the current context.
I also attempted to add a SentryAttribute to clangDecl->getAttrs(), but encountered an issue: a new DeclAttrKind is required to correctly identify the SentryAttribute in the list, which seems excessive for this purpose.

Using a sentry here like this seems to depend on the order of decl->getAttrs().getAttributes() being stable, and the implementation of decl->getAttrs().add() pushing to the front of a linked list. I'm not sure we should rely on these implementation details. Until looking at those details, I it wasn't immediately obvious to me how or why this code worked.

Can we instead create a SmallSet<ProtocolDecl *> that we populate across different calls to addExplicitProtocolConformance(), and use that to check for redundant conformances?

egorzhdan · 2025-03-28T13:04:34Z

@swift-ci please smoke test

egorzhdan · 2025-04-01T14:39:39Z

@swift-ci please smoke test Windows

j-hui

This mostly looks good to me! I'm fine with treating redundant protocols as a compiler error, but I'm concerned about the implementation of it. I'd like that to be addressed (and will give explicit approval if it is) but don't consider it to be blocking this PR.

j-hui · 2025-04-02T16:35:17Z

include/swift/AST/DiagnosticsClangImporter.def

+ERROR(redundant_conformance_protocol,none,
+     "redundant conformance of %0 to protocol '%1'", (Type, StringRef))


Sorry for missing the ping!

I'll say first that I share Egor's reservations here: I don't see what is semantically wrong with declaring duplicate conformances.

Insofar as consistency with the language conventions, I'm also unsure. Swift throws an error when you write something like struct S : P, P {}, which is inline with @CrazyFanFan's design here. Meanwhile, C/C++ are fairly lenient with respect to redundant specifiers: something like const const unsigned unsigned int * __nullable __nullable x; only raises warnings.

From a strategic standpoint, it's more difficult to convert an error into a warning (or remove it altogether), so unless there's a compelling use case for specifying redundant conformances, perhaps we should treat that as an error for now (i.e., keep this code as it is).

j-hui · 2025-04-02T17:13:45Z

lib/ClangImporter/ImportDecl.cpp

-          });
-      if (conformsToAttr == clangDecl->getAttrs().end())
-        return;
+      DeclAttribute *baseAttrsSentry = *decl->getAttrs().begin();


Using a sentry here like this seems to depend on the order of decl->getAttrs().getAttributes() being stable, and the implementation of decl->getAttrs().add() pushing to the front of a linked list. I'm not sure we should rely on these implementation details. Until looking at those details, I it wasn't immediately obvious to me how or why this code worked.

Can we instead create a SmallSet<ProtocolDecl *> that we populate across different calls to addExplicitProtocolConformance(), and use that to check for redundant conformances?

j-hui · 2025-04-03T18:01:22Z

@swift-ci please smoke test

j-hui

Thank you for addressing my comments @CrazyFanFan ! This looks great to me.

j-hui · 2025-04-03T18:04:07Z

test/Interop/Cxx/class/conforms-to-errors.swift

+// CHECK-NOT: error: redundant conformance of 'CDD' to protocol 'SwiftTest.D'
+// CHECK-NOT: error: redundant conformance of 'CCDCD2' to protocol 'SwiftTest.D'
+// CHECK-NOT: error: redundant conformance of 'CDE' to protocol 'SwiftTest.D'
+func test(_ dd: CDD, _ dd2: CCDCD2, de: CDE) {}


nit: add newline at end of file

egorzhdan

This looks great, thanks @CrazyFanFan!
I have one last comment, everything besides it looks awesome and good to be merged.

egorzhdan · 2025-04-04T11:53:58Z

lib/ClangImporter/ImportDecl.cpp

@@ -3040,6 +3041,8 @@ namespace {
      return result;
    }

+    using ProtocolSet = llvm::SmallSet<ProtocolDecl *, 4>;


Since this is a Set, I wonder if the non-deterministic order of protocols would bite us later. Looks like the compiler might start emitting diagnostics in a different order. These kind of things are usually harmless to users, but make testing harder. Could we switch to a SmallVector here to preserve the order here?

Currently, the set is only used to record whether a protocol has been added. The order of protocol addition and the generation of diagnostics is determined by clangDecl->getAttrs() in llvm::for_each(clangDecl->getAttrs(), [&](auto *attr) {}). As long as the order of clangDecl->getAttrs() remains unchanged, the set will not affect the order in which diagnostics are generated.

Ah, I see, thanks!

…attributes on a C++ class.

CrazyFanFan · 2025-04-06T14:53:08Z

@egorzhdan I have resolved the conflicts with the main branch. Could you please help to re-trigger the test job?

Xazax-hun · 2025-04-06T18:08:32Z

@swift-ci please smoke test

CrazyFanFan · 2025-04-07T12:06:07Z

I noticed that my PR test failed, but it seems to be unrelated to the changes I made.

Xazax-hun · 2025-04-07T12:08:39Z

@swift-ci please smoke test macos

CrazyFanFan requested review from zoecarver, hyp, egorzhdan, Xazax-hun, j-hui, fahadnayyar, beccadax and ian-twilightcoder as code owners February 17, 2025 01:54

j-hui reviewed Feb 17, 2025

View reviewed changes

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

test/Interop/Cxx/class/conforms-to-errors.swift Outdated Show resolved Hide resolved

CrazyFanFan force-pushed the feature/cxx_multiple_swift_conforms_to_protocol branch from 089780d to 868e999 Compare February 17, 2025 05:35

egorzhdan added the c++ interop Feature: Interoperability with C++ label Feb 28, 2025

CrazyFanFan force-pushed the feature/cxx_multiple_swift_conforms_to_protocol branch from 0eb22a6 to 1f89ba1 Compare March 3, 2025 23:17

CrazyFanFan requested a review from j-hui March 4, 2025 02:13

egorzhdan reviewed Mar 10, 2025

View reviewed changes

CrazyFanFan force-pushed the feature/cxx_multiple_swift_conforms_to_protocol branch from 1f89ba1 to 34d9804 Compare March 11, 2025 05:14

CrazyFanFan marked this pull request as draft March 11, 2025 05:31

CrazyFanFan force-pushed the feature/cxx_multiple_swift_conforms_to_protocol branch from 34d9804 to add29b1 Compare March 11, 2025 08:47

CrazyFanFan marked this pull request as ready for review March 11, 2025 08:51

CrazyFanFan requested a review from egorzhdan March 11, 2025 13:39

egorzhdan reviewed Mar 28, 2025

View reviewed changes

This comment was marked as spam.

Sign in to view

j-hui reviewed Apr 2, 2025

View reviewed changes

CrazyFanFan requested review from susmonteiro and hnrklssn as code owners April 3, 2025 03:08

CrazyFanFan force-pushed the feature/cxx_multiple_swift_conforms_to_protocol branch from da7931d to 14235e9 Compare April 3, 2025 03:36

CrazyFanFan requested review from egorzhdan and j-hui April 3, 2025 16:07

j-hui approved these changes Apr 3, 2025

View reviewed changes

egorzhdan reviewed Apr 4, 2025

View reviewed changes

egorzhdan approved these changes Apr 4, 2025

View reviewed changes

[c++ interop] Swift should allow multiple SWIFT_CONFORMS_TO_PROTOCOL …

c05d1fc

…attributes on a C++ class.

CrazyFanFan force-pushed the feature/cxx_multiple_swift_conforms_to_protocol branch from 7ca46dc to c05d1fc Compare April 6, 2025 14:50

egorzhdan merged commit 91e730a into swiftlang:main Apr 7, 2025
3 checks passed

CrazyFanFan deleted the feature/cxx_multiple_swift_conforms_to_protocol branch April 7, 2025 22:28

		ERROR(redundant_conformance_protocol,none,
		"redundant conformance of %0 to protocol '%1'", (Type, StringRef))

[c++ interop] Swift should allow multiple SWIFT_CONFORMS_TO_PROTOCOL attributes on a C++ class #79425

[c++ interop] Swift should allow multiple SWIFT_CONFORMS_TO_PROTOCOL attributes on a C++ class #79425

Uh oh!

Conversation

CrazyFanFan commented Feb 17, 2025

Uh oh!

j-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

j-hui commented Feb 17, 2025

Uh oh!

CrazyFanFan commented Feb 17, 2025

Uh oh!

Xazax-hun commented Feb 17, 2025

Uh oh!

j-hui commented Mar 4, 2025

Uh oh!

egorzhdan left a comment

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!

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!

j-hui Apr 3, 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!

egorzhdan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CrazyFanFan Mar 31, 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!

egorzhdan Apr 1, 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!

Choose a reason for hiding this comment

Uh oh!

egorzhdan commented Mar 28, 2025

Uh oh!

This comment was marked as spam.

egorzhdan commented Apr 1, 2025

Uh oh!

j-hui left a comment

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!

j-hui commented Apr 3, 2025

Uh oh!

j-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorzhdan left a comment

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

j-hui Apr 3, 2025 •

edited

Loading

CrazyFanFan Mar 31, 2025 •

edited

Loading

egorzhdan Apr 1, 2025 •

edited

Loading