feat: better comment parse (#1419)

* fix trailing comment in last line recognized as leading comment for current token.

* add preferTrailingComment to IParseOptions

Co-authored-by: Alexander Fenster <fenster@google.com>

index.d.ts

@@ -1046,6 +1046,9 @@ export interface IParseOptions {
 
     /** Recognize double-slash comments in addition to doc-block comments. */
     alternateCommentMode?: boolean;
+
+    /** Use trailing comment when both leading comment and trailing comment exist. */
+    preferTrailingComment?: boolean;
 }
 
 /** Options modifying the behavior of JSON serialization. */

src/parse.js

@@ -42,6 +42,7 @@ var base10Re    = /^[1-9][0-9]*$/,
  * @interface IParseOptions
  * @property {boolean} [keepCase=false] Keeps field casing instead of converting to camel case
  * @property {boolean} [alternateCommentMode=false] Recognize double-slash comments in addition to doc-block comments.
+ * @property {boolean} [preferTrailingComment=false] Use trailing comment when both leading comment and trailing comment exist.
  */
 
 /**
@@ -68,6 +69,7 @@ function parse(source, root, options) {
     if (!options)
         options = parse.defaults;
 
+    var preferTrailingComment = options.preferTrailingComment || false;
     var tn = tokenize(source, options.alternateCommentMode || false),
         next = tn.next,
         push = tn.push,
@@ -291,8 +293,8 @@ function parse(source, root, options) {
             if (fnElse)
                 fnElse();
             skip(";");
-            if (obj && typeof obj.comment !== "string")
-                obj.comment = cmnt(trailingLine); // try line-type comment if no block
+            if (obj && (typeof obj.comment !== "string" || preferTrailingComment))
+                obj.comment = cmnt(trailingLine) || obj.comment; // try line-type comment
         }
     }
 

src/tokenize.js

@@ -106,7 +106,8 @@ function tokenize(source, alternateCommentMode) {
         commentType = null,
         commentText = null,
         commentLine = 0,
-        commentLineEmpty = false;
+        commentLineEmpty = false,
+        commentIsLeading = false;
 
     var stack = [];
 
@@ -154,13 +155,15 @@ function tokenize(source, alternateCommentMode) {
      * Sets the current comment text.
      * @param {number} start Start offset
      * @param {number} end End offset
+     * @param {boolean} isLeading set if a leading comment
      * @returns {undefined}
      * @inner
      */
-    function setComment(start, end) {
+    function setComment(start, end, isLeading) {
         commentType = source.charAt(start++);
         commentLine = line;
         commentLineEmpty = false;
+        commentIsLeading = isLeading;
         var lookback;
         if (alternateCommentMode) {
             lookback = 2;  // alternate comment parsing: "//" or "/*"
@@ -222,14 +225,17 @@ function tokenize(source, alternateCommentMode) {
             prev,
             curr,
             start,
-            isDoc;
+            isDoc,
+            isLeadingComment = offset === 0;
         do {
             if (offset === length)
                 return null;
             repeat = false;
             while (whitespaceRe.test(curr = charAt(offset))) {
-                if (curr === "\n")
+                if (curr === "\n") {
+                    isLeadingComment = true;
                     ++line;
+                }
                 if (++offset === length)
                     return null;
             }
@@ -250,7 +256,7 @@ function tokenize(source, alternateCommentMode) {
                         }
                         ++offset;
                         if (isDoc) {
-                            setComment(start, offset - 1);
+                            setComment(start, offset - 1, isLeadingComment);
                         }
                         ++line;
                         repeat = true;
@@ -271,7 +277,7 @@ function tokenize(source, alternateCommentMode) {
                             offset = Math.min(length, findEndOfLine(offset) + 1);
                         }
                         if (isDoc) {
-                            setComment(start, offset);
+                            setComment(start, offset, isLeadingComment);
                         }
                         line++;
                         repeat = true;
@@ -292,7 +298,7 @@ function tokenize(source, alternateCommentMode) {
                     } while (prev !== "*" || curr !== "/");
                     ++offset;
                     if (isDoc) {
-                        setComment(start, offset - 2);
+                        setComment(start, offset - 2, isLeadingComment);
                     }
                     repeat = true;
                 } else {
@@ -370,15 +376,15 @@ function tokenize(source, alternateCommentMode) {
         var ret = null;
         if (trailingLine === undefined) {
             if (commentLine === line - 1 && (alternateCommentMode || commentType === "*" || commentLineEmpty)) {
-                ret = commentText;
+                ret = commentIsLeading ? commentText : null;
             }
         } else {
             /* istanbul ignore else */
             if (commentLine < trailingLine) {
                 peek();
             }
             if (commentLine === trailingLine && !commentLineEmpty && (alternateCommentMode || commentType === "/")) {
-                ret = commentText;
+                ret = commentIsLeading ? null : commentText;
             }
         }
         return ret;

tests/api_tokenize.js

@@ -35,6 +35,13 @@ tape.test("tokenize", function(test) {
     tn = tokenize("/// line comment\na\n");
     tn.next();
     test.equal(tn.cmnt(), "line comment", "should keep leading comments around while on the next line");
+    tn = tokenize("/// leading comment A\na /// trailing comment A\nb /// trailing comment B\n");
+    tn.next();
+    test.equal(tn.cmnt(), "leading comment A", "should parse leading comment");
+    test.equal(tn.cmnt(tn.line), "trailing comment A", "should parse trailing comment");
+    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");
 
     test.ok(expectError("something; /"), "should throw for unterminated line comments");
     test.ok(expectError("something; /* comment"), "should throw for unterminated block comments");

tests/data/comments-alternate-parse.proto

@@ -42,6 +42,10 @@ message Test1 {
      * multi-line doc-block comment.
      */
     string field10 = 10;
+
+    // Field with both block comment
+    string field11 = 11; // and trailing comment.
+    string field12 = 12; // Trailing comment in last line should not be recognized as leading comment for this field.
 }
 
 /* Message

tests/docs_comments.js

@@ -24,3 +24,28 @@ tape.test("proto comments", function(test) {
         test.end();
     });
 });
+
+tape.test("proto comments with trailing comment preferred", function(test) {
+    test.plan(10);
+    var options = {preferTrailingComment: true};
+    var root = new protobuf.Root();
+    root.load("tests/data/comments.proto", options, function(err, root) {
+        if (err)
+            throw test.fail(err.message);
+
+        test.equal(root.lookup("Test1").comment, "Message\nwith\na\ncomment.", "should parse /**-blocks");
+        test.equal(root.lookup("Test2").comment, null, "should not parse //-blocks");
+        test.equal(root.lookup("Test3").comment, null, "should not parse /*-blocks");
+
+        test.equal(root.lookup("Test1.field1").comment, "Field with a comment.", "should parse blocks for message fields");
+        test.equal(root.lookup("Test1.field2").comment, null, "should not parse lines for message fields");
+        test.equal(root.lookup("Test1.field3").comment, "Field with a comment and a <a href=\"http://example.com/foo/\">link</a>", "should parse triple-slash lines for message fields");
+
+        test.equal(root.lookup("Test3").comments.ONE, "Value with a comment.", "should parse blocks for enum values");
+        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.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(21);
+    test.plan(23);
     var options = {alternateCommentMode: true};
     var root = new protobuf.Root();
     root.load("tests/data/comments-alternate-parse.proto", options, function(err, root) {
@@ -24,6 +24,8 @@ tape.test("proto comments in alternate-parse mode", function(test) {
         test.equal(root.lookup("Test1.field8").comment, null, "should parse no comment");
         test.equal(root.lookup("Test1.field9").comment, "Field with a\nmulti-line comment.", "should parse multiline double-slash field comment");
         test.equal(root.lookup("Test1.field10").comment, "Field with a\nmulti-line doc-block comment.", "should parse multiline doc-block field comment");
+        test.equal(root.lookup("Test1.field11").comment, "Field with both block comment", "should parse both trailing comment and trailing comment");
+        test.equal(root.lookup("Test1.field12").comment, "Trailing comment in last line should not be recognized as leading comment for this field.", "trailing comment in last line should not be recognized as leading comment for this field");
 
         test.equal(root.lookup("Test3").comments.ONE, "Value with a comment.", "should parse blocks for enum values");
         test.equal(root.lookup("Test3").comments.TWO, "Value with a single-line comment.", "should parse double-slash comments for enum values");
@@ -38,3 +40,42 @@ tape.test("proto comments in alternate-parse mode", function(test) {
         test.end();
     });
 });
+
+tape.test("proto comments in alternate-parse mode with trailing comment preferred", function(test) {
+    test.plan(23);
+    var options = {alternateCommentMode: true, preferTrailingComment: true};
+    var root = new protobuf.Root();
+    root.load("tests/data/comments-alternate-parse.proto", options, function(err, root) {
+        if (err)
+            throw test.fail(err.message);
+
+        test.equal(root.lookup("Test1").comment, "Message with\na\nmulti-line comment.", "should parse double-slash multiline comment");
+        test.equal(root.lookup("Test2").comment, "Message\nwith\na multiline plain slash-star\ncomment.", "should parse slash-star multiline comment");
+        test.equal(root.lookup("Test3").comment, "Message\nwith\na\ncomment and stars.", "should parse doc-block multiline comment");
+
+        test.equal(root.lookup("Test1.field1").comment, "Field with a doc-block comment.", "should parse doc-block field comment");
+        test.equal(root.lookup("Test1.field2").comment, "Field with a single-line comment starting with two slashes.", "should parse double-slash field comment");
+        test.equal(root.lookup("Test1.field3").comment, "Field with a single-line comment starting with three slashes.", "should parse triple-slash field comment");
+        test.equal(root.lookup("Test1.field4").comment, "Field with a single-line slash-star comment.", "should parse single-line slash-star field comment");
+        test.equal(root.lookup("Test1.field5").comment, "Field with a trailing single-line two-slash comment.", "should parse trailing double-slash comment");
+        test.equal(root.lookup("Test1.field6").comment, "Field with a trailing single-line three-slash comment.", "should parse trailing triple-slash comment");
+        test.equal(root.lookup("Test1.field7").comment, "Field with a trailing single-line slash-star comment.", "should parse trailing slash-star comment");
+        test.equal(root.lookup("Test1.field8").comment, null, "should parse no comment");
+        test.equal(root.lookup("Test1.field9").comment, "Field with a\nmulti-line comment.", "should parse multiline double-slash field comment");
+        test.equal(root.lookup("Test1.field10").comment, "Field with a\nmulti-line doc-block comment.", "should parse multiline doc-block field comment");
+        test.equal(root.lookup("Test1.field11").comment, "and trailing comment.", "should parse both trailing comment and trailing comment");
+        test.equal(root.lookup("Test1.field12").comment, "Trailing comment in last line should not be recognized as leading comment for this field.", "trailing comment in last line should not be recognized as leading comment for this field");
+
+        test.equal(root.lookup("Test3").comments.ONE, "Value with a comment.", "should parse blocks for enum values");
+        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("ServiceTest.SingleLineMethod").comment, 'My method does things');
+        test.equal(root.lookup("ServiceTest.TwoLineMethodWithComment").comment, 'TwoLineMethodWithComment documentation');
+        test.equal(root.lookup("ServiceTest.ThreeLine012345678901234567890123456712345671234567123456783927483923473892837489238749832432874983274983274983274").comment, 'Very very long method');
+        test.equal(root.lookup("ServiceTest.TwoLineMethodNoComment").comment, null);
+
+        test.end();
+    });
+});