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.
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 |
1 similar comment
|
@swift-ci please test |
|
@swift-ci Please Build Toolchain macOS Platform |
|
@swift-ci Please Build Toolchain Linux Platform |
lib/AST/NameLookup.cpp
Outdated
| 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.
There was a problem hiding this comment.
Done. While I was working on it, I discovered that some of my request caching code was horribly busted, so fixing that was also nice. :)
|
Won't be able to build a toolchain until swiftlang/llvm-project#8714 merges. |
lib/Sema/TypeCheckDeclObjC.cpp
Outdated
| @@ -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.
There was a problem hiding this comment.
Took some doing, but I've generalized fixDeclarationObjCName() so I can apply it to extensions instead of writing a one-off helper for objcImpl to use.
|
@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.) |
Several offsetting bugs both broke the caching of `ObjCInterfaceAndImplementationRequest` and caused it to usually miss. Fix this whole painful mess. Also has collateral improvements to simple_display().
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.
This functionality was previously reserved for ValueDecls. Move it all the way up to Decl; in the process, make it correctly handle EnumElementDecls and EnumCaseDecls. This change also allows us to generalize `swift::fixDeclarationObjCName()` to work on extensions, though we do not use that capability in this commit.
beccadax
force-pushed
the
objcimpl-category-on-objc
branch
from
4e7a35f to
2a437e3
Compare
|
@swift-ci please test |
…for extensions. This change also removes @implementation(CategoryName); you should attach the category name to the @objc attribute instead. And there are small changes to how much checking the compiler will do on an @objc @implementation after the decl checker has discovered a problem with it.
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.
beccadax
force-pushed
the
objcimpl-category-on-objc
branch
from
2a437e3 to
0d6b808
Compare
|
@swift-ci please test |
Before the update to support the new syntax, the decl checker treated private and fileprivate members of objcImpl extensions as non-@objc by default. But SE-0436 specified that private and fileprivate members should be implicitly @objc unless opted out, so when support for the final syntax was added, this behavior was changed retroactively. Unfortunately, we’ve found that some early adopters depended on the old behavior. Restore some logic deleted by swiftlang#73309 for early adopter syntax *only*, with a warning telling the developer to put `final` or `@nonobjc` on the declaration if they want to preserve the behavior after updating it. Also tweaks the ObjCImplementationChecker to ensure this logic will actually be run in time for it to suppress the “upgrade to @objc @implementation” warning, and corrects a couple of regressions arising from that change. Fixes rdar://135747897.
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.