Skip to content

Fix bug: keep comments unless --removeComments #7213

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,21 +296,21 @@ namespace ts {
* If loop contains block scoped binding captured in some function then loop body is converted to a function.
* Lexical bindings declared in loop initializer will be passed into the loop body function as parameters,
* however if this binding is modified inside the body - this new value should be propagated back to the original binding.
* This is done by declaring new variable (out parameter holder) outside of the loop for every binding that is reassigned inside the body.
* This is done by declaring new variable (out parameter holder) outside of the loop for every binding that is reassigned inside the body.
* On every iteration this variable is initialized with value of corresponding binding.
* At every point where control flow leaves the loop either explicitly (break/continue) or implicitly (at the end of loop body)
* we copy the value inside the loop to the out parameter holder.
*
*
* for (let x;;) {
* let a = 1;
* let b = () => a;
* x++
* if (...) break;
* ...
* }
*
*
* will be converted to
*
*
* var out_x;
* var loop = function(x) {
* var a = 1;
Expand All @@ -326,7 +326,7 @@ namespace ts {
* x = out_x;
* if (state === "break") break;
* }
*
*
* NOTE: values to out parameters are not copies if loop is abrupted with 'return' - in this case this will end the entire enclosing function
* so nobody can observe this new value.
*/
Expand Down Expand Up @@ -3073,7 +3073,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
}

writeLine();
// end of loop body -> copy out parameter
// end of loop body -> copy out parameter
copyLoopOutParameters(convertedLoopState, CopyDirection.ToOutParameter, /*emitAsStatements*/true);

decreaseIndent();
Expand Down Expand Up @@ -7269,7 +7269,7 @@ const _super = (function (geti, seti) {
}

// text should be quoted string
// for deduplication purposes in key remove leading and trailing quotes so 'a' and "a" will be considered the same
// for deduplication purposes in key remove leading and trailing quotes so 'a' and "a" will be considered the same
const key = text.substr(1, text.length - 2);

if (hasProperty(groupIndices, key)) {
Expand Down Expand Up @@ -8070,7 +8070,7 @@ const _super = (function (geti, seti) {
leadingComments = getLeadingCommentsToEmit(node);
}
else {
// If the node will not be emitted in JS, remove all the comments(normal, pinned and ///) associated with the node,
// If the node will not be emitted in JS, remove /// reference comments associated with the node,
// unless it is a triple slash comment at the top of the file.
// For Example:
// /// <reference-path ...>
Expand All @@ -8081,6 +8081,11 @@ const _super = (function (geti, seti) {
if (node.pos === 0) {
leadingComments = filter(getLeadingCommentsToEmit(node), isTripleSlashComment);
}
else {
leadingComments = filter(getLeadingCommentsToEmit(node), function (comment: CommentRange) {
return !isTripleSlashComment(comment);
});
}
}

emitNewLineBeforeLeadingComments(currentLineMap, writer, node, leadingComments);
Expand Down
57 changes: 57 additions & 0 deletions tests/baselines/reference/1.0lib-noErrors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1158,3 +1158,60 @@ MERCHANTABLITY OR NON-INFRINGEMENT.
See the Apache Version 2.0 License for specific language governing permissions
and limitations under the License.
***************************************************************************** */
/**
* Evaluates JavaScript code and executes it.
* @param x A String value that contains valid JavaScript code.
*/
/**
* Converts A string to an integer.
* @param s A string to convert into a number.
* @param radix A value between 2 and 36 that specifies the base of the number in numString.
* If this argument is not supplied, strings with a prefix of '0x' are considered hexadecimal.
* All other strings are considered decimal.
*/
/**
* Converts a string to a floating-point number.
* @param string A string that contains a floating-point number.
*/
/**
* Returns a Boolean value that indicates whether a value is the reserved value NaN (not a number).
* @param number A numeric value.
*/
/**
* Determines whether a supplied number is finite.
* @param number Any numeric value.
*/
/**
* Gets the unencoded version of an encoded Uniform Resource Identifier (URI).
* @param encodedURI A value representing an encoded URI.
*/
/**
* Gets the unencoded version of an encoded component of a Uniform Resource Identifier (URI).
* @param encodedURIComponent A value representing an encoded URI component.
*/
/**
* Encodes a text string as a valid Uniform Resource Identifier (URI)
* @param uri A value representing an encoded URI.
*/
/**
* Encodes a text string as a valid component of a Uniform Resource Identifier (URI).
* @param uriComponent A value representing an encoded URI component.
*/
/**
* Provides functionality common to all JavaScript objects.
*/
/**
* Creates a new function.
*/
/**
* Allows manipulation and formatting of text strings and determination and location of substrings within strings.
*/
/** An object that represents a number of any kind. All JavaScript numbers are 64-bit floating-point numbers. */
/** An intrinsic object that provides basic mathematics functionality and constants. */
/** Enables basic storage and retrieval of dates and times. */
/**
* An intrinsic object that provides functions to convert JavaScript values to and from the JavaScript Object Notation (JSON) format.
*/
/////////////////////////////
/// ECMAScript Array API (specially handled by compiler)
/////////////////////////////
12 changes: 12 additions & 0 deletions tests/baselines/reference/ambientDeclarations.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,19 @@ declare module 'external1' {


//// [ambientDeclarations.js]
// Ambient variable with type annotation
// Ambient function with no type annotations
// Ambient function with type annotations
// Ambient function with valid overloads
// Ambient function with optional parameters
// Ambient class
// Ambient enum
// Ambient enum with integer literal initializer
// Ambient enum members are always exported with or without export keyword
Copy link
Member

Choose a reason for hiding this comment

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

This output is undesirable because it's extremely misleading. There's a reason we don't emit every comment, and this is the typical example of why.

Copy link
Author

Choose a reason for hiding this comment

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

But issue #2546 is precisely about keeping these comments. And it said PRs are welcomed. Can we define what are precisely the conditions to hide or show comments? Should we keep all comments of the type /** */ or also // (the original reporter in #2546 wanted the latter too)?

Copy link
Member

Choose a reason for hiding this comment

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

Can we define what are precisely the conditions to hide or show comments?

Honestly, that's the hard part (if we had a good rule, we would have done it already). Clearly leading comments for type declarations shouldn't be present in the emit as they look like misleading nonsense, but comments between an opening brace of a statement block and another expression statement should be present.

Figuring out where it makes sense to emit the comments and where it doesn't is the hard part. Simply emitting every comment isn't the right fix.

var x = E3.B;
// Ambient module
// Ambient module members are always exported with or without export keyword
var p = M1.x;
var q = M1.fn();
// Ambient external module in the global module
// Ambient external module with a string literal name that is a top level external module name
1 change: 1 addition & 0 deletions tests/baselines/reference/ambientEnum1.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@
}

//// [ambientEnum1.js]
// Ambient enum with computer member
11 changes: 11 additions & 0 deletions tests/baselines/reference/ambientErrors.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,15 @@ declare module 'bar' {


//// [ambientErrors.js]
// Ambient functions with invalid overloads
// Ambient functions with duplicate signatures
// Ambient function overloads that differ only by return type
// Ambient function with default parameter values
// Ambient function with function body
;
// Ambient enum with non - integer literal constant member
// Ambient enum with computer member
// Ambient module with initializers for values, bodies for functions / classes
// Ambient external module not in the global module
// Ambient external module with a string literal name that isn't a top level external module name
// Ambient external module with export assignment and other exported members
1 change: 1 addition & 0 deletions tests/baselines/reference/ambientExternalModuleMerging.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,4 @@ define(["require", "exports", "M"], function (require, exports, M) {
var y = M.y;
});
//// [ambientExternalModuleMerging_declare.js]
// Merge
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,11 @@ var Derived2 = (function (_super) {
}(Derived));
var TargetHasOptional;
(function (TargetHasOptional) {
// targets
var c;
var a;
var b = { opt: new Base() };
// sources
var d;
var e;
var f;
Expand All @@ -139,9 +141,11 @@ var TargetHasOptional;
})(TargetHasOptional || (TargetHasOptional = {}));
var SourceHasOptional;
(function (SourceHasOptional) {
// targets
var c;
var a;
var b = { opt: new Base() };
// sources
var d;
var e;
var f;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,11 @@ var Derived2 = (function (_super) {
}(Derived));
var TargetHasOptional;
(function (TargetHasOptional) {
// targets
var c;
var a;
var b = { opt: new Base() };
// sources
var d;
var e;
var f;
Expand All @@ -141,9 +143,11 @@ var TargetHasOptional;
})(TargetHasOptional || (TargetHasOptional = {}));
var SourceHasOptional;
(function (SourceHasOptional) {
// targets
var c;
var a;
var b = { opt: new Base() };
// sources
var d;
var e;
var f;
Expand Down
2 changes: 2 additions & 0 deletions tests/baselines/reference/augmentExportEquals1.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ define(["require", "exports"], function (require, exports) {
//// [file2.js]
define(["require", "exports"], function (require, exports) {
"use strict";
// augmentation for './file1'
// should error since './file1' does not have namespace meaning
});
//// [file3.js]
define(["require", "exports", "./file2"], function (require, exports) {
Expand Down
2 changes: 2 additions & 0 deletions tests/baselines/reference/augmentExportEquals1_1.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ let a: x.A; // should not work
//// [file2.js]
define(["require", "exports"], function (require, exports) {
"use strict";
// augmentation for 'file1'
// should error since 'file1' does not have namespace meaning
});
//// [file3.js]
define(["require", "exports", "file2"], function (require, exports) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals2.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ define(["require", "exports"], function (require, exports) {
//// [file2.js]
define(["require", "exports"], function (require, exports) {
"use strict";
// should error since './file1' does not have namespace meaning
});
//// [file3.js]
define(["require", "exports", "./file2"], function (require, exports) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals2_1.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ let a: x.A; // should not work
//// [file2.js]
define(["require", "exports"], function (require, exports) {
"use strict";
// should error since './file1' does not have namespace meaning
});
//// [file3.js]
define(["require", "exports", "file2"], function (require, exports) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals3.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ define(["require", "exports"], function (require, exports) {
define(["require", "exports", "./file1"], function (require, exports, x) {
"use strict";
x.b = 1;
// OK - './file1' is a namespace
});
//// [file3.js]
define(["require", "exports", "./file1", "./file2"], function (require, exports, x) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals3_1.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ let b = x.b;
define(["require", "exports", "file1"], function (require, exports, x) {
"use strict";
x.b = 1;
// OK - './file1' is a namespace
});
//// [file3.js]
define(["require", "exports", "file1", "file2"], function (require, exports, x) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals4.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ define(["require", "exports"], function (require, exports) {
define(["require", "exports", "./file1"], function (require, exports, x) {
"use strict";
x.b = 1;
// OK - './file1' is a namespace
});
//// [file3.js]
define(["require", "exports", "./file1", "./file2"], function (require, exports, x) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals4_1.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ let b = x.b;
define(["require", "exports", "file1"], function (require, exports, x) {
"use strict";
x.b = 1;
// OK - './file1' is a namespace
});
//// [file3.js]
define(["require", "exports", "file1", "file2"], function (require, exports, x) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals6.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ define(["require", "exports"], function (require, exports) {
define(["require", "exports", "./file1"], function (require, exports, x) {
"use strict";
x.B.b = 1;
// OK - './file1' is a namespace
});
//// [file3.js]
define(["require", "exports", "./file1", "./file2"], function (require, exports, x) {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/augmentExportEquals6_1.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ let b = a.a;
//// [file2.js]
define(["require", "exports"], function (require, exports) {
"use strict";
// OK - './file1' is a namespace
});
//// [file3.js]
define(["require", "exports", "file2"], function (require, exports) {
Expand Down
3 changes: 3 additions & 0 deletions tests/baselines/reference/augmentedTypesInterface.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ interface i4 {

//// [augmentedTypesInterface.js]
// interface then interface
// interface then class
var i2 = (function () {
function i2() {
}
Expand All @@ -43,9 +44,11 @@ var i2 = (function () {
};
return i2;
}());
// interface then enum
var i3;
(function (i3) {
i3[i3["One"] = 0] = "One";
})(i3 || (i3 = {}));
; // error
// interface then import
//import i4 = require(''); // error
4 changes: 4 additions & 0 deletions tests/baselines/reference/augmentedTypesModules.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ var m1d;
m1d.I = I;
})(m1d || (m1d = {}));
var m1d = 1; // error
// module then function
function m2() { }
; // ok since the module is not instantiated
var m2a;
Expand Down Expand Up @@ -156,6 +157,7 @@ var m2g;
}());
m2g.C = C;
})(m2g || (m2g = {}));
// module then class
var m3 = (function () {
function m3() {
}
Expand Down Expand Up @@ -209,6 +211,8 @@ var m3g;
}());
m3g.C = C;
})(m3g || (m3g = {}));
// module then enum
// should be errors
var m4;
(function (m4) {
})(m4 || (m4 = {}));
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/baseIndexSignatureResolution.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ var Derived = (function (_super) {
}
return Derived;
}(Base));
// Note - commmenting "extends Foo" prevents the error
var x = null;
var y = x[0];
/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,4 @@ var OtherDerived = (function (_super) {
}
return OtherDerived;
}(Base));
// S's
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,4 @@ var OtherDerived = (function (_super) {
}
return OtherDerived;
}(Base));
// S's
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,4 @@ var OtherDerived = (function (_super) {
}
return OtherDerived;
}(Base));
// S's
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,4 @@ var OtherDerived = (function (_super) {
}
return OtherDerived;
}(Base));
// S's
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ function foo11() {
return M;
}
var r11 = foo11();
// merged declarations
function foo12() {
var i2;
return i2;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ var C = (function () {
var c;
c.foo();
c.foo(1);
// these are errors
var i;
i();
i(1);
Expand Down
Loading