chore: better ignore code handling (#11606)

Instead of hacking an ignores array onto each node (and possibly degrading perf a bit because the object shape is mutated) we keep track of ignores in a stack. The new approach also avoids the indirection the old one had to do because the new approach looks upwards (checking if parent is a fragment) instead of iterating the children (checking for comments in them).
As a bonus unknown code warnings are now in order (line-column-wise) with the other warnings. Also fixes #11482 because text nodes of all shapes are ok

packages/svelte/scripts/process-messages/templates/compile-warnings.js

@@ -1,4 +1,4 @@
-import { filename, locator, warnings } from './state.js';
+import { filename, locator, warnings, ignore_stack } from './state.js';
 
 /** @typedef {{ start?: number, end?: number }} NodeLike */
 
@@ -8,8 +8,7 @@ import { filename, locator, warnings } from './state.js';
  * @param {string} message
  */
 function w(node, code, message) {
-	// @ts-expect-error
+	if (ignore_stack.at(-1)?.has(code)) return;
-	if (node?.ignores?.has(code)) return;
 
 	warnings.push({
 		code,

packages/svelte/src/compiler/phases/2-analyze/a11y.js

@@ -727,9 +727,6 @@ function check_element(node, state) {
 	for (const attribute of node.attributes) {
 		if (attribute.type !== 'Attribute') continue;
 
-		// @ts-expect-error gross hack
-		attribute.ignores = node.ignores;
-
 		const name = attribute.name.toLowerCase();
 		// aria-props
 		if (name.startsWith('aria-')) {

packages/svelte/src/compiler/phases/2-analyze/index.js

@@ -25,6 +25,7 @@ import { prune } from './css/css-prune.js';
 import { hash } from './utils.js';
 import { warn_unused } from './css/css-warn.js';
 import { extract_svelte_ignore } from '../../utils/extract_svelte_ignore.js';
+import { pop_ignore, push_ignore } from '../../state.js';
 
 /**
  * @param {import('#compiler').Script | null} script
@@ -439,8 +440,7 @@ export function analyze_component(root, source, options) {
 				component_slots: new Set(),
 				expression: null,
 				private_derived_state: [],
-				function_depth: scope.function_depth,
+				function_depth: scope.function_depth
-				ignores: new Set()
 			};
 
 			walk(
@@ -511,8 +511,7 @@ export function analyze_component(root, source, options) {
 				component_slots: new Set(),
 				expression: null,
 				private_derived_state: [],
-				function_depth: scope.function_depth,
+				function_depth: scope.function_depth
-				ignores: new Set()
 			};
 
 			walk(
@@ -1100,67 +1099,51 @@ function is_safe_identifier(expression, scope) {
 
 /** @type {import('./types').Visitors} */
 const common_visitors = {
-	_(node, context) {
+	_(node, { state, next, path }) {
-		// @ts-expect-error
+		const parent = path.at(-1);
-		const comments = /** @type {import('estree').Comment[]} */ (node.leadingComments);
+		if (parent?.type === 'Fragment' && node.type !== 'Comment' && node.type !== 'Text') {
-
+			const idx = parent.nodes.indexOf(/** @type {any} */ (node));
-		if (comments) {
 			/** @type {string[]} */
 			const ignores = [];
-
+			for (let i = idx - 1; i >= 0; i--) {
-			for (const comment of comments) {
+				const prev = parent.nodes[i];
-				const start = /** @type {any} */ (comment).start + 2;
+				if (prev.type === 'Comment') {
-				ignores.push(...extract_svelte_ignore(start, comment.value, context.state.analysis.runes));
+					ignores.push(
+						...extract_svelte_ignore(
+							prev.start + 2 /* '//'.length */,
+							prev.data,
+							state.analysis.runes
+						)
+					);
+				} else if (prev.type !== 'Text') {
+					break;
+				}
 			}
 
 			if (ignores.length > 0) {
-				// @ts-expect-error see below
+				push_ignore(ignores);
-				node.ignores = new Set([...context.state.ignores, ...ignores]);
+				next();
+				pop_ignore();
 			}
-		}
+		} else {
-
+			const comments = /** @type {any} */ (node).leadingComments;
-		// @ts-expect-error
+			if (comments) {
-		if (node.ignores) {
+				/** @type {string[]} */
-			context.next({
+				const ignores = [];
-				...context.state,
+				for (const comment of comments) {
-				// @ts-expect-error see below
+					ignores.push(
-				ignores: node.ignores
+						...extract_svelte_ignore(
-			});
+							comment.start + 4 /* '<!--'.length */,
-		} else if (context.state.ignores.size > 0) {
+							comment.value,
-			// @ts-expect-error
+							state.analysis.runes
-			node.ignores = context.state.ignores;
+						)
-		}
+					);
-	},
+				}
-	Fragment(node, context) {
+				if (ignores.length > 0) {
-		/** @type {string[]} */
+					push_ignore(ignores);
-		let ignores = [];
+					next();
-
+					pop_ignore();
-		for (const child of node.nodes) {
-			if (child.type === 'Text' && child.data.trim() === '') {
-				continue;
-			}
-
-			if (child.type === 'Comment') {
-				const start =
-					child.start +
-					(context.state.analysis.source.slice(child.start, child.start + 4) === '<!--' ? 4 : 2);
-
-				ignores.push(...extract_svelte_ignore(start, child.data, context.state.analysis.runes));
-			} else {
-				const combined_ignores = new Set(context.state.ignores);
-				for (const ignore of ignores) combined_ignores.add(ignore);
-
-				if (combined_ignores.size > 0) {
-					// TODO this is a grotesque hack that's made necessary by the fact that
-					// we can't call `context.visit(...)` here, because we do the convoluted
-					// visitor merging thing. I'm increasingly of the view that we should
-					// rearchitect this stuff and have a single visitor per node. It'd be
-					// more efficient and much simpler.
-					// @ts-expect-error
-					child.ignores = combined_ignores;
 				}
-
-				ignores = [];
 			}
 		}
 	},

packages/svelte/src/compiler/phases/2-analyze/types.d.ts

@@ -22,7 +22,6 @@ export interface AnalysisState {
 	expression: ExpressionTag | ClassDirective | SpreadAttribute | null;
 	private_derived_state: string[];
 	function_depth: number;
-	ignores: Set<string>;
 }
 
 export interface LegacyAnalysisState extends AnalysisState {

packages/svelte/src/compiler/state.js

@@ -10,6 +10,21 @@ export let filename;
 
 export let locator = getLocator('', { offsetLine: 1 });
 
+/** @type {Set<string>[]} */
+export let ignore_stack = [];
+
+/**
+ * @param {string[]} ignores
+ */
+export function push_ignore(ignores) {
+	const next = new Set([...(ignore_stack.at(-1) || []), ...ignores]);
+	ignore_stack.push(next);
+}
+
+export function pop_ignore() {
+	ignore_stack.pop();
+}
+
 /**
  * @param {{
  *   source: string;
@@ -20,4 +35,5 @@ export function reset(options) {
 	filename = options.filename;
 	locator = getLocator(options.source, { offsetLine: 1 });
 	warnings = [];
+	ignore_stack = [];
 }

packages/svelte/src/compiler/warnings.js

@@ -1,6 +1,6 @@
 /* This file is generated by scripts/process-messages/index.js. Do not edit! */
 
-import { filename, locator, warnings } from './state.js';
+import { filename, locator, warnings, ignore_stack } from './state.js';
 
 /** @typedef {{ start?: number, end?: number }} NodeLike */
 /**
@@ -9,8 +9,7 @@ import { filename, locator, warnings } from './state.js';
  * @param {string} message
  */
 function w(node, code, message) {
-	// @ts-expect-error
+	if (ignore_stack.at(-1)?.has(code)) return;
-	if (node?.ignores?.has(code)) return;
 
 	warnings.push({
 		code,

packages/svelte/tests/validator/samples/unknown-code/warnings.json

@@ -1,74 +1,74 @@
 [
 	{
 		"code": "legacy_code",
-		"end": {
-			"column": 41,
-			"line": 3
-		},
 		"message": "`a11y-missing-attribute` is no longer valid — please use `a11y_missing_attribute` instead",
 		"start": {
-			"column": 19,
+			"line": 3,
-			"line": 3
+			"column": 17
+		},
+		"end": {
+			"line": 3,
+			"column": 39
 		}
 	},
 	{
-		"code": "unknown_code",
+		"code": "a11y_missing_attribute",
-		"end": {
+		"message": "`<img>` element should have an alt attribute",
-			"column": 41,
-			"line": 8
-		},
-		"message": "`ally_missing_attribute` is not a recognised code (did you mean `a11y_missing_attribute`?)",
 		"start": {
-			"column": 19,
+			"line": 5,
-			"line": 8
+			"column": 1
+		},
+		"end": {
+			"line": 5,
+			"column": 29
 		}
 	},
 	{
-		"code": "legacy_code",
+		"code": "unknown_code",
-		"end": {
+		"message": "`ally_missing_attribute` is not a recognised code (did you mean `a11y_missing_attribute`?)",
-			"column": 39,
-			"line": 13
-		},
-		"message": "`a11y-misplaced-scope` is no longer valid — please use `a11y_misplaced_scope` instead",
 		"start": {
-			"column": 19,
+			"line": 8,
-			"line": 13
+			"column": 17
+		},
+		"end": {
+			"line": 8,
+			"column": 39
 		}
 	},
 	{
 		"code": "a11y_missing_attribute",
-		"end": {
-			"column": 29,
-			"line": 5
-		},
 		"message": "`<img>` element should have an alt attribute",
 		"start": {
-			"column": 1,
+			"line": 10,
-			"line": 5
+			"column": 1
+		},
+		"end": {
+			"line": 10,
+			"column": 29
 		}
 	},
 	{
-		"code": "a11y_missing_attribute",
+		"code": "legacy_code",
-		"end": {
+		"message": "`a11y-misplaced-scope` is no longer valid — please use `a11y_misplaced_scope` instead",
-			"column": 29,
-			"line": 10
-		},
-		"message": "`<img>` element should have an alt attribute",
 		"start": {
-			"column": 1,
+			"line": 13,
-			"line": 10
+			"column": 17
+		},
+		"end": {
+			"line": 13,
+			"column": 37
 		}
 	},
 	{
 		"code": "a11y_misplaced_scope",
-		"end": {
-			"column": 10,
-			"line": 14
-		},
 		"message": "The scope attribute should only be used with `<th>` elements",
 		"start": {
-			"column": 5,
+			"line": 14,
-			"line": 14
+			"column": 5
+		},
+		"end": {
+			"line": 14,
+			"column": 10
 		}
 	}
 ]