Merge pull request #3185 from Microsoft/variableStatementComments

Don't emit leading/trailing comments for omitted variable statements
2026-02-14 19:16:17 -06:00 · 2015-05-16 15:50:47 -07:00 · 2015-05-16 15:50:47 -07:00 · 0287d2fc80
commit 0287d2fc80
parent 74f1dd61ad 3664f29689
26 changed files with 31 additions and 42 deletions
--- a/src/compiler/emitter.ts
+++ b/src/compiler/emitter.ts
@ -2229,9 +2229,10 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                emitEmbeddedStatement(node.statement);
            }

-            /* Returns true if start of variable declaration list was emitted.
-             * Return false if nothing was written - this can happen for source file level variable declarations
-             * in system modules - such variable declarations are hoisted.
+            /**
+             * Returns true if start of variable declaration list was emitted.
+             * Returns false if nothing was written - this can happen for source file level variable declarations
+             *     in system modules where such variable declarations are hoisted.
             */
            function tryEmitStartOfVariableDeclarationList(decl: VariableDeclarationList, startPos?: number): boolean {
                if (shouldHoistVariable(decl, /*checkIfSourceFileLevelDecl*/ true)) {
@ -3060,6 +3061,7 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {

            function emitVariableStatement(node: VariableStatement) {
                let startIsEmitted = false;
+
                if (node.flags & NodeFlags.Export) {
                    if (isES6ExportedDeclaration(node)) {
                        // Exported ES6 module member
@ -3070,6 +3072,7 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                else {
                    startIsEmitted = tryEmitStartOfVariableDeclarationList(node.declarationList);
                }
+
                if (startIsEmitted) {
                    emitCommaList(node.declarationList.declarations);
                    write(";");
@ -3085,6 +3088,28 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                }
            }

+            function shouldEmitLeadingAndTrailingCommentsForVariableStatement(node: VariableStatement) {
+                // If we're not exporting the variables, there's nothing special here.
+                // Always emit comments for these nodes.
+                if (!(node.flags & NodeFlags.Export)) {
+                    return true;
+                }
+
+                // If we are exporting, but it's a top-level ES6 module exports,
+                // we'll emit the declaration list verbatim, so emit comments too.
+                if (isES6ExportedDeclaration(node)) {
+                    return true;
+                }
+
+                // Otherwise, only emit if we have at least one initializer present.
+                for (let declaration of node.declarationList.declarations) {
+                    if (declaration.initializer) {
+                        return true;
+                    }
+                }
+                return false;
+            }
+
            function emitParameter(node: ParameterDeclaration) {
                if (languageVersion < ScriptTarget.ES6) {
                    if (isBindingPattern(node.name)) {
@ -5747,6 +5772,9 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                    case SyntaxKind.ExportAssignment:
                        return false;

+                    case SyntaxKind.VariableStatement:
+                        return shouldEmitLeadingAndTrailingCommentsForVariableStatement(<VariableStatement>node);
+
                    case SyntaxKind.ModuleDeclaration:
                        // Only emit the leading/trailing comments for a module if we're actually
                        // emitting the module as well.
--- a/tests/baselines/reference/commentsBeforeVariableStatement1.js
+++ b/tests/baselines/reference/commentsBeforeVariableStatement1.js
@ -5,5 +5,4 @@ export var b: number;

 //// [commentsBeforeVariableStatement1.js]
 define(["require", "exports"], function (require, exports) {
-    /** b's comment*/
 });
--- a/tests/baselines/reference/commentsExternalModules.js
+++ b/tests/baselines/reference/commentsExternalModules.js
@ -66,7 +66,6 @@ define(["require", "exports"], function (require, exports) {
    /** Module comment*/
    var m1;
    (function (m1) {
-        /** b's comment*/
        /** foo's comment*/
        function foo() {
            return m1.b;
@ -96,7 +95,6 @@ define(["require", "exports"], function (require, exports) {
    /** Module comment */
    var m4;
    (function (m4) {
-        /** b's comment */
        /** foo's comment
        */
        function foo() {
--- a/tests/baselines/reference/commentsExternalModules2.js
+++ b/tests/baselines/reference/commentsExternalModules2.js
@ -66,7 +66,6 @@ define(["require", "exports"], function (require, exports) {
    /** Module comment*/
    var m1;
    (function (m1) {
-        /** b's comment*/
        /** foo's comment*/
        function foo() {
            return m1.b;
@ -96,7 +95,6 @@ define(["require", "exports"], function (require, exports) {
    /** Module comment */
    var m4;
    (function (m4) {
-        /** b's comment */
        /** foo's comment
        */
        function foo() {
--- a/tests/baselines/reference/commentsExternalModules3.js
+++ b/tests/baselines/reference/commentsExternalModules3.js
@ -65,7 +65,6 @@ export var newVar2 = new extMod.m4.m2.c();
 /** Module comment*/
 var m1;
 (function (m1) {
-    /** b's comment*/
    /** foo's comment*/
    function foo() {
        return m1.b;
@ -95,7 +94,6 @@ var myvar = new m1.m2.c();
 /** Module comment */
 var m4;
 (function (m4) {
-    /** b's comment */
    /** foo's comment
    */
    function foo() {
--- a/tests/baselines/reference/commentsModules.js
+++ b/tests/baselines/reference/commentsModules.js
@ -101,7 +101,6 @@ new m7.m8.m9.c();
 /** Module comment*/
 var m1;
 (function (m1) {
-    /** b's comment*/
    /** foo's comment*/
    function foo() {
        return m1.b;
--- a/tests/baselines/reference/declFileTypeAnnotationVisibilityErrorTypeAlias.js
+++ b/tests/baselines/reference/declFileTypeAnnotationVisibilityErrorTypeAlias.js
@ -52,7 +52,6 @@ var M;
            return Window;
        })();
        N.Window = Window;
-         // Should report error that W is private
    })(N = M.N || (M.N = {}));
 })(M || (M = {}));
 var M1;
@ -65,7 +64,6 @@ var M1;
            return Window;
        })();
        N.Window = Window;
-         // No error
    })(N = M1.N || (M1.N = {}));
 })(M1 || (M1 = {}));
 var M2;
--- a/tests/baselines/reference/declFileTypeAnnotationVisibilityErrorTypeLiteral.js
+++ b/tests/baselines/reference/declFileTypeAnnotationVisibilityErrorTypeLiteral.js
@ -59,8 +59,6 @@ var m;
        }
    };
    m.x3 = m.x;
-    // Function type
    m.y2 = m.y;
-    // constructor type
    m.z2 = m.z;
 })(m || (m = {}));
--- a/tests/baselines/reference/declarationEmit_nameConflicts3.js
+++ b/tests/baselines/reference/declarationEmit_nameConflicts3.js
@ -74,7 +74,6 @@ var M;
            D[D["f"] = 0] = "f";
        })(P.D || (P.D = {}));
        var D = P.D;
-         // ok
        P.w = M.D.f; // error, should be typeof M.D.f
        P.x = M.C.f; // error, should be typeof M.C.f
        P.x = M.E.f; // error, should be typeof M.E.f
--- a/tests/baselines/reference/declarationEmit_nameConflictsWithAlias.js
+++ b/tests/baselines/reference/declarationEmit_nameConflictsWithAlias.js
@ -9,7 +9,6 @@ export module M {
 //// [declarationEmit_nameConflictsWithAlias.js]
 var M;
 (function (M) {
-     // Gets emitted as C.I, which is the wrong interface
 })(M = exports.M || (exports.M = {}));


--- a/tests/baselines/reference/duplicateIdentifiersAcrossContainerBoundaries.js
+++ b/tests/baselines/reference/duplicateIdentifiersAcrossContainerBoundaries.js
@ -117,7 +117,6 @@ var Foo = (function () {
 })();
 var Foo;
 (function (Foo) {
-     // error for redeclaring var in a different parent
 })(Foo || (Foo = {}));
 var N;
 (function (N) {
--- a/tests/baselines/reference/duplicateSymbolsExportMatching.js
+++ b/tests/baselines/reference/duplicateSymbolsExportMatching.js
@ -83,9 +83,7 @@ define(["require", "exports"], function (require, exports) {
    var M2;
    (function (M2) {
        var v;
-         // one error (visibility)
        var w;
-         // two errors (visibility and type mismatch)
    })(M2 || (M2 = {}));
    var M;
    (function (M) {
--- a/tests/baselines/reference/exportPrivateType.js
+++ b/tests/baselines/reference/exportPrivateType.js
@ -44,6 +44,5 @@ var foo;
        C2.prototype.test = function () { return true; };
        return C2;
    })();
-    // None of the types are exported, so per section 10.3, should all be errors
 })(foo || (foo = {}));
 var y = foo.g; // Exported variable 'y' has or is using private type 'foo.C2'.
--- a/tests/baselines/reference/invalidNestedModules.js
+++ b/tests/baselines/reference/invalidNestedModules.js
@ -73,6 +73,5 @@ var M2;
 (function (M2) {
    var X;
    (function (X) {
-         // Error
    })(X = M2.X || (M2.X = {}));
 })(M2 || (M2 = {}));
--- a/tests/baselines/reference/privacyCheckTypeOfInvisibleModuleNoError.js
+++ b/tests/baselines/reference/privacyCheckTypeOfInvisibleModuleNoError.js
@ -14,5 +14,4 @@ var Outer;
    var Inner;
    (function (Inner) {
    })(Inner || (Inner = {}));
-     // Since we dont unwind inner any more, it is error here
 })(Outer || (Outer = {}));
--- a/tests/baselines/reference/privacyGloVar.js
+++ b/tests/baselines/reference/privacyGloVar.js
@ -125,7 +125,6 @@ var m1;
    })();
    var m1_v1_private;
    var m1_v3_private;
-     // error
    var m1_v11_private = new C1_public();
    m1.m1_v12_public = new C1_public();
    var m1_v13_private = new C2_private();
--- a/tests/baselines/reference/privacyVar.js
+++ b/tests/baselines/reference/privacyVar.js
@ -219,7 +219,6 @@ var m1;
    })();
    var m1_v1_private;
    var m1_v3_private;
-     // error
    var m1_v11_private = new C1_public();
    m1.m1_v12_public = new C1_public();
    var m1_v13_private = new C2_private();
@ -324,7 +323,6 @@ var glo_C4_public = (function () {
 })();
 var glo_v1_private;
 var glo_v3_private;
- // error
 var glo_v11_private = new glo_C1_public();
 exports.glo_v12_public = new glo_C1_public();
 var glo_v13_private = new glo_C2_private();
--- a/tests/baselines/reference/privacyVarDeclFile.js
+++ b/tests/baselines/reference/privacyVarDeclFile.js
@ -459,7 +459,6 @@ var privateClassWithWithPublicPropertyTypes = (function () {
    }
    return privateClassWithWithPublicPropertyTypes;
 })();
- // Error
 var privateVarWithPrivatePropertyTypes;
 var privateVarWithPublicPropertyTypes;
 var publicClassWithPrivateModulePropertyTypes = (function () {
@ -468,7 +467,6 @@ var publicClassWithPrivateModulePropertyTypes = (function () {
    return publicClassWithPrivateModulePropertyTypes;
 })();
 exports.publicClassWithPrivateModulePropertyTypes = publicClassWithPrivateModulePropertyTypes;
- // Error
 var privateClassWithPrivateModulePropertyTypes = (function () {
    function privateClassWithPrivateModulePropertyTypes() {
    }
@ -510,7 +508,6 @@ var publicModule;
        }
        return privateClassWithWithPublicPropertyTypes;
    })();
-     // Error
    var privateVarWithPrivatePropertyTypes;
    var privateVarWithPublicPropertyTypes;
    var publicClassWithPrivateModulePropertyTypes = (function () {
@ -519,7 +516,6 @@ var publicModule;
        return publicClassWithPrivateModulePropertyTypes;
    })();
    publicModule.publicClassWithPrivateModulePropertyTypes = publicClassWithPrivateModulePropertyTypes;
-     // Error
    var privateClassWithPrivateModulePropertyTypes = (function () {
        function privateClassWithPrivateModulePropertyTypes() {
        }
@ -674,7 +670,6 @@ var publicModuleInGlobal;
        }
        return privateClassWithWithPublicPropertyTypes;
    })();
-     // Error
    var privateVarWithPrivatePropertyTypes;
    var privateVarWithPublicPropertyTypes;
    var publicClassWithPrivateModulePropertyTypes = (function () {
@ -683,7 +678,6 @@ var publicModuleInGlobal;
        return publicClassWithPrivateModulePropertyTypes;
    })();
    publicModuleInGlobal.publicClassWithPrivateModulePropertyTypes = publicClassWithPrivateModulePropertyTypes;
-     // Error
    var privateClassWithPrivateModulePropertyTypes = (function () {
        function privateClassWithPrivateModulePropertyTypes() {
        }
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType1.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType1.js
@ -27,5 +27,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType1_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType2.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType2.js
@ -31,5 +31,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType2_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType3.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType3.js
@ -35,5 +35,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType3_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType4.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType4.js
@ -27,5 +27,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType4_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType5.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType5.js
@ -34,5 +34,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType5_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType6.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType6.js
@ -41,5 +41,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType6_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType7.js
+++ b/tests/baselines/reference/recursiveExportAssignmentAndFindAliasedType7.js
@ -46,5 +46,4 @@ define(["require", "exports"], function (require, exports) {
 });
 //// [recursiveExportAssignmentAndFindAliasedType7_moduleA.js]
 define(["require", "exports"], function (require, exports) {
-     // This should result in type ClassB
 });
--- a/tests/baselines/reference/typeGuardsInModule.js
+++ b/tests/baselines/reference/typeGuardsInModule.js
@ -107,7 +107,6 @@ var m1;
    else {
        num = var2; // number
    }
-    // exported variable in the module
    if (typeof m1.var3 === "string") {
        strOrNum = m1.var3; // string | number
    }
@ -135,7 +134,6 @@ var m2;
        else {
            num = var4; // number
        }
-        // exported variable in the module
        if (typeof m3.var5 === "string") {
            strOrNum = m3.var5; // string | number
        }
@ -159,7 +157,6 @@ var m3;
        else {
            num = var2; // number
        }
-        // exported variable in the module
        if (typeof m4.var3 === "string") {
            strOrNum = m4.var3; // string | number
        }