Make sourcemaps more resilient against out-of-range modifications

.gitpod.setup.sh

@@ -9,4 +9,5 @@ git clone https://github.com/bcmi-labs/arduino-editor
 cd arduino-editor
 yarn
 
-echo "start an Arduino IDE with: yarn --cwd /workspace/arduino-editor/browser-app start"
+echo "starting an Arduino IDE with: yarn --cwd /workspace/arduino-editor/browser-app start"
+yarn --cwd /workspace/arduino-editor/browser-app start

handler/handler.go

@@ -312,15 +312,18 @@ func (handler *InoHandler) createFileData(ctx context.Context, sourceURI lsp.Doc
 	return data, targetBytes, nil
 }
 
-func (handler *InoHandler) updateFileData(data *FileData, change *lsp.TextDocumentContentChangeEvent) error {
+func (handler *InoHandler) updateFileData(data *FileData, change *lsp.TextDocumentContentChangeEvent) (err error) {
 	rang := change.Range
 	if rang == nil || rang.Start.Line != rang.End.Line {
 		// Update the source text and regenerate the cpp code
 		var newSourceText string
 		if rang == nil {
 			newSourceText = change.Text
 		} else {
-			newSourceText = applyTextChange(data.sourceText, *rang, change.Text)
+			newSourceText, err = applyTextChange(data.sourceText, *rang, change.Text)
+			if err != nil {
+				return err
+			}
 		}
 		targetBytes, err := updateCpp([]byte(newSourceText), uriToPath(data.sourceURI), handler.config.SelectedBoard.Fqbn, false, uriToPath(data.targetURI))
 		if err != nil {
@@ -353,7 +356,10 @@ func (handler *InoHandler) updateFileData(data *FileData, change *lsp.TextDocume
 	} else {
 		// Apply an update to a single line both to the source and the target text
 		targetLine := data.targetLineMap[rang.Start.Line]
-		data.sourceText = applyTextChange(data.sourceText, *rang, change.Text)
+		data.sourceText, err = applyTextChange(data.sourceText, *rang, change.Text)
+		if err != nil {
+			return err
+		}
 		updateSourceMaps(data.sourceLineMap, data.targetLineMap, 0, rang.Start.Line, change.Text)
 
 		rang.Start.Line = targetLine

handler/sourcemap.go

@@ -2,6 +2,7 @@ package handler
 
 import (
 	"bufio"
+	"fmt"
 	"io"
 	"strconv"
 	"strings"
@@ -113,34 +114,91 @@ func copyMappings(sourceLineMap, targetLineMap, newMappings map[int]int) {
 	}
 }
 
-func applyTextChange(text string, rang lsp.Range, insertText string) string {
-	if rang.Start.Line != rang.End.Line {
-		startOffset := getLineOffset(text, rang.Start.Line) + rang.Start.Character
-		endOffset := getLineOffset(text, rang.End.Line) + rang.End.Character
-		return text[:startOffset] + insertText + text[endOffset:]
-	} else if rang.Start.Character != rang.End.Character {
-		lineOffset := getLineOffset(text, rang.Start.Line)
-		startOffset := lineOffset + rang.Start.Character
-		endOffset := lineOffset + rang.End.Character
-		return text[:startOffset] + insertText + text[endOffset:]
-	} else {
-		offset := getLineOffset(text, rang.Start.Line) + rang.Start.Character
-		return text[:offset] + insertText + text[offset:]
+// OutOfRangeError returned if one attempts to access text out of its range
+type OutOfRangeError struct {
+	Max int
+	Req lsp.Position
+}
+
+func (oor OutOfRangeError) Error() string {
+	return fmt.Sprintf("text access out of range: max=%d requested=%d", oor.Max, oor.Req)
+}
+
+func applyTextChange(text string, rang lsp.Range, insertText string) (res string, err error) {
+	start, err := getOffset(text, rang.Start)
+	if err != nil {
+		return "", err
+	}
+	end, err := getOffset(text, rang.End)
+	if err != nil {
+		return "", err
+	}
+
+	return text[:start] + insertText + text[end:], nil
+}
+
+// getOffset computes the offset in the text expressed by the lsp.Position.
+// Returns OutOfRangeError if the position is out of range.
+func getOffset(text string, pos lsp.Position) (off int, err error) {
+	// find line
+	lineOffset := getLineOffset(text, pos.Line)
+	if lineOffset < 0 {
+		return -1, OutOfRangeError{len(text), pos}
+	}
+	off = lineOffset
+
+	// walk towards the character
+	var charFound bool
+	for offset, c := range text[off:] {
+		if c == '\n' {
+			// We've reached the end of line. LSP spec says we should default back to the line length.
+			// See https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/#position
+			off += offset
+			charFound = true
+			break
+		}
+
+		// we've fond the character
+		if offset == pos.Character {
+			off += offset
+			charFound = true
+			break
+		}
 	}
+	if !charFound {
+		return -1, OutOfRangeError{Max: len(text), Req: pos}
+	}
+
+	return off, nil
 }
 
+// getLineOffset finds the offset/position of the beginning of a line within the text.
+// For example:
+//    text := "foo\nfoobar\nbaz"
+//    getLineOffset(text, 0) == 0
+//    getLineOffset(text, 1) == 4
+//    getLineOffset(text, 2) == 11
 func getLineOffset(text string, line int) int {
+	if line < 0 {
+		return -1
+	}
 	if line == 0 {
 		return 0
 	}
-	count := 0
+
+	// find the line and return its offset within the text
+	var count int
 	for offset, c := range text {
-		if c == '\n' {
-			count++
-			if count == line {
-				return offset + 1
-			}
+		if c != '\n' {
+			continue
+		}
+
+		count++
+		if count == line {
+			return offset + 1
 		}
 	}
-	return len(text)
+
+	// we didn't find the line in the text
+	return -1
 }

handler/sourcemap_test.go

@@ -72,32 +72,157 @@ func TestUpdateSourceMaps2(t *testing.T) {
 }
 
 func TestApplyTextChange(t *testing.T) {
-	text1 := applyTextChange("foo\nbar\nbaz\n!", lsp.Range{
-		Start: lsp.Position{Line: 1, Character: 1},
-		End:   lsp.Position{Line: 2, Character: 2},
-	}, "i")
-	if text1 != "foo\nbiz\n!" {
-		t.Error(text1)
+	tests := []struct {
+		InitialText string
+		Range       lsp.Range
+		Insertion   string
+		Expectation string
+		Err         error
+	}{
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				Start: lsp.Position{Line: 1, Character: 1},
+				End:   lsp.Position{Line: 2, Character: 2},
+			},
+			"i",
+			"foo\nbiz\n!",
+			nil,
+		},
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				Start: lsp.Position{Line: 1, Character: 1},
+				End:   lsp.Position{Line: 1, Character: 2},
+			},
+			"ee",
+			"foo\nbeer\nbaz\n!",
+			nil,
+		},
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				Start: lsp.Position{Line: 1, Character: 1},
+				End:   lsp.Position{Line: 1, Character: 1},
+			},
+			"eer from the st",
+			"foo\nbeer from the star\nbaz\n!",
+			nil,
+		},
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				Start: lsp.Position{Line: 0, Character: 10},
+				End:   lsp.Position{Line: 2, Character: 20},
+			},
+			"i",
+			"fooi\n!",
+			nil,
+		},
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				// out of range start offset
+				Start: lsp.Position{Line: 0, Character: 100},
+				End:   lsp.Position{Line: 2, Character: 0},
+			},
+			"i",
+			"fooibaz\n!",
+			nil,
+		},
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				// out of range start offset
+				Start: lsp.Position{Line: 20, Character: 0},
+				End:   lsp.Position{Line: 2, Character: 0},
+			},
+			"i",
+			"",
+			OutOfRangeError{13, lsp.Position{Line: 20, Character: 0}},
+		},
+		{
+			"foo\nbar\nbaz\n!",
+			lsp.Range{
+				// out of range start offset
+				Start: lsp.Position{Line: 0, Character: 0},
+				End:   lsp.Position{Line: 20, Character: 0},
+			},
+			"i",
+			"",
+			OutOfRangeError{13, lsp.Position{Line: 20, Character: 0}},
+		},
 	}
