(fix) better fix range handling (#160)

Fixes don't need to start at the same line where the error message is shown, and they don't need to be at the same line as the start of the error message. Therefore the previous handling of re-adding dedents was flawed. Instead, map the range to positions (line/character) and use them to find the correct dedent offsets.
Additionally add indentation after each newline a fix has because the fix doesn't know about the additional indentation.

To properly test fixes, the test setup was enhanced. If there's a Fixed.svelte, that one will be compared against a fixed version of Input.svelte.

Fixes #110
Fixes #156
Fixes #157

Author: dummdidumm

src/block.js

@@ -13,10 +13,11 @@ export const get_translation = (text, block, node, options = {}) => {
 	block.transformed_code += '\n';
 	const translation = { options, unoffsets: get_offsets(block.transformed_code) };
 	translation.range = [node.start, node.end];
-	const { dedented, offsets } = dedent_code(text.slice(node.start, node.end));
+	const { dedented, offsets, indentation } = dedent_code(text.slice(node.start, node.end));
 	block.transformed_code += dedented;
 	translation.offsets = get_offsets(text.slice(0, node.start));
 	translation.dedent = offsets;
+	translation.indentation = indentation;
 	translation.end = get_offsets(block.transformed_code).lines;
 	for (let i = translation.unoffsets.lines; i <= translation.end; i++) {
 		block.translations.set(i, translation);

src/mapping.js

@@ -137,7 +137,7 @@ export class DocumentMapper {
  * @param position Line and character position
  * @param text The text for which the offset should be retrieved
  */
-function offset_at(position, text) {
+export function offset_at(position, text) {
 	const line_offsets = get_line_offsets(text);
 
 	if (position.line >= line_offsets.length) {
@@ -153,7 +153,12 @@ function offset_at(position, text) {
 	return clamp(next_line_offset, line_offset, line_offset + position.column);
 }
 
-function position_at(offset, text) {
+/**
+ * Get the line and character position of an offset
+ * @param offset Idx of the offset
+ * @param text The text for which the position should be retrieved
+ */
+export function position_at(offset, text) {
 	offset = clamp(offset, 0, text.length);
 
 	const line_offsets = get_line_offsets(text);

src/postprocess.js

@@ -1,10 +1,13 @@
+import { position_at } from './mapping.js';
 import { state, reset } from './state.js';
 import { get_line_offsets } from './utils.js';
 
 // transform a linting message according to the module/instance script info we've gathered
-const transform_message = ({ transformed_code }, { unoffsets, dedent, offsets, range }, message) => {
-	// strip out the start and end of the fix if they are not actually changes
+const transform_message = ({ transformed_code }, { unoffsets, dedent, indentation, offsets, range }, message) => {
+	let fix_pos_start;
+	let fix_pos_end;
 	if (message.fix) {
+		// strip out the start and end of the fix if they are not actually changes
 		while (message.fix.range[0] < message.fix.range[1] && transformed_code[message.fix.range[0]] === message.fix.text[0]) {
 			message.fix.range[0]++;
 			message.fix.text = message.fix.text.slice(1);
@@ -13,7 +16,15 @@ const transform_message = ({ transformed_code }, { unoffsets, dedent, offsets, r
 			message.fix.range[1]--;
 			message.fix.text = message.fix.text.slice(0, -1);
 		}
+		// If the fix spans multiple lines, add the indentation to each one
+		message.fix.text = message.fix.text.split('\n').join(`\n${indentation}`);
+		// The fix can span multiple lines and doesn't need to be on the same line
+		// as the lint message, so we can't just add total_offsets at message.line to it
+		// (see dedent-step below)
+		fix_pos_start = position_at(message.fix.range[0], transformed_code);
+		fix_pos_end = position_at(message.fix.range[1], transformed_code);
 	}
+
 	// shift position reference backward according to unoffsets
 	// (aka: throw out the generated code lines prior to the original code)
 	{
@@ -33,6 +44,7 @@ const transform_message = ({ transformed_code }, { unoffsets, dedent, offsets, r
 			message.fix.range[1] -= length;
 		}
 	}
+
 	// adjust position reference according to the previous dedenting
 	// (aka: re-add the stripped indentation)
 	{
@@ -42,10 +54,15 @@ const transform_message = ({ transformed_code }, { unoffsets, dedent, offsets, r
 			message.endColumn += offsets[message.endLine - 1];
 		}
 		if (message.fix) {
-			message.fix.range[0] += total_offsets[message.line];
-			message.fix.range[1] += total_offsets[message.line];
+			// We need the offset at the line relative to dedent's total_offsets. The dedents
+			// start at the point in the total transformed code where a subset of the code was transformed.
+			// Therefore substract (unoffsets.lines + 1) which marks the start of that transformation.
+			// Add +1 afterwards because total_offsets are 1-index-based. 
+			message.fix.range[0] += total_offsets[fix_pos_start.line - unoffsets.lines + 2];
+			message.fix.range[1] += total_offsets[fix_pos_end.line - unoffsets.lines + 2];
 		}
 	}
+
 	// shift position reference forward according to offsets
 	// (aka: re-add the code that is originally prior to the code)
 	{
@@ -65,6 +82,7 @@ const transform_message = ({ transformed_code }, { unoffsets, dedent, offsets, r
 			message.fix.range[1] += length;
 		}
 	}
+
 	// make sure the fix doesn't include anything outside the range of the script
 	if (message.fix) {
 		if (message.fix.range[0] < range[0]) {

src/utils.js

@@ -46,7 +46,7 @@ export const dedent_code = str => {
 		}
 		dedented += str[i];
 	}
-	return { dedented, offsets: { offsets, total_offsets } };
+	return { dedented, offsets: { offsets, total_offsets }, indentation };
 };
 
 // get character offsets of each line in a string

test/index.js

@@ -3,10 +3,11 @@
 process.chdir(__dirname);
 
 const { ESLint } = require('eslint');
+const { SourceCodeFixer } = require('eslint/lib/linter');
 const assert = require('assert');
 const fs = require('fs');
 
-const linter = new ESLint({ reportUnusedDisableDirectives: "error" });
+const linter = new ESLint({ reportUnusedDisableDirectives: 'error' });
 
 async function run() {
 	const tests = fs.readdirSync('samples').filter(name => name[0] !== '.');
@@ -15,24 +16,31 @@ async function run() {
 		console.log(name);
 
 		const path_input = `samples/${name}/Input.svelte`;
+		const path_fixed = `samples/${name}/Fixed.svelte`;
 		const path_ple = `samples/${name}/preserve_line_endings`;
 		const path_expected = `samples/${name}/expected.json`;
 		const path_actual = `samples/${name}/actual.json`;
 
 		if (process.platform === 'win32' && !exists(path_ple)) {
-			const file = fs.readFileSync(path_input, "utf-8");
+			const file = fs.readFileSync(path_input, 'utf-8');
 			fs.writeFileSync(path_input, file.replace(/\r/g, ''));
 		}
 
 		const result = await linter.lintFiles(path_input);
 
 		const actual = result[0] ? result[0].messages : [];
-		const expected = JSON.parse(fs.readFileSync(path_expected, "utf-8"));
+		const expected = JSON.parse(fs.readFileSync(path_expected, 'utf-8'));
 
 		fs.writeFileSync(path_actual, JSON.stringify(actual, null, '\t'));
 
 		assert.equal(actual.length, expected.length);
 		assert.deepStrictEqual(actual, actual.map((msg, i) => ({ ...msg, ...expected[i] })));
+		
+		if (fs.existsSync(path_fixed)) {
+			const fixed = SourceCodeFixer.applyFixes(fs.readFileSync(path_input, 'utf-8'), actual).output;
+			assert.deepStrictEqual(fixed, fs.readFileSync(path_fixed, 'utf-8'))
+		}
+
 		console.log('passed!\n');
 	}
 }

test/samples/multiline-fixes/.eslintrc.js

@@ -0,0 +1,7 @@
+module.exports = {
+	rules: {
+		curly: 'error',
+		'no-else-return': 'error',
+		'no-lonely-if': 'error',
+	},
+};

test/samples/multiline-fixes/Fixed.svelte

@@ -0,0 +1,24 @@
+<script>
+    if (window.foo)
+        {window.foo =
+            window.bar
+                .toLowerCase()
+                .replace('something', 'else')
+                .trim();}
+
+    function noElseReturn() {
+        if (window) {
+            return 'foo';
+        } 
+            return 'bar';
+        
+    }
+
+    function noLonelyIf() {
+        if (window) {
+            doX();
+        } else if (!window) {
+                doY();
+            }
+    }
+</script>

test/samples/multiline-fixes/Input.svelte

@@ -0,0 +1,26 @@
+<script>
+    if (window.foo)
+        window.foo =
+            window.bar
+                .toLowerCase()
+                .replace('something', 'else')
+                .trim();
+
+    function noElseReturn() {
+        if (window) {
+            return 'foo';
+        } else {
+            return 'bar';
+        }
+    }
+
+    function noLonelyIf() {
+        if (window) {
+            doX();
+        } else {
+            if (!window) {
+                doY();
+            }
+        }
+    }
+</script>

test/samples/multiline-fixes/expected.json

@@ -0,0 +1,56 @@
+[
+	{
+		"ruleId": "curly",
+		"severity": 2,
+		"message": "Expected { after 'if' condition.",
+		"line": 3,
+		"column": 9,
+		"nodeType": "IfStatement",
+		"messageId": "missingCurlyAfterCondition",
+		"endLine": 7,
+		"endColumn": 25,
+		"fix": {
+			"range": [
+				37,
+				174
+			],
+			"text": "{window.foo =\n            window.bar\n                .toLowerCase()\n                .replace('something', 'else')\n                .trim();}"
+		}
+	},
+	{
+		"ruleId": "no-else-return",
+		"severity": 2,
+		"message": "Unnecessary 'else' after 'return'.",
+		"line": 12,
+		"column": 16,
+		"nodeType": "BlockStatement",
+		"messageId": "unexpected",
+		"endLine": 14,
+		"endColumn": 10,
+		"fix": {
+			"range": [
+				264,
+				306
+			],
+			"text": "\n            return 'bar';\n        "
+		}
+	},
+	{
+		"ruleId": "no-lonely-if",
+		"severity": 2,
+		"message": "Unexpected if as the only statement in an else block.",
+		"line": 21,
+		"column": 13,
+		"nodeType": "IfStatement",
+		"messageId": "unexpectedLonelyIf",
+		"endLine": 23,
+		"endColumn": 14,
+		"fix": {
+			"range": [
+				398,
+				468
+			],
+			"text": "if (!window) {\n                doY();\n        "
+		}
+	}
+]

test/samples/typescript-multiline-fixes/.eslintrc.js

@@ -0,0 +1,12 @@
+module.exports = {
+	parser: '@typescript-eslint/parser',
+	plugins: ['@typescript-eslint'],
+	settings: {
+		'svelte3/typescript': require('typescript'),
+	},
+	rules: {
+		curly: 'error',
+		'no-else-return': 'error',
+		'no-lonely-if': 'error',
+	},
+};