[cxx-interop] Allow anonymous enums to be imported as non-constants. #42223

zoecarver · 2022-04-06T20:03:08Z

No description provided.

zoecarver · 2022-04-06T20:03:51Z

@swift-ci please test.

zoecarver · 2022-04-06T20:04:06Z

@swift-ci please test source compatibility.

zoecarver · 2022-04-06T20:04:39Z

@swift-ci Please Test Source Compatibility

beccadax · 2022-04-06T23:53:50Z

test/Interop/Cxx/enum/Inputs/anonymous-with-swift-name.h

+#ifndef TEST_INTEROP_CXX_ENUM_INPUTS_ANONYMOUS_WITH_SWIFT_NAME_H
+#define TEST_INTEROP_CXX_ENUM_INPUTS_ANONYMOUS_WITH_SWIFT_NAME_H
+
+#define SOME_OPTIONS(_type, _name) __attribute__((swift_name("_TYPEDEF_" #_name))) _type _name; enum __attribute__((swift_name(#_name))) __attribute__((flag_enum,enum_extensibility(open))) : _type


I don't think this pattern will work for us, if only because users may want to use swift_name() to give their option sets custom names:

typedef SOME_OPTIONS(unsigned, SOColorMask) { kSOColorMaskRed = (1 << 1), kSOColorMaskGreen = (1 << 2), kSOColorMaskBlue = (1 << 3), kSOColorMaskAll = ~0U } __attribute__((swift_name("SOColorOptions"))); // Oops! Now the name relationship between typedef and enum is totally busted.

However, I think your general approach of modifying the macro to link the typedef and enum together by name is a good one. It just needs to be done by the clang name, not the Swift name. For instance, suppose we extended __attribute__((flag_enum)) to allow an optional (C++) type name; we could then link the two by their clang names, leaving the swift_name attribute to do its current job:

#define SOME_OPTIONS(_type, _name) _type _name; \ enum __attribute__((flag_enum(_name),enum_extensibility(open))) : _type

