Add -testable-import-module frontend flag (+ refactoring) #33935

beccadax · 2020-09-14T00:09:10Z

This PR has two functional changes:

Implicit imports specified using the -import-module flag are now processed by import resolution. This means they are fully validated and cross-imported. The -import-underlying-module flag is now also processed via this route, and it gets the same benefits (for what they're worth).
There is now a -testable-import-module frontend flag to match -import-module. As you might guess, this adds an implicit @testable import to every file, instead of an implicit ordinary import.

Those two functional changes are in the last two commits; they represent maybe 5% of the changes here. The other 95% of the PR enables these changes by refactoring the data types used to represent imports:

Centralizes more import-related types in AST/Import.h.
Creates one common currency type representing "an import with all of the options and attributes that can be specified by a user" and adopts it everywhere relevant. The new type can be used with either an unloaded module's name or a loaded module.
Allows the full range of import options through more of import resolution handling.

Fixes rdar://66544654.

Review suggestions: Probably best reviewed by commit. A lot of commits (especially the first one) are very mechanical, with lots of boring renaming at use sites. You can probably skim those bits. The commit messages may give some guidance on this.

The reason for a given change is not always clear until several commits later. In particular:

We un-nest and move types related to ModuleDecl::ImportedModule and SourceFile::ImportedModuleDesc because types currently in these two headers will become interdependent by the time we're finished.
We make AttributedImport a template because we will eventually use both the existing ImportedModule and a new type with a module name and access path instead. The template lets us have three types instead of four or five.
We introduce the ImplicitImportList type which just wraps ArrayRef<ImplicitImport> because (a) shifting to AttributedImport will complicate its definition enough to require a type, and (b) introducing that type at the outset avoids multiple rounds of renaming several use sites.
We start using AttributedImport in implicit imports to (a) allow the frontend to specify additional behaviors, and (b) make it easier to shift responsibility for loading implicit imports into ImportResolver.
We make ImportResolver resolve implicit imports because (a) this makes it validate the implicit @testable attribute added by -testable-import-module, and (b) it simplifies the code and is the right way to do things anyway.

@hamishknight, you worked a lot on import resolution recently and might have some opinions about what I'm doing. @CodaFi, you're most likely familiar with Hamish's work, but unlike him you're currently working on the compiler full-time. @varungandhi-apple, you clearly have opinions about names in this area.

beccadax · 2020-09-26T01:09:32Z

include/swift/AST/Import.h

  using ImportOptionsDMI = DenseMapInfo<swift::ImportOptions>;
  using StringRefDMI = DenseMapInfo<StringRef>;
+  // FIXME: SPI groups not used by DenseMapInfo???


I was very surprised by this. @xymus, is this intentional?

I'm not familiar with the DenseMapInfo logic and the doc is sparse so I can't say this was intentional. However SPI groups on an import should affect type checking at the source file level only. Does it apply here?

The only use of DenseMapInfo with AttributedImport keys that I can find is during cross-importing (in the form of a SmallSetVector). I think it could mean that, if you wrote something like:

@_spi(TreasureMaps) import MapKit @_spi(IlluminatiBases) import MapKit import SwiftUI

We would only consider the @_spi(TreasureMaps) import MapKit, ignoring the second import.

Come to think of it, though, I don't think we've actually defined how cross-import overlays should interact with @_spi. I'll look into that on Monday—might ping you for input at that time.

I'll include the SPI groups in isEqual(), but not in the hash value.

beccadax · 2020-09-26T01:44:33Z

@swift-ci please test

varungandhi-apple · 2020-09-26T02:21:30Z

include/swift/AST/Import.h

  }
 };

 // MARK: - Abstractions of imports

+/// Convenience struct to keep track of an import path and whether or not it
+/// is scoped.
+class UnloadedImportedModule {


Since you pinged me about naming :P, UnloadedImportedModule to me conveys a sense of "the module was imported and loaded then unloaded", i.e. unloading as something that undoes loading. However, IIUC that's not how this is being used. Rather the semantics are more like NotYetImportedModule (strawman alternative). Is that right?

I suspect part of the issue (for my lack of understanding) is that there are several moving parts here -- there's UnboundImport as well as UnloadedImportedModule (which kinda' sound similar-ish) -- and I only have passing familiarity with this code, so I don't fully grasp the distinction between loading modules and binding imports.

Think of it this way. We have these three public types which are used throughout the compiler:

ImportedModule is basically just a ModuleDecl * and an access path. It's the minimum information you need to, for instance, figure out if a particular declaration is visible through that import, and it's also more or less what's still known about an import after deserialization.

UnloadedImportedModule is basically the name of a module and an access path. You use it in place of ImportedModule when you haven't actually loaded a ModuleDecl * yet.

