[Sema] Report use of implicitly imported decls in inlinable code #59799

xymus · 2022-06-29T23:21:28Z

Implicitly imported decls references from inlinable code can break the module API. This has been known to lead to deserialization crash and could break the generated swiftinterfaces files. Let's explicitly check for such a case, keeping it to a warning until Swift language version 6 where it's an error.

This should address cases crashing the compiler, but it will require more work. Mainly, lookup should not allow any reference to non-imported decls, since this is source breaking we could also report them as a warning in implementation details by adding a similar check to diagnoseDeclAvailability. On the performance side, because we're using more of getRestrictedImportKind we may want to cache its result at this point.

rdar://95816286

Renaming isImportedImplementationOnly to getRestrictedImportKind will allow us to support different kind of restrictive imports in the next commits.

This warning will be used more generally so let's make bring back the diagnostic in line with the error version.

Implicitly imported decls may end up in inlinable code and break the module API. This have been known to lead to deserialization crash and could in theory break the generated swiftinterfaces files. Let's explicitly check for such a case, keeping it to a warning until Swift 6 where we can make it an error. rdar://95816286

xymus · 2022-06-29T23:21:40Z

@swift-ci Please smoke test

tshortli · 2022-06-29T23:45:45Z

lib/AST/Module.cpp

+    return RestrictedImportKind::None;
+
+  // Special case the __ObjC module. This module is implicitly imported but
+  // represents any clang module so we can consider it public.


I'm having trouble following this part because I thought the __ObjC module is the one that gets implicitly imported as a result of having a bridging header. If that's true, I would think that it is harmful to reference decls from __ObjC in public decls and inlinable code, at least for library evolution enabled modules. Am I thinking about this wrong?

I though frameworks aren't meant to have bridging headers at all, especially not if they're built with library evolution.

I experimented with it and it looks like there's an automatic @_exported import __ObjC emitted in the swiftinterface file for a module with a bridging header, so __ObjC is implicitly publicly imported. Using these decls in API should be fine in theory. This appeared to work even without that special case, I believe the import of a bridging header __ObjC is usually recognized by the check a few lines above: if (imports.isImportedBy(module, desc.module.importedModule))

I actually added this special case to support the how the C++ interop represents C++ namespaces in Swift. They synthesize an enum in the __ObjC module and extend it in each imported module adding decls to the namespace. This lead to this diagnostics reporting references to __ObjC as errors in the Swift code in the compiler for each use of a decl under a namespace. There's a few more issues here, the first one being that extension members can be referenced without being imported so the C++ interop solution suffers greatly from the very bug this PR tries to limit. As a proper alternative to this special case, maybe the C++ interop should instead declare the import more cleanly as we already do for a module with a bridging header.

Eventually I'd like to fix that, though. I think the reason it's discouraged is what I'm talking about here, but if we got the diagnostics correct and changed the semantics for library evolution modules to have the bridging header be implicitly implementation only imported I think we we could allow it. Many framework developers need something like a bridging header but have to resort to creating a custom clang module to import implementation only instead.

