Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[api-extractor] .d.ts rollup sometimes emits names that conflict with global names #1095

Closed
octogonz opened this issue Feb 11, 2019 · 8 comments · Fixed by #1767
Closed

[api-extractor] .d.ts rollup sometimes emits names that conflict with global names #1095

octogonz opened this issue Feb 11, 2019 · 8 comments · Fixed by #1767
Labels
bug Something isn't working as intended

Comments

@octogonz
Copy link
Collaborator

octogonz commented Feb 11, 2019

Consider this example:

import { Promise as MyPromise } from './file1';

export function f(p1: Promise<void>, p2: MyPromise<void>): void {
  // p1 is using the ambient Promise from the compiler runtime
  // p2 is using the declaration from file1, which happens to use the same name "Promise"
  // in that file
}

In the .d.ts rollup, p1 and p2 accidentally end up with the same type:

declare class Promise<T> {
    private x;
}

export declare function f(p1: Promise<void>, p2: Promise<void>): void;

This happens because the algorithm doesn't realize that Promise is a global name introduced by lib.es2015.promise.d.ts, which comes in via this part of tsconfig.json:

    "lib": [
      "es5",
      "scripthost",
      "es2015.collection",
      "es2015.promise",
      "es2015.iterable",
      "dom"
    ],

This is breaking @microsoft/sp-application-base.

@iclanton

@octogonz octogonz added the bug Something isn't working as intended label Feb 11, 2019
@octogonz octogonz changed the title [api-extractor] .d.ts rollup sometimes chooses names that conflict with ambient declarations [api-extractor] .d.ts rollup sometimes emits names that conflict with ambient declarations Feb 11, 2019
@octogonz octogonz changed the title [api-extractor] .d.ts rollup sometimes emits names that conflict with ambient declarations [api-extractor] .d.ts rollup sometimes emits names that conflict with global names Feb 12, 2019
@octogonz
Copy link
Collaborator Author

In typescript.d.ts I found this interface, which checks whether a name is global or not:

    interface PrintHandlers {
        /**
         * A hook used by the Printer when generating unique names to avoid collisions with
         * globally defined names that exist outside of the current source file.
         */
        hasGlobalName?(name: string): boolean;

However, it is a contract meant to be implemented by the caller of the Printer. Internally the compiler seems to implement it via the EmitResolver, which has this code:

namespace ts {
    . . .
    export function createTypeChecker(host: TypeCheckerHost, produceDiagnostics: boolean): TypeChecker {
        . . .
        const globals = createSymbolTable();
        . . .
        const checker: TypeChecker = {
            . . .
            hasGlobalName
            . . .
        };
        . . .
        return checker;
        . . .
        function hasGlobalName(name: string): boolean {
            return globals.has(escapeLeadingUnderscores(name));
        }

But when I try to use program.getTypeChecker() it complains that it was created with produceDiagnostics = false.

@octogonz
Copy link
Collaborator Author

The "es2015.promise" module can be found in the list of processed source files via this.program.getSourceFiles(), but these modules don't have symbols, so there's no "exports" that we can easily retrieve from them.

From any source file, you can enumerate Promise and Map and most other global names like this:

this.typeChecker.getSymbolsInScope(sourceFile, ts.SymbolFlags.Transient)

...however I'm pretty sure there exist global symbols that are not "transient", as well as "transient" symbols that are not global names.

The compiler docs don't say much about what "transient" actually means besides this, but it sounds like the wrong concept for what we need:

    export const enum SymbolFlags {
        . . .
        Transient               = 1 << 25,  // Transient symbol (created during type check)

@RyanCavanaugh @DanielRosenwasser any idea how to find the global names? For example, is there any example anywhere of someone implementing the PrintHandlers.hasGlobalName contract mentioned above?

@octogonz
Copy link
Collaborator Author

Okay, I found several internal APIs that can be combined together to reach hasGlobalName(): :-)

const typeChecker = (this.program as any).getDiagnosticsProducingTypeChecker();
const resolver = (typeChecker as any).getEmitResolver();
resolver.hasGlobalName('Promise'); // --> true

Is this a valid/safe solution?

@octogonz
Copy link
Collaborator Author

octogonz commented Feb 12, 2019

Hmm... the symbol table being queried by hasGlobalName() has 1,650 names in it... is that really correct?

Besides Promise, it also finds:

  • Partial
  • RTCStatsIceCandidateType
  • SVGAnimatedInteger
  • encodeURI
  • eval
  • it
  • "https" (quotes included)
  • "v8"

octogonz added a commit that referenced this issue Feb 12, 2019
octogonz added a commit that referenced this issue Feb 12, 2019
[api-extractor] Temporary workaround for issue #1095
@RyanCavanaugh
Copy link
Member

@octogonz that's correct. The global symbol table has everything you could reach from an unqualified position (Partial, encodeURI, etc) as well as all module names you could import without path qualification ("https", etc).

@octogonz
Copy link
Collaborator Author

@RyanCavanaugh Welcome back, BTW! 😄

@octogonz
Copy link
Collaborator Author

octogonz commented Mar 6, 2020

const typeChecker = (this.program as any).getDiagnosticsProducingTypeChecker();
const resolver = (typeChecker as any).getEmitResolver();
resolver.hasGlobalName('Promise'); // --> true

@rbuckton @RyanCavanaugh

I'm looking at implementing this now. Is getDiagnosticsProducingTypeChecker() still the most reasonable way to access hasGlobalName()?

I noticed this slightly concerning comment:

        // For testing purposes only.  Should not be used by any other consumers (including the
        // language service).
        /* @internal */ getDiagnosticsProducingTypeChecker(): TypeChecker;

@octogonz
Copy link
Collaborator Author

Fixed by PR #1767 which was published with API Extractor 7.7.12.

stevengum added a commit to microsoft/botbuilder-js that referenced this issue Mar 31, 2020
Stevenic pushed a commit to microsoft/botbuilder-js that referenced this issue Apr 20, 2020
* add @microsoft/api-extractor to botbuilder-core

* add api-extractor to libraries

* run api-extractor on all libs except for:
 * botframework-connector
 * adaptive-expressions (in preview)
 * botbuilder-lg (in preview)

* support friendlier jsonc render on GitHub

* bump @microsoft/api-extractor to ^7.7.12 b/c microsoft/rushstack/issues/1095

* regenerate bf-schema.api.md after master merge

* add codeowners, regen core & dialogs api.md

* regenerate botbuilder.api.md after merging master
javier-garcia-meteologica pushed a commit to javier-garcia-meteologica/rushstack that referenced this issue Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants