[Clang Importer] Do not rely on being able to always import Foundation on-demand #34331

artemcm · 2020-10-16T18:34:36Z

When importing Clang types.
This is not an option with Explicit Module Builds. If the module being built does not (directly or transitively) depend on Foundation, then attempting to load Foundation will produce an error because implicit module loading is no longer allowed.

This change addresses a small number of cases where ClangImporter relies on being able to load Foundation on-demand:

When importing a single Decl from a clang module, we check whether it has certain conformances by checking all extensions of the NominalTypeDecl of the Decl in question, to see if any of the extensions contain the conformance we are looking for, but we only check extensions whose parent module is either the original module of the NominalTypeDecl or the overlay module of the NominalTypeDecl or Foundation. It seems that we do not need to actually import Foundation here, just checking the module Identifier should be sufficient.
In maybeImportNSErrorOutParameter, change the behavior to have an exit condition based on whether Foundation can be imported, before attempting to load it.
When checking whether or not we are allowed to bridge an Objective-C type, it also looks sufficient to query whether or not Foundation can be imported, without loading it.

Resolves rdar://70381338

artemcm · 2020-10-16T18:34:55Z

@swift-ci please test

artemcm · 2020-10-16T18:36:18Z

@swift-ci please smoke test

artemcm · 2020-10-16T18:37:35Z

@brentdax, @slavapestov, I don't have as deep of an understanding to know what I may be breaking here, really curious to hear what you think.

artemcm · 2020-10-17T04:48:05Z

@swift-ci please test

artemcm · 2020-10-19T18:12:59Z

@swift-ci Please Test Source Compatibility

artemcm · 2020-10-20T16:00:23Z

@swift-ci Please Test Source Compatibility

artemcm · 2020-10-21T17:32:51Z

@swift-ci Please Test Source Compatibility

artemcm · 2020-10-22T16:39:34Z

@swift-ci Please Test Source Compatibility

artemcm · 2020-10-22T21:59:16Z

@swift-ci Please Test Source Compatibility

artemcm · 2020-10-23T00:12:06Z

@swift-ci Please Test Source Compatibility Debug

artemcm · 2020-10-23T18:15:38Z

@swift-ci Please Test Source Compatibility Debug

artemcm · 2020-10-23T18:43:00Z

@swift-ci Please Test Source Compatibility Debug

beccadax

I think this will be okay, but I wouldn't be surprised if we see regressions in some weird edge cases where ClangImporter implicitly loading Foundation turns out to have been load-bearing. We'll need to keep an eye out for that.

beccadax · 2020-10-23T23:12:04Z

lib/ClangImporter/ImportDecl.cpp

@@ -5583,8 +5583,7 @@ namespace {
 }

 static bool conformsToProtocolInOriginalModule(NominalTypeDecl *nominal,


Nit: Now that we're not passing in Foundation, this should probably be called something like conformsToProtocolInFoundationModule().

artemcm · 2020-10-26T16:17:08Z

@swift-ci please smoke test

artemcm · 2020-10-26T16:57:41Z

@swift-ci please smoke test Windows platform

artemcm · 2020-10-27T16:40:11Z

@swift-ci please smoke test

artemcm · 2020-10-27T20:26:55Z

@swift-ci please smoke test

artemcm · 2020-10-28T15:32:04Z

@swift-ci please smoke test

artemcm · 2020-10-28T15:50:32Z

@swift-ci please smoke test macOS platform

…n on-demand When importing Clang types. This is not an option with Explicit Module Builds. If the module being built does not (directly or transitively) depend on `Foundation`, then attempting to load it will produce an error because Implicit module loading is no longer allowed.. This change addresses a small number of cases where ClangImporter relies on being able to load `Foundation` on-demand: - When importing a single Decl from a clang module, we check whether it has certain conformances by checking all extensions of the NominalTypeDecl of the Decl in question, to see if any of the extensions contain the conformance we are looking for, but we only check extensions whose parent module is either the original module of the NominalTypeDecl or the overlay module of the NominalTypeDecl or Foundation. It seems that we do not need to actually import `Foundation` here, just checking the module Identifier should be sufficient. - In `maybeImportNSErrorOutParameter`, change the behavior to have an exit condition based on whether `Foundation` can be imported, before attempting to load it. - When checking whether or not we are allowed to bridge an Objective-C type, it also looks sufficient the query whether or not `Foundation` *can* be imported, without loading it.

artemcm · 2020-10-28T23:12:02Z

It looks like it wasn't load-bearing when I put up the PR, but became so since then for these tests:

Swift(macosx-x86_64) :: Interop/Cxx/class/constructors-objc-silgen.swift
Swift(macosx-x86_64) :: Interop/Cxx/class/constructors-objc-irgen.swift

… types

artemcm · 2020-10-29T17:38:07Z

@swift-ci please smoke test

artemcm · 2020-10-29T20:20:10Z

@swift-ci please smoke test macOS platform

artemcm · 2020-10-29T21:54:03Z

@swift-ci please smoke test macOS platform

artemcm · 2020-10-29T22:40:52Z

@swift-ci please smoke test macOS platform

artemcm · 2020-10-30T17:15:59Z

@swift-ci please smoke test macOS platform

artemcm · 2020-10-30T20:16:00Z

@swift-ci please smoke test macOS platform

artemcm · 2020-11-02T16:27:11Z

@swift-ci please smoke test

artemcm · 2020-11-02T16:32:07Z

@swift-ci please smoke test

artemcm · 2020-11-02T18:01:23Z

@swift-ci please smoke test Windows platform

artemcm · 2020-11-02T19:41:39Z

@swift-ci please smoke test Windows platform

artemcm marked this pull request as ready for review October 16, 2020 18:36

artemcm requested review from slavapestov and beccadax and removed request for slavapestov October 16, 2020 18:36

beccadax approved these changes Oct 23, 2020

View reviewed changes

beccadax reviewed Oct 23, 2020

View reviewed changes

Import Foundation in cxx interop tests that explicitly use/bridge its…

e98dbc8

… types

artemcm force-pushed the NoFoundationOnExplicitImportSoil branch from a30cb65 to e98dbc8 Compare October 29, 2020 17:37

artemcm merged commit 3b54549 into swiftlang:main Nov 2, 2020

		@@ -5583,8 +5583,7 @@ namespace {
		}

		static bool conformsToProtocolInOriginalModule(NominalTypeDecl *nominal,

[Clang Importer] Do not rely on being able to always import Foundation on-demand #34331

[Clang Importer] Do not rely on being able to always import Foundation on-demand #34331

Uh oh!

Conversation

artemcm commented Oct 16, 2020

Uh oh!

artemcm commented Oct 16, 2020

Uh oh!

artemcm commented Oct 16, 2020

Uh oh!

artemcm commented Oct 16, 2020

Uh oh!

artemcm commented Oct 17, 2020

Uh oh!

artemcm commented Oct 19, 2020

Uh oh!

artemcm commented Oct 20, 2020

Uh oh!

artemcm commented Oct 21, 2020

Uh oh!

artemcm commented Oct 22, 2020

Uh oh!

artemcm commented Oct 22, 2020

Uh oh!

artemcm commented Oct 23, 2020

Uh oh!

artemcm commented Oct 23, 2020

Uh oh!

artemcm commented Oct 23, 2020

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

beccadax Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

artemcm commented Oct 26, 2020

Uh oh!

artemcm commented Oct 26, 2020

Uh oh!

artemcm commented Oct 27, 2020

Uh oh!

artemcm commented Oct 27, 2020

Uh oh!

artemcm commented Oct 28, 2020

Uh oh!

artemcm commented Oct 28, 2020

Uh oh!

artemcm commented Oct 28, 2020

Uh oh!

artemcm commented Oct 29, 2020

Uh oh!

artemcm commented Oct 29, 2020

Uh oh!

artemcm commented Oct 29, 2020

Uh oh!

artemcm commented Oct 29, 2020

Uh oh!

artemcm commented Oct 30, 2020

Uh oh!

artemcm commented Oct 30, 2020

Uh oh!

artemcm commented Nov 2, 2020

Uh oh!

artemcm commented Nov 2, 2020

Uh oh!

artemcm commented Nov 2, 2020

Uh oh!

artemcm commented Nov 2, 2020

Uh oh!

Uh oh!