From 9011aac161e1bf8eaa3cbf0f17e8f321b6e0d9c4 Mon Sep 17 00:00:00 2001 From: Iaroslav Kolbin Date: Sat, 8 May 2021 01:04:51 +0100 Subject: [PATCH] fix: handling properly fields with leading and trailing comments after field with trailing comment (#1593) Co-authored-by: Iaroslav Kolbin Co-authored-by: Alexander Fenster --- src/tokenize.js | 47 +++++++++++++++-------- tests/api_tokenize.js | 7 +++- tests/data/comments-alternate-parse.proto | 3 ++ tests/data/comments.proto | 3 ++ tests/docs_comments.js | 6 ++- tests/docs_comments_alternate_parse.js | 6 ++- 6 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/tokenize.js b/src/tokenize.js index b55b29273..bfb784bd2 100644 --- a/src/tokenize.js +++ b/src/tokenize.js @@ -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 = []; @@ -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 "/*" @@ -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"); @@ -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) { @@ -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; @@ -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; @@ -299,6 +308,7 @@ function tokenize(source, alternateCommentMode) { ++offset; if (isDoc) { setComment(start, offset - 2, isLeadingComment); + isLeadingComment = true; } repeat = true; } else { @@ -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; diff --git a/tests/api_tokenize.js b/tests/api_tokenize.js index 4ee556ae8..7f408fb54 100644 --- a/tests/api_tokenize.js +++ b/tests/api_tokenize.js @@ -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"); @@ -78,4 +83,4 @@ function expectError(proto) { } catch (e) { return e; } -} \ No newline at end of file +} diff --git a/tests/data/comments-alternate-parse.proto b/tests/data/comments-alternate-parse.proto index 409851aa4..8ec992450 100644 --- a/tests/data/comments-alternate-parse.proto +++ b/tests/data/comments-alternate-parse.proto @@ -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 { diff --git a/tests/data/comments.proto b/tests/data/comments.proto index 7f1032502..700929761 100644 --- a/tests/data/comments.proto +++ b/tests/data/comments.proto @@ -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. } diff --git a/tests/docs_comments.js b/tests/docs_comments.js index c4c33c387..4f7702cf5 100644 --- a/tests/docs_comments.js +++ b/tests/docs_comments.js @@ -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); @@ -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) { @@ -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(); }); diff --git a/tests/docs_comments_alternate_parse.js b/tests/docs_comments_alternate_parse.js index 8b1254e47..01c2a2452 100644 --- a/tests/docs_comments_alternate_parse.js +++ b/tests/docs_comments_alternate_parse.js @@ -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) { @@ -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'); @@ -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) { @@ -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');