Skip to content

Commit

Permalink
fix: handling properly fields with leading and trailing comments afte…
Browse files Browse the repository at this point in the history 
…r field with trailing comment (#1593)

Co-authored-by: Iaroslav Kolbin <y.kolbin@team.bumble.com>
Co-authored-by: Alexander Fenster <fenster@google.com>
  • Loading branch information
@ykolbin @alexander-fenster
3 people committed May 8, 2021
1 parent 57fc524 commit 9011aac
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 21 deletions.
47 changes: 31 additions & 16 deletions src/tokenize.js
View file
Expand Up @@ -103,11 +103,8 @@ function tokenize(source, alternateCommentMode) {
var offset = 0,
length = source.length,
line = 1,
commentType = null,
commentText = null,
commentLine = 0,
commentLineEmpty = false,
commentIsLeading = false;
lastCommentLine = 0,
comments = {};

var stack = [];

Expand Down Expand Up @@ -160,10 +157,11 @@ function tokenize(source, alternateCommentMode) {
* @inner
*/
function setComment(start, end, isLeading) {
commentType = source.charAt(start++);
commentLine = line;
commentLineEmpty = false;
commentIsLeading = isLeading;
var comment = {
type: source.charAt(start++),
lineEmpty: false,
leading: isLeading,
};
var lookback;
if (alternateCommentMode) {
lookback = 2; // alternate comment parsing: "//" or "/*"
Expand All @@ -175,7 +173,7 @@ function tokenize(source, alternateCommentMode) {
do {
if (--commentOffset < 0 ||
(c = source.charAt(commentOffset)) === "\n") {
commentLineEmpty = true;
comment.lineEmpty = true;
break;
}
} while (c === " " || c === "\t");
Expand All @@ -186,9 +184,12 @@ function tokenize(source, alternateCommentMode) {
lines[i] = lines[i]
.replace(alternateCommentMode ? setCommentAltRe : setCommentRe, "")
.trim();
commentText = lines
comment.text = lines
.join("\n")
.trim();

comments[line] = comment;
lastCommentLine = line;
}

function isDoubleSlashCommentLine(startOffset) {
Expand Down Expand Up @@ -257,6 +258,9 @@ function tokenize(source, alternateCommentMode) {
++offset;
if (isDoc) {
setComment(start, offset - 1, isLeadingComment);
// Trailing comment cannot not be multi-line,
// so leading comment state should be reset to handle potential next comments
isLeadingComment = true;
}
++line;
repeat = true;
Expand All @@ -272,12 +276,17 @@ function tokenize(source, alternateCommentMode) {
break;
}
offset++;
if (!isLeadingComment) {
// Trailing comment cannot not be multi-line
break;
}
} while (isDoubleSlashCommentLine(offset));
} else {
offset = Math.min(length, findEndOfLine(offset) + 1);
}
if (isDoc) {
setComment(start, offset, isLeadingComment);
isLeadingComment = true;
}
line++;
repeat = true;
Expand All @@ -299,6 +308,7 @@ function tokenize(source, alternateCommentMode) {
++offset;
if (isDoc) {
setComment(start, offset - 2, isLeadingComment);
isLeadingComment = true;
}
repeat = true;
} else {
Expand Down Expand Up @@ -374,17 +384,22 @@ function tokenize(source, alternateCommentMode) {
*/
function cmnt(trailingLine) {
var ret = null;
var comment;
if (trailingLine === undefined) {
if (commentLine === line - 1 && (alternateCommentMode || commentType === "*" || commentLineEmpty)) {
ret = commentIsLeading ? commentText : null;
comment = comments[line - 1];
delete comments[line - 1];
if (comment && (alternateCommentMode || comment.type === "*" || comment.lineEmpty)) {
ret = comment.leading ? comment.text : null;
}
} else {
/* istanbul ignore else */
if (commentLine < trailingLine) {
if (lastCommentLine < trailingLine) {
peek();
}
if (commentLine === trailingLine && !commentLineEmpty && (alternateCommentMode || commentType === "/")) {
ret = commentIsLeading ? null : commentText;
comment = comments[trailingLine];
delete comments[trailingLine];
if (comment && !comment.lineEmpty && (alternateCommentMode || comment.type === "/")) {
ret = comment.leading ? null : comment.text;
}
}
return ret;
Expand Down
7 changes: 6 additions & 1 deletion tests/api_tokenize.js
View file
Expand Up @@ -42,6 +42,11 @@ tape.test("tokenize", function(test) {
tn.next();
test.equal(tn.cmnt(), null, "trailing comment should not be recognized as leading comment for next line");
test.equal(tn.cmnt(tn.line), "trailing comment B", "should parse trailing comment");
tn = tokenize("/// leading comment A\na /// trailing comment A\n/// leading comment B\nb /// trailing comment B\n");
tn.next();
test.equal(tn.cmnt(tn.line), "trailing comment A", "trailing comment should not contain leading comment from next line");
tn.next();
test.equal(tn.cmnt(), 'leading comment B', "leading comment should be present after trailing comment");

test.ok(expectError("something; /"), "should throw for unterminated line comments");
test.ok(expectError("something; /* comment"), "should throw for unterminated block comments");
Expand Down Expand Up @@ -78,4 +83,4 @@ function expectError(proto) {
} catch (e) {
return e;
}
}
}
3 changes: 3 additions & 0 deletions tests/data/comments-alternate-parse.proto
View file
Expand Up @@ -74,6 +74,9 @@ enum Test3 {
THREE = 3; // ignored

FOUR = 4; /// Other value with a comment.

// Leading comment for value with both types of comments after field with trailing comment.
FIVE = 5; // Trailing comment for value with both types of comments after field with trailing comment.
}

service ServiceTest {
Expand Down
3 changes: 3 additions & 0 deletions tests/data/comments.proto
View file
Expand Up @@ -48,4 +48,7 @@ enum Test3 {
THREE = 3; /// Value with a comment.

FOUR = 4; /// Other value with a comment.

/// Leading comment for value with both types of comments after field with trailing comment.
FIVE = 5; /// Trailing comment for value with both types of comments after field with trailing comment.
}
6 changes: 4 additions & 2 deletions tests/docs_comments.js
View file
Expand Up @@ -3,7 +3,7 @@ var tape = require("tape");
var protobuf = require("..");

tape.test("proto comments", function(test) {
test.plan(10);
test.plan(11);
protobuf.load("tests/data/comments.proto", function(err, root) {
if (err)
throw test.fail(err.message);
Expand All @@ -20,13 +20,14 @@ tape.test("proto comments", function(test) {
test.equal(root.lookup("Test3").comments.TWO, null, "should not parse lines for enum values");
test.equal(root.lookup("Test3").comments.THREE, "Preferred value with a comment.", "should parse lines for enum values and prefer on top over trailing");
test.equal(root.lookup("Test3").comments.FOUR, "Other value with a comment.", "should not confuse previous trailing comments with comments for the next field");
test.equal(root.lookup("Test3").comments.FIVE, "Leading comment for value with both types of comments after field with trailing comment.", "should not confuse previous field with trailing comment when leading comment is present");

test.end();
});
});

tape.test("proto comments with trailing comment preferred", function(test) {
test.plan(10);
test.plan(11);
var options = {preferTrailingComment: true};
var root = new protobuf.Root();
root.load("tests/data/comments.proto", options, function(err, root) {
Expand All @@ -45,6 +46,7 @@ tape.test("proto comments with trailing comment preferred", function(test) {
test.equal(root.lookup("Test3").comments.TWO, null, "should not parse lines for enum values");
test.equal(root.lookup("Test3").comments.THREE, "Value with a comment.", "should prefer trailing comment when preferTrailingComment option enabled");
test.equal(root.lookup("Test3").comments.FOUR, "Other value with a comment.", "should not confuse previous trailing comments with comments for the next field");
test.equal(root.lookup("Test3").comments.FIVE, "Trailing comment for value with both types of comments after field with trailing comment.", "should not confuse previous field with trailing comment when leading comment is present");

test.end();
});
Expand Down
6 changes: 4 additions & 2 deletions tests/docs_comments_alternate_parse.js
View file
Expand Up @@ -3,7 +3,7 @@ var tape = require("tape");
var protobuf = require("..");

tape.test("proto comments in alternate-parse mode", function(test) {
test.plan(23);
test.plan(24);
var options = {alternateCommentMode: true};
var root = new protobuf.Root();
root.load("tests/data/comments-alternate-parse.proto", options, function(err, root) {
Expand Down Expand Up @@ -31,6 +31,7 @@ tape.test("proto comments in alternate-parse mode", function(test) {
test.equal(root.lookup("Test3").comments.TWO, "Value with a single-line comment.", "should parse double-slash comments for enum values");
test.equal(root.lookup("Test3").comments.THREE, "Value with a triple-slash comment.", "should parse lines for enum values and prefer on top over trailing");
test.equal(root.lookup("Test3").comments.FOUR, "Other value with a comment.", "should not confuse previous trailing comments with comments for the next field");
test.equal(root.lookup("Test3").comments.FIVE, "Leading comment for value with both types of comments after field with trailing comment.", "should not confuse previous field with trailing comment when leading comment is present");

test.equal(root.lookup("ServiceTest.SingleLineMethod").comment, 'My method does things');
test.equal(root.lookup("ServiceTest.TwoLineMethodWithComment").comment, 'TwoLineMethodWithComment documentation');
Expand All @@ -42,7 +43,7 @@ tape.test("proto comments in alternate-parse mode", function(test) {
});

tape.test("proto comments in alternate-parse mode with trailing comment preferred", function(test) {
test.plan(23);
test.plan(24);
var options = {alternateCommentMode: true, preferTrailingComment: true};
var root = new protobuf.Root();
root.load("tests/data/comments-alternate-parse.proto", options, function(err, root) {
Expand Down Expand Up @@ -70,6 +71,7 @@ tape.test("proto comments in alternate-parse mode with trailing comment preferre
test.equal(root.lookup("Test3").comments.TWO, "Value with a single-line comment.", "should parse double-slash comments for enum values");
test.equal(root.lookup("Test3").comments.THREE, "ignored", "should prefer trailing comment when preferTrailingComment option enabled");
test.equal(root.lookup("Test3").comments.FOUR, "Other value with a comment.", "should not confuse previous trailing comments with comments for the next field");
test.equal(root.lookup("Test3").comments.FIVE, "Trailing comment for value with both types of comments after field with trailing comment.", "should not confuse previous field with trailing comment when leading comment is present");

test.equal(root.lookup("ServiceTest.SingleLineMethod").comment, 'My method does things');
test.equal(root.lookup("ServiceTest.TwoLineMethodWithComment").comment, 'TwoLineMethodWithComment documentation');
Expand Down

0 comments on commit 9011aac

Please sign in to comment.