Skip to content

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

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
Apr 7, 2025
Merged

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

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
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsClangImporter.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ ERROR(both_returns_retained_returns_unretained, none,
"SWIFT_RETURNS_UNRETAINED",
(const clang::NamedDecl *))

ERROR(redundant_conformance_protocol,none,
"redundant conformance of %0 to protocol '%1'", (Type, StringRef))
Comment on lines +297 to +298
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 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@j-hui j-hui Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha! Agreed


ERROR(returns_retained_or_returns_unretained_for_non_cxx_frt_values, none,
"%0 cannot be annotated with either SWIFT_RETURNS_RETAINED or "
"SWIFT_RETURNS_UNRETAINED because it is not returning "
Expand Down
49 changes: 32 additions & 17 deletions lib/ClangImporter/ImportDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@

#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/ADT/StringExtras.h"
Expand Down Expand Up @@ -3105,6 +3106,8 @@ namespace {
return result;
}

using ProtocolSet = llvm::SmallSet<ProtocolDecl *, 4>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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, thanks!


void
addExplicitProtocolConformances(NominalTypeDecl *decl,
const clang::CXXRecordDecl *clangDecl) {
Expand All @@ -3119,57 +3122,69 @@ namespace {
if (!clangDecl->hasAttrs())
return;

SmallVector<ValueDecl *, 1> results;
auto conformsToAttr =
llvm::find_if(clangDecl->getAttrs(), [](auto *attr) {
if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr))
return swiftAttr->getAttribute().starts_with("conforms_to:");
return false;
});
if (conformsToAttr == clangDecl->getAttrs().end())
return;
ProtocolSet alreadyAdded;
llvm::for_each(clangDecl->getAttrs(), [&](auto *attr) {
if (auto swiftAttr = dyn_cast<clang::SwiftAttrAttr>(attr)) {
if (swiftAttr->getAttribute().starts_with("conforms_to:"))
addExplicitProtocolConformance(decl, swiftAttr, alreadyAdded);
}
});
}

auto conformsToValue = cast<clang::SwiftAttrAttr>(*conformsToAttr)
->getAttribute()
void addExplicitProtocolConformance(NominalTypeDecl *decl,
clang::SwiftAttrAttr *conformsToAttr,
ProtocolSet &alreadyAdded) {
auto conformsToValue = conformsToAttr->getAttribute()
.drop_front(StringRef("conforms_to:").size())
.str();
auto names = StringRef(conformsToValue).split('.');
auto moduleName = names.first;
auto protocolName = names.second;
if (protocolName.empty()) {
HeaderLoc attrLoc((*conformsToAttr)->getLocation());
HeaderLoc attrLoc(conformsToAttr->getLocation());
Impl.diagnose(attrLoc, diag::conforms_to_missing_dot, conformsToValue);
return;
}

auto *mod = Impl.SwiftContext.getModuleByIdentifier(
Impl.SwiftContext.getIdentifier(moduleName));
if (!mod) {
HeaderLoc attrLoc((*conformsToAttr)->getLocation());
HeaderLoc attrLoc(conformsToAttr->getLocation());
Impl.diagnose(attrLoc, diag::cannot_find_conforms_to_module,
conformsToValue, moduleName);
return;
}

SmallVector<ValueDecl *, 1> results;
mod->lookupValue(Impl.SwiftContext.getIdentifier(protocolName),
NLKind::UnqualifiedLookup, results);
if (results.empty()) {
HeaderLoc attrLoc((*conformsToAttr)->getLocation());
HeaderLoc attrLoc(conformsToAttr->getLocation());
Impl.diagnose(attrLoc, diag::cannot_find_conforms_to, protocolName,
moduleName);
return;
} else if (results.size() != 1) {
HeaderLoc attrLoc((*conformsToAttr)->getLocation());
}

if (results.size() != 1) {
HeaderLoc attrLoc(conformsToAttr->getLocation());
Impl.diagnose(attrLoc, diag::conforms_to_ambiguous, protocolName,
moduleName);
return;
}

auto result = results.front();
if (auto protocol = dyn_cast<ProtocolDecl>(result)) {
auto [_, inserted] = alreadyAdded.insert(protocol);
if (!inserted) {
HeaderLoc attrLoc(conformsToAttr->getLocation());
Impl.diagnose(attrLoc, diag::redundant_conformance_protocol,
decl->getDeclaredInterfaceType(), conformsToValue);
}

decl->getAttrs().add(
new (Impl.SwiftContext) SynthesizedProtocolAttr(protocol, &Impl, false));
} else {
HeaderLoc attrLoc((*conformsToAttr)->getLocation());
HeaderLoc attrLoc((conformsToAttr)->getLocation());
Impl.diagnose(attrLoc, diag::conforms_to_not_protocol, result,
conformsToValue);
}
Expand Down
9 changes: 9 additions & 0 deletions test/Interop/Cxx/class/Inputs/conforms-to.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ struct
void play() const;
};

