From 6d1fbf78e1b3484f30ab6794bc2e00f24a7c1457 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Wed, 8 May 2024 21:12:14 +0200 Subject: [PATCH] backwards compat in legacy mode, no backwards compat in runes mode. Also adjusted the approach to ignores. --- .../templates/compile-warnings.js | 73 ++++++++++++++--- packages/svelte/src/compiler/index.js | 10 +-- .../src/compiler/phases/2-analyze/a11y.js | 6 -- .../src/compiler/phases/2-analyze/index.js | 76 +++++++----------- .../src/compiler/phases/2-analyze/types.d.ts | 1 - packages/svelte/src/compiler/warnings.js | 79 ++++++++++++++++--- .../input.svelte | 6 +- .../ignore-warning-dash-runes/input.svelte | 9 +++ .../ignore-warning-dash-runes/warnings.json | 26 ++++++ 9 files changed, 200 insertions(+), 86 deletions(-) create mode 100644 packages/svelte/tests/validator/samples/ignore-warning-dash-runes/input.svelte create mode 100644 packages/svelte/tests/validator/samples/ignore-warning-dash-runes/warnings.json diff --git a/packages/svelte/scripts/process-messages/templates/compile-warnings.js b/packages/svelte/scripts/process-messages/templates/compile-warnings.js index 53eaeb4321..3c9c8d13fe 100644 --- a/packages/svelte/scripts/process-messages/templates/compile-warnings.js +++ b/packages/svelte/scripts/process-messages/templates/compile-warnings.js @@ -2,9 +2,12 @@ import { getLocator } from 'locate-character'; /** @typedef {{ start?: number, end?: number }} NodeLike */ -/** @type {import('#compiler').Warning[]} */ +/** @type {Array<{ warning: import('#compiler').Warning; legacy_code: string | null }>} */ let warnings = []; +/** @type {Set[]} */ +let ignore_stack = []; + /** @type {string | undefined} */ let filename; @@ -15,33 +18,79 @@ let locator = getLocator('', { offsetLine: 1 }); * source: string; * filename: string | undefined; * }} options - * @returns {import('#compiler').Warning[]} */ export function reset_warnings(options) { filename = options.filename; + ignore_stack = []; + warnings = []; locator = getLocator(options.source, { offsetLine: 1 }); +} - return (warnings = []); +/** + * @param {boolean} is_runes + */ +export function get_warnings(is_runes) { + /** @type {import('#compiler').Warning[]} */ + const final = []; + for (const { warning, legacy_code } of warnings) { + if (legacy_code) { + if (is_runes) { + final.push({ + ...warning, + message: + (warning.message += ` (this warning was tried to silence using code ${legacy_code}. In runes mode, use the new code ${warning.code} instead)`) + }); + } + } else { + final.push(warning); + } + } + + return final; } +/** + * @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(); +} + +// TODO add more mappings for prominent codes that have been renamed +const legacy_codes = new Map([ + ['reactive_declaration_invalid_placement', 'non-top-level-reactive-declaration'] +]); + /** * @param {null | NodeLike} node * @param {string} code * @param {string} message */ function w(node, code, message) { - // @ts-expect-error - if (node?.ignores?.has(code)) return; + const ignores = ignore_stack.at(-1); + if (ignores?.has(code)) return; + // backwards compat: Svelte 5 changed all warnings from dash to underscore - // @ts-expect-error - if (node?.ignores?.has(code.replaceAll('_', '-'))) return; + /** @type {string | null} */ + let legacy_code = legacy_codes.get(code) || code.replaceAll('_', '-'); + if (!ignores?.has(legacy_code)) { + legacy_code = null; + } warnings.push({ - code, - message, - filename, - start: node?.start !== undefined ? locator(node.start) : undefined, - end: node?.end !== undefined ? locator(node.end) : undefined + legacy_code, + warning: { + code, + message, + filename, + start: node?.start !== undefined ? locator(node.start) : undefined, + end: node?.end !== undefined ? locator(node.end) : undefined + } }); } diff --git a/packages/svelte/src/compiler/index.js b/packages/svelte/src/compiler/index.js index ba312a1a63..4a10b84d05 100644 --- a/packages/svelte/src/compiler/index.js +++ b/packages/svelte/src/compiler/index.js @@ -8,7 +8,7 @@ import { remove_typescript_nodes } from './phases/1-parse/remove_typescript_node import { analyze_component, analyze_module } from './phases/2-analyze/index.js'; import { transform_component, transform_module } from './phases/3-transform/index.js'; import { validate_component_options, validate_module_options } from './validate-options.js'; -import { reset_warnings } from './warnings.js'; +import { get_warnings, reset_warnings } from './warnings.js'; export { default as preprocess } from './preprocess/index.js'; /** @@ -21,7 +21,7 @@ export { default as preprocess } from './preprocess/index.js'; */ export function compile(source, options) { try { - const warnings = reset_warnings({ source, filename: options.filename }); + reset_warnings({ source, filename: options.filename }); const validated = validate_component_options(options, ''); let parsed = _parse(source); @@ -46,7 +46,7 @@ export function compile(source, options) { const analysis = analyze_component(parsed, source, combined_options); const result = transform_component(analysis, source, combined_options); - result.warnings = warnings; + result.warnings = get_warnings(analysis.runes); result.ast = to_public_ast(source, parsed, options.modernAst); return result; } catch (e) { @@ -68,11 +68,11 @@ export function compile(source, options) { */ export function compileModule(source, options) { try { - const warnings = reset_warnings({ source, filename: options.filename }); + reset_warnings({ source, filename: options.filename }); const validated = validate_module_options(options, ''); const analysis = analyze_module(parse_acorn(source, false), validated); const result = transform_module(analysis, source, validated); - result.warnings = warnings; + result.warnings = get_warnings(true); return result; } catch (e) { if (e instanceof CompileError) { diff --git a/packages/svelte/src/compiler/phases/2-analyze/a11y.js b/packages/svelte/src/compiler/phases/2-analyze/a11y.js index 5e6e05ab01..a45c3c1f97 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/a11y.js +++ b/packages/svelte/src/compiler/phases/2-analyze/a11y.js @@ -702,12 +702,6 @@ function check_element(node, state) { let has_contenteditable_binding = false; for (const attribute of node.attributes) { - // gross but necessary: the visitor that sets this isn't called in time - if (state.ignores.size > 0) { - // @ts-expect-error - attribute.ignores = state.ignores; - } - if (attribute.type === 'SpreadAttribute') { has_spread = true; } else if (attribute.type === 'OnDirective') { diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index e4d6f6a4db..306c740f7b 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -439,8 +439,7 @@ export function analyze_component(root, source, options) { component_slots: new Set(), expression: null, private_derived_state: [], - function_depth: scope.function_depth, - ignores: new Set() + function_depth: scope.function_depth }; walk( @@ -511,8 +510,7 @@ export function analyze_component(root, source, options) { component_slots: new Set(), expression: null, private_derived_state: [], - function_depth: scope.function_depth, - ignores: new Set() + function_depth: scope.function_depth }; walk( @@ -1093,62 +1091,44 @@ function is_safe_identifier(expression, scope) { /** @type {import('./types').Visitors} */ const common_visitors = { - _(node, context) { - // @ts-expect-error - const comments = /** @type {import('estree').Comment[]} */ (node.leadingComments); - - if (comments) { + _(node, { next, path }) { + const parent = path.at(-1); + if (parent?.type === 'Fragment') { + const idx = parent.nodes.indexOf(/** @type {any} */ (node)); /** @type {string[]} */ const ignores = []; - for (const comment of comments) { - ignores.push(...extract_svelte_ignore(comment.value)); + for (let i = idx - 1; i >= 0; i--) { + const prev = parent.nodes[i]; + if (prev.type === 'Comment') { + ignores.push(...extract_svelte_ignore(prev.data)); + } else if (prev.type !== 'Text') { + break; + } } if (ignores.length > 0) { - // @ts-expect-error see below - node.ignores = new Set([...context.state.ignores, ...ignores]); + w.push_ignore(ignores); + next(); + w.pop_ignore(); } - } - - // @ts-expect-error - if (node.ignores) { - context.next({ - ...context.state, - // @ts-expect-error see below - ignores: node.ignores - }); - } else if (context.state.ignores.size > 0) { + } else { // @ts-expect-error - node.ignores = context.state.ignores; - } - }, - Fragment(node, context) { - /** @type {string[]} */ - let ignores = []; + const comments = /** @type {import('estree').Comment[]} */ (node.leadingComments); - for (const child of node.nodes) { - if (child.type === 'Text' && child.data.trim() === '') { - continue; - } + if (comments) { + /** @type {string[]} */ + const ignores = []; - if (child.type === 'Comment') { - ignores.push(...extract_svelte_ignore(child.data)); - } 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; + for (const comment of comments) { + ignores.push(...extract_svelte_ignore(comment.value)); } - ignores = []; + if (ignores.length > 0) { + w.push_ignore(ignores); + next(); + w.pop_ignore(); + } } } }, diff --git a/packages/svelte/src/compiler/phases/2-analyze/types.d.ts b/packages/svelte/src/compiler/phases/2-analyze/types.d.ts index cc815fae7b..d2c503e8b1 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/types.d.ts +++ b/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; } export interface LegacyAnalysisState extends AnalysisState { diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js index 8cd855af85..5d43f466dc 100644 --- a/packages/svelte/src/compiler/warnings.js +++ b/packages/svelte/src/compiler/warnings.js @@ -3,8 +3,10 @@ import { getLocator } from 'locate-character'; /** @typedef {{ start?: number, end?: number }} NodeLike */ -/** @type {import('#compiler').Warning[]} */ +/** @type {Array<{ warning: import('#compiler').Warning; legacy_code: string | null }>} */ let warnings = []; +/** @type {Set[]} */ +let ignore_stack = []; /** @type {string | undefined} */ let filename; let locator = getLocator('', { offsetLine: 1 }); @@ -14,32 +16,85 @@ let locator = getLocator('', { offsetLine: 1 }); * source: string; * filename: string | undefined; * }} options - * @returns {import('#compiler').Warning[]} */ export function reset_warnings(options) { filename = options.filename; + ignore_stack = []; + warnings = []; locator = getLocator(options.source, { offsetLine: 1 }); - return warnings = []; } +/** + * @param {boolean} is_runes + */ +export function get_warnings(is_runes) { + /** @type {import('#compiler').Warning[]} */ + const final = []; + + for (const { warning, legacy_code } of warnings) { + if (legacy_code) { + if (is_runes) { + final.push({ + ...warning, + message: warning.message += ` (this warning was tried to silence using code ${legacy_code}. In runes mode, use the new code ${warning.code} instead)` + }); + } + } else { + final.push(warning); + } + } + + return final; +} + +/** + * @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(); +} + +// TODO add more mappings for prominent codes that have been renamed +const legacy_codes = new Map([ + [ + 'reactive_declaration_invalid_placement', + 'non-top-level-reactive-declaration' + ] +]); + /** * @param {null | NodeLike} node * @param {string} code * @param {string} message */ function w(node, code, message) { - // @ts-expect-error - if (node?.ignores?.has(code)) return; + const ignores = ignore_stack.at(-1); + + if (ignores?.has(code)) return; + // backwards compat: Svelte 5 changed all warnings from dash to underscore - // @ts-expect-error - if (node?.ignores?.has(code.replaceAll('_', '-'))) return; + /** @type {string | null} */ + let legacy_code = legacy_codes.get(code) || code.replaceAll('_', '-'); + + if (!ignores?.has(legacy_code)) { + legacy_code = null; + } warnings.push({ - code, - message, - filename, - start: node?.start !== undefined ? locator(node.start) : undefined, - end: node?.end !== undefined ? locator(node.end) : undefined + legacy_code, + warning: { + code, + message, + filename, + start: node?.start !== undefined ? locator(node.start) : undefined, + end: node?.end !== undefined ? locator(node.end) : undefined + } }); } diff --git a/packages/svelte/tests/validator/samples/ignore-warning-dash-backwards-compat/input.svelte b/packages/svelte/tests/validator/samples/ignore-warning-dash-backwards-compat/input.svelte index 9a25e77ac1..faae4636e5 100644 --- a/packages/svelte/tests/validator/samples/ignore-warning-dash-backwards-compat/input.svelte +++ b/packages/svelte/tests/validator/samples/ignore-warning-dash-backwards-compat/input.svelte @@ -1,6 +1,8 @@ diff --git a/packages/svelte/tests/validator/samples/ignore-warning-dash-runes/input.svelte b/packages/svelte/tests/validator/samples/ignore-warning-dash-runes/input.svelte new file mode 100644 index 0000000000..268bb2e867 --- /dev/null +++ b/packages/svelte/tests/validator/samples/ignore-warning-dash-runes/input.svelte @@ -0,0 +1,9 @@ + + + +
+ +
+ + +
diff --git a/packages/svelte/tests/validator/samples/ignore-warning-dash-runes/warnings.json b/packages/svelte/tests/validator/samples/ignore-warning-dash-runes/warnings.json new file mode 100644 index 0000000000..8bb98aff0e --- /dev/null +++ b/packages/svelte/tests/validator/samples/ignore-warning-dash-runes/warnings.json @@ -0,0 +1,26 @@ +[ + { + "code": "a11y_missing_attribute", + "end": { + "column": 29, + "line": 5 + }, + "message": "`` element should have an alt attribute (this warning was tried to silence using code a11y-missing-attribute. In runes mode, use the new code a11y_missing_attribute instead)", + "start": { + "column": 1, + "line": 5 + } + }, + { + "code": "a11y_misplaced_scope", + "end": { + "column": 10, + "line": 9 + }, + "message": "The scope attribute should only be used with `` elements (this warning was tried to silence using code a11y-misplaced-scope. In runes mode, use the new code a11y_misplaced_scope instead)", + "start": { + "column": 5, + "line": 9 + } + } +]