SE-0169: Add support for shared private #9098

KingOfBrian · 2017-04-28T14:28:03Z

This PR implements SE-0169 and allows the private scope to be shared between types and extensions inside of a file. This follows the approach Jordan spelled out in SR-4616.

The one slight deviation is that I didn't change the way the private AccessScope is constructed, but changed the isChildOf check to also use the private scope check. Doing both didn't seem to make sense and I believe isChildOf needs to use the private scope check.

For testing I added a test Sema/accessibility_shared_private.swift with explicit checks for multi-file shared private behavior. I also copied the existing Sema/accessibility_private.swift to the Compatibility directory, and updated the test for Swift 4 behavior. I think the test coverage is pretty good, but any ideas for additional testing would be great.

Resolves SR-4616.

…d extensions in Swift 4

slavapestov · 2017-04-28T19:37:42Z

lib/AST/DeclContext.cpp

+      lastExtension = ED;
+
+  // If there's no last extension, return the supplied context.
+  return lastExtension ?: DC;


We try not to use GNU extensions like ?:, please just write lastExtension ? DC : nullptr

I think you mean lastExtension ? lastExtension : DC.

slavapestov · 2017-04-28T19:38:10Z

I have a cosmetic nitpick -- the name 'shared private' is not great. How about we talk about 'private', 'fileprivate', and 'swift 3 private'.

xwu · 2017-04-28T19:58:32Z

At this rate, might as well future-proof and call these things "Swift 4 private", "Swift 2 private", and "Swift 3 private" :P

jrose-apple

Thanks for working on this, Brian! (And Xiaodi.)

Hm. I'm not comfortable with this solution as is because it only changes isChildOf, and not == or intersectWith. Won't that lead to troubles down the line?

jrose-apple · 2017-04-28T20:35:52Z

lib/AST/DeclContext.cpp

+  // sourceDC does not represent a type.
+  auto sourceNTD = sourceDC->getAsNominalTypeOrNominalTypeExtensionContext();
+  auto useSF = useDC->getParentSourceFile();
+  if (useSF != sourceDC->getParentSourceFile() || !sourceNTD)


Nitpick: Please don't try to combine these checks because it means wasting effort (either looking up the base nominal, or looking for the useDC's parent file).

KingOfBrian · 2017-04-28T20:54:59Z

intersectsWith uses isChildOf so I think that will work correctly. I totally missed the equal operator. I'll add back your original suggestion of using the private context in the constructor, I think that will fix ==.

slavapestov · 2017-04-28T20:55:58Z

@KingOfBrian You might want to try enabling this unconditionally, and building the standard library and overlays, just to get a bit of additional testing -- if you hit a crash, add some new tests.

… equality support

KingOfBrian · 2017-04-29T00:55:36Z

Good thought -- I rebuilt everything with the swift 3 guard commented out and no crashes.

How would I just rebuild the stdlib and overlays btw?

CodaFi · 2017-04-29T04:32:38Z

How would I just rebuild the stdlib and overlays btw?

If you build with ninja, you can cd into the Swift build directory and just call

$ ninja swift-stdlib

I'm not sure the overlays have a target, but you can try to find it by grepping the output of

$ ninja -t targets all

DougGregor · 2017-05-01T16:14:34Z

@swift-ci please smoke test

jrose-apple

I'm definitely happier with this. Nice work, Brian! I still have several small comments here but they're all things that could be addressed post-commit.

jrose-apple · 2017-05-02T22:56:21Z

include/swift/AST/AccessScope.h

