Skip to content

fix: stack-trace-based readonly validation #10464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 51 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
f0d3740
fix: remove readonly validation
dummdidumm Feb 2, 2024
643e4eb
Merge branch 'main' into remove-readonly-check
Rich-Harris Feb 9, 2024
07161e6
Merge branch 'main' into remove-readonly-check
Rich-Harris Feb 12, 2024
b126e6a
reinstate tests
Rich-Harris Feb 12, 2024
f05319f
track ownership of state and mutations
Rich-Harris Feb 12, 2024
17eae45
working?
Rich-Harris Feb 13, 2024
1e48ccb
remove old changeset
Rich-Harris Feb 13, 2024
446e15c
tidy
Rich-Harris Feb 13, 2024
918faac
error
Rich-Harris Feb 13, 2024
a6b151d
simplify
Rich-Harris Feb 13, 2024
5213c72
fix
Rich-Harris Feb 13, 2024
7e48b13
fix
Rich-Harris Feb 13, 2024
26c5fbb
fix
Rich-Harris Feb 13, 2024
0781211
tidy
Rich-Harris Feb 13, 2024
9da0abb
Merge branch 'main' into stack-trace-based-readonly
Rich-Harris Feb 18, 2024
00b6626
make it a warning
Rich-Harris Feb 18, 2024
7b3fede
rename test
Rich-Harris Feb 18, 2024
fc2dec1
remove unused test
Rich-Harris Feb 18, 2024
8cd72c0
update tests
Rich-Harris Feb 18, 2024
50188c3
slap ts-expect-error everywhere, because its too finicky otherwise
Rich-Harris Feb 18, 2024
2f202ed
oops
Rich-Harris Feb 18, 2024
4930535
oh no the hall monitor is here
Rich-Harris Feb 18, 2024
87a2514
only call add_owner in dev
Rich-Harris Feb 18, 2024
6f81a15
only owners can transfer ownership
Rich-Harris Feb 18, 2024
f807b7c
simplify
Rich-Harris Feb 18, 2024
d665569
fixes
Rich-Harris Feb 18, 2024
2e2c950
Merge branch 'main' into stack-trace-based-readonly
Rich-Harris Feb 18, 2024
2c0215f
tidy up
Rich-Harris Feb 18, 2024
d4022b6
fix type error
Rich-Harris Feb 18, 2024
8603e10
while we're at it
Rich-Harris Feb 18, 2024
13fbf92
merge main
Rich-Harris Feb 19, 2024
7b96a7d
rename file
Rich-Harris Feb 19, 2024
551f500
rename functions
Rich-Harris Feb 19, 2024
560b856
add some comments
Rich-Harris Feb 19, 2024
48579de
move ownership checking logic
Rich-Harris Feb 19, 2024
bde311b
ugh eslint
Rich-Harris Feb 19, 2024
9f83a0f
more detailed message
Rich-Harris Feb 19, 2024
954da01
only add filename in dev
Rich-Harris Feb 19, 2024
cd972d9
comment
Rich-Harris Feb 19, 2024
f4f3d3a
update tests
Rich-Harris Feb 19, 2024
fc40b79
move more code
Rich-Harris Feb 20, 2024
adf542a
undo change to sourcemap tests
Rich-Harris Feb 20, 2024
b025d48
allow proxy to have multiple owners
Rich-Harris Feb 20, 2024
8d2a7d0
use SignalDebug
Rich-Harris Feb 20, 2024
9dc3205
i was doing this all wrong
Rich-Harris Feb 20, 2024
6dd301d
tidy up
Rich-Harris Feb 20, 2024
c6214ce
implement inheritance
Rich-Harris Feb 20, 2024
9eb17cf
fix
Rich-Harris Feb 20, 2024
78e7963
tidy up
Rich-Harris Feb 20, 2024
f88c32f
update filename stuff
Rich-Harris Feb 20, 2024
26455c3
changeset
Rich-Harris Feb 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rich-olives-yell.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: replace proxy-based readonly validation with stack-trace-based ownership tracking
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,11 @@ export function client_component(source, analysis, options) {
}
}

const push_args = [b.id('$$props'), b.literal(analysis.runes)];
if (options.dev) push_args.push(b.id(analysis.name));

const component_block = b.block([
b.stmt(b.call('$.push', b.id('$$props'), b.literal(analysis.runes))),
b.stmt(b.call('$.push', ...push_args)),
...store_setup,
...legacy_reactive_declarations,
...group_binding_declarations,
Expand Down Expand Up @@ -343,6 +346,27 @@ export function client_component(source, analysis, options) {
)
];

if (options.dev) {
if (options.filename) {
let filename = options.filename;
if (/(\/|\w:)/.test(options.filename)) {
// filename is absolute — truncate it
const parts = filename.split(/[/\\]/);
filename = parts.length > 3 ? ['...', ...parts.slice(-3)].join('/') : filename;
}

// add `App.filename = 'App.svelte'` so that we can print useful messages later
body.push(
b.stmt(
b.assignment('=', b.member(b.id(analysis.name), b.id('filename')), b.literal(filename))
)
);
}

body.unshift(b.stmt(b.call(b.id('$.mark_module_start'), b.id(analysis.name))));
body.push(b.stmt(b.call(b.id('$.mark_module_end'))));
}

if (options.discloseVersion) {
body.unshift(b.imports([], 'svelte/internal/disclose-version'));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,8 @@ function serialize_inline_component(node, component_name, context) {
/** @type {import('estree').Identifier | import('estree').MemberExpression | null} */
let bind_this = null;

const binding_initializers = [];

/**
* If this component has a slot property, it is a named slot within another component. In this case
* the slot scope applies to the component itself, too, and not just its children.
Expand Down Expand Up @@ -843,8 +845,6 @@ function serialize_inline_component(node, component_name, context) {
arg = b.call('$.get', id);
}

if (context.state.options.dev) arg = b.call('$.readonly', arg);

push_prop(b.get(attribute.name, [b.return(arg)]));
} else {
push_prop(b.init(attribute.name, value));
Expand All @@ -853,14 +853,23 @@ function serialize_inline_component(node, component_name, context) {
if (attribute.name === 'this') {
bind_this = attribute.expression;
} else {
push_prop(
b.get(attribute.name, [
b.return(
/** @type {import('estree').Expression} */ (context.visit(attribute.expression))
)
])
const expression = /** @type {import('estree').Expression} */ (
context.visit(attribute.expression)
);