-	text2 := applyTextChange("foo\nbar\nbaz\n!", lsp.Range{
-		Start: lsp.Position{Line: 1, Character: 1},
-		End:   lsp.Position{Line: 1, Character: 2},
-	}, "ee")
-	if text2 != "foo\nbeer\nbaz\n!" {
-		t.Error(text2)
+
+	for _, test := range tests {
+		initial := strings.ReplaceAll(test.InitialText, "\n", "\\n")
+		insertion := strings.ReplaceAll(test.Insertion, "\n", "\\n")
+		expectation := strings.ReplaceAll(test.Expectation, "\n", "\\n")
+
+		t.Logf("applyTextChange(\"%s\", %v, \"%s\") == \"%s\"", initial, test.Range, insertion, expectation)
+		act, err := applyTextChange(test.InitialText, test.Range, test.Insertion)
+		if act != test.Expectation {
+			t.Errorf("applyTextChange(\"%s\", %v, \"%s\") != \"%s\", got \"%s\"", initial, test.Range, insertion, expectation, strings.ReplaceAll(act, "\n", "\\n"))
+		}
+		if err != test.Err {
+			t.Errorf("applyTextChange(\"%s\", %v, \"%s\") error != %v, got %v instead", initial, test.Range, insertion, test.Err, err)
+		}
+	}
+}
+
+func TestGetOffset(t *testing.T) {
+	tests := []struct {
+		Text string
+		Line int
+		Char int
+		Exp  int
+		Err  error
+	}{
+		{"foo\nfoobar\nbaz", 0, 0, 0, nil},
+		{"foo\nfoobar\nbaz", 1, 0, 4, nil},
+		{"foo\nfoobar\nbaz", 1, 3, 7, nil},
+		{"foo\nba\nr\nbaz\n!", 3, 0, 9, nil},
+		{"foo\nba\nr\nbaz\n!", 1, 10, 6, nil},
+		{"foo\nba\nr\nbaz\n!", -1, 0, -1, OutOfRangeError{14, lsp.Position{Line: -1, Character: 0}}},
+		{"foo\nba\nr\nbaz\n!", 4, 20, -1, OutOfRangeError{14, lsp.Position{Line: 4, Character: 20}}},
 	}
-	text3 := applyTextChange("foo\nbar\nbaz\n!", lsp.Range{
-		Start: lsp.Position{Line: 1, Character: 1},
-		End:   lsp.Position{Line: 1, Character: 1},
-	}, "eer from the st")
-	if text3 != "foo\nbeer from the star\nbaz\n!" {
-		t.Error(text3)
+
+	for _, test := range tests {
+		st := strings.Replace(test.Text, "\n", "\\n", -1)
+
+		t.Logf("getOffset(\"%s\", {Line: %d, Character: %d}) == %d", st, test.Line, test.Char, test.Exp)
+		act, err := getOffset(test.Text, lsp.Position{Line: test.Line, Character: test.Char})
+		if act != test.Exp {
+			t.Errorf("getOffset(\"%s\", {Line: %d, Character: %d}) != %d, got %d instead", st, test.Line, test.Char, test.Exp, act)
+		}
+		if err != test.Err {
+			t.Errorf("getOffset(\"%s\", {Line: %d, Character: %d}) error != %v, got %v instead", st, test.Line, test.Char, test.Err, err)
+		}
 	}
 }
 
 func TestGetLineOffset(t *testing.T) {
-	offset := getLineOffset("foo\nba\nr\nbaz\n!", 3)
-	if offset != 9 {
-		t.Error(offset)
+	tests := []struct {
+		Text   string
+		Line   int
+		Offset int
+	}{
+		{"foo\nfoobar\nbaz", 0, 0},
+		{"foo\nfoobar\nbaz", 1, 4},
+		{"foo\nfoobar\nbaz", 2, 11},
+		{"foo\nfoobar\nbaz", 3, -1},
+		{"foo\nba\nr\nbaz\n!", 3, 9},
+		{"foo\nba\nr\nbaz\n!", -1, -1},
+		{"foo\nba\nr\nbaz\n!", 20, -1},
+	}
+
+	for _, test := range tests {
+		st := strings.Replace(test.Text, "\n", "\\n", -1)
+
+		t.Logf("getLineOffset(\"%s\", %d) == %d", st, test.Line, test.Offset)
+		act := getLineOffset(test.Text, test.Line)
+		if act != test.Offset {
+			t.Errorf("getLineOffset(\"%s\", %d) != %d, got %d instead", st, test.Line, test.Offset, act)
+		}
 	}
 }