Skip to content

Commit f92aa86

Browse files
yuityuit
committed
Merge pull request #4230 from Microsoft/commentsNotPreserveForCallExp
Preserve comments on parameters in call expressions
2 parents a32bac2 + 6adf7fe commit f92aa86

23 files changed

+439
-30
lines changed

‎src/compiler/emitter.ts

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,14 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
832832
write(", ");
833833
}
834834
}
835-
emitNode(nodes[start + i]);
835+
let node = nodes[start + i];
836+
// This emitting is to make sure we emit following comment properly
837+
// ...(x, /*comment1*/ y)...
838+
// ^ => node.pos
839+
// "comment1" is not considered leading comment for "y" but rather
840+
// considered as trailing comment of the previous node.
841+
emitTrailingCommentsOfPosition(node.pos);
842+
emitNode(node);
836843
leadingComma = true;
837844
}
838845
if (trailingComma) {
@@ -1976,6 +1983,14 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
19761983
function emitPropertyAssignment(node: PropertyDeclaration) {
19771984
emit(node.name);
19781985
write(": ");
1986+
// This is to ensure that we emit comment in the following case:
1987+
// For example:
1988+
// obj = {
1989+
// id: /*comment1*/ ()=>void
1990+
// }
1991+
// "comment1" is not considered to be leading comment for node.initializer
1992+
// but rather a trailing comment on the previous node.
1993+
emitTrailingCommentsOfPosition(node.initializer.pos);
19791994
emit(node.initializer);
19801995
}
19811996

@@ -3639,8 +3654,24 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
36393654
return emitOnlyPinnedOrTripleSlashComments(node);
36403655
}
36413656

3642-
if (node.kind !== SyntaxKind.MethodDeclaration && node.kind !== SyntaxKind.MethodSignature) {
3643-
// Methods will emit the comments as part of emitting method declaration
3657+
// TODO (yuisu) : we should not have special cases to condition emitting comments
3658+
// but have one place to fix check for these conditions.
3659+
if (node.kind !== SyntaxKind.MethodDeclaration && node.kind !== SyntaxKind.MethodSignature &&
3660+
node.parent && node.parent.kind !== SyntaxKind.PropertyAssignment &&
3661+
node.parent.kind !== SyntaxKind.CallExpression) {
3662+
// 1. Methods will emit the comments as part of emitting method declaration
3663+
3664+
// 2. If the function is a property of object literal, emitting leading-comments
3665+
// is done by emitNodeWithoutSourceMap which then call this function.
3666+
// In particular, we would like to avoid emit comments twice in following case:
3667+
// For example:
3668+
// var obj = {
3669+
// id:
3670+
// /*comment*/ () => void
3671+
// }
3672+
3673+
// 3. If the function is an argument in call expression, emitting of comments will be
3674+
// taken care of in emit list of arguments inside of emitCallexpression
36443675
emitLeadingComments(node);
36453676
}
36463677

@@ -6961,6 +6992,18 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
69616992
emitComments(currentSourceFile, writer, trailingComments, /*trailingSeparator*/ false, newLine, writeComment);
69626993
}
69636994

