Import Forward Declared Objective-C Interfaces and Protocols #61606

NuriAmari · 2022-10-17T22:22:48Z

This patch modifies the ClangImporter to synthesize a placeholder for forward declared Objective-C interfaces and Protocols.

This is the implementation for SE-0384

NuriAmari · 2022-10-17T22:23:29Z

@swift-ci Please smoke test

drodriguez · 2022-10-17T23:46:28Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-11-30T20:22:49Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-04T00:43:49Z

@swift-ci Please test

NuriAmari · 2022-12-04T00:44:16Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-06T19:40:42Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-06T19:40:52Z

@swift-ci Please test

test/IDE/print_clang_ObjectiveC.swift

NuriAmari · 2022-12-06T22:34:11Z

@swift-ci Please test

NuriAmari · 2022-12-07T17:40:33Z

@swift-ci Please test

NuriAmari · 2022-12-07T17:40:58Z

@swift-ci Please Test Source Compatibility

lib/Serialization/Serialization.cpp

NuriAmari · 2022-12-07T19:38:12Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-08T05:46:36Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-08T17:23:16Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-09T23:38:41Z

@swift-ci please test

NuriAmari · 2022-12-09T23:39:04Z

@swift-ci Please Test Source Compatibility

NuriAmari · 2022-12-10T00:32:42Z

@swift-ci please test

NuriAmari · 2022-12-10T00:32:58Z

@swift-ci please test source compatibility

NuriAmari · 2022-12-15T19:48:27Z

@swift-ci please smoke test

NuriAmari · 2022-12-15T19:48:36Z

@swift-ci please test source compatibility

NuriAmari · 2023-02-13T02:02:43Z

@swift-ci Please test

NuriAmari · 2023-02-13T02:02:56Z

@swift-ci Please test source compatibility

NuriAmari · 2023-02-13T15:30:27Z

@beccadax When you get a chance, I've updated the PR with the diagnostic you suggested.

NuriAmari · 2023-02-13T18:48:42Z

Source compat passes with the flag on: #62649. Stress test fails, but other jobs are failing in the same way, seems unrelated. The Windows test here isn't really pending, it finished successfully: https://ci-external.swift.org/job/swift-PR-windows/8806/

NuriAmari · 2023-02-16T19:23:16Z

@swift-ci Please test windows platform

NuriAmari · 2023-02-17T16:41:19Z

@beccadax Could I get a re-review here? Thanks

NuriAmari · 2023-02-23T17:30:32Z

Hi everyone,

I wanted to draw attention to two aspects of this implementation that maybe were not entirely clear.

The ClangImporter option ImportForwardDeclarations existed before this change and was used by swift-ide-test to print interfaces with placeholders to represent forward declared interfaces and protocols. Per the discussion above, we are no longer synthesizing a placeholder if the declaration corresponds to a Swift definition. This also goes for swift-ide-test and will have an immediate effect for anyone using swift-ide-test and passing --enable-objc-forward-declarations. I think swift-ide-test and the compiler should be as consistent with one another as possible, so this makes sense, but I wanted to make sure to call out it is possible we break something here, but only for clients forward declaring Swift types.
I previously voiced some concern about this implementation allowing users to break previously working code by importing a new Swift module:

Drawbacks to consider. Importing new Swift modules will then have the ability to break previously working code. Maybe justly so, but I'm sure users will find this confusing.

Tony's reply to my concern:

Regarding the last point, I don't think that breakage would occur in practice because forward-declaring a Swift @objc class already shouldn't be expected to work. As an example, let's say Swift module A defines an @objc class, and Clang module B only forward declares that class and has some API that references it. Even if Swift module C that imports both A and B, the APIs in B that refer to A won't work, so anyone trying to forward-declare a Swift class in Obj-C is already putting their build in a fragile and unreliable state.

I'm not trying to advocate for forward declaring Swift types as a supported practice, but I do want to call out that I think this breakage can occur in practice. Firstly, Clang module B is usable by Objective-C clients without issue, and anecdotally that is where we run into most forward declaration issues (ie. only Objective-C clients used the interface before and had no problems with forward declarations, then a Swift client arrives and has problems). Even if clients shouldn't expect it to work, unfortunately it has worked for a long time, so I don't think its unlikely for clients to have working code on the ground doing this.