AttributedImport<ImportedModule or UnloadedImportedModule> adds to that type abstract information about the attributes of an import (like whether it's exported, implementation-only, which SPIs it imports, etc.).

And we have one other type that's private to ImportResolution.cpp:

UnboundImport contains state and logic used to process a single import. (Could be an ImportDecl, a cross-import overlay we need to import, or now an implicit import.) In terms of state, it includes an AttributedImport<UnloadedImportedModule> plus a couple extra things that are used when emitting diagnostics. But its primary purpose is to perform certain steps of import resolution on a particular import, not to store state. (Arguably it should actually be UnresolvedImport; when I wrote this class, "import resolution" was called "name binding".)

I don't mind the name UnloadedImportedModule, though perhaps another suggestion could be ImportedModuleToLoad?

Does ImportedModule always represent the state where a ModuleDecl * has been loaded? Or does it sometimes have a ModuleDecl * that hasn't been loaded?

ModuleDecls are, by definition, already loaded.

include/swift/AST/Import.h

swift-ci · 2020-09-26T02:45:21Z

Build failed
Swift Test Linux Platform
Git Sha - 3a60f09cf3929cef82cef2aa5f92dab050790d5b

swift-ci · 2020-09-26T03:26:10Z

Build failed
Swift Test OS X Platform
Git Sha - 3a60f09cf3929cef82cef2aa5f92dab050790d5b

hamishknight

Nice cleanups!

include/swift/AST/Import.h

lib/AST/TypeCheckRequests.cpp

lib/IDE/REPLCodeCompletion.cpp

lib/Frontend/Frontend.cpp

include/swift/AST/Import.h

lib/Sema/ImportResolution.cpp

To help consolidate our various types describing imports, this commit moves the following types and methods to Import.h: * ImplicitImports * ImplicitStdlibKind * ImplicitImportInfo * ModuleDecl::ImportedModule * ModuleDecl::OrderImportedModules (as ImportedModule::Order) * ModuleDecl::removeDuplicateImports() (as ImportedModule::removeDuplicates()) * SourceFile::ImportFlags * SourceFile::ImportOptions * SourceFile::ImportedModuleDesc This commit is large and intentionally kept mechanical—nothing interesting to see here.

Rename ImportedModuleDesc to AttributedImport and make it a template that’s parameterized on the representation of the module. This will allow us to reduce duplicated representations of “abstract” ImportDecls.

Also renames a member in ImportResolution.cpp to align with this naming.

Removes what amount to redundant definitions from UnboundImport.

This doesn’t really change the design yet.

Instead, we will use AttributedImport<ImportedModule>, a common currency type which supports a superset of ImplicitImport’s current behavior.

Replace with an array of AttributedImport<UnloadedImportedModule> to add flexibility.

Allows it to be used in PointerIntPair and PointerUnion.

Unloaded implicit imports (e.g. the `-import-module` flag) will now be processed by import resolution, and will be fully validated and cross-imported. Preloaded imports, like the standard library import, are still not run through full import resolution, but this is a definite improvement over the status quo. This also folds `-import-underlying-module` and `@_exported import <ParentModule>` into a single code path, slightly changing the diagnostic for a failed overlay-style underlying module import.

This is just like -import-module, but it does an @testable import. Simple, right? Fixes rdar://66544654.

This ought to result in more consistent behavior.

beccadax · 2020-10-10T02:29:06Z

@swift-ci please smoke test

beccadax · 2020-10-10T05:28:55Z

[snip]/llvm-project/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp:1192:16: error: no member named 'AdditionalModules' in 'swift::ImplicitImportInfo'
20:08:25     importInfo.AdditionalModules.emplace_back(module, /*exported*/ false);
20:08:25     ~~~~~~~~~~ ^

Of course, I’ll need to update this.

beccadax · 2020-10-11T05:47:01Z

With swiftlang/llvm-project#1937

@swift-ci please smoke test

hamishknight

Looks great!

lib/Sema/ImportResolution.cpp

…atience [lldb] Update to match swiftlang/swift#33935

Previously, when importing a Swift module it the REPL or a Playground, LLDB would only cache the module name and reload the module at every expression evaluation. Doing so means that, when LLDB transfers imports information from the original source file to the new one, it is doing it with low fidelity, because LLDB would not keep track of the import attributes used in the previous expression evaluations. Thanks to swiftlang/swift#33935, we can now cache the whole `swift::AttributedImport<swift::ImportedModule>>` instead of only storing the module name. This struct contains both the `ImportedModule` with the `ModuleDecl` but also the `ImportOptions` set containing all the possible import attributes (Exported / Testable / PrivateImport ...) of that import statement. Now, before compiling and evaluating a Swift expression, LLDB fetches all the attributed imports cached previously and add them to the `ImplicitImportInfo` object that is used to create the `ModuleDecl` of the evaluated expression. It allows for the `ImportOptions` to be propagated in the compiled expression. This should fix some irregularities between the debugger and the compiler regarding import statements. rdar://65319954 Signed-off-by: Med Ismail Bennani <[email protected]>

beccadax force-pushed the these-imports-are-testing-my-patience branch from 13e5e82 to 483920d Compare September 24, 2020 21:06

beccadax changed the base branch from master to main September 24, 2020 21:06

beccadax changed the base branch from main to master September 24, 2020 21:07

beccadax changed the base branch from master to main September 24, 2020 21:08

beccadax force-pushed the these-imports-are-testing-my-patience branch 2 times, most recently from 958a1f0 to 0e8007c Compare September 25, 2020 21:14

beccadax marked this pull request as ready for review September 25, 2020 21:16

beccadax force-pushed the these-imports-are-testing-my-patience branch from 0e8007c to 3a60f09 Compare September 26, 2020 01:04

beccadax commented Sep 26, 2020

View reviewed changes

beccadax requested review from hamishknight, varungandhi-apple and CodaFi September 26, 2020 01:40

varungandhi-apple reviewed Sep 26, 2020

View reviewed changes

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

varungandhi-apple reviewed Sep 26, 2020

View reviewed changes

include/swift/AST/Import.h Outdated Show resolved Hide resolved

hamishknight reviewed Sep 28, 2020

View reviewed changes

beccadax force-pushed the these-imports-are-testing-my-patience branch 2 times, most recently from 2dd993e to 6aedab2 Compare October 10, 2020 01:50

beccadax added 9 commits October 9, 2020 18:57

[NFC] Turn ImportedModuleDesc into AttributedImport

f3d99ce

Rename ImportedModuleDesc to AttributedImport and make it a template that’s parameterized on the representation of the module. This will allow us to reduce duplicated representations of “abstract” ImportDecls.

[NFC] Rename and document fields of AttributedImport

c489fff

Also renames a member in ImportResolution.cpp to align with this naming.

[NFC] Use AttributedImport in import resolution

8070e8a

Removes what amount to redundant definitions from UnboundImport.

[NFC] Add type for ModuleDecl::getImplicitImports()

d55c5bd

This doesn’t really change the design yet.

[NFC] Eliminate ImplicitImport

563386b

Instead, we will use AttributedImport<ImportedModule>, a common currency type which supports a superset of ImplicitImport’s current behavior.

[NFC] Rework ImplicitImportInfo::ModuleNames

c13067a

Replace with an array of AttributedImport<UnloadedImportedModule> to add flexibility.

[NFC] Add conveniences to clean up import code

d02b34c

[NFC] Give NullablePtr some PointerLikeTypeTraits

7f14aff

Allows it to be used in PointerIntPair and PointerUnion.

beccadax added 4 commits October 9, 2020 18:59

Add -testable-import-module frontend flag

f719237

This is just like -import-module, but it does an @testable import. Simple, right? Fixes rdar://66544654.

[NFC] Compare spiGroups in DenseMapInfo

48b866b

This ought to result in more consistent behavior.

[NFC] Improve simple_display for AttributedImport

944ad88

beccadax force-pushed the these-imports-are-testing-my-patience branch from 6aedab2 to 944ad88 Compare October 10, 2020 02:24

beccadax mentioned this pull request Oct 11, 2020

[lldb] Update to match apple/swift#33935 swiftlang/llvm-project#1937

Merged

beccadax requested a review from hamishknight October 11, 2020 05:47

hamishknight approved these changes Oct 11, 2020

View reviewed changes

lib/Sema/ImportResolution.cpp Show resolved Hide resolved

beccadax merged commit e843a16 into swiftlang:main Oct 11, 2020

beccadax added a commit to swiftlang/llvm-project that referenced this pull request Oct 11, 2020

Merge pull request #1937 from brentdax/these-imports-are-testing-my-p…

9f1d8f2

…atience [lldb] Update to match swiftlang/swift#33935

medismailben mentioned this pull request Oct 15, 2020

[lldb/Swift] Fix import attributes handing in following expression evaluation swiftlang/llvm-project#1978

Merged

medismailben pushed a commit to swiftlang/llvm-project that referenced this pull request Oct 16, 2020

[lldb] Update to match swiftlang/swift#33935

e353a88

Add -testable-import-module frontend flag (+ refactoring) #33935

Add -testable-import-module frontend flag (+ refactoring) #33935

Uh oh!

Conversation

beccadax commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beccadax Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

xymus Sep 26, 2020 • 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!

beccadax Sep 26, 2020 • 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!

beccadax Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

beccadax commented Sep 26, 2020

Uh oh!

varungandhi-apple Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

varungandhi-apple Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

beccadax Sep 26, 2020 • 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!

hamishknight Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

varungandhi-apple Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

beccadax Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swift-ci commented Sep 26, 2020

Uh oh!

swift-ci commented Sep 26, 2020

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beccadax commented Oct 10, 2020

Uh oh!

beccadax commented Oct 10, 2020

Uh oh!

beccadax commented Oct 11, 2020

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beccadax commented Sep 14, 2020 •

edited

Loading

xymus Sep 26, 2020 •

edited

Loading

beccadax Sep 26, 2020 •

edited

Loading

beccadax Sep 26, 2020 •

edited

Loading