[SymbolGraphGen] improve handling of underscored protocols

lib/SymbolGraphGen/Edge.cpp

@@ -20,6 +20,29 @@
 using namespace swift;
 using namespace symbolgraphgen;
 
+namespace {
+
+bool parentDeclIsPrivate(const ValueDecl *VD, SymbolGraph *Graph) {
+  if (!VD)
+    return false;
+
+  auto *ParentDecl = VD->getDeclContext()->getAsDecl();
+
+  if (auto *ParentExtension = dyn_cast_or_null<ExtensionDecl>(ParentDecl)) {
+    if (auto *Nominal = ParentExtension->getExtendedNominal()) {
+      return Graph->isImplicitlyPrivate(Nominal);
+    }
+  }
+
+  if (ParentDecl) {
+    return Graph->isImplicitlyPrivate(ParentDecl);
+  } else {
+    return false;
+  }
+}
+
+} // anonymous namespace
+
 void Edge::serialize(llvm::json::OStream &OS) const {
   OS.object([&](){
     OS.attribute("kind", Kind.Name);
@@ -62,7 +85,7 @@ void Edge::serialize(llvm::json::OStream &OS) const {
 
     // If our source symbol is a inheriting decl, write in information about
     // where it's inheriting docs from.
-    if (InheritingDecl) {
+    if (InheritingDecl && !parentDeclIsPrivate(InheritingDecl, Graph)) {
       Symbol inheritedSym(Graph, InheritingDecl, nullptr);
       SmallString<256> USR, Display;
       llvm::raw_svector_ostream DisplayOS(Display);

lib/SymbolGraphGen/SymbolGraph.cpp

@@ -315,9 +315,13 @@ bool SymbolGraph::synthesizedMemberIsBestCandidate(const ValueDecl *VD,
 }
 
 void SymbolGraph::recordConformanceSynthesizedMemberRelationships(Symbol S) {
-  if (!Walker.Options.EmitSynthesizedMembers || Walker.Options.SkipProtocolImplementations) {
-    return;
-  }
+  // Even if we don't want to emit synthesized members or protocol
+  // implementations, we still want to emit synthesized members from hidden
+  // underscored protocols. Save this check so we can skip emitting members
+  // later if needed.
+  bool dropSynthesizedMembers = !Walker.Options.EmitSynthesizedMembers ||
+                                Walker.Options.SkipProtocolImplementations;
+
   const auto D = S.getLocalSymbolDecl();
   const NominalTypeDecl *OwningNominal = nullptr;
   if (const auto *ThisNominal = dyn_cast<NominalTypeDecl>(D)) {
@@ -341,59 +345,89 @@ void SymbolGraph::recordConformanceSynthesizedMemberRelationships(Symbol S) {
       PrintOptions::printModuleInterface(
           OwningNominal->getASTContext().TypeCheckerOpts.PrintFullConvention));
   auto MergeGroupKind = SynthesizedExtensionAnalyzer::MergeGroupKind::All;
-  ExtensionAnalyzer.forEachExtensionMergeGroup(MergeGroupKind,
-      [&](ArrayRef<ExtensionInfo> ExtensionInfos){
-    for (const auto &Info : ExtensionInfos) {
-      if (!Info.IsSynthesized) {
-        continue;
-      }
-
-      // We are only interested in synthesized members that come from an
-      // extension that we defined in our module.
-      if (Info.EnablingExt) {
-        const auto *ExtM = Info.EnablingExt->getModuleContext();
-        if (!Walker.isOurModule(ExtM))
-          continue;
-      }
-
-      // If D is not the OwningNominal, it is an ExtensionDecl. In that case
-      // we only want to get members that were enabled by this exact extension.
-      if (D != OwningNominal && Info.EnablingExt != D) {
-        continue;
-      }
-
-      for (const auto ExtensionMember : Info.Ext->getMembers()) {
-        if (const auto SynthMember = dyn_cast<ValueDecl>(ExtensionMember)) {
-          if (SynthMember->isObjC()) {
+  ExtensionAnalyzer.forEachExtensionMergeGroup(
+      MergeGroupKind, [&](ArrayRef<ExtensionInfo> ExtensionInfos) {
+        const auto StdlibModule =
+            OwningNominal->getASTContext().getStdlibModule(
+                /*loadIfAbsent=*/true);
+
+        for (const auto &Info : ExtensionInfos) {
+          if (!Info.IsSynthesized) {
             continue;
           }
 
-          const auto StdlibModule = OwningNominal->getASTContext()
-              .getStdlibModule(/*loadIfAbsent=*/true);
+          // We are only interested in synthesized members that come from an
+          // extension that we defined in our module.
+          if (Info.EnablingExt) {
+            const auto *ExtM = Info.EnablingExt->getModuleContext();
+            if (!Walker.isOurModule(ExtM))
+              continue;
+          }
 
-          // There can be synthesized members on effectively private protocols
-          // or things that conform to them. We don't want to include those.
-          if (isImplicitlyPrivate(SynthMember,
-              /*IgnoreContext =*/
-              SynthMember->getModuleContext() == StdlibModule)) {
+          // If D is not the OwningNominal, it is an ExtensionDecl. In that case
+          // we only want to get members that were enabled by this exact
+          // extension.
+          if (D != OwningNominal && Info.EnablingExt != D) {
             continue;
           }
 
-          if (!synthesizedMemberIsBestCandidate(SynthMember, OwningNominal)) {
+          // Extensions to protocols should generate synthesized members only if
+          // that protocol would otherwise be hidden.
+          if (auto *Nominal = Info.Ext->getExtendedNominal()) {
+            if (dropSynthesizedMembers &&
+                !isImplicitlyPrivate(
+                    Nominal, /*IgnoreContext =*/Nominal->getModuleContext() ==
+                                 StdlibModule))
+              continue;
+          } else if (dropSynthesizedMembers) {
             continue;
           }
 
-          auto ExtendedSG = Walker.getModuleSymbolGraph(OwningNominal);
+          for (const auto ExtensionMember : Info.Ext->getMembers()) {
+            if (const auto SynthMember = dyn_cast<ValueDecl>(ExtensionMember)) {
+              if (SynthMember->isObjC()) {
+                continue;
+              }
+
+              // There can be synthesized members on effectively private
+              // protocols or things that conform to them. We don't want to
+              // include those.
+              if (isImplicitlyPrivate(SynthMember,
+                                      /*IgnoreContext =*/
+                                      SynthMember->getModuleContext() ==
+                                          StdlibModule)) {
+                continue;
+              }
+
+              if (!synthesizedMemberIsBestCandidate(SynthMember,
+                                                    OwningNominal)) {
+                continue;
+              }
+
+              Symbol Source(this, SynthMember, OwningNominal);
+
+              if (auto *InheritedDecl = Source.getInheritedDecl()) {
+                if (auto *ParentDecl =
+                        InheritedDecl->getDeclContext()->getAsDecl()) {
+                  if (dropSynthesizedMembers &&
+                      !isImplicitlyPrivate(
+                          ParentDecl,
+                          /*IgnoreContext =*/ParentDecl->getModuleContext() ==
+                              StdlibModule)) {
+                    continue;
+                  }
+                }
+              }
 
-          Symbol Source(this, SynthMember, OwningNominal);
+              auto ExtendedSG = Walker.getModuleSymbolGraph(OwningNominal);
 
-          ExtendedSG->Nodes.insert(Source);
+              ExtendedSG->Nodes.insert(Source);
 
-          ExtendedSG->recordEdge(Source, S, RelationshipKind::MemberOf());
-         }
-      }
-    }
-  });
+              ExtendedSG->recordEdge(Source, S, RelationshipKind::MemberOf());
+            }
+          }
+        }
+      });
 }
 
 void
@@ -783,6 +817,21 @@ bool SymbolGraph::isImplicitlyPrivate(const Decl *D,
   // thing is also private. We could be looking at the `B` of `_A.B`.
   if (const auto *DC = D->getDeclContext()) {
     if (const auto *Parent = DC->getAsDecl()) {
+      // Exception: Children of underscored protocols should be considered
+      // public, even though the protocols themselves aren't. This way,
+      // synthesized copies of those symbols are correctly added to the public
+      // API of public types that conform to underscored protocols.
+      if (isa<ProtocolDecl>(Parent) && Parent->hasUnderscoredNaming()) {
+        return false;
+      }
+      if (const auto *ParentExtension = dyn_cast<ExtensionDecl>(Parent)) {
+        if (const auto *ParentNominal = ParentExtension->getExtendedNominal()) {
+          if (isa<ProtocolDecl>(ParentNominal) &&
+              ParentNominal->hasUnderscoredNaming()) {
+            return false;
+          }
+        }
+      }
       return isImplicitlyPrivate(Parent, IgnoreContext);
     }
   }

test/SymbolGraph/ClangImporter/ErrorEnum.swift

@@ -0,0 +1,48 @@
+// REQUIRES: OS=macosx
+
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-symbolgraph-extract -sdk %sdk -module-name MyError -I %t/MyError -output-dir %t -pretty-print -v -skip-protocol-implementations
+// RUN: %FileCheck %s --input-file %t/MyError.symbols.json
+
+// CHECK-NOT: "sourceOrigin"
+
+// CHECK-NOT: "displayName": "_BridgedStoredNSError.Code"
+
+// == is implemented as part of the hidden _BridgedStoredNSError protocol,
+// but it should be hidden since it ultimately comes from Equatable
+// CHECK-NOT: "precise": "s:10Foundation21_BridgedStoredNSErrorPAAE2eeoiySbx_xtFZ::SYNTHESIZED::s:SC11MyErrorCodeLeV"
+
+// hash(into:) is implemented as part of an extension on that same protocol,
+// but it should be hidden for a similar reason
+// CHECK-NOT: "precise": "s:SYsSHRzSH8RawValueSYRpzrlE4hash4intoys6HasherVz_tF::SYNTHESIZED::c:@E@MyErrorCode"
+
+// MyErrorCode.Code
+// CHECK-DAG: "precise": "c:@E@MyErrorCode"
+
+// MyErrorCode.Code.init(rawValue:)
+// CHECK-DAG: "precise": "s:So11MyErrorCodeV8rawValueABSgs5Int32V_tcfc"
+
+// the following header and module map were copied from test/SourceKit/DocSupport/Inputs
+//--- MyError/module.modulemap
+module "MyError" {
+  header "MyError.h"
+  export *
+}
+
+//--- MyError/MyError.h
+@import Foundation;
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+@class NSString;
+extern const NSString *const MyErrorDomain;
+/// This is my cool error code.
+typedef NS_ERROR_ENUM(int, MyErrorCode, MyErrorDomain) {
+  /// This is first error.
+  MyErrFirst,
+  /// This is second error.
+  MyErrSecond,
+};

test/SymbolGraph/Symbols/SkipProtocolImplementations.swift

@@ -36,10 +36,9 @@
 // There should only be three symbols with sourceOrigin information:
 // - SomeStruct.otherFunc()
 // - ExtraStruct.Inner
-// - HiddenStruct.T
 // (ExtraStruct.Inner will have two sourceOrigins because it has two relationships: a memberOf and a
 // conformsTo)
-// COUNT-COUNT-4: sourceOrigin
+// COUNT-COUNT-3: sourceOrigin
 // COUNT-NOT: sourceOrigin
 
 public protocol SomeProtocol {

test/SymbolGraph/Symbols/UnderscoredProtocols.swift

@@ -1,17 +1,40 @@
 // RUN: %empty-directory(%t)
 // RUN: %target-build-swift %s -module-name UnderscoredProtocols -emit-module -emit-module-path %t/
+
 // RUN: %target-swift-symbolgraph-extract -module-name UnderscoredProtocols -I %t -pretty-print -output-dir %t
+// RUN: %FileCheck %s --input-file %t/UnderscoredProtocols.symbols.json
+
+// RUN: %target-swift-symbolgraph-extract -module-name UnderscoredProtocols -I %t -skip-protocol-implementations -pretty-print -output-dir %t
+// RUN: %FileCheck %s --input-file %t/UnderscoredProtocols.symbols.json
 
 // Make sure that underscored protocols do not appear in public symbol graphs, but their
 // requirements do appear on types which conform to them.
 
+// Also ensure that default implementations of underscored protocols appear on implementing types as
+// if they were native children of that type. No 'sourceOrigin' information should appear in their
+// relationships and no implementation relationships should exist.
+
+// CHECK-DAG: "precise": "s:20UnderscoredProtocols10SomeStructV8someFuncyyF",
+// CHECK-DAG: "precise": "s:20UnderscoredProtocols15_HiddenProtocolPAAE9bonusFuncyyF::SYNTHESIZED::s:20UnderscoredProtocols10SomeStructV"
+
 // CHECK-NOT: "precise": "s:20UnderscoredProtocols15_HiddenProtocolP",
-// CHECK:     "precise": "s:20UnderscoredProtocols10SomeStructV8someFuncyyF",
+
+// CHECK-NOT: "defaultImplementationOf"
+// CHECK-NOT: "requirementOf"
+// CHECK-NOT: "optionalRequirementOf"
+// CHECK-NOT: "overrides"
+
+// CHECK-NOT: "sourceOrigin"
+// CHECK-NOT: "s:20UnderscoredProtocols15_HiddenProtocolPAAE9bonusFuncyyF"
 
 public protocol _HiddenProtocol {
     func someFunc()
 }
 
+extension _HiddenProtocol {
+    public func bonusFunc() {}
+}
+
 public struct SomeStruct {}
 
 extension SomeStruct: _HiddenProtocol {