6995+
/**
6996+
* Emit trailing comments at the position. The term trailing comment is used here to describe following comment:
6997+
* x, /comment1/ y
6998+
* ^ => pos; the function will emit "comment1" in the emitJS
6999+
*/
7000+
function emitTrailingCommentsOfPosition(pos: number) {
7001+
let trailingComments = filterComments(getTrailingCommentRanges(currentSourceFile.text, pos), /*onlyPinnedOrTripleSlashComments:*/ compilerOptions.removeComments);
7002+
7003+
// trailing comments are emitted at space/*trailing comment1 */space/*trailing comment*/
7004+
emitComments(currentSourceFile, writer, trailingComments, /*trailingSeparator*/ true, newLine, writeComment);
7005+
}
7006+
69647007
function emitLeadingCommentsOfPosition(pos: number) {
69657008
let leadingComments: CommentRange[];
69667009
if (hasDetachedComments(pos)) {

‎src/compiler/utilities.ts

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -381,24 +381,12 @@ namespace ts {
381381
}
382382

383383
export function getLeadingCommentRangesOfNode(node: Node, sourceFileOfNode: SourceFile) {
384-
// If parameter/type parameter, the prev token trailing comments are part of this node too
385-
if (node.kind === SyntaxKind.Parameter || node.kind === SyntaxKind.TypeParameter) {
386-
// e.g. (/** blah */ a, /** blah */ b);
387-
388-
// e.g.: (
389-
// /** blah */ a,
390-
// /** blah */ b);
391-
return concatenate(
392-
getTrailingCommentRanges(sourceFileOfNode.text, node.pos),
393-
getLeadingCommentRanges(sourceFileOfNode.text, node.pos));
394-
}
395-
else {
396-
return getLeadingCommentRanges(sourceFileOfNode.text, node.pos);
397-
}
384+
return getLeadingCommentRanges(sourceFileOfNode.text, node.pos);
398385
}
399386

400387
export function getJsDocComments(node: Node, sourceFileOfNode: SourceFile) {
401-
return filter(getLeadingCommentRangesOfNode(node, sourceFileOfNode), isJsDocComment);
388+
let commentRanges = (node.kind === SyntaxKind.Parameter || node.kind === SyntaxKind.TypeParameter) ? concatenate(getTrailingCommentRanges(sourceFileOfNode.text, node.pos), getLeadingCommentRanges(sourceFileOfNode.text, node.pos)) : getLeadingCommentRangesOfNode(node, sourceFileOfNode);
389+
return filter(commentRanges, isJsDocComment);
402390

403391
function isJsDocComment(comment: CommentRange) {
404392
// True if the comment starts with '/**' but not if it is '/**/'

‎tests/baselines/reference/APISample_linter.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ exports.delint = delint;
112112
var fileNames = process.argv.slice(2);
113113
fileNames.forEach(function (fileName) {
114114
// Parse a file
115-
var sourceFile = ts.createSourceFile(fileName, readFileSync(fileName).toString(), 2 /* ES6 */, true);
115+
var sourceFile = ts.createSourceFile(fileName, readFileSync(fileName).toString(), 2 /* ES6 */, /*setParentNodes */ true);
116116
// delint it
117117
delint(sourceFile);
118118
});

‎tests/baselines/reference/commentInMethodCall.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,5 @@ s.map(// do something
88
//// [commentInMethodCall.js]
99
//commment here
1010
var s;
11-
s.map(function () { });
11+
s.map(// do something
12+
function () { });

‎tests/baselines/reference/commentsArgumentsOfCallExpression1.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
//// [commentsArgumentsOfCallExpression1.ts]
2+
function foo(/*c1*/ x: any) { }
3+
foo(/*c2*/ 1);
4+
foo(/*c3*/ function () { });
5+
foo(
6+
/*c4*/
7+
() => { });
8+
foo(
9+
/*c5*/
10+
/*c6*/
11+
() => { });
12+
foo(/*c7*/
13+
() => { });
14+
foo(
15+
/*c7*/
16+
/*c8*/() => { });
17+
18+
//// [commentsArgumentsOfCallExpression1.js]
19+
function foo(/*c1*/ x) { }
20+
foo(/*c2*/ 1);
21+
foo(/*c3*/ function () { });
22+
foo(
23+
/*c4*/
24+
function () { });
25+
foo(
26+
/*c5*/
27+
/*c6*/
28+
function () { });
29+
foo(/*c7*/ function () { });
30+
foo(
31+
/*c7*/
32+
/*c8*/ function () { });

‎tests/baselines/reference/commentsArgumentsOfCallExpression1.symbols

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
=== tests/cases/compiler/commentsArgumentsOfCallExpression1.ts ===
2+
function foo(/*c1*/ x: any) { }
3+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
4+
>x : Symbol(x, Decl(commentsArgumentsOfCallExpression1.ts, 0, 13))
5+
6+
foo(/*c2*/ 1);
7+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
8+
9+
foo(/*c3*/ function () { });
10+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
11+
12+
foo(
13+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
14+
15+
/*c4*/
16+
() => { });
17+
foo(
18+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
19+
20+
/*c5*/
21+
/*c6*/
22+
() => { });
23+
foo(/*c7*/
24+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
25+
26+
() => { });
27+
foo(
28+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression1.ts, 0, 0))
29+
30+
/*c7*/
31+
/*c8*/() => { });

‎tests/baselines/reference/commentsArgumentsOfCallExpression1.types

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
=== tests/cases/compiler/commentsArgumentsOfCallExpression1.ts ===
2+
function foo(/*c1*/ x: any) { }
3+
>foo : (x: any) => void
4+
>x : any
5+
6+
foo(/*c2*/ 1);
7+
>foo(/*c2*/ 1) : void
8+
>foo : (x: any) => void
9+
>1 : number
10+
11+
foo(/*c3*/ function () { });
12+
>foo(/*c3*/ function () { }) : void
13+
>foo : (x: any) => void
14+
>function () { } : () => void
15+
16+
foo(
17+
>foo( /*c4*/ () => { }) : void
18+
>foo : (x: any) => void
19+
20+
/*c4*/
21+
() => { });
22+
>() => { } : () => void
23+
24+
foo(
25+
>foo( /*c5*/ /*c6*/ () => { }) : void
26+
>foo : (x: any) => void
27+
28+
/*c5*/
29+
/*c6*/
30+
() => { });
31+
>() => { } : () => void
32+
33+
foo(/*c7*/
34+
>foo(/*c7*/ () => { }) : void
35+
>foo : (x: any) => void
36+
37+
() => { });
38+
>() => { } : () => void
39+
40+
foo(
41+
>foo( /*c7*/ /*c8*/() => { }) : void
42+
>foo : (x: any) => void
43+
44+
/*c7*/
45+
/*c8*/() => { });
46+
>() => { } : () => void
47+

‎tests/baselines/reference/commentsArgumentsOfCallExpression2.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//// [commentsArgumentsOfCallExpression2.ts]
2+
function foo(/*c1*/ x: any, /*d1*/ y: any,/*e1*/w?: any) { }
3+
var a, b: any;
4+
foo(/*c2*/ 1, /*d2*/ 1 + 2, /*e1*/ a + b);
5+
foo(/*c3*/ function () { }, /*d2*/() => { }, /*e2*/ a + /*e3*/ b);
6+
foo(/*c3*/ function () { }, /*d3*/() => { }, /*e3*/(a + b));
7+
foo(
8+
/*c4*/ function () { },
9+
/*d4*/() => { },
10+
/*e4*/
11+
/*e5*/ "hello");
12+
13+
//// [commentsArgumentsOfCallExpression2.js]
14+
function foo(/*c1*/ x, /*d1*/ y, /*e1*/ w) { }
15+
var a, b;
16+
foo(/*c2*/ 1, /*d2*/ 1 + 2, /*e1*/ a + b);
17+
foo(/*c3*/ function () { }, /*d2*/ function () { }, /*e2*/ a + b);
18+
foo(/*c3*/ function () { }, /*d3*/ function () { }, /*e3*/ (a + b));
19+
foo(
20+
/*c4*/ function () { },
21+
/*d4*/ function () { },
22+
/*e4*/
23+
/*e5*/ "hello");

‎tests/baselines/reference/commentsArgumentsOfCallExpression2.symbols

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
=== tests/cases/compiler/commentsArgumentsOfCallExpression2.ts ===
2+
function foo(/*c1*/ x: any, /*d1*/ y: any,/*e1*/w?: any) { }
3+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression2.ts, 0, 0))
4+
>x : Symbol(x, Decl(commentsArgumentsOfCallExpression2.ts, 0, 13))
5+
>y : Symbol(y, Decl(commentsArgumentsOfCallExpression2.ts, 0, 27))
6+
>w : Symbol(w, Decl(commentsArgumentsOfCallExpression2.ts, 0, 42))
7+
8+
var a, b: any;
9+
>a : Symbol(a, Decl(commentsArgumentsOfCallExpression2.ts, 1, 3))
10+
>b : Symbol(b, Decl(commentsArgumentsOfCallExpression2.ts, 1, 6))
11+
12+
foo(/*c2*/ 1, /*d2*/ 1 + 2, /*e1*/ a + b);
13+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression2.ts, 0, 0))
14+
>a : Symbol(a, Decl(commentsArgumentsOfCallExpression2.ts, 1, 3))
15+
>b : Symbol(b, Decl(commentsArgumentsOfCallExpression2.ts, 1, 6))
16+
17+
foo(/*c3*/ function () { }, /*d2*/() => { }, /*e2*/ a + /*e3*/ b);
18+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression2.ts, 0, 0))
19+
>a : Symbol(a, Decl(commentsArgumentsOfCallExpression2.ts, 1, 3))
20+
>b : Symbol(b, Decl(commentsArgumentsOfCallExpression2.ts, 1, 6))
21+
22+
foo(/*c3*/ function () { }, /*d3*/() => { }, /*e3*/(a + b));
23+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression2.ts, 0, 0))
24+
>a : Symbol(a, Decl(commentsArgumentsOfCallExpression2.ts, 1, 3))
25+
>b : Symbol(b, Decl(commentsArgumentsOfCallExpression2.ts, 1, 6))
26+
27+
foo(
28+
>foo : Symbol(foo, Decl(commentsArgumentsOfCallExpression2.ts, 0, 0))
29+
30+
/*c4*/ function () { },
31+
/*d4*/() => { },
32+
/*e4*/
33+
/*e5*/ "hello");

