Repository navigation
feat(core): add CORRECT005 — flag mutation of a non-bindable $props - #139
Conversation
Svelte's docs say plainly: don't mutate props unless they are $bindable. Two failure modes are invisible today: mutating a plain-object prop is a silent no-op (the object isn't a state proxy — not even the dev-time warning fires), and mutating a reactive-state-proxy prop only triggers the ownership_invalid_mutation warning if that code path is exercised at runtime. Neither is caught by the compiler. Plain reassignment of the prop itself (count = 5) is intentionally NOT flagged — the docs explicitly sanction that for ephemeral state; only mutation is prohibited. Detection (component-parse.ts): collectNonBindableProps walks $props() destructuring to build the set of non-$bindable local names (handles plain/renamed/rest bindings, and the non-destructured `let props = $props()` case, where no field can be individually $bindable either). collectPropMutations then flags member writes, `delete`, and a conservative list of mutating methods (push/splice/set/...) rooted at one of those names, over both the instance script and the template (inline handlers). Also fixes an AGENTS.md gap found while wiring this up: adding a rule has a fourth registration spot beyond the three already documented — packages/core/src/index.ts keeps its own duplicate re-export list from rules/index.js, which a missed entry doesn't fail typecheck on. Fixes #134 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new ChangesCORRECT005 Rule Implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Linter
participant parseComponentFacts
participant PropsAnalyzer
participant correct005PropMutation
Linter->>parseComponentFacts: parse(source, filename)
parseComponentFacts->>PropsAnalyzer: derive non-bindable props from $props()
PropsAnalyzer->>PropsAnalyzer: scan for member writes, delete, mutating calls
PropsAnalyzer-->>parseComponentFacts: mutatedProps [{name, line}]
parseComponentFacts-->>Linter: ComponentFacts with mutatedProps
Linter->>correct005PropMutation: applies(ComponentFacts)
correct005PropMutation-->>Linter: diagnostic per mutated prop
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/component-parse.ts (1)
360-375: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGeneric mutating-method name list can false-positive on non-collection objects.
MUTATING_METHODSflags any call topush/set/add/delete/clear/etc. on a prop-rooted expression, regardless of the actual runtime type. A prop holding a custom object with an immutable.set(key, val)builder method (returning a new instance, common in some functional-style APIs) would be flagged even though nothing is mutated. This is an inherent tradeoff of a type-unaware static heuristic and is explicitly framed as "conservative" in the PR description, so it may be acceptable, but worth being aware it trades false negatives for occasional false positives on generically-named methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/component-parse.ts` around lines 360 - 375, The `MUTATING_METHODS` heuristic in `component-parse.ts` is too broad because it matches method names like `set` and `add` even on non-collection objects; either narrow the check in the prop-rooted call analysis to only known collection-like receivers in the relevant parser logic, or explicitly document in the `MUTATING_METHODS`/CORRECT005 comment that this is a conservative static rule and may false-positive on builder-style APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/component-parse.ts`:
- Around line 385-410: collectPropMutations currently flags mutations based only
on the base identifier returned by rootObjectName, so shadowed locals/parameters
can be mistaken for prop writes. Update the logic in collectPropMutations (and
any helper it relies on) to resolve bindings in scope before pushing to acc, or
add a guard that skips identifiers redeclared by nested function params, let,
const, or catch bindings. Make sure the check still handles
AssignmentExpression, UpdateExpression, UnaryExpression delete, and mutating
CallExpression cases correctly while distinguishing the real prop reference from
a shadowed name.
---
Nitpick comments:
In `@packages/core/src/component-parse.ts`:
- Around line 360-375: The `MUTATING_METHODS` heuristic in `component-parse.ts`
is too broad because it matches method names like `set` and `add` even on
non-collection objects; either narrow the check in the prop-rooted call analysis
to only known collection-like receivers in the relevant parser logic, or
explicitly document in the `MUTATING_METHODS`/CORRECT005 comment that this is a
conservative static rule and may false-positive on builder-style APIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 215057b7-40f8-405d-9ca1-fdcd87ba24b0
📒 Files selected for processing (21)
.changeset/correct005-prop-mutation.mdAGENTS.mddocs/src/content/docs/guides/cli.mddocs/src/content/docs/ja/guides/cli.mddocs/src/content/docs/ja/rules/correct005.mddocs/src/content/docs/rules/correct005.mdpackages/cli/test/malformed-svelte.test.tspackages/cli/test/suppression-e2e.test.tspackages/core/src/component-collect.tspackages/core/src/component-parse.tspackages/core/src/component.tspackages/core/src/index.tspackages/core/src/rules/correctness/correct005-prop-mutation.tspackages/core/src/rules/index.tspackages/core/test/architecture-rules.test.tspackages/core/test/bundle-rules.test.tspackages/core/test/component-collect.test.tspackages/core/test/component-parse.test.tspackages/core/test/component-rule.test.tspackages/core/test/correctness-rules.test.tspackages/core/test/security-rules.test.ts
…(review)
Matching mutations by base identifier name alone (rootObjectName, no
scope resolution) means a nested function parameter or {#each ... as x}
loop variable reusing a prop's name would be misattributed as a prop
mutation — a real false-positive risk CodeRabbit flagged (Major).
collectPropMutations now tracks the two realistic shadow sources in a
Svelte component (function parameters, each-block context bindings)
while descending the AST, and skips a match whose resolved root name is
shadowed at that point. Full lexical scope resolution (block-scoped
let/const redeclaration, {#snippet}/{:then}/{:catch} bindings) is out of
scope here — this mirrors the identifier-only matching CORRECT004's
collectStateWrites already ships with, just tightened for the two most
likely collisions given the asymmetric cost (a false positive here,
unlike CORRECT004's false negative, contradicts the project's
stated precision principle). Documented as a deliberate partial
mitigation in the rule docs (en/ja) and in code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Fixes #134 — the last remaining slice from the code-health roadmap (#69). New Correctness rule flagging mutation of a non-
$bindableprop destructured from$props().Svelte's docs say plainly: "don't mutate props" unless they are
$bindable. Two failure modes are invisible today, neither caught by the compiler:ownership_invalid_mutationdev warning if that code path is exercised at runtime — static analysis catches it at review/CI time instead.Plain reassignment of the prop itself (
count = 5) is intentionally not flagged — the docs explicitly sanction that for ephemeral state ("the child component is able to temporarily override the prop value"); only mutation is prohibited. (An earlier draft of the issue proposed flagging reassignment too — corrected after checking the official docs via the Svelte MCP server, since that would have false-positived on documented, blessed code.)Detection
collectNonBindableProps(new,component-parse.ts) walks$props()destructuring to build the set of non-$bindablelocal names: plain and renamed destructured props, the...restbinding (rest props can never be individually$bindable), and the non-destructuredlet props = $props()case (no field there can be$bindableeither). Ambiguous shapes (nested destructuring, more than one$props()call) are skipped conservatively — an empty set, not a guess.collectPropMutations(new) flags member writes (prop.x = …,prop.x += …,prop.x++),delete prop.x, and calls to a conservative list of mutating methods (push,splice,set, …) rooted at one of those names — over both the instance script and the template (inline handlers mutate props too).$bindable(...)-declared props are excluded by construction.Docs gap fixed along the way
Wiring this up surfaced an undocumented fourth registration spot for a new rule, beyond the three AGENTS.md already lists:
packages/core/src/index.tskeeps its own duplicateexport { ... } from './rules/index.js'list, and a missed entry there doesn't fail typecheck (plain re-export, not a type error) — it silently drops the rule from the public API whileallRulesstill finds it internally. Documented in AGENTS.md so this doesn't bite the next rule.Test plan
component-parse.test.ts): member write / update expression /delete/ mutating method call are flagged; plain reassignment and$bindableprops are not; renamed and rest-prop bindings are tracked by their local name; the non-destructured$props()case; template inline-handler mutation; no false positive on a non-prop variable or a script with no$props().correctness-rules.test.ts): one finding per mutation occurrence, no-signal when there are none.packages/cli/test/docs-links.test.ts(existing) verifies both the en and ja CORRECT005 doc pages exist — it would fail the build if either were missing.pnpm build && pnpm typecheck && pnpm test && pnpm lint: all pass (core 364, cli 345, vite 84, mcp 13).pnpm --filter docs check: 0 errors.@svelte-vitals/core+svelte-vitals(new rule, user-facing).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
$props()unless they are marked$bindable.Documentation
Tests
Chores