Add @_private(from: "SourceFile.swift") imports #20428

aschwaighofer · 2018-11-08T16:20:04Z

A module compiled with -enable-private-imports allows other modules to
import private declarations if the importing source file uses an
``@_private(from: "SourceFile.swift") import statement.

rdar://29318654

A module compiled with `-enable-private-imports` allows other modules to import private declarations if the importing source file uses an ``@_private(from: "SourceFile.swift") import statement. rdar://29318654

aschwaighofer · 2018-11-08T16:20:15Z

@swift-ci Please test

swift-ci · 2018-11-08T16:50:05Z

Build failed
Swift Test Linux Platform
Git Sha - 963c64e

swift-ci · 2018-11-08T17:16:18Z

Build failed
Swift Test OS X Platform
Git Sha - 963c64e

aschwaighofer · 2018-11-08T17:26:41Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

jrose-apple · 2018-11-08T17:31:25Z

include/swift/AST/Module.h

+    Testable = 0x2,
+
+    /// This source file has access to private declarations in the imported
+    /// moduled.


Typo: "moduled".

jrose-apple · 2018-11-08T17:32:04Z

include/swift/AST/Module.h

@@ -857,7 +870,9 @@ class SourceFile final : public FileUnit {
  /// This is the list of modules that are imported by this module.
  ///
  /// This is filled in by the Name Binding phase.
-  ArrayRef<std::pair<ModuleDecl::ImportedModule, ImportOptions>> Imports;
+  ArrayRef<std::pair<ModuleDecl::ImportedModule,
+                     std::pair<ImportOptions, StringRef>>>


Can you make a proper type or at least a typedef for this? Or put the StringRef into ImportOptions?

jrose-apple · 2018-11-08T17:33:42Z

include/swift/AST/Module.h

+    auto it = FilenameForPrivateDecls.find(decl);
+    if (it == FilenameForPrivateDecls.end())
+      return StringRef();
+    else return it->second.str();


Nitpick: no else after an early return.

jrose-apple · 2018-11-08T17:34:45Z

include/swift/AST/Module.h

@@ -1224,12 +1242,29 @@ class LoadedFile : public FileUnit {
    assert(classof(this) && "invalid kind");
  }

+  /// A map from private/fileprivate decls to the file they were defined in.
+  llvm::DenseMap<const ValueDecl *, Identifier> FilenameForPrivateDecls;


Can you explain why this is necessary? It seems like it should be an on-demand on-disk hash table like the private discriminator map.

This works in the same way as PrivateDiscriminatorsByValue when we deserialize a value decl we put it in the map.

When we need to determine hasPrivateImport(const swift::ValueDecl *ofDecl) we check it.

Oops. I didn't realize we did that eagerly for private discriminators too.

…and now I'm working through the reasoning that leads there again: you can't easily look up a decl pointer in a module. My mistake!

jrose-apple · 2018-11-08T17:34:59Z

include/swift/Frontend/FrontendOptions.h

@@ -192,6 +192,11 @@ class FrontendOptions {
  /// \see ModuleDecl::isTestingEnabled
  bool EnableTesting = false;

+  /// Indicates whether we are compiling for testing.


Comment didn't get updated.

jrose-apple · 2018-11-08T17:42:57Z

lib/AST/Module.cpp

+    filename = file->getFilenameForPrivateDecl(ofDecl);
+  } else if (auto *file = dyn_cast<SourceFile>(scope)) {
+    // Source files cannot be imported from another module.
+    return false;


I don't think you even need to check this.

jrose-apple · 2018-11-08T17:43:43Z

lib/AST/Module.cpp

+  auto *module = ofDecl->getModuleContext();
+
+  if (accessLevel == AccessLevel::Internal ||
+      accessLevel == AccessLevel::Public) {


And Open doesn't need any of this. Please use a switch here so that we don't miss cases!

jrose-apple · 2018-11-08T17:45:23Z

lib/Parse/ParseDecl.cpp


+      // Parse ':'.
+      if (Tok.getText() != ":") {


This should check the token kind as tok::colon instead.

jrose-apple · 2018-11-08T17:48:15Z

lib/Serialization/Serialization.cpp

+        if (M->arePrivateImportsEnabled()) {
+          auto filename = getFilename(enclosingFile, value);
+          if (!filename.empty()) {
+            auto filenameID = addFilename(llvm::sys::path::filename(filename));


We only have to do this if we're starting from a SourceFile, right? So you might as well push this into getFilename.

You have to do it if we are starting from a source file or if we are merging modules so that we reemit the information.

Sorry, I just meant the llvm::sys::path::filename part.

jrose-apple · 2018-11-08T17:49:53Z

test/Serialization/private_import.swift

+#elseif MAIN
+
+  @_private(sourceFile: "private_import.swift") import private_import
+  @_private(sourceFile: "private_import.swift") import client


Don't forget some negative test cases: importing a different file shouldn't give you access. It's probably also worth testing a case where we actually have a naming conflict, both at the top level and for members.

aschwaighofer · 2018-11-08T19:14:59Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

aschwaighofer · 2018-11-08T19:38:20Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test OS X platform

aschwaighofer · 2018-11-08T20:13:48Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test OS X platform

jrose-apple · 2018-11-08T20:43:19Z

lib/AST/Module.cpp

@@ -1422,7 +1422,14 @@ bool SourceFile::hasPrivateImport(AccessLevel accessLevel,
                                importPair.second.first.contains(
                                    ImportFlags::PrivateImport);
                       });
+  case AccessLevel::Open:


This probably wasn't supposed to fall through here.

there is a return before case AccessLevel::Open

Oops, sorry. Misleading partial diff!

jrose-apple · 2018-11-08T20:43:51Z

test/Serialization/private_import.swift

+
+  extension Base {
+    // This should not cause a failure.
+    private func shouldNotBeVisible() {}


That wouldn't cause a failure anyway; shadowing is always okay. You have to have a conflict within the same module.

swift-ci · 2018-11-08T20:59:02Z

Build failed
Swift Test OS X Platform
Git Sha - 963c64e

aschwaighofer · 2018-11-08T21:19:06Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

swift-ci · 2018-11-08T21:37:44Z

Build failed
Swift Test Linux Platform
Git Sha - e4f4dfc

swift-ci · 2018-11-08T21:39:50Z

Build failed
Swift Test OS X Platform
Git Sha - e4f4dfc

slavapestov

This is just bikeshedding but perhaps @_private(from: ...) is better than @_private(sourceFile: ...)

slavapestov · 2018-11-09T00:03:01Z

lib/AST/Decl.cpp

+    // @testable/@_private import attributes.
+    auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext());
+    if (!useSF) return access;
+    if (useSF->hasPrivateImport(access, VD))


Can you combine hasPrivateImport() with hasTestableImport(), and have it take an access level?

slavapestov · 2018-11-09T00:05:15Z

include/swift/AST/Module.h

 private:
  std::unique_ptr<LookupCache> Cache;
  LookupCache &getCache() const;

  /// This is the list of modules that are imported by this module.
  ///
  /// This is filled in by the Name Binding phase.
-  ArrayRef<std::pair<ModuleDecl::ImportedModule, ImportOptions>> Imports;
+  ArrayRef<std::pair<ModuleDecl::ImportedModule, ImportOptionsAndFilename>>


A pair where the second type is again a pair is a bit too much IMHO. Can you just define a struct with three fields instead?

slavapestov · 2018-11-09T00:06:54Z

include/swift/AST/DiagnosticsParse.def

+ERROR(attr_private_import_expected_sourcefile, none,
+      "expected 'sourceFile' in '_private' attribute", ())
+ERROR(attr_private_import_expected_sourcefile_name,none,
+      "expected a source file name in @_private(sourceFile:)", ())


I'm a bit confused, is it @_private(sourceFile:) or @_private(from:) (like in the commit message :) )

aschwaighofer · 2018-11-09T02:33:05Z

@swift-ci Please test

aschwaighofer · 2018-11-09T02:34:32Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

swift-ci · 2018-11-09T02:59:57Z

Build failed
Swift Test OS X Platform
Git Sha - f580778

swift-ci · 2018-11-09T04:25:30Z

Build failed
Swift Test Linux Platform
Git Sha - f580778

aschwaighofer · 2018-11-09T12:06:22Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

swift-ci · 2018-11-09T12:20:02Z

Build failed
Swift Test OS X Platform
Git Sha - 6cc3d37

swift-ci · 2018-11-09T12:26:42Z

Build failed
Swift Test Linux Platform
Git Sha - 6cc3d37

aschwaighofer · 2018-11-09T12:37:49Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

swift-ci · 2018-11-09T12:39:28Z

Build failed
Swift Test OS X Platform
Git Sha - 6cc3d37

aschwaighofer · 2018-11-09T12:58:12Z

Please test with following PR:
apple/swift-lldb#1056

@swift-ci Please test

swift-ci · 2018-11-09T12:58:18Z

Build failed
Swift Test Linux Platform
Git Sha - 6cc3d37

swift-ci · 2018-11-09T12:58:39Z

Build failed
Swift Test OS X Platform
Git Sha - 6cc3d37

swift-ci · 2018-11-09T13:00:07Z

Build failed
Swift Test Linux Platform
Git Sha - 6cc3d37

jrose-apple · 2018-11-09T22:19:01Z

lib/AST/Decl.cpp

@@ -3613,7 +3632,8 @@ bool EnumDecl::isFormallyExhaustive(const DeclContext *useDC) const {
  // Testably imported enums are exhaustive, on the grounds that only the author
  // of the original library can import it testably.
  if (auto *useSF = dyn_cast<SourceFile>(useDC->getModuleScopeContext()))
-    if (useSF->hasTestableImport(containingModule))
+    if (useSF->hasTestableOrPrivateImport(AccessLevel::Internal,
+                                          containingModule))


You probably want this one to do private imports too.

This works for either private or testable imports. the access level is used inside of the function to determine whether we need to check the filename (if it is private or file private).

Using AccessLevel::Internal means we will use the check that checks whether an private or testable import occurred (without checking the filename in the private case).

Uh, hm. But this should be specific to the enum we're asking the question of, right?

You are correct it would make a difference for private imports and private/fileprivate declarations.

What you don't see in the diff is that there is a test before this test that checks if the enum is not public.

// Non-public, non-versioned enums are always exhaustive. AccessScope accessScope = getFormalAccessScope(/*useDC*/nullptr, /*respectVersioned*/true); if (!accessScope.isPublic()) return true;

We end up at useSF->hasTestableOrPrivateImport only with a public enum.

In this case we end up in the internal/public branch that does not depend on the file.

bool SourceFile::hasTestableOrPrivateImport( AccessLevel accessLevel, const swift::ValueDecl *ofDecl) const { auto *module = ofDecl->getModuleContext(); switch (accessLevel) { case AccessLevel::Internal: case AccessLevel::Public: // internal/public access only needs an import marked as @_private. The // filename does not need to match (and we don't serialize it for such // decls). return std::any_of( Imports.begin(), Imports.end(), [module](ImportedModuleDesc desc) -> bool { return desc.module.second == module && (desc.importOptions.contains(ImportFlags::PrivateImport) || desc.importOptions.contains(ImportFlags::Testable)); }); case AccessLevel::Open: return true; case AccessLevel::FilePrivate: case AccessLevel::Private: // Fallthrough. break; }

Therefore this check is sufficient.

Oof, right. Okay. Too many components depending on each other…

jrose-apple · 2018-11-09T22:21:34Z

lib/IDE/CodeCompletion.cpp

+            TheModule->getName().str(),
+            AccessPath,
+            Request.NeedLeadingDot,
+            SF.hasTestableOrPrivateImport(AccessLevel::Internal, TheModule),


This is interesting. How is code completion going to work here?

This is NFC compared to before. This was only rewritten to use the SF.hasTestableOrPrivateImport instead of the hasTestableImport function. The outcome should be the same (whatever that is).

Right, that's not the problem. What I'm worried about is that the private-imported members either won't show up in code completion, or they will and could incorrectly show up in the cache for that module.

jrose-apple · 2018-11-09T22:22:35Z

lib/Serialization/Serialization.cpp

+          if (auto *SF = dyn_cast<SourceFile>(enclosingFile)) {
+            return llvm::sys::path::filename(SF->getFilename());
+          } else if (auto *LF = dyn_cast<LoadedFile>(enclosingFile)) {
+            return llvm::sys::path::filename(


My point was that you can skip this step, because any LoadedFile has already applied it.

Right. Thanks. Will fix.

jrose-apple · 2018-11-09T22:23:57Z

test/Serialization/private_import.swift

+
+  Base().foo()
+  // This should not be ambigious.
+  Base().bar()


You're still missing a case where it's the same type but different methods in each file.

jrose-apple · 2018-11-09T22:24:26Z

test/attr/private_import.swift

+// RUN: %target-swift-frontend -typecheck -I %t -I %S/Inputs/custom-modules %s -verify
+
+@_private(sourceFile: "Array.swift") import Swift // expected-error {{module 'Swift' was not compiled for private import}}
+@_private(sourceFile: "empty.swift") import empty // no-error


What happens if I private-import a file that isn't in that module? ("Nothing", I know, but it's probably worth having a test case for it.)

Add @_private(from: "SourceFile.swift") imports

963c64e

A module compiled with `-enable-private-imports` allows other modules to import private declarations if the importing source file uses an ``@_private(from: "SourceFile.swift") import statement. rdar://29318654

aschwaighofer mentioned this pull request Nov 8, 2018

Change for @_private imports apple/swift-lldb#1056

Merged

jrose-apple reviewed Nov 8, 2018

View reviewed changes

Address feedback

e4f4dfc

jrose-apple reviewed Nov 8, 2018

View reviewed changes

More feedback

f580778

Address remaining feedback from Jordan

e4099d0

slavapestov reviewed Nov 9, 2018

View reviewed changes

aschwaighofer added 2 commits November 8, 2018 17:50

Refactor

9feb758

Refactor part2

6cc3d37

Adjust to upstream change

fbb4236

aschwaighofer merged commit c83f638 into swiftlang:master Nov 9, 2018

jrose-apple reviewed Nov 9, 2018

View reviewed changes

Add @_private(from: "SourceFile.swift") imports #20428

Add @_private(from: "SourceFile.swift") imports #20428

Uh oh!

Conversation

aschwaighofer commented Nov 8, 2018

Uh oh!

aschwaighofer commented Nov 8, 2018

Uh oh!

swift-ci commented Nov 8, 2018

Uh oh!

swift-ci commented Nov 8, 2018

Uh oh!

aschwaighofer commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented Nov 8, 2018

Uh oh!

aschwaighofer commented Nov 8, 2018

Uh oh!

aschwaighofer commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 8, 2018

Uh oh!

aschwaighofer commented Nov 8, 2018

Uh oh!

swift-ci commented Nov 8, 2018

Uh oh!

swift-ci commented Nov 8, 2018

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented Nov 9, 2018

Uh oh!

aschwaighofer commented Nov 9, 2018

Uh oh!

swift-ci commented Nov 9, 2018

Uh oh!

swift-ci commented Nov 9, 2018