If Swift module C imports only Clang module B, after this patch the APIs will work, since the compiler thinks the forward declarations refer to Clang definitions. It will only break once Swift module A is imported. So it is possible a Swift client starts using a Clang module that forward declares Swift types, and everything works for some time until they import the complete definition of that type. That said, I'm not overly worried about this since I think its relatively uncommon and easy to fix.

I'm comfortable with both these caveats, and I don't see a better solution, but I wanted to make them explicit for others so that there are no surprises down the line.

cc: @allevato @beccadax @drodriguez @plotfi

lib/AST/Module.cpp

plotfi · 2023-02-23T18:35:16Z

lib/AST/Module.cpp

+    }
+  }
+
+  for (Decl *decl : topLevelObjCExposedDeclsInFileUnit) {


Could Decl *decl be const Decl *decl here?

plotfi · 2023-02-27T16:28:16Z

I looked over the patch once myself and once with Nuri. Overall this PR looks good to me, and the set of included test cases looks very goods.

However I am not the code owner, could Nuri please get a re-review for this PR please? Thanks

@beccadax @drodriguez @zoecarver @allevato @hyp

include/swift/AST/ASTContext.h

lib/AST/Module.cpp

lib/ClangImporter/ClangImporter.cpp

lib/ClangImporter/ImportDecl.cpp

NuriAmari · 2023-03-07T22:03:59Z

@swift-ci Please test

NuriAmari · 2023-03-07T22:18:37Z

Hi @allevato @beccadax,

With the Swift 5.9 branch cut quickly approaching on March 20th, we'd like to merge this in time for the release. The implementation hasn't changed for a month and if no new concerns arise, I intend on merging in the next week. If concerns crop up afterwards, I'll be available to fix them.

That said, @DougGregor it seems you are the 5.9 release manager for the Swift compiler. If you have the time, I'd appreciate a review; an approval from the Apple side before merging would be preferred.

This modifies the ClangImporter to introduce an opaque placeholder representation for forward declared Objective-C interfaces and protocols when imported into Swift. In the compiler, the new functionality is hidden behind a frontend flag -enable-import-objc-forward-declarations, and is on by default for language mode >6. The feature is disabled entirely in LLDB expression evaluation / Swift REPL, regardless of language version.

zoecarver

I did a quick review (not very in-depth) and this looks good to me. I think it would still be good to get an in depth review from Becca, but I understand that she might not have time.

Please make sure to run the source compatibility test suite before landing this.

NuriAmari · 2023-03-08T00:08:38Z

@swift-ci Please test

NuriAmari · 2023-03-08T00:11:02Z

@swift-ci Please test source compatibility

NuriAmari · 2023-03-08T17:05:02Z

@swift-ci Please test source compatibility

NuriAmari · 2023-03-08T23:35:12Z

@swift-ci Please test source compatibility

…tions" This reverts commit d55cfd2, reversing changes made to c95e11c.

swiftlang/swift#61606 was merged.

NuriAmari force-pushed the fwd-declarations branch from a4f5ac0 to 8037f87 Compare November 30, 2022 20:22

NuriAmari force-pushed the fwd-declarations branch from 8037f87 to 318da9d Compare December 4, 2022 00:43

NuriAmari force-pushed the fwd-declarations branch 2 times, most recently from 150fef5 to 9410ca3 Compare December 6, 2022 19:40

NuriAmari force-pushed the fwd-declarations branch from 9410ca3 to f8423f6 Compare December 6, 2022 22:07

NuriAmari commented Dec 6, 2022

View reviewed changes

test/IDE/print_clang_ObjectiveC.swift Outdated Show resolved Hide resolved

NuriAmari force-pushed the fwd-declarations branch from f8423f6 to 1a93949 Compare December 7, 2022 04:41

NuriAmari commented Dec 7, 2022

View reviewed changes

lib/Serialization/Serialization.cpp Show resolved Hide resolved

NuriAmari force-pushed the fwd-declarations branch from 1a93949 to c68ffdf Compare December 9, 2022 23:37

NuriAmari force-pushed the fwd-declarations branch from c68ffdf to 573b7a9 Compare December 10, 2022 00:32

NuriAmari force-pushed the fwd-declarations branch from 573b7a9 to f8e8e42 Compare December 15, 2022 19:47

NuriAmari added the objective-c interop Feature: Interoperability with Objective-C label Feb 23, 2023

NuriAmari self-assigned this Feb 23, 2023

plotfi reviewed Feb 23, 2023

View reviewed changes

lib/AST/Module.cpp Outdated Show resolved Hide resolved

plotfi reviewed Feb 23, 2023

View reviewed changes

lib/AST/Module.cpp

}

}