(You could instead introduce a new attribute—say, anonymous_typedef(TypeName)—to represent this link; it wouldn't be much different.)

Another way to do it might be to require the typedef to be used as the underlying type of the anonymous enum, like so:

#define SOME_OPTIONS(_type, _name) _type _name; \ enum __attribute__((flag_enum,enum_extensibility(open))) : _name // Note that we're using `: _name` instead of `: _type`.

I believe these are equivalent to C++ (right?), but we can give them different meanings to Swift.

(There is a danger that someone might accidentally turn, say, NSInteger into an options enum by using the old macro with the new compiler. Perhaps you'd need to also mark the typedef with flag_enum—this might require an extension to clang so that it would allow the attribute to be applied to typedefs.)

In any case, we're going to want attributes applied to the enum to also be respected when we import the typedef. Actually, what we probably want is for ImportDecl.cpp:canSkipOverTypedef() to tell us to import the enum in the typedef's place, but also fall back to the typedef's name if there isn't a swift_name attribute (which I assume would require changes to ImportName.cpp).

zoecarver · 2022-04-13T21:47:36Z

@swift-ci please test.

zoecarver · 2022-04-14T00:14:21Z

lib/ClangImporter/ImportEnumInfo.cpp

+    if (!isa<clang::TypedefType>(decl->getIntegerType().getTypePtr()) ||
+        // If the typedef is available in Swift, the user will get ambiguity.
+        // It also means they may not have intended this API to be imported like this.
+        !unavailableOnSwift(cast<clang::TypedefType>(decl->getIntegerType().getTypePtr())->getDecl())) {


Not sure this is an acceptable heuristic. We may have to add another attr to make sure this is actually what the user wants. CC @beccadax

We talked this over and we think this will work.

The deal here is that:

Swift will import both the typedef and the enum.

The typedef will be unavailable from Swift.

Name lookup sets aside unavailable-from-Swift declarations and only uses them in diagnostics when there are no available decls, so the typedef import should be harmless.

So from the user's perspective, marking the typedef as unavailable-in-Swift means that the typedef should be set aside so the enum can be imported instead.

@beccadax I am a little confused. If the typedef is ignored how does swift know for instance that NSEnumerationOptions=Int ?

I applied this patch and modified CFAvailability.h on my local machine and updated these module dumps:

https://github.dev/plotfi/cxx-interop-diff/pull/1

Taking a look at Foundation.NSSet it seems without the typedef we get func objects(withOptions opts: Int, instead of func objects(withOptions opts: NSEnumerationOptions,

If the typedef is ignored how does swift know for instance that NSEnumerationOptions=Int ?

We set the enum's name as the typdef's name and the base type as the typedef's type. We are able to derive this information from the enum's base type (which is the typedef, now).

Taking a look at Foundation.NSSet it seems without the typedef we get func objects(withOptions opts: Int, instead of func objects(withOptions opts: NSEnumerationOptions,

Yeah, I think that's the behavior we want. opts should be an enum (where the raw value is an Int), not a plain Int. Does that make sense?

I agree with the parameter being a raw Int, but could there be anyway that we insert a typealias NSEnumerationOptions = Int and use that as the parameter type? Is NSEnumerationOptions not a raw Int here?

zoecarver · 2022-04-14T00:14:42Z

@swift-ci please test.

zoecarver · 2022-04-14T16:54:10Z

@swift-ci please test.

zoecarver · 2022-04-14T21:55:58Z

@swift-ci Please Test Source Compatibility

beccadax

I have some changes to request, but the basic approach looks workable to me.

beccadax · 2022-04-14T22:34:16Z

lib/ClangImporter/ImportEnumInfo.cpp

+    // If there's no decl name, just use the prefix we already created.
+    if (!decl->hasNameForLinkage()) {
+      constantNamePrefix = commonPrefix;
+      return;


I don't like this—the type name contributes to the prefix computation for a reason. We should probably continue to the code below with enumNameStr containing the underlying typedef's name instead of the enum's lack-of-name.

(Code structuring note: there might be a case for a "get C name of type" helper which encompasses checking the decl's name, checking its linkage name, and then this special check for an underlying typedef. There might even be such a helper already that you could add to—I'm not sure.)

lib/ClangImporter/ImporterImpl.h

test/Interop/Cxx/enum/anonymous-with-swift-name.swift

beccadax · 2022-04-14T22:46:36Z

lib/ClangImporter/ImportEnumInfo.cpp

+    if (!isa<clang::TypedefType>(decl->getIntegerType().getTypePtr()) ||
+        // If the typedef is available in Swift, the user will get ambiguity.
+        // It also means they may not have intended this API to be imported like this.
+        !unavailableOnSwift(cast<clang::TypedefType>(decl->getIntegerType().getTypePtr())->getDecl())) {


We talked this over and we think this will work.

The deal here is that:

Swift will import both the typedef and the enum.

The typedef will be unavailable from Swift.

Name lookup sets aside unavailable-from-Swift declarations and only uses them in diagnostics when there are no available decls, so the typedef import should be harmless.

So from the user's perspective, marking the typedef as unavailable-in-Swift means that the typedef should be set aside so the enum can be imported instead.

< #define CF_OPTIONS(_type, _name) _type __attribute__((availability(swift, unavailable))) _name; enum __CF_OPTIONS_ATTRIBUTES : _name --- > #define CF_OPTIONS(_type, _name) _type _name; enum __CF_OPTIONS_ATTRIBUTES : _type along with Zoe's swiftlang/swift#42223

If an anonymous enum inherits from a typedef, it will have the typedef's name.

zoecarver · 2022-04-18T18:06:24Z

Thank you for the review, Becca! Updated based on your comments :)

zoecarver · 2022-04-18T18:06:31Z

@swift-ci please test.

zoecarver · 2022-04-18T18:06:35Z

@swift-ci Please Test Source Compatibility

zoecarver · 2022-04-18T20:01:38Z

Please Test Source Compatibility

There were actually not logic changes between this patch and the last one, and the source compatibility passed last time (technically it failed because of a UPass, but only with one project, and that project is known to pass on main).

zoecarver · 2022-04-18T21:17:28Z

test/Interop/Cxx/enum/anonymous-with-swift-name.swift

+  let red: SOColorMask = .red
+  let green = SOColorMask.green
+  let blue = .blue as SOColorMask
+  let all: SOColorMask = .all


These need to be CF

Fixed in: 1d74713

zoecarver · 2022-04-18T21:41:25Z

For the record: the source compatibility test succeeded.

zoecarver added the c++ interop Feature: Interoperability with C++ label Apr 6, 2022

zoecarver requested review from hyp and beccadax April 6, 2022 20:03

zoecarver force-pushed the cxx-enum-pattern branch from 3487ef6 to 3057d0d Compare April 6, 2022 20:03

beccadax reviewed Apr 6, 2022

View reviewed changes

zoecarver force-pushed the cxx-enum-pattern branch from 3057d0d to 588fa53 Compare April 13, 2022 21:47

zoecarver force-pushed the cxx-enum-pattern branch from 588fa53 to 8286f10 Compare April 14, 2022 00:13

zoecarver commented Apr 14, 2022

View reviewed changes

zoecarver force-pushed the cxx-enum-pattern branch from 8286f10 to 150061a Compare April 14, 2022 16:52

beccadax requested changes Apr 14, 2022

View reviewed changes

plotfi mentioned this pull request Apr 16, 2022

[C++-Interop] Teach omitNeedlessWords to handle raw integer C-enums in C++ #42412

Merged

[cxx-interop] Allow anonymous enums to use the name of their base.

965c7ca

If an anonymous enum inherits from a typedef, it will have the typedef's name.

zoecarver force-pushed the cxx-enum-pattern branch from 150061a to 965c7ca Compare April 18, 2022 18:06

zoecarver commented Apr 18, 2022

View reviewed changes

zoecarver mentioned this pull request Apr 18, 2022

[cxx-interop] Import enum, not typedef for parameter types. #42431

Merged

zoecarver merged commit b8684af into swiftlang:main Apr 18, 2022

[cxx-interop] Allow anonymous enums to be imported as non-constants. #42223

[cxx-interop] Allow anonymous enums to be imported as non-constants. #42223

Uh oh!

Conversation

zoecarver commented Apr 6, 2022

Uh oh!

zoecarver commented Apr 6, 2022

Uh oh!

zoecarver commented Apr 6, 2022

Uh oh!

zoecarver commented Apr 6, 2022

Uh oh!

beccadax Apr 6, 2022 • 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!

zoecarver commented Apr 13, 2022

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!

zoecarver commented Apr 14, 2022

Uh oh!

zoecarver commented Apr 14, 2022

Uh oh!

zoecarver commented Apr 14, 2022

Uh oh!

beccadax 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Apr 18, 2022

Uh oh!

zoecarver commented Apr 18, 2022

Uh oh!

zoecarver commented Apr 18, 2022

Uh oh!

zoecarver commented Apr 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Apr 18, 2022

Uh oh!

Uh oh!

beccadax Apr 6, 2022 •

edited

Loading