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

Alias for module.exports.x = x #40228

Merged
merged 4 commits into from
Sep 10, 2020
Merged

Alias for module.exports.x = x #40228

merged 4 commits into from
Sep 10, 2020

Conversation

sandersn
Copy link
Member

This fixes #40155 in a surprisingly small amount of code.

@weswigham any idea how alias resolution Just Works in the rest of the compiler?

This fixes #40155 in a surprisingly small amount of code.
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 24, 2020
const m = function () {
// I have no idea what to put here
Copy link
Member Author

Choose a reason for hiding this comment

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

I added m here because otherwise the generated code was export { unknown as methods }, which is correct, and better than before, but caused an error in the emitted .d.ts that I didn't want.

@weswigham
Copy link
Member

Once it's bound as an alias, once getTargetOfAliasDeclaration has an appropriate implementation for the alias declaration, everything should be lit up (except maybe some syntax remapping in the declaration serializer).

In the case of a lot of JS stuff, I added implementations for some things to getTargetOfAliasDeclaration even though they weren't yet bound as aliases so I could pretend they were in the declaration serializer (because the checker fallback lookup in JS often works a lot like an alias).

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

You'll probably also wanna add some module.exports.x = a.b.c and module.exports = class Foo {} style tests, too (since those should make aliases, too!)

SymbolFlags.Property | SymbolFlags.ExportValue | SymbolFlags.Class :
SymbolFlags.Property | SymbolFlags.ExportValue;
declareSymbol(symbol.exports!, symbol, node.left, flags, SymbolFlags.None);
const flags = isIdentifier(node.right) ? SymbolFlags.Alias
Copy link
Member

Choose a reason for hiding this comment

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

While isIdentifier is very narrow and works, we should probably reuse the isAliasableExpression condition we use for exportAssignmentIsAlias which is used for bindExportAssignment, so the rules for what gets an alias vs not is consistent for all possibly-an-alias expressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that isAliasableExpression is isEntityNameExpression || isClassExpression; previously module.x = class { } was not handled as aan alias, but doing so is more consistent. I'm going to see whether it breaks any of our tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated tests, it was an improvement overall. (But it does not, on its own, make module.exports.x = NS.C work; I'm looking to see what else needs to happen)

@sandersn
Copy link
Member Author

sandersn commented Sep 9, 2020

OK, the newest commit treats any aliasable expression as an alias. This applies to classes, too, so I removed the ad-hoc code that was in the binder before. I think this is an improvment.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Looks pretty good! It might be worth testing having some references to K inside K's body's types, just to check that local names of aliased things are getting serialized correctly in declaration emit.



//// [mod1.d.ts]
export var K: {
Copy link
Member

Choose a reason for hiding this comment

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

Hm - can we have an issue/update to get this to emit as

declare namespace NS {
    class _K {
      values(): void;
    }
    export {_K as K};
}
import _K = NS.K;
export {_K as K};

? That'd be more precise, and is reasonable enough given the (alias) symbol shape of the input, IMO. I imagine the serializer just isn't handling the NS nesting well right now, and it is falling back to a var-based emit, even though it all uses alias symbols.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sandersn
Copy link
Member Author

sandersn commented Sep 10, 2020

Adding return new NS.K() inside the nested class's method ends up serialising the return type as any. =( I'm going to go see why.

Edit: Works in the non-nested case, though.

@sandersn sandersn merged commit e350c35 into master Sep 10, 2020
@sandersn sandersn deleted the alias-for-export-assignment branch September 10, 2020 18:26
elibarzilay added a commit to elibarzilay/TypeScript that referenced this pull request Feb 11, 2021
…tPropertyOfType`

e350c35 (microsoft#40228) introduced a subtle bug: it switched the flags to an
alias, dropping `SymbolFlags.Property` --- and that makes
`symbolIsValue()` get to the `resolveAlias(symbol)` call, which leads to
`getPropertyOfType()` with`resolved.callSignatures`+`constructSignatures`
being `undefined`.  So initialize them in `setStructuredTypeMembers`
before calling `getNamedMembers()`.

Fixes microsoft#42350
elibarzilay added a commit to elibarzilay/TypeScript that referenced this pull request Feb 11, 2021
…tPropertyOfType`

e350c35 (microsoft#40228) introduced a subtle bug: it switched the flags to an
alias, dropping `SymbolFlags.Property` --- and that makes
`symbolIsValue()` get to the `resolveAlias(symbol)` call, which leads to
`getPropertyOfType()` with`resolved.callSignatures`+`constructSignatures`
being `undefined`.  So initialize them in `setStructuredTypeMembers`
before calling `getNamedMembers()`.

Fixes microsoft#42350
elibarzilay added a commit that referenced this pull request Feb 11, 2021
…tPropertyOfType`

e350c35 (#40228) introduced a subtle bug: it switched the flags to an
alias, dropping `SymbolFlags.Property` --- and that makes
`symbolIsValue()` get to the `resolveAlias(symbol)` call, which leads to
`getPropertyOfType()` with`resolved.callSignatures`+`constructSignatures`
being `undefined`.  So initialize them in `setStructuredTypeMembers`
before calling `getNamedMembers()`.

Fixes #42350
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Feb 25, 2021
Component commits:
ed26816 Avoid getting undefined `callSignatures`/`constructSignatures` in `getPropertyOfType`
e350c35 (microsoft#40228) introduced a subtle bug: it switched the flags to an
alias, dropping `SymbolFlags.Property` --- and that makes
`symbolIsValue()` get to the `resolveAlias(symbol)` call, which leads to
`getPropertyOfType()` with`resolved.callSignatures`+`constructSignatures`
being `undefined`.  So initialize them in `setStructuredTypeMembers`
before calling `getNamedMembers()`.

Fixes microsoft#42350
RyanCavanaugh pushed a commit that referenced this pull request Feb 25, 2021
Component commits:
ed26816 Avoid getting undefined `callSignatures`/`constructSignatures` in `getPropertyOfType`
e350c35 (#40228) introduced a subtle bug: it switched the flags to an
alias, dropping `SymbolFlags.Property` --- and that makes
`symbolIsValue()` get to the `resolveAlias(symbol)` call, which leads to
`getPropertyOfType()` with`resolved.callSignatures`+`constructSignatures`
being `undefined`.  So initialize them in `setStructuredTypeMembers`
before calling `getNamedMembers()`.

Fixes #42350

Co-authored-by: Eli Barzilay <eli@barzilay.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

In JS, commonjs require imports typeof class, not class instance type.
3 participants