(Not sure how the comment order got mixed up, but my last comment was in response to Slava's).

How would the compiler resolve the import __ObjC when encountering it in a swiftinterface in an SDK? It seems like that would rely on the corresponding clang header being available at some well known location relative to the swiftmodule. In an Xcode project a bridging header is usually just a project header, though. And if you didn't make it a project header, I think the build system would need to know to treat it specially and synthesize a module definition file to install with it or something.

I don't know how building that swiftinterface works in practice, like Slava I'm used to Xcode (I believe it's Xcode) rejecting frameworks with bridging-headers. Thinking about it, the use case with the C++ interop in the compiler was for a mixed source module, I'll have to take a look at how it behaves in such a case.

I'll remove the special case for now to keep the check as strict as it was before, even if it may raise more warnings now as it's executed on more files.

slavapestov · 2022-06-30T00:54:08Z

Maybe instead of "implicitly imported" you can be more explicit (hah!) and say "defined in an extension in a transitively-imported module" or something even more specific? Otherwise someone who encounters this diagnostic will have a hard time figuring out what's going on.

xymus · 2022-06-30T01:51:38Z

@slavapestov What do you think of 'X' cannot be used in an '@inlinable' function because 'libB' was not imported by this file? It points to the practical solution to add the missing import locally and avoid describing how the leak works.

xymus · 2022-06-30T20:08:04Z

@swift-ci Please smoke test

xymus · 2022-06-30T20:08:58Z

Changed the diagnostic to the more direct 'X' cannot be used in an '@inlinable' function because 'libB' was not imported by this file and removed the special case for __ObjC.

xymus · 2022-07-01T03:00:38Z

@swift-ci Please smoke test

…nitions The test swift-class-instantiation-module-interface triggered crashes in this logic because some decls don't have a owning module. It appears to be the synthesized templates that are at cause here.

xymus · 2022-07-01T14:43:14Z

The synthesized decls for C++ templates don't have an owning module, this broke a workaround for imported decls with multiple definitions. We can skip those decls in the workaround as they wouldn't help to establish that there's a visible import of any one of the multiple definitions.

@swift-ci Please smoke test

xymus · 2022-07-01T15:22:36Z

@swift-ci Please smoke test macOS

xymus · 2022-07-01T15:23:07Z

@swift-ci Please smoke test Linux

…f-implicitly-imported" This reverts commit 2bc2021, reversing changes made to e34eda4.

xymus added 4 commits June 29, 2022 12:11

[OSLog] Add missing imports for implicit uses of NSObject

fbf0f5a

[Sema] Generalize isImplementationOnlyImported

4c995a3

Renaming isImportedImplementationOnly to getRestrictedImportKind will allow us to support different kind of restrictive imports in the next commits.

[Sema] Generalize the downgraded warning for exportability check

4d9c8ee

This warning will be used more generally so let's make bring back the diagnostic in line with the error version.

xymus requested review from slavapestov and tshortli June 29, 2022 23:30

tshortli approved these changes Jun 29, 2022

View reviewed changes

xymus added 2 commits June 30, 2022 12:56

[Sema] Make "not-imported" diagnostic more direct

8fc80f9

[SwiftCompilerSources] Add missing import

e7b1434

xymus force-pushed the report-export-of-implicitly-imported branch from d628734 to e7b1434 Compare June 30, 2022 20:07

[Sema] Skip decls without a owning module in workaround for dupe defi…

a89b71d

…nitions The test swift-class-instantiation-module-interface triggered crashes in this logic because some decls don't have a owning module. It appears to be the synthesized templates that are at cause here.

xymus merged commit 2bc2021 into swiftlang:main Jul 1, 2022

xymus deleted the report-export-of-implicitly-imported branch July 1, 2022 20:27

meg-gupta added a commit to meg-gupta/swift that referenced this pull request Jul 8, 2022

Revert "Merge pull request swiftlang#59799 from xymus/report-export-o…

7c733ed

…f-implicitly-imported" This reverts commit 2bc2021, reversing changes made to e34eda4.

[Sema] Report use of implicitly imported decls in inlinable code #59799

[Sema] Report use of implicitly imported decls in inlinable code #59799

Uh oh!

Conversation

xymus commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xymus commented Jun 29, 2022

Uh oh!

tshortli Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

slavapestov Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

xymus Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

tshortli Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

tshortli Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

xymus Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

xymus Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Jun 30, 2022

Uh oh!

xymus commented Jun 30, 2022

Uh oh!

xymus commented Jun 30, 2022

Uh oh!

xymus commented Jun 30, 2022

Uh oh!

xymus commented Jul 1, 2022

Uh oh!

xymus commented Jul 1, 2022

Uh oh!

xymus commented Jul 1, 2022

Uh oh!

xymus commented Jul 1, 2022

Uh oh!

Uh oh!

xymus commented Jun 29, 2022 •

edited

Loading

xymus Jun 30, 2022 •

edited

Loading