Improve support for Swift-only initializers in @_objcImplementation checker #69257
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please test |
tshortli
reviewed
Oct 23, 2023
tshortli
approved these changes
Oct 24, 2023
The fix-it suggesting that an unmatched `@objc`-able member can be turned `private` always suggested adding a new modifier, even if one already existed. Make it suggest replacing the existing one instead.
Initializers can’t be made final, but they *can* be made @nonobjc. (This isn’t always enough to ensure they’re actually valid, but I’ll get to that in a follow-up commit.)
Besides improving diagnostics, this also allows us to track whether there were any @objc failures using the invalid bit on the @_objcImplementation attribute.
Nothing in an `@_objcImplementation` block—including Swift-only declarations—should ever require a vtable entry. Diagnose any such members. The diagnostic emitted here is intended to be a fallback that will be used when a vtable entry is needed but we don’t know the specific reason. A future commit will add a more specific diagnostic for Swift-only non-convenience inits.
Required and designated inits have to be overridable—the former so that uses of the initializer on `any T.Type` will call the subclass initializer, the latter so that inherited convenience inits will call the subclass initializer. However, Swift-only members use vtables to dispatch to subclasses, and @objcImpl classes don’t have vtables, so their Swift-only inits cannot be made overridable. Upshot: Swift-only inits on @objcImpl classes must be `convenience`, not `required` or designated. Enforce this rule in the ObjCImplementationChecker. Fixes rdar://109121293.
beccadax
force-pushed
the
convenience-implementation
branch
from
50eac01 to
1b7adbe
Compare
|
@swift-ci please smoke test and merge |
beccadax
added a commit
to beccadax/swift
that referenced
this pull request
Sep 6, 2024
In swiftlang#69257, we modified `ObjCReason` to carry a pointer to the @implementation attribute for the `MemberOfObjCImplementationExtension` kind. This made it mark the @implementation attribute as invalid, suppressing diagnostics from the ObjCImplementationChecker. However, invalidating the attribute *also* causes it to be skipped by serialization. That isn’t a problem if the diagnostics are errors, since we’ll never emit the serialized module, but swiftlang#74135 softened these diagnostics to warnings for early adopters. The upshot was that if Swift emitted one of these warnings when it compiled a library, clients of that library would see the objcImpl extension as a normal extension instead. This would cause various kinds of mischief: ambiguous name lookups because implementations weren’t being excluded, overrides failing because an implementation was `public` instead of `open`, asserts and crashes in SILGen and IRGen because stored properties were found in seemingly normal extensions, etc. Fix this by setting a separate bit on ObjCImplementationAttr, rather than the invalid bit, and modifying the implementation checker to manually suppress many diagnostics when that bit is set. Fixes rdar://134730183.
beccadax
added a commit
to beccadax/swift
that referenced
this pull request
Sep 6, 2024
In swiftlang#69257, we modified `ObjCReason` to carry a pointer to the @implementation attribute for the `MemberOfObjCImplementationExtension` kind. This made it mark the @implementation attribute as invalid, suppressing diagnostics from the ObjCImplementationChecker. However, invalidating the attribute *also* causes it to be skipped by serialization. That isn’t a problem if the diagnostics are errors, since we’ll never emit the serialized module, but swiftlang#74135 softened these diagnostics to warnings for early adopters. The upshot was that if Swift emitted one of these warnings when it compiled a library, clients of that library would see the objcImpl extension as a normal extension instead. This would cause various kinds of mischief: ambiguous name lookups because implementations weren’t being excluded, overrides failing because an implementation was `public` instead of `open`, asserts and crashes in SILGen and IRGen because stored properties were found in seemingly normal extensions, etc. Fix this by setting a separate bit on ObjCImplementationAttr, rather than the invalid bit, and modifying the implementation checker to manually suppress many diagnostics when that bit is set. Fixes rdar://134730183.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The typical rules for member implementations don't work very well for initializers:
final, so we need to support@nonobjcas an opt-out keyword for them.@_objcImplementationclasses don't have a vtable, so it shouldn't be possible to define Swift-only designated and required initializers on an@_objcImplementationclass. (Swift-onlyconvenienceinits and@objcinits of any kind are okay.)Enforce these new rules, and additionally make several related improvements to
@_objcImplementationdiagnostics:privateon an@objcmember replace an existing access control keyword if one is present.ObjCReasonfor members that are implicitly@objcbecause they belong to an@_objcImplementationextension; this improves the wording of some diagnostics and allows us to avoid emitting redundant diagnostics about members that aren't valid as@objc.@_objcImplementationextension members that require a vtable entry. This would have caught the designated/required init issue even without knowing the specific cause of the problem.Fixes rdar://109121293.
This PR will probably be easier to review commit-by-commit.