Skip to content

Elide import namespace from which only const enums are used#20320

Merged
weswigham merged 1 commit into
microsoft:masterfrom
weswigham:elide-namespace-only-used-as-const-enum
Nov 30, 2017
Merged

Elide import namespace from which only const enums are used#20320
weswigham merged 1 commit into
microsoft:masterfrom
weswigham:elide-namespace-only-used-as-const-enum

Conversation

@weswigham
Copy link
Copy Markdown
Member

Fixes #18644

This was a bit more involved than expected. For one, we needed to not mark the namespace alias as referenced (as discussed in the issue), but only after we've verified that we're only accessing a const enum from it. For two, it turns out we actually didn't handle the visibility of export import ... = aliases correctly -
once Z.v in a type position no longer marked Z as referenced via point one, we hadn't considered that since it is exported it still needed to be considered referenced if they are values.

Comment thread src/compiler/checker.ts

function checkPropertyAccessExpressionOrQualifiedName(node: PropertyAccessExpression | QualifiedName, left: Expression | QualifiedName, right: Identifier) {
let propType: Type;
let leftSymbol = getNodeLinks(left) && getNodeLinks(left).resolvedSymbol;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is defined in the block below already.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind

Comment thread src/compiler/checker.ts
function checkPropertyAccessExpressionOrQualifiedName(node: PropertyAccessExpression | QualifiedName, left: Expression | QualifiedName, right: Identifier) {
let propType: Type;
let leftSymbol = getNodeLinks(left) && getNodeLinks(left).resolvedSymbol;
const leftWasReferenced = leftSymbol && getSymbolLinks(leftSymbol).referenced;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be only used in the else block as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nevermind.. i understand now :)

@weswigham weswigham merged commit a625dec into microsoft:master Nov 30, 2017
@weswigham weswigham deleted the elide-namespace-only-used-as-const-enum branch November 30, 2017 00:36
@sandersn
Copy link
Copy Markdown
Member

sandersn commented Dec 8, 2017

This is causing some unexpected emit changes in RWC. Specifically, a pattern like this:

namespace X {
  import Internal = Y.Z.Internal;
  class C {
    private x: Internal.D;
    constructor() {
      this.x = new Internal.D();
    }
  }
}

where D is a class in some other file, not a const enum.

At least I think this is unexpected. Am I correct?

@weswigham
Copy link
Copy Markdown
Member Author

@sandersn What's the issue? Is the import being elided when it should not? I encountered some issues WRT to import aliases when working on the fix that I thought I had gotten all of (at least all of the ones witnessed by our normal test suite) by correcting export import statement visibility. What does the export for Y.Z.Internal look like?

@sandersn
Copy link
Copy Markdown
Member

sandersn commented Dec 8, 2017

Yes, the import is elided. I’ll anonymize a sample when I get to my computer, but I recall it was just an exported class inside a namespace.

@sandersn
Copy link
Copy Markdown
Member

sandersn commented Dec 8, 2017

namespace Microsoft.Y.Z {
  // ...
  export namespace Internal {
    // ...
    export class D ...

and the structure of the user is

/// <reference "y.z.ts" />
namespace Microsoft.Software.Thing.Internal {
  // ...
  namespace X {
    import Internal = Y.Z.Internal // This line is no longer emitted
    class C {
      private x: Internal.D;
      constructor() {
        this.x = new Internal.D();
      }
    }
  }
}

weswigham added a commit to weswigham/TypeScript that referenced this pull request Dec 9, 2017
weswigham added a commit that referenced this pull request Dec 11, 2017
…#20588)

* Reimplement #20320 less elegantly but handle odd check orders better

* Consolidate 2 of 3 conditions
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants