[InterfaceGen] Print private/internal properties (#19127)

* [Interface] Print private/internal properties

All properties which contribute to the storage of a type should be
printed, and their names should be hidden from interfaces. Print them
with '_' as their name, and teach the parser to recognize these special
patterns when parsing interface files.

Partially resolves rdar://43810647

* Address review comments

* Disable accessor generation for nameless vars

* Test to ensure interface files preserve type layout

* Ignore attribute differences on Linux

Author: Harlan

include/swift/AST/PrintOptions.h

@@ -322,6 +322,9 @@ struct PrintOptions {
   /// for optionals that are nested within other optionals.
   bool PrintOptionalAsImplicitlyUnwrapped = false;
 
+  /// Replaces the name of private and internal properties of types with '_'.
+  bool OmitNameOfInaccessibleProperties = false;
+
   /// \brief Print dependent types as references into this generic environment.
   GenericEnvironment *GenericEnv = nullptr;
 

lib/AST/ASTPrinter.cpp

@@ -64,13 +64,22 @@ void PrintOptions::clearSynthesizedExtension() {
   TransformContext.reset();
 }
 
+static bool contributesToParentTypeStorage(const AbstractStorageDecl *ASD) {
+  auto *DC = ASD->getDeclContext()->getAsDecl();
+  if (!DC) return false;
+  auto *ND = dyn_cast<NominalTypeDecl>(DC);
+  if (!ND) return false;
+  return !ND->isResilient() && ASD->hasStorage() && !ASD->isStatic();
+}
+
 PrintOptions PrintOptions::printTextualInterfaceFile() {
   PrintOptions result;
   result.PrintLongAttrsOnSeparateLines = true;
   result.TypeDefinitions = true;
   result.PrintIfConfig = false;
   result.FullyQualifiedTypes = true;
   result.SkipImports = true;
+  result.OmitNameOfInaccessibleProperties = true;
 
   class ShouldPrintForTextualInterface : public ShouldPrintChecker {
     bool shouldPrint(const Decl *D, const PrintOptions &options) override {
@@ -79,8 +88,14 @@ PrintOptions PrintOptions::printTextualInterfaceFile() {
         AccessScope accessScope =
             VD->getFormalAccessScope(/*useDC*/nullptr,
                                      /*treatUsableFromInlineAsPublic*/true);
-        if (!accessScope.isPublic())
+        if (!accessScope.isPublic()) {
+          // We do want to print private stored properties, without their
+          // original names present.
+          if (auto *ASD = dyn_cast<AbstractStorageDecl>(VD))
+            if (contributesToParentTypeStorage(ASD))
+              return true;
           return false;
+        }
       }
 
       // Skip typealiases that just redeclare generic parameters.
@@ -821,7 +836,13 @@ void PrintAST::printPattern(const Pattern *pattern) {
 
   case PatternKind::Named: {
     auto named = cast<NamedPattern>(pattern);
-    recordDeclLoc(named->getDecl(), [&]{
+    auto decl = named->getDecl();
+    recordDeclLoc(decl, [&]{
+      if (Options.OmitNameOfInaccessibleProperties &&
+          contributesToParentTypeStorage(decl) &&
+          decl->getFormalAccess() < AccessLevel::Public)
+        Printer << "_";
+      else
         Printer.printName(named->getBoundName());
       });
     break;
@@ -1362,6 +1383,12 @@ bool ShouldPrintChecker::shouldPrint(const Decl *D,
     return !EED->getSourceRange().isValid();
   }
 
+  if (auto *ASD = dyn_cast<AbstractStorageDecl>(D)) {
+    if (Options.OmitNameOfInaccessibleProperties &&
+        contributesToParentTypeStorage(ASD))
+      return true;
+  }
+
   // Skip declarations that are not accessible.
   if (auto *VD = dyn_cast<ValueDecl>(D)) {
     if (Options.AccessFilter > AccessLevel::Private &&

lib/Parse/ParsePattern.cpp

@@ -18,6 +18,7 @@
 
 #include "swift/AST/ASTWalker.h"
 #include "swift/AST/Initializer.h"
+#include "swift/AST/Module.h"
 #include "swift/Basic/StringExtras.h"
 #include "swift/Parse/CodeCompletionCallbacks.h"
 #include "swift/Parse/SyntaxParsingContext.h"
@@ -907,22 +908,34 @@ ParserResult<Pattern> Parser::parseTypedPattern() {
 ///
 ParserResult<Pattern> Parser::parsePattern() {
   SyntaxParsingContext PatternCtx(SyntaxContext, SyntaxContextKind::Pattern);
+  bool isLet = (InVarOrLetPattern != IVOLP_InVar);
+  auto specifier = isLet
+    ? VarDecl::Specifier::Let
+    : VarDecl::Specifier::Var;
   switch (Tok.getKind()) {
   case tok::l_paren:
     return parsePatternTuple();
 
   case tok::kw__:
+    // Normally, '_' is invalid in type context for patterns, but they show up
+    // in interface files as the name for type members that are non-public.
+    // Treat them as an implicitly synthesized NamedPattern with a nameless
+    // VarDecl inside.
+    if (CurDeclContext->isTypeContext() &&
+        SF.Kind == SourceFileKind::Interface) {
+      PatternCtx.setCreateSyntax(SyntaxKind::IdentifierPattern);
+      auto VD = new (Context) VarDecl(
+        /*IsStatic*/false, specifier, /*IsCaptureList*/false,
+        consumeToken(tok::kw__), Identifier(), CurDeclContext);
+      return makeParserResult(new (Context) NamedPattern(VD, /*implicit*/true));
+    }
     PatternCtx.setCreateSyntax(SyntaxKind::WildcardPattern);
     return makeParserResult(new (Context) AnyPattern(consumeToken(tok::kw__)));
 
   case tok::identifier: {
     PatternCtx.setCreateSyntax(SyntaxKind::IdentifierPattern);
     Identifier name;
     SourceLoc loc = consumeIdentifier(&name);
-    bool isLet = (InVarOrLetPattern != IVOLP_InVar);
-    auto specifier = isLet
-                   ? VarDecl::Specifier::Let
-                   : VarDecl::Specifier::Var;
     if (Tok.isIdentifierOrUnderscore() && !Tok.isContextualDeclKeyword())
       diagnoseConsecutiveIDs(name.str(), loc, isLet ? "constant" : "variable");
 

lib/Sema/CodeSynthesis.cpp

@@ -994,6 +994,11 @@ static void addOpaqueAccessorToStorage(TypeChecker &TC,
 
 static void addExpectedOpaqueAccessorsToStorage(TypeChecker &TC,
                                                 AbstractStorageDecl *storage) {
+  // Nameless vars from interface files should not have any accessors.
+  // TODO: Replace this check with a broader check that all storage decls
+  //       from interface files have all their accessors up front.
+  if (storage->getBaseName().empty())
+    return;
   storage->visitExpectedOpaqueAccessors([&](AccessorKind kind) {
     // If the accessor is already present, there's nothing to do.
     if (storage->getAccessor(kind))

test/ModuleInterface/private-stored-member-type-layout.swift

@@ -0,0 +1,53 @@
+// RUN: %empty-directory(%t)
+
+// Check that importing this module creates the right types
+
+// RUN: %target-swift-frontend -emit-interface-path %t/private-stored-members.swiftinterface -module-name PrivateStoredMembers -emit-module -o %t/PrivateStoredMembers.swiftmodule %S/private-stored-members.swift
+// RUN: %target-swift-frontend -emit-ir %s -I %t 2>&1 -DSHOULD_IMPORT | %FileCheck %s --check-prefix CHECK-EXEC --check-prefix CHECK
+
+// Check that the types are also correct when importing a module created from an interface
+
+// RUN: %target-swift-frontend -emit-module -o %t/PrivateStoredMembers.swiftmodule -module-name PrivateStoredMembers %t/private-stored-members.swiftinterface -disable-objc-attr-requires-foundation-module
+// RUN: %target-swift-frontend -emit-ir %s -I %t 2>&1 -DSHOULD_IMPORT | %FileCheck %s --check-prefix CHECK-EXEC --check-prefix CHECK
+
+// Check the types generated when the source file is the primary file, and ensure they're the same layout.
+
+// RUN: %target-swift-frontend -emit-ir %S/private-stored-members.swift %s 2>&1 -module-name main | %FileCheck %s --check-prefix CHECK-MAIN --check-prefix CHECK-EXEC
+
+// These two appear out-of-order between run lines
+
+// CHECK-DAG: [[MYCLASS:%T20PrivateStoredMembers7MyClassC]] = type opaque
+// CHECK-DAG: [[MYSTRUCT:%T20PrivateStoredMembers8MyStructV]] = type <{ %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V }>
+
+// CHECK-MAIN-DAG: [[MYCLASS:%T4main7MyClassC]] = type <{ %swift.refcounted, %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V }>
+// CHECK-MAIN-DAG: [[MYSTRUCT:%T4main8MyStructV]] = type <{ %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V, %TSb, [7 x i8], %Ts5Int64V }>
+
+#if SHOULD_IMPORT
+import PrivateStoredMembers
+#endif
+
+// CHECK-EXEC: swiftcc void @"$S{{[^ ]+}}8makeUseryyF"() #0 {
+public func makeUser() {
+  let ptr = UnsafeMutablePointer<MyStruct>.allocate(capacity: 1)
+  // CHECK-EXEC: %.publicEndVar = getelementptr inbounds [[MYSTRUCT]], [[MYSTRUCT]]* %{{[0-9]+}}, i32 0, i32 [[PUBLIC_END_VAR_IDX:9]]
+  // CHECK-EXEC: %.publicEndVar._value = getelementptr inbounds %Ts5Int64V, %Ts5Int64V* %.publicEndVar, i32 0, i32 0
+  // CHECK-EXEC: store i64 4, i64* %.publicEndVar._value
+  ptr.pointee.publicEndVar = 4
+
+  // CHECK-EXEC: %.publicEndVar1 = getelementptr inbounds [[MYSTRUCT]], [[MYSTRUCT]]* %{{[0-9]+}}, i32 0, i32 [[PUBLIC_END_VAR_IDX]]
+  // CHECK-EXEC: %.publicEndVar1._value = getelementptr inbounds %Ts5Int64V, %Ts5Int64V* %.publicEndVar1, i32 0, i32 0
+  // CHECK-EXEC: [[PUBLIC_END_VAR_LOAD:%[0-9]+]] = load i64, i64* %.publicEndVar1._value, align 8
+
+  // CHECK-EXEC: %.publicVar = getelementptr inbounds [[MYSTRUCT]], [[MYSTRUCT]]* %{{[0-9]+}}, i32 0, i32 0
+  // CHECK-EXEC: %.publicVar._value = getelementptr inbounds %Ts5Int64V, %Ts5Int64V* %.publicVar, i32 0, i32 0
+  // CHECK-EXEC: store i64 [[PUBLIC_END_VAR_LOAD]], i64* %.publicVar._value, align 8
+  ptr.pointee.publicVar = ptr.pointee.publicEndVar
+  ptr.deallocate()
+
+  // CHECK-EXEC: %[[MYCLASS_INIT:[0-9]+]] = call swiftcc [[MYCLASS]]* @"$S{{[^ ]+}}7MyClassCACycfC"(%swift.type* swiftself %{{[0-9]+}})
+  let myClass = MyClass()
+
+  // These are uninteresting as they just call into the standard getter and setter.
+  myClass.publicEndVar = 4
+  myClass.publicVar = myClass.publicEndVar
+}

test/ModuleInterface/private-stored-members.swift

@@ -0,0 +1,106 @@
+// RUN: %empty-directory(%t)
+
+// RUN: %target-swift-frontend -emit-interface-path %t.swiftinterface -emit-module -o /dev/null %s
+// RUN: %FileCheck %s < %t.swiftinterface
+
+// RUN: %target-swift-frontend -emit-interface-path %t-resilient.swiftinterface -enable-resilience -emit-module -o /dev/null %s
+// RUN: %FileCheck %s --check-prefix RESILIENT < %t-resilient.swiftinterface
+
+// RUN: %target-swift-frontend -emit-module -o %t/Test.swiftmodule %t.swiftinterface -disable-objc-attr-requires-foundation-module
+// FIXME(rdar44144435): %target-swift-frontend -emit-module -o /dev/null -merge-modules %t/Test.swiftmodule -emit-interface-path - | %FileCheck %s
+
+// RUN: %target-swift-frontend -emit-module -o %t/TestResilient.swiftmodule -enable-resilience %t.swiftinterface -disable-objc-attr-requires-foundation-module
+// FIXME(rdar44144435): %target-swift-frontend -emit-module -o /dev/null -merge-modules %t/Test.swiftmodule -enable-resilience -emit-interface-path - | %FileCheck %s
+
+
+// CHECK: struct MyStruct {{{$}}
+// RESILIENT: struct MyStruct {{{$}}
+public struct MyStruct {
+// CHECK-NEXT: var publicVar: Int64{{$}}
+// RESILIENT-NEXT: var publicVar: Int64{{$}}
+  public var publicVar: Int64
+
+// CHECK-NEXT: let publicLet: Bool{{$}}
+// RESILIENT-NEXT: let publicLet: Bool{{$}}
+  public let publicLet: Bool
+
+// CHECK-NEXT: internal var _: Int64{{$}}
+// RESILIENT-NOT: internal var _: Int64{{$}}
+  var internalVar: Int64
+
+// CHECK-NEXT: internal let _: Bool{{$}}
+// RESILIENT-NOT: internal let _: Bool{{$}}
+  let internalLet: Bool
+
+// CHECK-NEXT: private var _: Int64{{$}}
+// RESILIENT-NOT: private var _: Int64{{$}}
+  private var privateVar: Int64
+
+// CHECK-NEXT: private let _: Bool{{$}}
+// RESILIENT-NOT: private let _: Bool{{$}}
+  private let privateLet: Bool
+
+// CHECK-NOT: private var
+// RESILIENT-NOT: private var
+  private var computedPrivateVar: Int64 {
+    return 0
+  }
+
+// CHECK-NOT: private static var
+// RESILIENT-NOT: private static var
+  private static var staticPrivateVar: Int64 = 0
+
+// CHECK-NEXT: var publicEndVar: Int64{{$}}
+// RESILIENT-NEXT: var publicEndVar: Int64{{$}}
+  public var publicEndVar: Int64 = 0
+
+// CHECK: }{{$}}
+// RESILIENT: }{{$}}
+}
+
+// CHECK: class MyClass {{{$}}
+// RESILIENT: class MyClass {{{$}}
+public class MyClass {
+// CHECK-NEXT: var publicVar: Int64{{$}}
+// RESILIENT-NEXT: var publicVar: Int64{{$}}
+  public var publicVar: Int64 = 0
+
+// CHECK-NEXT: let publicLet: Bool{{$}}
+// RESILIENT-NEXT: let publicLet: Bool{{$}}
+  public let publicLet: Bool = true
+
+// CHECK-NEXT: internal var _: Int64{{$}}
+// RESILIENT-NOT: internal var _: Int64{{$}}
+  var internalVar: Int64 = 0
+
+// CHECK-NEXT: internal let _: Bool{{$}}
+// RESILIENT-NOT: internal let _: Bool{{$}}
+  let internalLet: Bool = true
+
+// CHECK-NEXT: private var _: Int64{{$}}
+// RESILIENT-NOT: private var _: Int64{{$}}
+  private var privateVar: Int64 = 0
+
+// CHECK-NEXT: private let _: Bool{{$}}
+// RESILIENT-NOT: private let _: Bool{{$}}
+  private let privateLet: Bool = true
+
+// CHECK-NOT: private var
+// RESILIENT-NOT: private var
+  private var computedPrivateVar: Int64 {
+    return 0
+  }
+
+// CHECK-NOT: private static var
+// RESILIENT-NOT: private static var
+  private static var staticPrivateVar: Int64 = 0
+
+// CHECK-NEXT: var publicEndVar: Int64{{$}}
+// RESILIENT-NEXT: var publicEndVar: Int64{{$}}
+  public var publicEndVar: Int64 = 0
+
+  public init() {}
+
+// CHECK: }{{$}}
+// RESILIENT: }{{$}}
+}