From 8005b216d5496a168b32bc3d991991396d33de76 Mon Sep 17 00:00:00 2001
From: Gerrit Birkeland <gerrit@gerritbirkeland.com>
Date: Tue, 14 Jan 2020 15:59:57 -0700
Subject: [PATCH] fix: External symbols should not be referenced

As discussed in #1166.
---
 src/lib/converter/context.ts                  | 34 +++++++++++++++----
 src/lib/converter/factories/reference.ts      |  8 ++++-
 src/lib/converter/nodes/export.ts             |  3 +-
 src/test/converter/exports/mod.ts             |  6 ++++
 .../exports/specs-without-exported.json       |  4 +--
 src/test/converter/exports/specs.json         |  4 +--
 6 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/src/lib/converter/context.ts b/src/lib/converter/context.ts
index f8f48c9c7..be6a747f3 100644
--- a/src/lib/converter/context.ts
+++ b/src/lib/converter/context.ts
@@ -205,12 +205,8 @@ export class Context {
      * @param callback  The callback that should be executed.
      */
     withSourceFile(node: ts.SourceFile, callback: Function) {
-        let isExternal = !this.fileNames.includes(node.fileName);
-        if (!isExternal && this.externalPattern) {
-            isExternal = this.externalPattern.some(mm => mm.match(node.fileName));
-        }
-
-        if (isExternal && this.converter.excludeExternals) {
+        const isExternal = this.isExternalFile(node.fileName);
+        if (this.isOutsideDocumentation(node.fileName, isExternal)) {
             return;
         }
 
@@ -343,6 +339,32 @@ export class Context {
         return target;
     }
 
+    /**
+     * Determines if the given file is outside of the project's generated documentation.
+     * This is not, and is not intended to be, foolproof. Plugins may remove reflections
+     * in the file that this method returns true for, See GH#1166 for discussion on tradeoffs.
+     *
+     * @param fileName
+     * @internal
+     */
+    isOutsideDocumentation(fileName: string, isExternal = this.isExternalFile(fileName)) {
+        return isExternal && this.converter.excludeExternals;
+    }
+
+    /**
+     * Checks if the given file is external.
+     *
+     * @param fileName
+     * @internal
+     */
+    isExternalFile(fileName: string) {
+        let isExternal = !this.fileNames.includes(fileName);
+        if (!isExternal && this.externalPattern) {
+            isExternal = this.externalPattern.some(mm => mm.match(fileName));
+        }
+        return isExternal;
+    }
+
     /**
      * Convert the given list of type parameter declarations into a type mapping.
      *
diff --git a/src/lib/converter/factories/reference.ts b/src/lib/converter/factories/reference.ts
index f8b74394c..dbd92a2f2 100644
--- a/src/lib/converter/factories/reference.ts
+++ b/src/lib/converter/factories/reference.ts
@@ -28,11 +28,17 @@ export function createReferenceType(context: Context, symbol: ts.Symbol | undefi
     return new ReferenceType(name, context.checker.getFullyQualifiedName(symbol));
 }
 
-export function createReferenceReflection(context: Context, source: ts.Symbol, target: ts.Symbol): ReferenceReflection {
+export function createReferenceReflection(context: Context, source: ts.Symbol, target: ts.Symbol): ReferenceReflection | undefined {
     if (!(context.scope instanceof ContainerReflection)) {
         throw new Error('Cannot add reference to a non-container');
     }
 
+    // If any declaration is outside, the symbol should be considered outside. Some declarations may
+    // be inside due to declaration merging.
+    if (target.declarations.some(d => context.isOutsideDocumentation(d.getSourceFile().fileName))) {
+        return;
+    }
+
     const reflection = new ReferenceReflection(source.name, [ReferenceState.Unresolved, context.checker.getFullyQualifiedName(target)], context.scope);
     reflection.flags.setFlag(ReflectionFlag.Exported, true); // References are exported by necessity
     if (!context.scope.children) {
diff --git a/src/lib/converter/nodes/export.ts b/src/lib/converter/nodes/export.ts
index 7da63aac9..19dc0f90e 100644
--- a/src/lib/converter/nodes/export.ts
+++ b/src/lib/converter/nodes/export.ts
@@ -93,7 +93,8 @@ export class ExportDeclarationConverter extends ConverterNodeComponent<ts.Export
                 }
                 const source = context.checker.tryGetMemberInModuleExports(key.toString().replace(/^__/, '_'), thisModule);
                 if (source) {
-                    createReferenceReflection(context, source, symbol);
+                    const target = context.resolveAliasedSymbol(symbol);
+                    createReferenceReflection(context, source, target);
                 }
             });
         }
diff --git a/src/test/converter/exports/mod.ts b/src/test/converter/exports/mod.ts
index 2dc50fe83..b7e32bb6c 100644
--- a/src/test/converter/exports/mod.ts
+++ b/src/test/converter/exports/mod.ts
@@ -27,3 +27,9 @@ export default function() {
  * @hidden
  */
 export const hidden = true;
+
+/**
+ * This export is from a file external to the documentation that will not be included in the resulting docs.
+ * No reference should be created.
+ */
+export { Node } from 'typescript';
diff --git a/src/test/converter/exports/specs-without-exported.json b/src/test/converter/exports/specs-without-exported.json
index 4331ea498..74f6ecb0b 100644
--- a/src/test/converter/exports/specs-without-exported.json
+++ b/src/test/converter/exports/specs-without-exported.json
@@ -32,7 +32,7 @@
           "flags": {
             "isExported": true
           },
-          "target": 11
+          "target": 10
         },
         {
           "id": 17,
@@ -42,7 +42,7 @@
           "flags": {
             "isExported": true
           },
-          "target": 12
+          "target": 10
         },
         {
           "id": 19,
diff --git a/src/test/converter/exports/specs.json b/src/test/converter/exports/specs.json
index e5d9068c0..32586c974 100644
--- a/src/test/converter/exports/specs.json
+++ b/src/test/converter/exports/specs.json
@@ -32,7 +32,7 @@
           "flags": {
             "isExported": true
           },
-          "target": 21
+          "target": 20
         },
         {
           "id": 29,
@@ -42,7 +42,7 @@
           "flags": {
             "isExported": true
           },
-          "target": 22
+          "target": 20
         },
         {
           "id": 31,