Change objcImpl syntax to @objc @implementation
#73309
Conversation
beccadax
force-pushed
the
objcimpl-category-on-objc
branch
from
e1ef9d1
to
80b9929
Compare
|
@swift-ci please test |
This contains the category name, if there is one.
This now specifies a category name that’s used in TBDGen, IRGen, and PrintAsClang. There are also now category name conflict diagnostics; these subsume some @implementation diagnostics. (It turns out there was already a check for @objc(CustomName) to make sure it wasn’t a selector!)
Their syntaxes are about to diverge, so let’s make sure that we maintain source compatibility for @_objcImplementation.
…for extensions. This change also removes @implementation(CategoryName); you should attach the category name to the @objc attribute instead.
beccadax
force-pushed
the
objcimpl-category-on-objc
branch
from
80b9929
to
cc8edf0
Compare
beccadax
requested review from
hborla,
slavapestov,
xedin,
zoecarver,
hyp,
egorzhdan,
ahoppen,
bnbarham,
CodaFi,
DougGregor,
hamishknight and
rintaro
as code owners
|
@swift-ci please test |
Previous changes have made it so that `private let`s in an objcImpl extension are implicitly `@objc`, which changed downstream behavior in a SILOptimizer test. The new behavior is actually more correct, so codify this behavior change by modifying the test.
|
@swift-ci please test |
|
@swift-ci Please Build Toolchain macOS Platform |
|
@swift-ci Please Build Toolchain Linux Platform |
| return; | ||
|
|
||
| // Use a linear scan on the assumption that there aren't very many extensions. | ||
| for (auto *other : getExtensions()) { |
There was a problem hiding this comment.
It's probably not a performance problem in practice, but I do worry about things like generated code breaking this kind of assumption. This could be pretty easily transformed into a request that caches an Obj-C category name to ExtensionDecl map.
There was a problem hiding this comment.
I probably don't want the map to be per-ClassDecl because the vast majority of classes won't have any extensions with a category name, but maybe it can be per-module or something, with a std::pair<ClassDecl *, Identifier> key.
There was a problem hiding this comment.
Request result storage goes in a DenseMap unless you choose to customize it to be inline, and you'd only need to allocate the extension map result for classes that have recordObjCCategory() called on them so in practice I think it wouldn't create much overhead to do it per-class.
There was a problem hiding this comment.
The problem is that adding an extension invalidates the lookup table, and recordCategory(ext) is called immediately after ext is added to its class, so it's likely the table will be incomplete. But if we defer the conflict-checking logic until after we've added all the extensions to the class, we can then use a lookup table to accelerate conflict checking. It's just a bigger redesign of this logic than "drop a lookup table into this existing function", which is what I thought you were suggesting!
There was a problem hiding this comment.
I see, that makes sense. If you were to delay this check to after all extensions were registered, no extra storage would be needed I guess because it can just be a single pass over the complete list of extensions.
|
Won't be able to build a toolchain until apple/llvm-project#8714 merges. |
| @@ -3614,6 +3812,18 @@ class ObjCImplementationChecker { | |||
| } | |||
| } | |||
|
|
|||
| static SmallString<64> makeObjCAttrInsertionText(Identifier categoryName) { | |||
There was a problem hiding this comment.
Nitpick: is there a way we could share the two copies of makeObjCAttrInsertionText?
There was a problem hiding this comment.
Yeah, I was being a little lazy here. Let me see if I can clean this up.
|
@swift-ci please build toolchain |
|
Toolchains:
(Of course, this is only really going to work on Apple platforms, since it’s an ObjC interop feature.) |
This PR changes the type-checking rules for objcImpl to require an
@objcattribute. It also moves the category name from the@implementationattribute to the@objcattribute. Backwards compatibility is maintained for the pre-stabilization@_objcImplementationsyntax.As part of this change, plain
@objc(CustomName)on anextensionnow changes its category name when it is printed throughPrintAsClangor IRGen'd; the name previously was parsed and partially validated but ignored. The only new diagnostic related to this is a warning in Swift 5 mode but will be an error in Swift 6 mode.This draft currently incorporates commits from #73128 and will need to be rebased once that lands, so don't worry about the gigantic Reviewers list it has right now.