@@ -48,7 +53,7 @@ class AccessScope {
  /// \see DeclContext::isChildContextOf


Nitpick: This doc comment \see should probably be dropped or changed to \sa.

jrose-apple · 2017-05-02T22:58:07Z

include/swift/AST/AccessScope.h

@@ -48,7 +53,7 @@ class AccessScope {
  /// \see DeclContext::isChildContextOf
  bool isChildOf(AccessScope AS) const {
    if (!isPublic() && !AS.isPublic())
-      return getDeclContext()->isChildContextOf(AS.getDeclContext());
+      return allowsPrivateAccess(getDeclContext(), AS.getDeclContext());


Nitpick: This case also handles internal, so maybe just make this allowsAccess? Or make it a method like isChildSlowPath, which would help preserve the distinction at the call site between the useDC and the sourceDC.

(Ah, I see now that NameLookup.cpp tries to access this logic directly. It might just make sense to change that checkAccessibility over to using access scopes explicitly instead of trying to be clever and do everything with DeclContexts.)

I'll update it to allowsAccess at the very least and will try to update checkAccessibility. I didn't like how there were multiple methods of doing this, but I was trying not to disturb too much. Let me see how it goes!

jrose-apple · 2017-05-02T23:01:04Z

test/Sema/accessibility_private.swift

-    _ = PrivateInner() // expected-error {{'PrivateInner' is inaccessible due to 'private' protection level}}
-    _ = Container.PrivateInner() // expected-error {{'PrivateInner' is inaccessible due to 'private' protection level}}
+    _ = PrivateInner()
+    _ = Container.PrivateInner()
  }

  // FIXME: Why do these errors happen twice?


Looks like the FIXME no longer applies.

jrose-apple · 2017-05-02T23:02:45Z

test/Sema/accessibility_shared_private.swift

@@ -0,0 +1,76 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: %utils/split_file.py -o %t %s
+// RUN: %target-swift-frontend -swift-version 4 -typecheck %t/declarations.swift %t/other_file_extensions.swift -verify


Can you also run this test in multi-file mode, using -primary-file before each file in turn? Sometimes that behaves differently from whole-module mode (which is the default when invoking %target-swift-frontend directly).

I'm struggling to find a clean way to do this. it looks like there's a @-1 style syntax, that changes which run of FileCheck it matches, but I can't get it to work right or find any documentation on it. Any documentation other than the lit source?

Looks like that @-1 syntax just changes the line, so I'm out of ideas how to make this test valid without 3 copies of this test =/

Looks like VerifyDiagnosticConsumer defines the expected-[] behavior. It would be great if that syntax supported the idea of a prefix like FileCheck, then this would be pretty trivial. Any other ideas on how to update this test @jrose-apple ?

(FileCheck docs)

I think using FileCheck instead of / in addition to -verify is probably the best way to go.I wouldn't worry about the notes, just the main errors.

Sounds good, thanks!

jrose-apple · 2017-05-02T23:07:37Z

lib/AST/DeclContext.cpp

+  sourceDC = getPrivateDeclContext(sourceDC, useSF);
+  while (!useDC->isModuleContext()) {
+    useDC = getPrivateDeclContext(useDC, useSF);
+    if (useDC == sourceDC)


Technically an access scope is not a child access scope if it's the same access scope.

SE-0169: Add support for shared private(cherry picked from commit 75a1dca)

Merge pull request #9098 from KingOfBrian/feature/SE-0169

ematejska · 2017-05-03T21:16:17Z

@KingOfBrian Please open a pull request with the Changelog updated as well if you haven't already.

Add support for sharing the private scope between type declaration an…

5447443

…d extensions in Swift 4

slavapestov reviewed Apr 28, 2017

View reviewed changes

slavapestov assigned DougGregor Apr 28, 2017

Do not use GNU ternary extension

b3d44e0

Update the language and method names to avoid the term 'shared private'

0a1d52e

jrose-apple reviewed Apr 28, 2017

View reviewed changes

Store the private scope context in the AccessScope constructor to fix…

0dce9b7

… equality support

BasThomas mentioned this pull request May 1, 2017

[68] Issue #68 - May 4, 2017 SwiftWeekly/swiftweekly.github.io#242

Closed

jrose-apple reviewed May 2, 2017

View reviewed changes

ematejska merged commit 75a1dca into swiftlang:master May 2, 2017

ematejska pushed a commit that referenced this pull request May 3, 2017

Merge pull request #9098 from KingOfBrian/feature/SE-0169

32e7a1d

SE-0169: Add support for shared private(cherry picked from commit 75a1dca)

ematejska pushed a commit that referenced this pull request May 3, 2017

Merge pull request #9213 from apple/Merge-9098-4-branch

5bd63fe

Merge pull request #9098 from KingOfBrian/feature/SE-0169

KingOfBrian mentioned this pull request May 10, 2017

[Gardening] SE-0169 PR Feedback #9465

Closed

tkremenek mentioned this pull request May 3, 2017

[SR-4616] Implement SE-0169: Improve Interaction Between private Declarations and Extensions #47193

Closed

		@@ -48,7 +53,7 @@ class AccessScope {
		/// \see DeclContext::isChildContextOf

SE-0169: Add support for shared private #9098

SE-0169: Add support for shared private #9098

Uh oh!

Conversation

KingOfBrian commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Apr 28, 2017

Uh oh!

xwu commented Apr 28, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KingOfBrian commented Apr 28, 2017

Uh oh!

slavapestov commented Apr 28, 2017

Uh oh!

KingOfBrian commented Apr 29, 2017

Uh oh!

CodaFi commented Apr 29, 2017

Uh oh!

DougGregor commented May 1, 2017

Uh oh!

jrose-apple 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!

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!

KingOfBrian May 4, 2017 • 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!

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!

ematejska commented May 3, 2017

Uh oh!

Uh oh!

KingOfBrian May 4, 2017 •

edited

Loading