Support declaration emit for late bound element accesses assigned to functions in both TS and JS #36593

weswigham · 2020-02-04T00:36:24Z

This is uglier than I'd expected because our handling of exports in commonjs is... hard. The layering of commonjs export assignments and late bound members is tough to get right without breaking normal augmentations.

…functions in both TS and JS

andrewbranch · 2020-02-06T18:47:33Z

tests/baselines/reference/lateBoundElementAccessAssignmentDeclarationsInJs.js

+//// [file.d.ts]
+export function foo(): void;
+export namespace foo {
+    export const bar: number;
+    export const strMemName: string;
+}


Is there a reason we couldn’t generate

export const foo: { (): void; bar: number; strMemName: string; "dashed-str-mem": string; 42: string; };

?

Hmm, we could; I think I already have the bail path for that setup, and just need to adjust the conditions for it to occur. Thing is, that has the potential to break, too - if one of my members was an alias, eg foo.Class = ClassRef, the only way to preserve that alias is by emitting it as an alias in a namespace, which can't then merge with a const. So it's like aliases and non-identifier names are mutually exclusive, syntactically T.T

Remind me -- did the previous PR also just skip declaration emit for late-bound symbols?

andrewbranch · 2020-02-11T18:40:00Z

src/compiler/checker.ts

+                // We _must_ resolve any possible commonjs export assignments _before_ merging in late bound members, as cjs export assignments may cause
+                // things to be merged (?!?) into this symbol; moreover, we _can't_ resolve those export assignments if we're already resolving the containing
+                // module (as then we'll issue a circularity error)


The reason I haven’t approved this PR is because I don’t really understand this (or really, why changes in alias resolution were involved generally), so I was hoping someone else would. But since nobody else has come by, if you want to expound on this, I’ll take a closer look.

I agree; it seems like this is needed to support checking, not necessarily declaration emit, since it skips late-bound symbols anyway.

The checking changed because the declaration emit made it evident it was wrong. We were using symbol.exports before; this was missing late bound members, so we swapped to using getExportsOfSymbol in this PR; however using that made it obvious that the resolved exports were being cached without any cjs merged exports in them! (So symbol.exports had members that were never reflected in the resolved exports!) That was because we were not reliably doing the cjs merge before caching the resolved exports; this new check guarantees that we try to do the cjs merge before caching resolved exports, every time.

sandersn

Questions and typo suggestions.

Github's suggestion comments seem to be turned off, which is weird.

sandersn · 2020-02-11T18:48:58Z

tests/baselines/reference/lateBoundElementAccessAssignmentDeclarationsInJs.js

+//// [file.d.ts]
+export function foo(): void;
+export namespace foo {
+    export const bar: number;
+    export const strMemName: string;
+}


Remind me -- did the previous PR also just skip declaration emit for late-bound symbols?

sandersn · 2020-02-11T18:53:00Z

src/compiler/transformers/declarations/diagnostics.ts

@@ -166,7 +170,7 @@ namespace ts {
            }
            // This check is to ensure we don't report error on constructor parameter property as that error would be reported during parameter emit
            // The only exception here is if the constructor was marked as private. we are not emitting the constructor parameters at all.