struct __attribute__((swift_attr("conforms_to:SwiftTest.Testable")))
__attribute__((swift_attr(
"conforms_to:SwiftTest.Playable"))) MultipleConformanceHasTestAndPlay {
void test() const;
void play() const;
};

struct
__attribute__((swift_attr("conforms_to:ImportedModule.ProtocolFromImportedModule")))
HasImportedConf {
Expand All @@ -24,6 +31,8 @@ struct

struct DerivedFromHasTest : HasTest {};
struct DerivedFromDerivedFromHasTest : HasTest {};
struct DerivedFromMultipleConformanceHasTestAndPlay
: MultipleConformanceHasTestAndPlay {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.Testable")))
DerivedFromDerivedFromHasTestWithDuplicateArg : HasTest {};
Expand Down
129 changes: 128 additions & 1 deletion test/Interop/Cxx/class/conforms-to-errors.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,65 @@ struct __attribute__((swift_attr("conforms_to:SwiftTest.X"))) CX {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.A"))) CA {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.B"))) CB {};

struct __attribute__((swift_attr("conforms_to:X")))
__attribute__((swift_attr("conforms_to:X"))) CXX {};
struct __attribute__((swift_attr("conforms_to:X")))
__attribute__((swift_attr("conforms_to:Mod.X"))) CXModX {};
struct __attribute__((swift_attr("conforms_to:X")))
__attribute__((swift_attr("conforms_to:SwiftTest.X"))) CXTextX {};
struct __attribute__((swift_attr("conforms_to:X")))
__attribute__((swift_attr("conforms_to:SwiftTest.A"))) CXA {};
struct __attribute__((swift_attr("conforms_to:X")))
__attribute__((swift_attr("conforms_to:SwiftTest.B"))) CXB {};
struct __attribute__((swift_attr("conforms_to:X")))
__attribute__((swift_attr("conforms_to:SwiftTest.C"))) CXC {};


struct __attribute__((swift_attr("conforms_to:Mod.X")))
__attribute__((swift_attr("conforms_to:Mod.X"))) CModXModX {};
struct __attribute__((swift_attr("conforms_to:Mod.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.X"))) CModXTestX {};
struct __attribute__((swift_attr("conforms_to:Mod.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.A"))) CModXA {};
struct __attribute__((swift_attr("conforms_to:Mod.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.B"))) CModXB {};
struct __attribute__((swift_attr("conforms_to:Mod.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.C"))) CModXC {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.X"))) CTestXTextX {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.A"))) CTextXA {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.B"))) CTextXB {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.X")))
__attribute__((swift_attr("conforms_to:SwiftTest.C"))) CTextXC {};


