fix: handling properly fields with leading and trailing comments after field with trailing comment (#1593)

Co-authored-by: Iaroslav Kolbin <[email protected]>
Co-authored-by: Alexander Fenster <[email protected]>

Author: 3 people

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;

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;
     }
-}
+}

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 {

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.
 }

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();
     });

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');