[api-extractor] Fix incorrect declaration references for local symbols within namespaces #3566

zelliott · 2022-08-02T15:18:57Z

Summary

Fixes the bug mentioned at #3552 (comment). Suppose you have the following code:

// package: "my-package"

export namespace n {
  type SomeType = number;
  export function someFunction(): SomeType { return 5; }
}

When building the excerpt tokens for someFunction's return type, API Extractor generates a canonical reference ID for SomeType. Today, it generates the wrong reference: my-package!SomeType:type. This is incorrect because the canonical reference needs to be qualified with the namespace n. The reference should be my-package!n~SomeType:type.

The reason it generates the wrong reference is because the logic in DeclarationReferenceGenerator is incomplete. The API symbol.parent is undefined for SomeType, presumably because it's unexported from the namespace (not entirely sure). Regardless, the solution implemented in this PR is to traverse the node tree to find the appropriate parent symbol.

Details

No additional details.

How it was tested

I added a new api-extractor-scenarios test directory called "referenceTokens" that I used to validate that the appropriate reference tokens were generated for various symbols.

I also cleaned up / extended some other api-extractor-scenarios test directories that I encountered throughout this PR.

zelliott · 2022-08-02T15:22:46Z

apps/api-extractor/src/generators/DeclarationReferenceGenerator.ts

+    // If that doesn't work, try to find a parent symbol via the node tree. As far as we can tell,
+    // this logic is only needed for local symbols within namespaces. For example:
+    //
+    // ```
+    // export namespace n {
+    //   type SomeType = number;
+    //   export function someFunction(): SomeType { return 5; }
+    // }
+    // ```
+    //
+    // In the example above, `SomeType` doesn't have a parent symbol per the TS internal API above,
+    // but its reference still needs to be qualified with the parent reference for `n`.
+    const grandParent: ts.Node | undefined = declaration?.parent?.parent;
+    if (grandParent && ts.isModuleDeclaration(grandParent)) {
+      const grandParentSymbol: ts.Symbol | undefined = TypeScriptInternals.tryGetSymbolForDeclaration(
+        grandParent,
+        this._typeChecker
+      );
+      if (grandParentSymbol) {
+        return this._symbolToDeclarationReference(
+          grandParentSymbol,
+          grandParentSymbol.flags,
+          /*includeModuleSymbols*/ true
+        );
+      }
+    }


This is the new logic that fixes the bug.

This code looks okay, but these sorts of changes sometimes cause a regression due to some obscure edge case. Let's publish this as as separate PATCH release from your other fix, and then we can get some large monorepos to try it.

zelliott · 2022-08-02T15:25:13Z

build-tests/api-extractor-scenarios/etc/apiItemKinds/api-extractor-scenarios.api.json