‎tests/baselines/reference/commentsArgumentsOfCallExpression2.types

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
=== tests/cases/compiler/commentsArgumentsOfCallExpression2.ts ===
2+
function foo(/*c1*/ x: any, /*d1*/ y: any,/*e1*/w?: any) { }
3+
>foo : (x: any, y: any, w?: any) => void
4+
>x : any
5+
>y : any
6+
>w : any
7+
8+
var a, b: any;
9+
>a : any
10+
>b : any
11+
12+
foo(/*c2*/ 1, /*d2*/ 1 + 2, /*e1*/ a + b);
13+
>foo(/*c2*/ 1, /*d2*/ 1 + 2, /*e1*/ a + b) : void
14+
>foo : (x: any, y: any, w?: any) => void
15+
>1 : number
16+
>1 + 2 : number
17+
>1 : number
18+
>2 : number
19+
>a + b : any
20+
>a : any
21+
>b : any
22+
23+
foo(/*c3*/ function () { }, /*d2*/() => { }, /*e2*/ a + /*e3*/ b);
24+
>foo(/*c3*/ function () { }, /*d2*/() => { }, /*e2*/ a + /*e3*/ b) : void
25+
>foo : (x: any, y: any, w?: any) => void
26+
>function () { } : () => void
27+
>() => { } : () => void
28+
>a + /*e3*/ b : any
29+
>a : any
30+
>b : any
31+
32+
foo(/*c3*/ function () { }, /*d3*/() => { }, /*e3*/(a + b));
33+
>foo(/*c3*/ function () { }, /*d3*/() => { }, /*e3*/(a + b)) : void
34+
>foo : (x: any, y: any, w?: any) => void
35+
>function () { } : () => void
36+
>() => { } : () => void
37+
>(a + b) : any
38+
>a + b : any
39+
>a : any
40+
>b : any
41+
42+
foo(
43+
>foo( /*c4*/ function () { }, /*d4*/() => { }, /*e4*/ /*e5*/ "hello") : void
44+
>foo : (x: any, y: any, w?: any) => void
45+
46+
/*c4*/ function () { },
47+
>function () { } : () => void
48+
49+
/*d4*/() => { },
50+
>() => { } : () => void
51+
52+
/*e4*/
53+
/*e5*/ "hello");
54+
>"hello" : string
55+

