From 6fe098c6f29d88f62e42a3868eddc8557026524c Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 1 Sep 2024 16:43:59 +0200 Subject: [PATCH 1/4] fix: error on duplicate style and class directive --- .changeset/dirty-jars-tap.md | 5 +++++ .../src/compiler/phases/1-parse/state/element.js | 10 +++++++++- .../samples/attribute-unique-class/_config.js | 9 +++++++++ .../samples/attribute-unique-class/main.svelte | 1 + .../samples/attribute-unique-style/_config.js | 9 +++++++++ .../samples/attribute-unique-style/main.svelte | 1 + 6 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 .changeset/dirty-jars-tap.md create mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-class/_config.js create mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-class/main.svelte create mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-style/_config.js create mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-style/main.svelte diff --git a/.changeset/dirty-jars-tap.md b/.changeset/dirty-jars-tap.md new file mode 100644 index 000000000000..35a2772e48ea --- /dev/null +++ b/.changeset/dirty-jars-tap.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: error on duplicate style and class directive 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 07af4c8d9854..e29e7ca63f93 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/element.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/element.js @@ -189,7 +189,15 @@ export default function element(parser) { let attribute; while ((attribute = read(parser))) { - if (attribute.type === 'Attribute' || attribute.type === 'BindDirective') { + // animate and transition can only be specified once per element so no need + // to check here, use can be used multiple times, same for the on directive + // finally let already has error handling in case of duplicate variable names + if ( + attribute.type === 'Attribute' || + attribute.type === 'BindDirective' || + attribute.type === 'StyleDirective' || + attribute.type === 'ClassDirective' + ) { if (unique_names.includes(attribute.name)) { e.attribute_duplicate(attribute); // is allowed diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-class/_config.js b/packages/svelte/tests/compiler-errors/samples/attribute-unique-class/_config.js new file mode 100644 index 000000000000..1100af993be5 --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/attribute-unique-class/_config.js @@ -0,0 +1,9 @@ +import { test } from '../../test'; + +export default test({ + error: { + code: 'attribute_duplicate', + message: 'Attributes need to be unique', + position: [23, 40] + } +}); diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-class/main.svelte b/packages/svelte/tests/compiler-errors/samples/attribute-unique-class/main.svelte new file mode 100644 index 000000000000..b3a79e704f3e --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/attribute-unique-class/main.svelte @@ -0,0 +1 @@ +
\ No newline at end of file diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-style/_config.js b/packages/svelte/tests/compiler-errors/samples/attribute-unique-style/_config.js new file mode 100644 index 000000000000..17b76e02b486 --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/attribute-unique-style/_config.js @@ -0,0 +1,9 @@ +import { test } from '../../test'; + +export default test({ + error: { + code: 'attribute_duplicate', + message: 'Attributes need to be unique', + position: [23, 42] + } +}); diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-style/main.svelte b/packages/svelte/tests/compiler-errors/samples/attribute-unique-style/main.svelte new file mode 100644 index 000000000000..330392961648 --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/attribute-unique-style/main.svelte @@ -0,0 +1 @@ +
\ No newline at end of file From fa087e9898488f831e70874bed4c42bb51880843 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 1 Sep 2024 17:10:54 +0200 Subject: [PATCH 2/4] fix: allow duplicates if they are different type --- .../compiler/phases/1-parse/state/element.js | 8 +++- .../attribute-unique-allowed/_config.js | 5 ++ .../attribute-unique-allowed/main.svelte | 1 + packages/svelte/tests/compiler-errors/test.ts | 48 +++++++++++-------- 4 files changed, 41 insertions(+), 21 deletions(-) create mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js create mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/main.svelte 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 e29e7ca63f93..a43b90e99963 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/element.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/element.js @@ -198,11 +198,15 @@ export default function element(parser) { attribute.type === 'StyleDirective' || attribute.type === 'ClassDirective' ) { - if (unique_names.includes(attribute.name)) { + // `bind:attribute` and `attribute` are just the same but `class:attribute`, + // `style:attribute` and `attribute` are different and should be allowed together + // so we concatenate the type while normalizing the type for BindDirective + const type = attribute.type === 'BindDirective' ? 'Attribute' : attribute.type; + if (unique_names.includes(type + attribute.name)) { e.attribute_duplicate(attribute); // is allowed } else if (attribute.name !== 'this') { - unique_names.push(attribute.name); + unique_names.push(type + attribute.name); } } diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js b/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js new file mode 100644 index 000000000000..2212f3b5cbcf --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js @@ -0,0 +1,5 @@ +import { test } from '../../test'; + +export default test({ + error: false +}); diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/main.svelte b/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/main.svelte new file mode 100644 index 000000000000..475be6f54600 --- /dev/null +++ b/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/main.svelte @@ -0,0 +1 @@ +
\ No newline at end of file diff --git a/packages/svelte/tests/compiler-errors/test.ts b/packages/svelte/tests/compiler-errors/test.ts index ea696890b4f8..431706ab6954 100644 --- a/packages/svelte/tests/compiler-errors/test.ts +++ b/packages/svelte/tests/compiler-errors/test.ts @@ -5,11 +5,13 @@ import { suite, type BaseTest } from '../suite'; import { read_file } from '../helpers.js'; interface CompilerErrorTest extends BaseTest { - error: { - code: string; - message: string; - position?: [number, number]; - }; + error: + | { + code: string; + message: string; + position?: [number, number]; + } + | false; } const { test, run } = suite((config, cwd) => { @@ -18,7 +20,7 @@ const { test, run } = suite((config, cwd) => { } if (fs.existsSync(`${cwd}/main.svelte`)) { - let caught_error = false; + let caught_error: CompileError | null = null; try { compile(read_file(`${cwd}/main.svelte`), { @@ -27,23 +29,27 @@ const { test, run } = suite((config, cwd) => { } catch (e) { const error = e as CompileError; - caught_error = true; + caught_error = error; - expect(error.code).toBe(config.error.code); - expect(error.message).toBe(config.error.message); + if (config.error) { + expect(error.code).toBe(config.error.code); + expect(error.message).toBe(config.error.message); - if (config.error.position) { - expect(error.position).toEqual(config.error.position); + if (config.error.position) { + expect(error.position).toEqual(config.error.position); + } } } - if (!caught_error) { + if (config.error && caught_error == null) { assert.fail('Expected an error'); + } else if (!config.error && caught_error != null) { + assert.fail(`Unexpected error: ${caught_error.code}`); } } if (fs.existsSync(`${cwd}/main.svelte.js`)) { - let caught_error = false; + let caught_error: CompileError | null = null; try { compileModule(read_file(`${cwd}/main.svelte.js`), { @@ -52,18 +58,22 @@ const { test, run } = suite((config, cwd) => { } catch (e) { const error = e as CompileError; - caught_error = true; + caught_error = error; - expect(error.code).toEqual(config.error.code); - expect(error.message).toEqual(config.error.message); + if (config.error) { + expect(error.code).toEqual(config.error.code); + expect(error.message).toEqual(config.error.message); - if (config.error.position) { - expect(error.position).toEqual(config.error.position); + if (config.error.position) { + expect(error.position).toEqual(config.error.position); + } } } - if (!caught_error) { + if (config.error && caught_error == null) { assert.fail('Expected an error'); + } else if (!config.error && caught_error != null) { + assert.fail(`Unexpected error: ${caught_error.code}`); } } }); From 577502b0ef5dcef8238742d7234daf2e42ba72ea Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 1 Sep 2024 17:15:43 +0200 Subject: [PATCH 3/4] chore: no need for multiple variables --- packages/svelte/tests/compiler-errors/test.ts | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/packages/svelte/tests/compiler-errors/test.ts b/packages/svelte/tests/compiler-errors/test.ts index 431706ab6954..aad2f1293614 100644 --- a/packages/svelte/tests/compiler-errors/test.ts +++ b/packages/svelte/tests/compiler-errors/test.ts @@ -27,16 +27,14 @@ const { test, run } = suite((config, cwd) => { generate: 'client' }); } catch (e) { - const error = e as CompileError; - - caught_error = error; + caught_error = e as CompileError; if (config.error) { - expect(error.code).toBe(config.error.code); - expect(error.message).toBe(config.error.message); + expect(caught_error.code).toBe(config.error.code); + expect(caught_error.message).toBe(config.error.message); if (config.error.position) { - expect(error.position).toEqual(config.error.position); + expect(caught_error.position).toEqual(config.error.position); } } } @@ -56,16 +54,14 @@ const { test, run } = suite((config, cwd) => { generate: 'client' }); } catch (e) { - const error = e as CompileError; - - caught_error = error; + caught_error = e as CompileError; if (config.error) { - expect(error.code).toEqual(config.error.code); - expect(error.message).toEqual(config.error.message); + expect(caught_error.code).toEqual(config.error.code); + expect(caught_error.message).toEqual(config.error.message); if (config.error.position) { - expect(error.position).toEqual(config.error.position); + expect(caught_error.position).toEqual(config.error.position); } } } From 49b97ae32fa881ee16c70f652005f86a27828255 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 1 Sep 2024 21:42:12 +0200 Subject: [PATCH 4/4] chore: revert `test.ts` changes and move test to `validator` --- .../attribute-unique-allowed/_config.js | 5 -- packages/svelte/tests/compiler-errors/test.ts | 52 ++++++++----------- .../attribute-unique-allowed/errors.json | 1 + .../attribute-unique-allowed/input.svelte} | 0 4 files changed, 24 insertions(+), 34 deletions(-) delete mode 100644 packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js create mode 100644 packages/svelte/tests/validator/samples/attribute-unique-allowed/errors.json rename packages/svelte/tests/{compiler-errors/samples/attribute-unique-allowed/main.svelte => validator/samples/attribute-unique-allowed/input.svelte} (100%) diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js b/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js deleted file mode 100644 index 2212f3b5cbcf..000000000000 --- a/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/_config.js +++ /dev/null @@ -1,5 +0,0 @@ -import { test } from '../../test'; - -export default test({ - error: false -}); diff --git a/packages/svelte/tests/compiler-errors/test.ts b/packages/svelte/tests/compiler-errors/test.ts index aad2f1293614..ea696890b4f8 100644 --- a/packages/svelte/tests/compiler-errors/test.ts +++ b/packages/svelte/tests/compiler-errors/test.ts @@ -5,13 +5,11 @@ import { suite, type BaseTest } from '../suite'; import { read_file } from '../helpers.js'; interface CompilerErrorTest extends BaseTest { - error: - | { - code: string; - message: string; - position?: [number, number]; - } - | false; + error: { + code: string; + message: string; + position?: [number, number]; + }; } const { test, run } = suite((config, cwd) => { @@ -20,56 +18,52 @@ const { test, run } = suite((config, cwd) => { } if (fs.existsSync(`${cwd}/main.svelte`)) { - let caught_error: CompileError | null = null; + let caught_error = false; try { compile(read_file(`${cwd}/main.svelte`), { generate: 'client' }); } catch (e) { - caught_error = e as CompileError; + const error = e as CompileError; - if (config.error) { - expect(caught_error.code).toBe(config.error.code); - expect(caught_error.message).toBe(config.error.message); + caught_error = true; - if (config.error.position) { - expect(caught_error.position).toEqual(config.error.position); - } + expect(error.code).toBe(config.error.code); + expect(error.message).toBe(config.error.message); + + if (config.error.position) { + expect(error.position).toEqual(config.error.position); } } - if (config.error && caught_error == null) { + if (!caught_error) { assert.fail('Expected an error'); - } else if (!config.error && caught_error != null) { - assert.fail(`Unexpected error: ${caught_error.code}`); } } if (fs.existsSync(`${cwd}/main.svelte.js`)) { - let caught_error: CompileError | null = null; + let caught_error = false; try { compileModule(read_file(`${cwd}/main.svelte.js`), { generate: 'client' }); } catch (e) { - caught_error = e as CompileError; + const error = e as CompileError; + + caught_error = true; - if (config.error) { - expect(caught_error.code).toEqual(config.error.code); - expect(caught_error.message).toEqual(config.error.message); + expect(error.code).toEqual(config.error.code); + expect(error.message).toEqual(config.error.message); - if (config.error.position) { - expect(caught_error.position).toEqual(config.error.position); - } + if (config.error.position) { + expect(error.position).toEqual(config.error.position); } } - if (config.error && caught_error == null) { + if (!caught_error) { assert.fail('Expected an error'); - } else if (!config.error && caught_error != null) { - assert.fail(`Unexpected error: ${caught_error.code}`); } } }); diff --git a/packages/svelte/tests/validator/samples/attribute-unique-allowed/errors.json b/packages/svelte/tests/validator/samples/attribute-unique-allowed/errors.json new file mode 100644 index 000000000000..fe51488c7066 --- /dev/null +++ b/packages/svelte/tests/validator/samples/attribute-unique-allowed/errors.json @@ -0,0 +1 @@ +[] diff --git a/packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/main.svelte b/packages/svelte/tests/validator/samples/attribute-unique-allowed/input.svelte similarity index 100% rename from packages/svelte/tests/compiler-errors/samples/attribute-unique-allowed/main.svelte rename to packages/svelte/tests/validator/samples/attribute-unique-allowed/input.svelte