struct __attribute__((swift_attr("conforms_to:SwiftTest.A")))
__attribute__((swift_attr("conforms_to:SwiftTest.A"))) CAA {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.A")))
__attribute__((swift_attr("conforms_to:SwiftTest.B"))) CAB {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.A")))
__attribute__((swift_attr("conforms_to:SwiftTest.C"))) CAC {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.B")))
__attribute__((swift_attr("conforms_to:SwiftTest.B"))) CBB {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.B")))
__attribute__((swift_attr("conforms_to:SwiftTest.C"))) CBC {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.C")))
__attribute__((swift_attr("conforms_to:SwiftTest.C"))) CCC {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.D"))) CD {};
struct __attribute__((swift_attr("conforms_to:SwiftTest.D"))) CDD: CD {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.D"))) CD2 {};
struct CCDCD2 : CD, CD2 {};

struct __attribute__((swift_attr("conforms_to:SwiftTest.D")))
__attribute__((swift_attr("conforms_to:SwiftTest.E"))) CDE {};

//--- test.swift

import Test
Expand All @@ -25,10 +84,78 @@ struct B {}
protocol A {}
protocol A {}

protocol C {}
protocol D {}
protocol E: D {}

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?
// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'
// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'
// CHECK: error: struct 'B' referenced in protocol conformance 'SwiftTest.B' is not a protocol

func test(_ inv: CInv, _ invMod: CModInv, _ x: CX, _ a: CA, _ b: CB) {}

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
// CHECK: error: struct 'B' referenced in protocol conformance 'SwiftTest.B' is not a protocol

// CHECK: error: expected module name and protocol name separated by '.' in protocol conformance; 'X' is invalid
func test(_ xx: CXX, _ xModx: CXModX, _ xTextX: CXTextX, _ cxa: CXA, _ cxb: CXB, _ cxc: CXC) {}

// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?
// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?

// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?
// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'

// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?
// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'

// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?
// CHECK: error: struct 'B' referenced in protocol conformance 'SwiftTest.B' is not a protocol

// CHECK: module 'Mod' in specified protocol conformance 'Mod.X' is not found; did you mean to import it first?
func test(_ modXModX: CModXModX, _ modXTestX: CModXTestX, _ modXA: CModXA, _ modXB: CModXB, _ modXC: CModXC) {}

// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'
// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'

// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'
// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'

// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'
// CHECK: error: struct 'B' referenced in protocol conformance 'SwiftTest.B' is not a protocol

// CHECK: error: protocol 'X' in specified protocol conformance is not found in module 'SwiftTest'
func test(_ testXTextX: CTestXTextX, _ textXA: CTextXA, _ textXB: CTextXB, _ textXC: CTextXC) {}

// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'
// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'

// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'
// CHECK: error: struct 'B' referenced in protocol conformance 'SwiftTest.B' is not a protocol

// CHECK: error: ambiguous reference to protocol 'A' in specified protocol conformance; module 'SwiftTest' contains multiple protocols named 'A'
func test(_ aa: CAA, _ ab: CAB, _ ac: CAC) {}

// CHECK: error: struct 'B' referenced in protocol conformance 'SwiftTest.B' is not a protocol
func test(_ bb: CBB, _ bc: CBC) {}

// CHECK: error: redundant conformance of 'CCC' to protocol 'SwiftTest.C'
func test(_ cc: CCC) {}

// 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) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add newline at end of file

11 changes: 11 additions & 0 deletions test/Interop/Cxx/class/conforms-to.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,24 @@ func callee(_ _: Playable) {
func caller(_ x: Playable) {
callee(x)
}

func caller(_ x: MultipleConformanceHasTestAndPlay) {
callee(x as Testable)
callee(x as Playable)
}

func caller(_ x: DerivedFromHasPlay) { callee(x) }
func caller(_ x: DerivedFromDerivedFromHasPlay) { callee(x) }
func caller(_ x: DerivedFromMultipleConformanceHasTestAndPlay) {
callee(x as Testable)
callee(x as Playable)
}

func caller(_ x: HasTestAndPlay) {
callee(x as Testable)
callee(x as Playable)
}

func caller(_ x: DerivedFromHasTestAndPlay) {
callee(x as Testable)
callee(x as Playable)
Expand Down