-            else if (node.kind === SyntaxKind.PropertyDeclaration || node.kind === SyntaxKind.PropertyAccessExpression || node.kind === SyntaxKind.PropertySignature ||
+            else if (node.kind === SyntaxKind.PropertyDeclaration || node.kind === SyntaxKind.PropertyAccessExpression || node.kind === SyntaxKind.ElementAccessExpression || node.kind === SyntaxKind.BinaryExpression || node.kind === SyntaxKind.PropertySignature ||


would be nice to break these long lines

sandersn · 2020-02-11T18:54:40Z

src/compiler/checker.ts

@@ -8884,6 +8896,17 @@ namespace ts {
        function getResolvedMembersOrExportsOfSymbol(symbol: Symbol, resolutionKind: MembersOrExportsResolutionKind): UnderscoreEscapedMap<Symbol> {
            const links = getSymbolLinks(symbol);
            if (!links[resolutionKind]) {
+                // We _must_ resolve any possible commonjs export assignments _before_ merging in late bound members, as cjs export assignments may cause
+                // things to be merged (?!?) into this symbol; moreover, we _can't_ resolve those export assignments if we're already resolving the containing


can you give an example test of this? I think I can imagine it, but I may be thinking of something different.

which means we need to do a cjs merge to merge the typedefs into the exports

I’m missing how we get to here... did you mean to use FooProp in your example scenario? I can’t tell why resolving anything related to Foo would require resolving any exports of the top-level module.

I’m missing how we get to here... did you mean to use FooProp in your example scenario?

Nope. I did not.

I can’t tell why resolving anything related to Foo would require resolving any exports of the top-level module.

The "top level module" is defined by a export= member which is an alias pointing at Foo.A. (And a typedef). We don't really support having both an export= and another top-level export, so to handle that scenario, we merge the other top-level export into the target of the export= alias symbol. I suppose the more telling example is when the final line is module.exports = Foo; instead. Foo clearly needs to have both FooProp and "field" members merged into it. OK, so you handle that by layering the export table merges (and forcing the cjs exports merge to always come first) - once that's done, you look at the module.exports = Foo.A case. Well, in resolving Foo.A, you need the exports of Foo, which means getting its exports; before it's safe to get its exports, we don't yet know if it's the target of the export = (imagine if Foo.A = Foo!), so we must perform the containing file's cjs merge (to ensure the exports of the target are complete, in case the target winds up being the symbol we're about to look up). Doing so would attempt to resolve export= again, which is a circularity, as we are already doing that.

we merge the other top-level export into the target of the export= alias symbol

Ohhh, yep, I get it now. Thanks 👍

sandersn · 2020-02-11T18:56:29Z

src/compiler/transformers/declarations.ts

+                            }
+                            const name = unescapeLeadingUnderscores(p.escapedName);
+                            if (!isIdentifierText(name, ScriptTarget.ES3)) {
+                                return undefined; // TODO: Rather than quietly eliding (as is current behavior), maybe we should issue errors?


issuing errors would essentially conflict with this PR, though, right? It seems like this PR's intent is to elide.

Issuing errors would be a breaking change, since our past behavior with late bound stuff has always been to elide what we couldn't emit. Still, it means we're quietly omitting type information from the declaration file. Usually that'd warrant an error in any other scenario, imo.

sandersn · 2020-02-11T18:57:21Z

src/compiler/checker.ts

+                if (!dontResolveAlias) {
+                    // we "cache" this result, but because both the input _and_ the output are mergeable, literally every call could need
+                    // a different result, assuming the inputs and outputs get merged with. So instead we just use this as a flag that
+                    // export assignment resolution has occured on this symbol at least once. (because it might happen many times!)


so this is basically just circularity detection? why do you need an initial value "circular" then? I think I'm misunderstanding your comments.

typo: occurred

So I have a value to flag before the result is ready to handle the reentrant case.

sandersn · 2020-02-11T18:58:52Z

src/compiler/checker.ts

@@ -5531,7 +5542,7 @@ namespace ts {
                function serializeSymbolWorker(symbol: Symbol, isPrivate: boolean, propertyAsAlias: boolean) {
                    const symbolName = unescapeLeadingUnderscores(symbol.escapedName);
                    const isDefault = symbol.escapedName === InternalSymbolName.Default;
-                    if (isStringANonContextualKeyword(symbolName) && !isDefault) {
+                    if ((isStringANonContextualKeyword(symbolName) || !isIdentifierText(symbolName, languageVersion)) && !isDefault && symbol.escapedName !== InternalSymbolName.ExportEquals) {
                        // Oh no. We cannot use this symbol's name as it's name... It's likely some jsdoc had an invalid name like `export` or `default` :(


suggestion: as its name
not as it's name

sandersn · 2020-02-11T19:02:43Z

src/compiler/checker.ts

+                if (p && isSourceFile(p) && p.symbol && !getSymbolLinks(p.symbol).resolvedExternalModuleSymbol) {
+                    const exported = resolveExternalModuleSymbol(p.symbol);
+                    const targetLinks = exported && getSymbolLinks(exported);
+                    if (targetLinks && targetLinks[resolutionKind]) {


would targetLinks?.[resolutionKind] work or did that make it into the spec?

Yeah that works

sandersn · 2020-02-11T19:03:44Z

src/compiler/checker.ts

+                // We _must_ resolve any possible commonjs export assignments _before_ merging in late bound members, as cjs export assignments may cause
+                // things to be merged (?!?) into this symbol; moreover, we _can't_ resolve those export assignments if we're already resolving the containing
+                // module (as then we'll issue a circularity error)


I agree; it seems like this is needed to support checking, not necessarily declaration emit, since it skips late-bound symbols anyway.

…gned to functions in both TS and JS (microsoft#36593)" This reverts commit 3e4ce47.

…gned to functions in both TS and JS (#36593)" (#37034) This reverts commit 3e4ce47.

…ses assigned to functions in both TS and JS (microsoft#36593)" (microsoft#37034)" This reverts commit 4d5464e.

Support declaration emit for late bound element accesses assigned to …

8db13ea

…functions in both TS and JS

weswigham requested review from sandersn, andrewbranch and RyanCavanaugh February 4, 2020 00:36

typescript-bot assigned weswigham Feb 4, 2020

typescript-bot added the Author: Team label Feb 4, 2020

andrewbranch reviewed Feb 6, 2020

View reviewed changes

andrewbranch reviewed Feb 11, 2020

View reviewed changes

sandersn requested changes Feb 11, 2020

View reviewed changes

sandersn approved these changes Feb 12, 2020

View reviewed changes

andrewbranch approved these changes Feb 25, 2020

View reviewed changes

weswigham merged commit 3e4ce47 into microsoft:master Feb 25, 2020

weswigham deleted the late-bound-element-access-declaration-emit branch February 25, 2020 21:45

weswigham added a commit to weswigham/TypeScript that referenced this pull request Feb 26, 2020

Revert "Support declaration emit for late bound element accesses assi…

6536e06

…gned to functions in both TS and JS (microsoft#36593)" This reverts commit 3e4ce47.

weswigham mentioned this pull request Feb 26, 2020

Revert "Support declaration emit for late bound element accesses assi… #37034

Merged

weswigham added a commit that referenced this pull request Feb 26, 2020

Revert "Support declaration emit for late bound element accesses assi…

4d5464e

…gned to functions in both TS and JS (#36593)" (#37034) This reverts commit 3e4ce47.

weswigham added a commit to weswigham/TypeScript that referenced this pull request Feb 26, 2020

Revert "Revert "Support declaration emit for late bound element acces…

a24659d

…ses assigned to functions in both TS and JS (microsoft#36593)" (microsoft#37034)" This reverts commit 4d5464e.

weswigham mentioned this pull request Feb 26, 2020

Revert "Revert "Support declaration emit for late bound element acces… #37035

Closed

Support declaration emit for late bound element accesses assigned to functions in both TS and JS #36593

Support declaration emit for late bound element accesses assigned to functions in both TS and JS #36593

Uh oh!

Conversation

weswigham commented Feb 4, 2020

Uh oh!

andrewbranch Feb 6, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham Feb 11, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewbranch Feb 6, 2020 •

edited

Loading

weswigham Feb 11, 2020 •

edited

Loading