Skip to content
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.

Sign up for GitHub

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

fix: invalid processing of leading and trailing comments #1593

Merged
merged 3 commits into from
May 8, 2021
Merged

fix: invalid processing of leading and trailing comments #1593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3cb8a8
fix: handling properly fields with leading and trailing comments afte…
Apr 20, 2021
a8bfb2a
Merge branch 'master' into fix_trailing_comments_parsing
alexander-fenster Apr 29, 2021
a600478
Merge branch 'master' into fix_trailing_comments_parsing
alexander-fenster May 7, 2021
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
47 changes: 31 additions & 16 deletions src/tokenize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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