‎tests/baselines/reference/commentsBeforeFunctionExpression1.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,5 +6,5 @@ var v = {
66

77
//// [commentsBeforeFunctionExpression1.js]
88
var v = {
9-
f: function (a) { return 0; }
9+
f: /**own f*/ function (a) { return 0; }
1010
};

‎tests/baselines/reference/commentsInterface.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ var i2_i_nc_fnfoo = i2_i.nc_fnfoo;
8989
var i2_i_nc_fnfoo_r = i2_i.nc_fnfoo(10);
9090
var i3_i;
9191
i3_i = {
92-
f: function (/**i3_i a*/ a) { return "Hello" + a; },
92+
f: /**own f*/ function (/**i3_i a*/ a) { return "Hello" + a; },
9393
l: this.f,
9494
/** own x*/
9595
x: this.f(10),

‎tests/baselines/reference/commentsOnPropertyOfObjectLiteral1.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
//// [commentsOnPropertyOfObjectLiteral1.ts]
2+
var resolve = {
3+
id: /*! @ngInject */ (details: any) => details.id,
4+
id1: /* c1 */ "hello",
5+
id2:
6+
/*! @ngInject */ (details: any) => details.id,
7+
id3:
8+
/*! @ngInject */
9+
(details: any) => details.id,
10+
id4:
11+
/*! @ngInject */
12+
/* C2 */
13+
(details: any) => details.id,
14+
};
15+
16+
//// [commentsOnPropertyOfObjectLiteral1.js]
17+
var resolve = {
18+
id: /*! @ngInject */ function (details) { return details.id; },
19+
id1: /* c1 */ "hello",
20+
id2:
21+
/*! @ngInject */ function (details) { return details.id; },
22+
id3:
23+
/*! @ngInject */
24+
function (details) { return details.id; },
25+
id4:
26+
/*! @ngInject */
27+
/* C2 */
28+
function (details) { return details.id; }
29+
};

0 commit comments

Comments
 (0)