for (Decl *decl : topLevelObjCExposedDeclsInFileUnit) {

Copy link

Contributor

plotfi Feb 23, 2023

Choose a reason for hiding this comment

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

Could Decl *decl be const Decl *decl here?

drodriguez approved these changes Mar 1, 2023

View reviewed changes

include/swift/AST/ASTContext.h Show resolved Hide resolved

lib/AST/Module.cpp Outdated Show resolved Hide resolved

lib/ClangImporter/ClangImporter.cpp Show resolved Hide resolved

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

allevato mentioned this pull request Mar 6, 2023

ClangImporter can cause Swift to abridge Objective-C Protocol definitions depending on link order #64158

Closed

NuriAmari force-pushed the fwd-declarations branch from 6faffd2 to cf7cbae Compare March 7, 2023 22:03

NuriAmari force-pushed the fwd-declarations branch from cf7cbae to 86c5698 Compare March 8, 2023 00:00

zoecarver approved these changes Mar 8, 2023

View reviewed changes

plotfi merged commit d55cfd2 into swiftlang:main Mar 9, 2023

Azoy added a commit to Azoy/swift that referenced this pull request Mar 14, 2023

Revert "Merge pull request swiftlang#61606 from NuriAmari/fwd-declara…

394ea43

…tions" This reverts commit d55cfd2, reversing changes made to c95e11c.

allevato added a commit to swiftlang/swift-evolution that referenced this pull request Mar 16, 2023

Mark SE-0384 as implemented in 5.9.

35ca88f

swiftlang/swift#61606 was merged.

allevato mentioned this pull request Mar 16, 2023

Mark SE-0384 as implemented in 5.9. swiftlang/swift-evolution#1981

Merged

This was referenced May 30, 2023

[Swift-ObjC-Interop] Speculative cyclic lookup fix #66177

Merged

[Swift-ObjC-Interop] Speculative cyclic lookup fix #66195

Merged

Import Forward Declared Objective-C Interfaces and Protocols #61606

Import Forward Declared Objective-C Interfaces and Protocols #61606

Uh oh!

Conversation

NuriAmari commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NuriAmari commented Oct 17, 2022

Uh oh!

drodriguez commented Oct 17, 2022

Uh oh!

NuriAmari commented Nov 30, 2022

Uh oh!

NuriAmari commented Dec 4, 2022

Uh oh!

NuriAmari commented Dec 4, 2022

Uh oh!

NuriAmari commented Dec 6, 2022

Uh oh!

NuriAmari commented Dec 6, 2022

Uh oh!

Uh oh!

NuriAmari commented Dec 6, 2022

Uh oh!

NuriAmari commented Dec 7, 2022

Uh oh!

NuriAmari commented Dec 7, 2022

Uh oh!

Uh oh!

NuriAmari commented Dec 7, 2022

Uh oh!

NuriAmari commented Dec 8, 2022

Uh oh!

NuriAmari commented Dec 8, 2022

Uh oh!

NuriAmari commented Dec 9, 2022

Uh oh!

NuriAmari commented Dec 9, 2022

Uh oh!

NuriAmari commented Dec 10, 2022

Uh oh!

NuriAmari commented Dec 10, 2022

Uh oh!

NuriAmari commented Dec 15, 2022

Uh oh!

NuriAmari commented Dec 15, 2022

Uh oh!

NuriAmari commented Feb 13, 2023

Uh oh!

NuriAmari commented Feb 13, 2023

Uh oh!

NuriAmari commented Feb 13, 2023

Uh oh!

NuriAmari commented Feb 13, 2023

Uh oh!

NuriAmari commented Feb 16, 2023

Uh oh!

NuriAmari commented Feb 17, 2023

Uh oh!

NuriAmari commented Feb 23, 2023

Uh oh!

Uh oh!

plotfi Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

plotfi commented Feb 27, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NuriAmari commented Mar 7, 2023

Uh oh!

NuriAmari commented Mar 7, 2023

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

NuriAmari commented Mar 8, 2023

Uh oh!

NuriAmari commented Oct 17, 2022 •

edited

Loading