+            },
+            {
+              "kind": "Class",
+              "canonicalReference": "api-extractor-scenarios!n1.SomeClass1:class",


This canonical reference should be api-extractor-scenarios!n1~SomeClass1:class, but this bug is tackled in #3552. It's a separate code path, because this reference is generated by api-extractor-model itself from the .api.json, whereas the canonical references in tokens are generated by DeclarationReferenceGenerator from the TypeScript nodes.

It's also an existing bug that this item is included at all in the doc model, as it's unexported from the n1 namespace and thus not consumable. This bug is mentioned at #3552 (comment), and is due to the fact that items within namespaces don't have their own CollectorEntitys, and the CollectorEntity is the unit at which we currently track whether an item is exported/consumable or not.

I think we have simply never tested this n1.SomeClass1 case. It's not our typical coding pattern. 🙂

zelliott · 2022-08-02T15:30:51Z

build-tests/api-extractor-scenarios/etc/apiItemKinds/api-extractor-scenarios.api.md

+export namespace n1 {
+    // (undocumented)
+    export namespace n2 {
+        // (undocumented)
+        export class SomeClass3 {
+        }
+    }
+    // (undocumented)
+    export class SomeClass1 {
+    }
+    // (undocumented)
+    export class SomeClass2 extends SomeClass1 {
+    }
+        {};
+}


This is also an instance of the pre-existing bug mentioned in #3552 (comment). Notice the empty {} and the fact that SomeClass1 is exported here even though it's not actually exported in the original source file.

There are a few instances of this bug throughout this PR, JFYI.

export { } is a special .d.ts directive that disables a deprecated semantics for modules. (I don't remember the details offhand, but I think it was something like, if NO declaration is exported, then ALL declarations are implicitly exported?)

Here's your test case in the playground -- on the .d.ts tab, export { } appears inside n1.

{}; without export is definitely a bug.

octogonz · 2022-08-10T07:59:59Z

Thanks for making this fix BTW! The code change is small, but the concepts aren't, and the extra test cases are appreciated. 😎

…erences # Conflicts: # build-tests/api-extractor-scenarios/etc/apiItemKinds/api-extractor-scenarios.api.json # build-tests/api-extractor-scenarios/etc/apiItemKinds/api-extractor-scenarios.api.md # build-tests/api-extractor-scenarios/etc/apiItemKinds/rollup.d.ts # build-tests/install-test-workspace/workspace/common/pnpm-lock.yaml

octogonz · 2022-08-10T18:56:50Z

This was published as API Extractor 7.29.2. Today I tested it on a somewhat large data set and validated that the canonicalReference's exactly matched an earlier API Extractor output. 👍👍

[api-extractor] Fix incorrect declaration references for local symbols within namespaces

zelliott added 4 commits August 1, 2022 18:26

Fix many bugs in DeclarationReferenceGenerator

47a8fa7

Bit more work on the declaration reference generator changes

ad86ef3

Ran rush change

085e11b

Cleanup comments a bit

4dd0347

zelliott requested review from D4N14L, apostolisms, iclanton and octogonz as code owners August 2, 2022 15:18

zelliott commented Aug 2, 2022

View reviewed changes

zelliott mentioned this pull request Aug 2, 2022

[api-extractor] Add options to include forgotten exports in API report and doc model files #3552

Merged

Removed unnecessary test scenario

4deb6f6

octogonz approved these changes Aug 10, 2022

View reviewed changes

octogonz added 2 commits August 10, 2022 01:01

rush build

3f978c2

octogonz approved these changes Aug 10, 2022

View reviewed changes

octogonz merged commit 528e4be into microsoft:main Aug 10, 2022

Qjuh pushed a commit to discordjs/rushstack that referenced this pull request Nov 7, 2023

Merge pull request microsoft#3566 from zelliott/namespace-references

1f1d472

[api-extractor] Fix incorrect declaration references for local symbols within namespaces

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[api-extractor] Fix incorrect declaration references for local symbols within namespaces #3566

[api-extractor] Fix incorrect declaration references for local symbols within namespaces #3566

Uh oh!

zelliott commented Aug 2, 2022 •

edited

Loading

Uh oh!

zelliott Aug 2, 2022

Uh oh!

octogonz Aug 10, 2022

Uh oh!

zelliott Aug 2, 2022 •

edited

Loading

Uh oh!

zelliott Aug 2, 2022 •

edited

Loading

Uh oh!

octogonz Aug 10, 2022

Uh oh!

zelliott Aug 2, 2022 •

edited

Loading

Uh oh!

octogonz Aug 10, 2022

Uh oh!

octogonz commented Aug 10, 2022

Uh oh!

octogonz commented Aug 10, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[api-extractor] Fix incorrect declaration references for local symbols within namespaces #3566

[api-extractor] Fix incorrect declaration references for local symbols within namespaces #3566

Uh oh!

Conversation

zelliott commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

How it was tested

Uh oh!

zelliott Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

octogonz Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

zelliott Aug 2, 2022 • 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!

zelliott Aug 2, 2022 • 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!

octogonz Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

zelliott Aug 2, 2022 • 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!

octogonz Aug 10, 2022

Choose a reason for hiding this comment

Uh oh!

octogonz commented Aug 10, 2022

Uh oh!

octogonz commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zelliott commented Aug 2, 2022 •

edited

Loading

zelliott Aug 2, 2022 •

edited

Loading

zelliott Aug 2, 2022 •

edited

Loading

zelliott Aug 2, 2022 •

edited

Loading

octogonz commented Aug 10, 2022 •

edited

Loading