Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters #38982

weswigham · 2020-06-08T19:46:53Z

cc @a-tarasyuk I pulled in your test from #38874; I went with a slightly different style of emit for the normal declaration emitter, and, well, the js declaration emitter was just another beast entirely (note for reviewers: I inlined one of the helper functions in the js declaration emitter, so the change isn't actually that large; I just had to inline the helper to set a condition in a branch of it).

Note that the OP in #38750 was hoping to also have non-identifier strings like "foo bar" also roundtrip; unfortunately, with this emit scheme (as @DanielRosenwasser proposed), such a thing is not possible (without amending syntax somewhere anyway).

…original names

… declarations in both declaration emitters

…nction-props

…t up

weswigham · 2020-06-08T19:49:12Z

tests/baselines/reference/jsDeclarationsExportDefinePropertyEmit.js

@@ -139,7 +139,7 @@ export namespace f {
     * @template T
     * @param {T} a
     */
-    export function self<T>(a: T): T;
+    function self<T>(a: T): T;


While working on this, I found a need for the js declaration emitter to automatically add/strip export modifiers from nested namespace members as needed; so the result of this can be seen in some baseline updates like this - since every member of the containing namespace was exported, there was no need to explicitly have the export modifiers. Now they get stripped, consistent with our transform-based declaration emitter.

weswigham · 2020-06-08T19:50:32Z

tests/baselines/reference/jsdocImplements_class.js

@@ -73,7 +73,7 @@ declare class B3 implements A {
 }
 declare namespace Ns {
    export { C1 };
-    const C5: {
+    export const C5: {


Yes, this was mistakenly missing an export modifier before. (The export statement above made it required, but the emitter never added it).

weswigham · 2020-06-08T19:56:11Z

.../initial-build/loads-js-based-projects-with-non-moved-json-files-and-emits-them-correctly.js

@@ -285,7 +285,7 @@ module.exports = x;
      },
      "./obj.json": {
        "version": "2151907832-{\n    \"val\": 42\n}",
-        "signature": "-6323167306-export declare const val: number;\r\n",
+        "signature": "-5546159834-export const val: number;\r\n",


So! Fun fact: We parse json documents essentially as export = { val: 42 }. Previous to this change, we'd serialize it to

declare namespace export_ { export declare const val: number; } export = _export;

then inline it to

export declare const val: number;

Now, we first make

declare namespace export_ { const val: number; } export = _export;

since we can omit the export and declare modifiers, and inline to

const val: number;

which we then patch an export modifier back onto (since we're inlining a namespace where everything was implicitly exported):

export const val: number;

so our frugality with the export modifier has, in this case, also caused us to be more frugal with the declare modifier, too. This should be fine, since everything in a .d.ts file is already implicitly declare'd anyway.

sandersn

The fix looks right. I have a few suggestions and one small question before you merge.

sandersn · 2020-06-10T18:22:27Z

tests/cases/conformance/declarationEmit/nullPropertyName.ts

@@ -0,0 +1,84 @@
+// @declaration: true


nitpick: shouldn't this test be named tsDeclarationsFunctionKeywordPropExhaustive to match the js test?

I pulled this in from @a-tarasyuk 's PR and wanted to preserve his history.

sandersn · 2020-06-10T18:25:30Z

tests/baselines/reference/nullPropertyName.js

+declare namespace foo {
+    export var x: number;
+    export var y: number;
+    var _a: number;


wwhy doesn't the ts version generate good names like _break? I guess it's because the JS declaration emit is purpose-built for this, but the TS code is pre-existing.

Pretty much that. The TS code is just using getGeneratedNameForNode (which is simple and uses these alphanumeric names), while the JS code has a lot of scope tracking machinery in getInternalSymbolName to generate better names where possible.

sandersn · 2020-06-10T18:29:22Z

src/compiler/checker.ts

+                            /*modifiers*/ undefined,
+                            createNamedExports([createExportSpecifier(d.expression, createIdentifier(InternalSymbolName.Default))])
+                        ) : d);
+                        const exportModifierStripped = every(defaultReplaced, d => hasSyntacticModifier(d, ModifierFlags.Export)) ? map(defaultReplaced, removeExportModifier) : defaultReplaced;


Don't we have a mapSame that would make this more succnctly expressed as

Suggested change

const exportModifierStripped = every(defaultReplaced, d => hasSyntacticModifier(d, ModifierFlags.Export)) ? map(defaultReplaced, removeExportModifier) : defaultReplaced;

const exportModifierStripped = mapSame(defaultReplaced, d => hasSyntacticModifier(d, ModifierFlags.Export) ? removeExportModifier(d) : d);

Nope, that's not the same at all 😄

If and only if every export has an export modifier, then we can strip the export modifier from every export and retain the same meaning. With your suggestion, we'd remove export modifiers from everything, even if some things didn't have export modifiers, which is very wrong and would make privates public.

sandersn · 2020-06-10T18:32:04Z

src/compiler/checker.ts

@@ -5936,6 +5939,13 @@ namespace ts {
                    statement.modifierFlagsCache = 0;
                }

+                function removeExportModifier(statement: Statement) {


could this be defined closer to its usage?

I prefer it to be here, next to it's cousin, addExportModifier, whose implementation is nearly identical.

* upstream/master: (22 commits) Small fix in `getIsContextSensitiveAssignmentOrContextType` Simplify `visitObjectLiteralExpression` Fix handling of `aruments` in the emitter Fix declaration emit for property references of imported object literal types (microsoft#39055) Fix casing for wild card keys for implicit globs to get wild card directories to watch (microsoft#39049) DOM update 2020-06-12 fix(a11y): make ISSUE_TEMPLATE/Bug_report.md more accessible for folks with screen readers (microsoft#39013) Update request-pr-review script to latest version of octokit (microsoft#39031) pin version of octokit skip implements types with no symbols isDynamicName skips parentheses for element access Allow `e: unknown` in `catch` arguments Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters (microsoft#38982) Patch to use this.timeout() > 0 rather than this.enableTimeout() to work with mocha 8+ Handle missing return type nodes and nested type references missing type arguments in existing jsdoc node serialization (microsoft#39011) Add containerName to CallHierarchyItem (microsoft#38997) Fix isSameEntityName (microsoft#38999) Fix typo for 'blocklist' (microsoft#39001) remove errant tab Switch to isSymbolAccessible for both. ...

a-tarasyuk and others added 4 commits June 1, 2020 11:40

fix(38750): create unique names for keywords and re-export them with …

6664d25

…original names

Serialize (noncontextual) keyword named namespace members with export…

944e1da

… declarations in both declaration emitters

Merge remote-tracking branch 'a-tara/bug/38750' into keyword-named-fu…

6f817b8

…nction-props

Add exhaustive keyword emit test for js declaration emitter and fix i…

f668e48

…t up

weswigham requested review from sandersn and DanielRosenwasser June 8, 2020 19:46

typescript-bot assigned weswigham and unassigned weswigham Jun 8, 2020

typescript-bot added the Author: Team label Jun 8, 2020

weswigham commented Jun 8, 2020

View reviewed changes

sandersn approved these changes Jun 10, 2020

View reviewed changes

weswigham merged commit 08cb0b2 into microsoft:master Jun 10, 2020

weswigham deleted the keyword-named-function-props branch June 10, 2020 21:42

weswigham mentioned this pull request Jul 7, 2020

Sibling namespace missing name from import in allowJs declaration #38834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters #38982

Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters #38982

weswigham commented Jun 8, 2020

weswigham Jun 8, 2020

weswigham Jun 8, 2020

weswigham Jun 8, 2020

sandersn left a comment

sandersn Jun 10, 2020

weswigham Jun 10, 2020

sandersn Jun 10, 2020

weswigham Jun 10, 2020

sandersn Jun 10, 2020

weswigham Jun 10, 2020

sandersn Jun 10, 2020

weswigham Jun 10, 2020

	const exportModifierStripped = every(defaultReplaced, d => hasSyntacticModifier(d, ModifierFlags.Export)) ? map(defaultReplaced, removeExportModifier) : defaultReplaced;
	const exportModifierStripped = mapSame(defaultReplaced, d => hasSyntacticModifier(d, ModifierFlags.Export) ? removeExportModifier(d) : d);

Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters #38982

Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters #38982

Conversation

weswigham commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment