From 5405ec6c91911f66f71a05b4fe79aef077896d9e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 23 Apr 2024 13:34:06 -0400 Subject: [PATCH] chore: refactor logic for ignoring warnings (#11300) * reimplement svelte-ignore logic * simplify * remove unused code * remove ignores array from comment node * unused * regenerate types * types * oops, again --- packages/svelte/src/compiler/legacy.js | 9 +- .../compiler/phases/1-parse/state/element.js | 5 +- .../src/compiler/phases/2-analyze/a11y.js | 10 +-- .../compiler/phases/2-analyze/css/css-warn.js | 2 +- .../src/compiler/phases/2-analyze/index.js | 88 ++++++++++++++++--- .../src/compiler/phases/2-analyze/types.d.ts | 1 + .../compiler/phases/2-analyze/validation.js | 47 ++++------ .../src/compiler/types/legacy-nodes.d.ts | 9 ++ .../svelte/src/compiler/types/template.d.ts | 2 - .../compiler/utils/extract_svelte_ignore.js | 38 -------- packages/svelte/src/compiler/warnings.js | 50 +---------- .../samples/comment-before-script/output.json | 3 +- packages/svelte/types/index.d.ts | 11 ++- 13 files changed, 132 insertions(+), 143 deletions(-) diff --git a/packages/svelte/src/compiler/legacy.js b/packages/svelte/src/compiler/legacy.js index 49d2f78260..0357953060 100644 --- a/packages/svelte/src/compiler/legacy.js +++ b/packages/svelte/src/compiler/legacy.js @@ -4,6 +4,7 @@ import { regex_not_whitespace, regex_starts_with_whitespaces } from './phases/patterns.js'; +import { extract_svelte_ignore } from './utils/extract_svelte_ignore.js'; /** * Some of the legacy Svelte AST nodes remove whitespace from the start and end of their children. @@ -197,7 +198,13 @@ export function convert(source, ast) { ClassDirective(node) { return { ...node, type: 'Class' }; }, - ComplexSelector(node, { visit }) { + Comment(node) { + return { + ...node, + ignores: extract_svelte_ignore(node.data) + }; + }, + ComplexSelector(node) { const children = []; for (const child of node.children) { diff --git a/packages/svelte/src/compiler/phases/1-parse/state/element.js b/packages/svelte/src/compiler/phases/1-parse/state/element.js index 134f322967..7696b23a66 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/element.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/element.js @@ -1,5 +1,3 @@ -import { extract_svelte_ignore } from '../../../utils/extract_svelte_ignore.js'; -import fuzzymatch from '../utils/fuzzymatch.js'; import { is_void } from '../utils/names.js'; import read_expression from '../read/expression.js'; import { read_script } from '../read/script.js'; @@ -87,8 +85,7 @@ export default function tag(parser) { type: 'Comment', start, end: parser.index, - data, - ignores: extract_svelte_ignore(data) + data }); return; diff --git a/packages/svelte/src/compiler/phases/2-analyze/a11y.js b/packages/svelte/src/compiler/phases/2-analyze/a11y.js index eb87b5ddbf..7d01c6fdb3 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/a11y.js +++ b/packages/svelte/src/compiler/phases/2-analyze/a11y.js @@ -667,9 +667,8 @@ function get_static_text_value(attribute) { /** * @param {import('#compiler').RegularElement | import('#compiler').SvelteElement} node * @param {import('./types.js').AnalysisState} state - * @param {import('#compiler').SvelteNode[]} path */ -function check_element(node, state, path) { +function check_element(node, state) { // foreign namespace means elements can have completely different meanings, therefore we don't check them if (state.options.namespace === 'foreign') return; @@ -680,8 +679,7 @@ function check_element(node, state, path) { * @param {Parameters} args * @returns {void} */ - const push_warning = (node, code, ...args) => - warn(state.analysis.warnings, node, path, code, ...args); + const push_warning = (node, code, ...args) => warn(state.analysis.warnings, node, code, ...args); /** @type {Map} */ const attribute_map = new Map(); @@ -1165,9 +1163,9 @@ function check_element(node, state, path) { */ export const a11y_validators = { RegularElement(node, context) { - check_element(node, context.state, context.path); + check_element(node, context.state); }, SvelteElement(node, context) { - check_element(node, context.state, context.path); + check_element(node, context.state); } }; diff --git a/packages/svelte/src/compiler/phases/2-analyze/css/css-warn.js b/packages/svelte/src/compiler/phases/2-analyze/css/css-warn.js index 3aa2382205..e2d137198b 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/css/css-warn.js +++ b/packages/svelte/src/compiler/phases/2-analyze/css/css-warn.js @@ -26,7 +26,7 @@ const visitors = { if (!node.metadata.used) { const content = context.state.stylesheet.content; const text = content.styles.substring(node.start - content.start, node.end - content.start); - warn(context.state.warnings, node, context.path, 'css-unused-selector', text); + warn(context.state.warnings, node, 'css-unused-selector', text); } context.next(); diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 10091a7105..d2a331e253 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -24,6 +24,7 @@ import { analyze_css } from './css/css-analyze.js'; 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'; /** * @param {import('#compiler').Script | null} script @@ -330,7 +331,7 @@ export function analyze_component(root, source, options) { } else if (declaration !== null && Runes.includes(/** @type {any} */ (name))) { for (const { node, path } of references) { if (path.at(-1)?.type === 'CallExpression') { - warn(warnings, node, [], 'store-with-rune-name', store_name); + warn(warnings, node, 'store-with-rune-name', store_name); } } } @@ -407,7 +408,7 @@ export function analyze_component(root, source, options) { }; if (!options.customElement && root.options?.customElement) { - warn(analysis.warnings, root.options, [], 'missing-custom-element-compile-option'); + warn(analysis.warnings, root.options, 'missing-custom-element-compile-option'); } if (analysis.runes) { @@ -433,7 +434,8 @@ 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( @@ -475,7 +477,8 @@ 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( @@ -495,7 +498,7 @@ export function analyze_component(root, source, options) { (r) => r.node !== binding.node && r.path.at(-1)?.type !== 'ExportSpecifier' ); if (!references.length && !instance.scope.declarations.has(`$${name}`)) { - warn(warnings, binding.node, [], 'unused-export-let', name); + warn(warnings, binding.node, 'unused-export-let', name); } } } @@ -535,7 +538,7 @@ export function analyze_component(root, source, options) { type === 'AwaitBlock' || type === 'KeyBlock' ) { - warn(warnings, binding.node, [], 'non-state-reference', name); + warn(warnings, binding.node, 'non-state-reference', name); continue outer; } } @@ -543,7 +546,7 @@ export function analyze_component(root, source, options) { } } - warn(warnings, binding.node, [], 'non-state-reference', name); + warn(warnings, binding.node, 'non-state-reference', name); continue outer; } } @@ -557,7 +560,13 @@ export function analyze_component(root, source, options) { for (const element of analysis.elements) { prune(analysis.css.ast, element); } - warn_unused(analysis.css.ast, analysis.warnings); + + if ( + !analysis.css.ast.content.comment || + !extract_svelte_ignore(analysis.css.ast.content.comment.data).includes('css-unused-selector') + ) { + warn_unused(analysis.css.ast, analysis.warnings); + } outer: for (const element of analysis.elements) { if (element.metadata.scoped) { @@ -679,7 +688,7 @@ const legacy_scope_tweaker = { (d) => d.scope === state.analysis.module.scope && d.declaration_kind !== 'const' ) ) { - warn(state.analysis.warnings, node, path, 'module-script-reactive-declaration'); + warn(state.analysis.warnings, node, 'module-script-reactive-declaration'); } if ( @@ -1045,6 +1054,65 @@ const function_visitor = (node, context) => { /** @type {import('./types').Visitors} */ const common_visitors = { + _(node, context) { + // @ts-expect-error + const comments = /** @type {import('estree').Comment[]} */ (node.leadingComments); + + if (comments) { + /** @type {string[]} */ + const ignores = []; + + for (const comment of comments) { + ignores.push(...extract_svelte_ignore(comment.value)); + } + + if (ignores.length > 0) { + // @ts-expect-error see below + node.ignores = new Set([...context.state.ignores, ...ignores]); + } + } + + // @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) { + // @ts-expect-error + node.ignores = context.state.ignores; + } + }, + Fragment(node, context) { + /** @type {string[]} */ + let ignores = []; + + for (const child of node.nodes) { + if (child.type === 'Text' && child.data.trim() === '') { + continue; + } + + 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; + } + + ignores = []; + } + } + }, Attribute(node, context) { if (node.value === true) return; @@ -1142,7 +1210,7 @@ const common_visitors = { binding.kind === 'derived') && context.state.function_depth === binding.scope.function_depth ) { - warn(context.state.analysis.warnings, node, context.path, 'static-state-reference'); + warn(context.state.analysis.warnings, node, 'static-state-reference'); } } }, 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 d2c503e8b1..cc815fae7b 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/types.d.ts +++ b/packages/svelte/src/compiler/phases/2-analyze/types.d.ts @@ -22,6 +22,7 @@ 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/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index cd1aed4b2d..f4321b0101 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -134,7 +134,6 @@ function validate_element(node, context) { warn( context.state.analysis.warnings, attribute, - context.path, 'global-event-reference', attribute.name ); @@ -147,7 +146,7 @@ function validate_element(node, context) { } if (attribute.name === 'is' && context.state.options.namespace !== 'foreign') { - warn(context.state.analysis.warnings, attribute, context.path, 'avoid-is'); + warn(context.state.analysis.warnings, attribute, 'avoid-is'); } const correct_name = react_attributes.get(attribute.name); @@ -155,7 +154,6 @@ function validate_element(node, context) { warn( context.state.analysis.warnings, attribute, - context.path, 'invalid-html-attribute', attribute.name, correct_name @@ -235,7 +233,7 @@ function validate_attribute_name(attribute, context) { !attribute.name.startsWith('xlink:') && !attribute.name.startsWith('xml:') ) { - warn(context.state.analysis.warnings, attribute, context.path, 'illegal-attribute-character'); + warn(context.state.analysis.warnings, attribute, 'illegal-attribute-character'); } } @@ -313,7 +311,7 @@ function validate_block_not_empty(node, context) { // Assumption: If the block has zero elements, someone's in the middle of typing it out, // so don't warn in that case because it would be distracting. if (node.nodes.length === 1 && node.nodes[0].type === 'Text' && !node.nodes[0].raw.trim()) { - warn(context.state.analysis.warnings, node.nodes[0], context.path, 'empty-block'); + warn(context.state.analysis.warnings, node.nodes[0], 'empty-block'); } } @@ -377,7 +375,6 @@ const validation = { warn( context.state.analysis.warnings, binding.node, - context.path, 'invalid-rest-eachblock-binding', binding.node.name ); @@ -534,13 +531,7 @@ const validation = { binding.declaration_kind === 'import' && binding.references.length === 0 ) { - warn( - context.state.analysis.warnings, - node, - context.path, - 'component-name-lowercase', - node.name - ); + warn(context.state.analysis.warnings, node, 'component-name-lowercase', node.name); } validate_element(node, context); @@ -579,13 +570,7 @@ const validation = { !VoidElements.includes(node.name) && !SVGElements.includes(node.name) ) { - warn( - context.state.analysis.warnings, - node, - context.path, - 'invalid-self-closing-tag', - node.name - ); + warn(context.state.analysis.warnings, node, 'invalid-self-closing-tag', node.name); } context.next({ @@ -781,7 +766,7 @@ export const validation_legacy = merge(validation, a11y_validators, { (state.ast_type !== 'instance' || /** @type {import('#compiler').SvelteNode} */ (path.at(-1)).type !== 'Program') ) { - warn(state.analysis.warnings, node, path, 'no-reactive-declaration'); + warn(state.analysis.warnings, node, 'no-reactive-declaration'); } }, UpdateExpression(node, { state }) { @@ -984,12 +969,12 @@ export const validation_runes_js = { const allowed_depth = context.state.ast_type === 'module' ? 0 : 1; if (context.state.scope.function_depth > allowed_depth) { - warn(context.state.analysis.warnings, node, context.path, 'avoid-nested-class'); + warn(context.state.analysis.warnings, node, 'avoid-nested-class'); } }, NewExpression(node, context) { if (node.callee.type === 'ClassExpression' && context.state.scope.function_depth > 0) { - warn(context.state.analysis.warnings, node, context.path, 'avoid-inline-class'); + warn(context.state.analysis.warnings, node, 'avoid-inline-class'); } } }; @@ -1134,7 +1119,7 @@ export const validation_runes = merge(validation, a11y_validators, { } next({ ...state }); }, - VariableDeclarator(node, { state, path }) { + VariableDeclarator(node, { state }) { ensure_no_module_import_conflict(node, state); const init = node.init; @@ -1142,7 +1127,7 @@ export const validation_runes = merge(validation, a11y_validators, { if (rune === null) { if (init?.type === 'Identifier' && init.name === '$props' && !state.scope.get('props')) { - warn(state.analysis.warnings, node, path, 'invalid-props-declaration'); + warn(state.analysis.warnings, node, 'invalid-props-declaration'); } return; } @@ -1195,29 +1180,29 @@ export const validation_runes = merge(validation, a11y_validators, { arg.type === 'CallExpression' && (arg.callee.type === 'ArrowFunctionExpression' || arg.callee.type === 'FunctionExpression') ) { - warn(state.analysis.warnings, node, path, 'derived-iife'); + warn(state.analysis.warnings, node, 'derived-iife'); } } }, - AssignmentPattern(node, { state, path }) { + AssignmentPattern(node, { state }) { if ( node.right.type === 'Identifier' && node.right.name === '$bindable' && !state.scope.get('bindable') ) { - warn(state.analysis.warnings, node, path, 'invalid-bindable-declaration'); + warn(state.analysis.warnings, node, 'invalid-bindable-declaration'); } }, - SlotElement(node, { state, path }) { + SlotElement(node, { state }) { if (!state.analysis.custom_element) { - warn(state.analysis.warnings, node, path, 'deprecated-slot-element'); + warn(state.analysis.warnings, node, 'deprecated-slot-element'); } }, OnDirective(node, { state, path }) { const parent_type = path.at(-1)?.type; // Don't warn on component events; these might not be under the author's control so the warning would be unactionable if (parent_type === 'RegularElement' || parent_type === 'SvelteElement') { - warn(state.analysis.warnings, node, path, 'deprecated-event-handler', node.name); + warn(state.analysis.warnings, node, 'deprecated-event-handler', node.name); } }, // TODO this is a code smell. need to refactor this stuff diff --git a/packages/svelte/src/compiler/types/legacy-nodes.d.ts b/packages/svelte/src/compiler/types/legacy-nodes.d.ts index 45b26aa70f..0ba61c053c 100644 --- a/packages/svelte/src/compiler/types/legacy-nodes.d.ts +++ b/packages/svelte/src/compiler/types/legacy-nodes.d.ts @@ -198,6 +198,14 @@ export interface LegacyWindow extends BaseElement { type: 'Window'; } +export interface LegacyComment extends BaseNode { + type: 'Comment'; + /** the contents of the comment */ + data: string; + /** any svelte-ignore directives — would result in ['a', 'b', 'c'] */ + ignores: string[]; +} + type LegacyDirective = | LegacyAnimation | LegacyBinding @@ -213,6 +221,7 @@ export type LegacyAttributeLike = LegacyAttribute | LegacySpread | LegacyDirecti export type LegacyElementLike = | LegacyBody | LegacyCatchBlock + | LegacyComment | LegacyDocument | LegacyElement | LegacyHead diff --git a/packages/svelte/src/compiler/types/template.d.ts b/packages/svelte/src/compiler/types/template.d.ts index 4ddee3096f..e8f20c9b33 100644 --- a/packages/svelte/src/compiler/types/template.d.ts +++ b/packages/svelte/src/compiler/types/template.d.ts @@ -131,8 +131,6 @@ export interface Comment extends BaseNode { type: 'Comment'; /** the contents of the comment */ data: string; - /** any svelte-ignore directives — would result in ['a', 'b', 'c'] */ - ignores: string[]; } /** A `{@const ...}` tag */ diff --git a/packages/svelte/src/compiler/utils/extract_svelte_ignore.js b/packages/svelte/src/compiler/utils/extract_svelte_ignore.js index e45ff53b03..bdc8c15851 100644 --- a/packages/svelte/src/compiler/utils/extract_svelte_ignore.js +++ b/packages/svelte/src/compiler/utils/extract_svelte_ignore.js @@ -15,41 +15,3 @@ export function extract_svelte_ignore(text) { .filter(Boolean) : []; } - -/** - * @template {{ leadingComments?: Array<{ value: string }> }} Node - * @param {Node} node - * @returns {string[]} - */ -export function extract_svelte_ignore_from_comments(node) { - return (node.leadingComments || []).flatMap( - /** @param {any} comment */ (comment) => extract_svelte_ignore(comment.value) - ); -} - -/** - * @param {import('#compiler').TemplateNode} node - * @param {import('#compiler').TemplateNode[]} template_nodes - * @returns {string[]} - */ -export function extract_ignores_above_position(node, template_nodes) { - const previous_node_idx = template_nodes.indexOf(node) - 1; - if (previous_node_idx < 0) { - return []; - } - - const ignores = []; - for (let i = previous_node_idx; i >= 0; i--) { - const node = template_nodes[i]; - if (node.type !== 'Comment' && node.type !== 'Text') { - return ignores; - } - if (node.type === 'Comment') { - if (node.ignores.length) { - ignores.push(...node.ignores); - } - } - } - - return ignores; -} diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js index fc5897aa93..3983d3686e 100644 --- a/packages/svelte/src/compiler/warnings.js +++ b/packages/svelte/src/compiler/warnings.js @@ -1,8 +1,3 @@ -import { - extract_ignores_above_position, - extract_svelte_ignore_from_comments -} from './utils/extract_svelte_ignore.js'; - /** @typedef {Record string>} Warnings */ /** @satisfies {Warnings} */ @@ -277,52 +272,15 @@ const warnings = { * @template {keyof AllWarnings} T * @param {import('./phases/types').RawWarning[]} array the array to push the warning to, if not ignored * @param {{ start?: number, end?: number, type?: string, parent?: import('#compiler').SvelteNode | null, leadingComments?: import('estree').Comment[] } | null} node the node related to the warning - * @param {import('#compiler').SvelteNode[]} path the path to the node, so that we can traverse upwards to find svelte-ignore comments * @param {T} code the warning code * @param {Parameters} args the arguments to pass to the warning function * @returns {void} */ -export function warn(array, node, path, code, ...args) { - const fn = warnings[code]; +export function warn(array, node, code, ...args) { + // @ts-expect-error + if (node.ignores?.has(code)) return; - // Traverse the AST upwards to find any svelte-ignore comments. - // This assumes that people don't have their code littered with warnings, - // at which point this might become inefficient. - /** @type {string[]} */ - const ignores = []; - - if (node) { - // comments inside JavaScript (estree) - if ('leadingComments' in node) { - ignores.push(...extract_svelte_ignore_from_comments(node)); - } - } - - for (let i = path.length - 1; i >= 0; i--) { - const current = path[i]; - - // comments inside JavaScript (estree) - if ('leadingComments' in current) { - ignores.push(...extract_svelte_ignore_from_comments(current)); - } - - // Svelte nodes - if (current.type === 'Fragment') { - ignores.push( - ...extract_ignores_above_position( - /** @type {import('#compiler').TemplateNode} */ (path[i + 1] ?? node), - current.nodes - ) - ); - } - - // Style nodes - if (current.type === 'StyleSheet' && current.content.comment) { - ignores.push(...current.content.comment.ignores); - } - } - - if (ignores.includes(code)) return; + const fn = warnings[code]; const start = node?.start; const end = node?.end; diff --git a/packages/svelte/tests/parser-modern/samples/comment-before-script/output.json b/packages/svelte/tests/parser-modern/samples/comment-before-script/output.json index e7e5dfe244..b0305d709b 100644 --- a/packages/svelte/tests/parser-modern/samples/comment-before-script/output.json +++ b/packages/svelte/tests/parser-modern/samples/comment-before-script/output.json @@ -11,8 +11,7 @@ "type": "Comment", "start": 0, "end": 27, - "data": "should not error out", - "ignores": [] + "data": "should not error out" }, { "type": "Text", diff --git a/packages/svelte/types/index.d.ts b/packages/svelte/types/index.d.ts index 633a910991..5563610b94 100644 --- a/packages/svelte/types/index.d.ts +++ b/packages/svelte/types/index.d.ts @@ -955,6 +955,14 @@ declare module 'svelte/compiler' { type: 'Window'; } + interface LegacyComment extends BaseNode_1 { + type: 'Comment'; + /** the contents of the comment */ + data: string; + /** any svelte-ignore directives — would result in ['a', 'b', 'c'] */ + ignores: string[]; + } + type LegacyDirective = | LegacyAnimation | LegacyBinding @@ -970,6 +978,7 @@ declare module 'svelte/compiler' { type LegacyElementLike = | LegacyBody | LegacyCatchBlock + | LegacyComment | LegacyDocument | LegacyElement | LegacyHead @@ -1355,8 +1364,6 @@ declare module 'svelte/compiler' { type: 'Comment'; /** the contents of the comment */ data: string; - /** any svelte-ignore directives — would result in ['a', 'b', 'c'] */ - ignores: string[]; } /** A `{@const ...}` tag */