Keep the extracted text one line per source line (#1135)

Author: Eljees

internal/lint/code/comments.go

@@ -3,7 +3,6 @@ package code
 import (
 	"bytes"
 	"context"
-	"fmt"
 	"sort"
 	"strings"
 
@@ -50,20 +49,37 @@ func doneMerging(curr, prev Comment) bool {
 	return false
 }
 
-func addSourceLine(line string, atEnd bool) string {
-	if line == "" {
-		return "\n\n"
-	}
+// appendLine adds one line to a pending run of line comments.
+//
+// One newline per line, so the run ends up with exactly as many lines as the
+// source it came from. A blank line comment -- `//` with nothing after it --
+// is empty and contributes only its newline; giving it two ended that line
+// twice and put a line in the extracted text that the source doesn't have.
+// See #1022.
+func appendLine(line string) string {
+	// The space after the delimiter belongs to the delimiter, and the padding
+	// added when an alert is mapped back already counts it. Trimming only the
+	// lines that still needed a newline left it on the ones that didn't --
+	// Rust's `///`, whose node content carries its own -- where it was then
+	// counted twice. The first line of a run is trimmed by the caller.
+	line = strings.TrimLeft(line, " ")
+	return strings.TrimRight(line, "\n") + "\n"
+}
 
-	if !strings.HasPrefix(line, "\n") && !atEnd {
-		line = strings.TrimLeft(line, " ")
-		line = fmt.Sprintf("\n%s", line)
-	} else if !strings.HasSuffix(line, "\n") && atEnd {
-		line = strings.TrimLeft(line, " ")
-		line = fmt.Sprintf("%s\n", line)
+// attachRun joins a pending run of lines to the text it belongs to.
+//
+// The run needs the line above it terminated, which is a question about that
+// text and not about the run: a comment whose node content already carried its
+// newline -- Rust's `//!`, for one -- is terminated, and adding another puts a
+// blank line between the two that the source doesn't have. Keying this off the
+// run instead lost the terminator whenever the run began with a blank comment
+// line, which cost a line in the other direction.
+func attachRun(text, run string) string {
+	if !strings.HasSuffix(text, "\n") {
+		text += "\n"
 	}
 
-	return line
+	return text + run
 }
 
 func coalesce(comments []Comment) []Comment {
@@ -78,8 +94,8 @@ func coalesce(comments []Comment) []Comment {
 		if tBuf.Len() > 0 {
 			last := joined[len(joined)-1]
 
-			last.Text += addSourceLine(tBuf.String(), false)
-			last.Source += addSourceLine(sBuf.String(), false)
+			last.Text = attachRun(last.Text, tBuf.String())
+			last.Source = attachRun(last.Source, sBuf.String())
 
 			joined[len(joined)-1] = last
 
@@ -100,8 +116,8 @@ func coalesce(comments []Comment) []Comment {
 			flush()
 			joined = append(joined, comment)
 		} else {
-			tBuf.WriteString(addSourceLine(comment.Text, true))
-			sBuf.WriteString(addSourceLine(comment.Source, true))
+			tBuf.WriteString(appendLine(comment.Text))
+			sBuf.WriteString(appendLine(comment.Source))
 		}
 	}
 

internal/lint/code/comments_test.go

@@ -8,6 +8,22 @@ import (
 	"testing"
 )
 
+func TestAppendLineNormalizesTrailingNewlines(t *testing.T) {
+	tests := map[string]string{
+		"":           "\n",
+		"text":       "text\n",
+		"text\n":     "text\n",
+		"text\n\n":   "text\n",
+		"  text\n\n": "text\n",
+	}
+
+	for input, expected := range tests {
+		if actual := appendLine(input); actual != expected {
+			t.Errorf("appendLine(%q) = %q, want %q", input, actual, expected)
+		}
+	}
+}
+
 var testDir = "../../../testdata/comments"
 var binDir = "../../../bin"
 

testdata/comments/out/0.json

@@ -7,7 +7,7 @@
         "Scope": "text.comment.block"
     },
     {
-        "Text": "Println formats using the default formats for its oprands and writes to\nstandard output.\n\n\nSpaces are always added between operands and a newline is appended.\n\n\nIt returns the number of bytes written and any write error encountered.\n",
+        "Text": "Println formats using the default formats for its oprands and writes to\nstandard output.\n\nSpaces are always added between operands and a newline is appended.\n\nIt returns the number of bytes written and any write error encountered.\n",
         "Source": "// Println formats using the default formats for its oprands and writes to\n// standard output.\n//\n// Spaces are always added between operands and a newline is appended.\n//\n// It returns the number of bytes written and any write error encountered.\n",
         "Line": 11,
         "Offset": 0,

testdata/comments/out/1.json

@@ -1,14 +1,14 @@
 [
     {
-        "Text": "This module defines the set of command line arguments that ripgrep supports,\nincluding some light validation.\n\n\nThis module is purposely written in a bare-bones way, since it is included\nin ripgrep's build.rs file as a way to generate a man page and completion\nfiles for common shells.\n\n\nThe only other place that ripgrep deals with clap is in src/args.rs, which\nis where we read clap's configuration from the end user's arguments and turn\nit into a ripgrep-specific configuration type that is not coupled with clap.\n",
+        "Text": "This module defines the set of command line arguments that ripgrep supports,\nincluding some light validation.\n\nThis module is purposely written in a bare-bones way, since it is included\nin ripgrep's build.rs file as a way to generate a man page and completion\nfiles for common shells.\n\nThe only other place that ripgrep deals with clap is in src/args.rs, which\nis where we read clap's configuration from the end user's arguments and turn\nit into a ripgrep-specific configuration type that is not coupled with clap.\n",
         "Source": "// This module defines the set of command line arguments that ripgrep supports,\n// including some light validation.\n//\n// This module is purposely written in a bare-bones way, since it is included\n// in ripgrep's build.rs file as a way to generate a man page and completion\n// files for common shells.\n//\n// The only other place that ripgrep deals with clap is in src/args.rs, which\n// is where we read clap's configuration from the end user's arguments and turn\n// it into a ripgrep-specific configuration type that is not coupled with clap.\n",
         "Line": 1,
         "Offset": 0,
         "Scope": "text.comment.line"
     },
     {
-        "Text": "A human being is representd here\n\n A human being is representd here\n",
-        "Source": "/// A human being is representd here\n\n///\n/// A human being is representd here\n",
+        "Text": "A human being is representd here\n\nA human being is representd here\n",
+        "Source": "/// A human being is representd here\n///\n/// A human being is representd here\n",
         "Line": 13,
         "Offset": 0,
         "Scope": "text.comment.line"
@@ -21,15 +21,15 @@
         "Scope": "text.comment.line"
     },
     {
-        "Text": "Returns a person with the name given them\n\n # Arguments\n\n * `foof` - A string slice doof that holds the nme of the person\n\n # Exmples\n\n ```\n  You can have rust code between fences inside the comments\n  If you pass --test to `rustdoc`, it will even test it for you!\n use doc::Person;\n let person = Person::new(\"name\");\n ```\n",
-        "Source": "/// Returns a person with the name given them\n\n///\n/// # Arguments\n///\n/// * `foof` - A string slice doof that holds the nme of the person\n///\n/// # Exmples\n///\n/// ```\n/// // You can have rust code between fences inside the comments\n/// // If you pass --test to `rustdoc`, it will even test it for you!\n/// use doc::Person;\n/// let person = Person::new(\"name\");\n/// ```\n",
+        "Text": "Returns a person with the name given them\n\n# Arguments\n\n* `foof` - A string slice doof that holds the nme of the person\n\n# Exmples\n\n```\nYou can have rust code between fences inside the comments\nIf you pass --test to `rustdoc`, it will even test it for you!\nuse doc::Person;\nlet person = Person::new(\"name\");\n```\n",
+        "Source": "/// Returns a person with the name given them\n///\n/// # Arguments\n///\n/// * `foof` - A string slice doof that holds the nme of the person\n///\n/// # Exmples\n///\n/// ```\n/// // You can have rust code between fences inside the comments\n/// // If you pass --test to `rustdoc`, it will even test it for you!\n/// use doc::Person;\n/// let person = Person::new(\"name\");\n/// ```\n",
         "Line": 22,
         "Offset": 4,
         "Scope": "text.comment.line"
     },
     {
-        "Text": "Gives a friendly hello!\n\n Says \"Hello, [name]\" to the `Person` it is called on.\n",
-        "Source": "/// Gives a friendly hello!\n\n///\n/// Says \"Hello, [name]\" to the `Person` it is called on.\n",
+        "Text": "Gives a friendly hello!\n\nSays \"Hello, [name]\" to the `Person` it is called on.\n",
+        "Source": "/// Gives a friendly hello!\n///\n/// Says \"Hello, [name]\" to the `Person` it is called on.\n",
         "Line": 42,
         "Offset": 4,
         "Scope": "text.comment.line"

testdata/comments/out/5.json

@@ -1,6 +1,6 @@
 [
     {
-        "Text": "This Deployment runs our API component\n\nTo increase the TODO number of replicas that run,\nchange the value of the spec.replicas field.\n\n\nAlways use a value higher than 1 to ensure\nmultiple replicas are running, as this\nguarantees redundancy if one instance fails.\n",
+        "Text": "This Deployment runs our API component\n\nTo increase the TODO number of replicas that run,\nchange the value of the spec.replicas field.\n\nAlways use a value higher than 1 to ensure\nmultiple replicas are running, as this\nguarantees redundancy if one instance fails.\n",
         "Source": "# This Deployment runs our API component\n#\n# To increase the TODO number of replicas that run,\n# change the value of the spec.replicas field.\n#\n# Always use a value higher than 1 to ensure\n# multiple replicas are running, as this\n# guarantees redundancy if one instance fails.\n",
         "Line": 1,
         "Offset": 0,

testdata/e2e/fragments.yaml

@@ -55,6 +55,49 @@ cases:
       test2.rs:409:38:Vale.Spelling:Did you really mean 'RGArg'?
       test2.rs:2860:34:Vale.Spelling:Did you really mean 'conlicts'?
 
+  - name: blank-comment-line
+    about: |
+      #1022: a blank line comment produced two lines in the extracted text
+      where the source has one, so every alert below it was reported a line
+      too far down with its column collapsed to 1.
+    files:
+      .vale.ini: |
+        StylesPath = styles
+        MinAlertLevel = suggestion
+
+        [*.{proto,go}]
+        BasedOnStyles = T
+      styles/T/Eror.yml: |
+        extends: existence
+        message: "Found '%s'."
+        level: error
+        tokens:
+          - Eror
+      test.proto: |
+        syntax = "proto3";
+
+        // Correct.
+        // Correct.
+        // Eror.
+        //
+        // Eror.
+        message Message {}
+      test.go: |
+        package main
+
+        // Eror.
+        //
+        //
+        // Eror.
+        func main() {}
+    args: .
+    exit: 1
+    want: |
+      test.go:3:4:T.Eror:Found 'Eror'.
+      test.go:6:4:T.Eror:Found 'Eror'.
+      test.proto:5:4:T.Eror:Found 'Eror'.
+      test.proto:7:4:T.Eror:Found 'Eror'.
+
   - name: tab-indented-block
     about: |
       #1130: only spaces were stripped when dedenting a block comment, so one