if (context.state.options.dev) {
binding_initializers.push(
b.stmt(
b.call(
b.id('$.pre_effect'),
b.thunk(b.call(b.id('$.add_owner'), expression, b.id(component_name)))
)
)
);
}

push_prop(b.get(attribute.name, [b.return(expression)]));

const assignment = b.assignment('=', attribute.expression, b.id('$$value'));
push_prop(
b.set(attribute.name, [
Expand Down Expand Up @@ -1004,14 +1013,13 @@ function serialize_inline_component(node, component_name, context) {
);
}

/** @type {import('estree').Statement} */
let statement = b.stmt(fn(context.state.node));

if (snippet_declarations.length > 0) {
statement = b.block([...snippet_declarations, statement]);
}
const statements = [
...snippet_declarations,
...binding_initializers,
b.stmt(fn(context.state.node))
];

return statement;
return statements.length > 1 ? b.block(statements) : statements[0];
}

/**
Expand Down
154 changes: 154 additions & 0 deletions packages/svelte/src/internal/client/dev/ownership.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/** @typedef {{ file: string, line: number, column: number }} Location */

import { STATE_SYMBOL } from '../proxy.js';
import { untrack } from '../runtime.js';

/** @type {Record<string, Array<{ start: Location, end: Location, component: Function }>>} */
const boundaries = {};

const chrome_pattern = /at (?:.+ \()?(.+):(\d+):(\d+)\)?$/;
const firefox_pattern = /@(.+):(\d+):(\d+)$/;

export function get_stack() {
const stack = new Error().stack;
if (!stack) return null;

const entries = [];

for (const line of stack.split('\n')) {
let match = chrome_pattern.exec(line) ?? firefox_pattern.exec(line);

if (match) {
entries.push({
file: match[1],
line: +match[2],
column: +match[3]
});
}
}

return entries;
}

/**
* Determines which `.svelte` component is responsible for a given state change
* @returns {Function | null}
*/
export function get_component() {
const stack = get_stack();
if (!stack) return null;

for (const entry of stack) {
const modules = boundaries[entry.file];
if (!modules) continue;

for (const module of modules) {
if (module.start.line < entry.line && module.end.line > entry.line) {
return module.component;
}
}
}

return null;
}

/**
* Together with `mark_module_end`, this function establishes the boundaries of a `.svelte` file,
* such that subsequent calls to `get_component` can tell us which component is responsible
* for a given state change
* @param {Function} component
*/
export function mark_module_start(component) {
const start = get_stack()?.[2];

if (start) {
(boundaries[start.file] ??= []).push({
start,
// @ts-expect-error
end: null,
component
});
}
}

export function mark_module_end() {
const end = get_stack()?.[2];

if (end) {
// @ts-expect-error
boundaries[end.file].at(-1).end = end;
}
}

/**
*
* @param {any} object
* @param {any} owner
*/
export function add_owner(object, owner) {
untrack(() => {
add_owner_to_object(object, owner);
});
}

/**
* @param {any} object
* @param {Function} owner
*/
function add_owner_to_object(object, owner) {
if (object?.[STATE_SYMBOL]?.o && !object[STATE_SYMBOL].o.has(owner)) {
object[STATE_SYMBOL].o.add(owner);

for (const key in object) {
add_owner_to_object(object[key], owner);
}
}
}

/**
* @param {any} object
*/
export function strip_owner(object) {
untrack(() => {
strip_owner_from_object(object);
});
}

/**
* @param {any} object
*/
function strip_owner_from_object(object) {
if (object?.[STATE_SYMBOL]?.o) {
object[STATE_SYMBOL].o = null;

for (const key in object) {
strip_owner(object[key]);
}
}
}

/**
* @param {Set<Function>} owners
*/
export function check_ownership(owners) {
const component = get_component();

if (component && !owners.has(component)) {
let original = [...owners][0];

let message =
// @ts-expect-error
original.filename !== component.filename
? // @ts-expect-error
`${component.filename} mutated a value owned by ${original.filename}. This is strongly discouraged`
: 'Mutating a value outside the component that created it is strongly discouraged';

// eslint-disable-next-line no-console
console.warn(
`${message}. Consider passing values to child components with \`bind:\`, or use a callback instead.`
);

// eslint-disable-next-line no-console
console.trace();
}
}
Loading