Repository navigation
fix(compiler-core): avoid codegen crash on template with invalid structural directive - #14955
sarathfrancis90 wants to merge 2 commits into
Conversation
…ctural directive A <template> that carries a structural directive which is invalid in its position (a misplaced v-slot, or a v-else/v-else-if with no adjacent v-if) has its directive transform report a compiler error and bail, leaving the node in the tree without a codegenNode. With a non-throwing onError the node still reaches codegen, which threw an internal "Codegen node is missing" error (and a TypeError in production builds). Emit a null placeholder for an element/if/for node whose codegenNode is absent so the compiler degrades gracefully after the reported error instead of throwing a raw JS error.
📝 WalkthroughWalkthroughThe PR fixes a code-generation crash that occurs when Vue templates use invalid structural directive positioning (e.g., ChangesInvalid Structural Directive Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/compiler-core/__tests__/compile.spec.ts (2)
286-294: ⚡ Quick winVerify error codes for all test cases, not just
v-else.The test suite includes four different invalid structural directive patterns (v-else, v-else-if,
#slotstandalone,#slotnested), but only thev-elsecase is verified to report the correct error code (X_V_ELSE_NO_ADJACENT_IF). Each pattern may report a different error code (e.g.,v-else-ifmight reportX_V_ELSE_NO_ADJACENT_IF, but#slotcases would report a different error code for misplaced slots).Consider expanding this test to verify each case reports its appropriate error code.
💡 Example expansion
- test('still reports the underlying error', () => { + test.each([ + { source: `<template v-else>x</template>`, code: ErrorCodes.X_V_ELSE_NO_ADJACENT_IF }, + { source: `<template v-else-if="a">x</template>`, code: ErrorCodes.X_V_ELSE_NO_ADJACENT_IF }, + { source: `<template `#slot`>x</template>`, code: ErrorCodes.X_V_SLOT_MISPLACED }, + { source: `<div><template `#slot`>x</template></div>`, code: ErrorCodes.X_V_SLOT_MISPLACED }, + ])('reports correct error for $source', ({ source, code: expectedCode }) => { const onError = vi.fn() - expect(() => - compile(`<template v-else>x</template>`, { onError }), - ).not.toThrow() + expect(() => compile(source, { onError })).not.toThrow() expect(onError).toHaveBeenCalledWith( - expect.objectContaining({ code: ErrorCodes.X_V_ELSE_NO_ADJACENT_IF }), + expect.objectContaining({ code: expectedCode }), ) })Note: Verify the actual error code names for slot-related errors in the ErrorCodes enum.
🤖 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/compiler-core/__tests__/compile.spec.ts` around lines 286 - 294, The test currently only asserts the error code for the v-else case; update the test around the compile(...) calls to verify each invalid structural directive reports its specific error code by invoking compile with the four inputs and asserting onError was called with expect.objectContaining({ code: ErrorCodes.X_V_ELSE_NO_ADJACENT_IF }) for the v-else and v-else-if cases (or the correct enum if v-else-if differs), and the appropriate slot-related ErrorCodes for the standalone and nested `#slot` cases (use the ErrorCodes enum names for misplaced slot errors); keep using the existing onError spy and expect(...) .not.toThrow() pattern so each compile invocation checks the corresponding ErrorCodes entry.
272-284: ⚡ Quick winConsider strengthening the generated code assertion.
The assertion
expect(code).toContain('return')only verifies that the string 'return' appears somewhere in the generated code. This is a weak check that could pass even if the code structure is broken.Consider adding assertions that verify the generated code is syntactically valid JavaScript or has the expected function structure.
💡 Example of stronger assertion
expect(() => { code = compile(source, { onError }).code }).not.toThrow() - expect(code).toContain(`return`) + expect(code).toContain(`return`) + expect(code).toMatch(/function\s+render\s*\([^)]*\)\s*\{[\s\S]*return[\s\S]*\}/)This regex verifies the code contains a render function with a return statement inside it, providing stronger validation of the generated structure.
🤖 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/compiler-core/__tests__/compile.spec.ts` around lines 272 - 284, The current test uses a weak assertion expect(code).toContain('return'); update the assertion to validate actual generated function structure and syntactic validity: after obtaining code from compile(source, { onError }), either (a) assert with a regex that the render function exists and contains a return (e.g. match /function\s+render\([^)]*\)\s*{[\s\S]*return[\s\S]*}/ or the emitted arrow/const render pattern used by compile), or (b) execute a syntactic check by wrapping the emitted code in new Function(code) and asserting it does not throw; apply this change in the test.each block (referencing compile, onError and code) so the test verifies both structure and valid JS instead of just the substring "return".
🤖 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.
Nitpick comments:
In `@packages/compiler-core/__tests__/compile.spec.ts`:
- Around line 286-294: The test currently only asserts the error code for the
v-else case; update the test around the compile(...) calls to verify each
invalid structural directive reports its specific error code by invoking compile
with the four inputs and asserting onError was called with
expect.objectContaining({ code: ErrorCodes.X_V_ELSE_NO_ADJACENT_IF }) for the
v-else and v-else-if cases (or the correct enum if v-else-if differs), and the
appropriate slot-related ErrorCodes for the standalone and nested `#slot` cases
(use the ErrorCodes enum names for misplaced slot errors); keep using the
existing onError spy and expect(...) .not.toThrow() pattern so each compile
invocation checks the corresponding ErrorCodes entry.
- Around line 272-284: The current test uses a weak assertion
expect(code).toContain('return'); update the assertion to validate actual
generated function structure and syntactic validity: after obtaining code from
compile(source, { onError }), either (a) assert with a regex that the render
function exists and contains a return (e.g. match
/function\s+render\([^)]*\)\s*{[\s\S]*return[\s\S]*}/ or the emitted arrow/const
render pattern used by compile), or (b) execute a syntactic check by wrapping
the emitted code in new Function(code) and asserting it does not throw; apply
this change in the test.each block (referencing compile, onError and code) so
the test verifies both structure and valid JS instead of just the substring
"return".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02d5878f-6b2c-4470-a093-fd08c1141330
📒 Files selected for processing (2)
packages/compiler-core/__tests__/compile.spec.tspackages/compiler-core/src/codegen.ts
|
Hi @sarathfrancis90 — I picked this fix up and re-implemented it on current |
|
Hi @sarathfrancis90 — I investigated why this PR has been stuck, and I believe it is CI-flaky, not a code problem. What I verifiedThe e2e failure from the 6-12 CI run is: That is an animation-timing test in To confirm, I applied this PR's diff onto current
So the fix is healthy on current Suggested next steps
I had opened #15297 as a re-implementation of this fix on current main; it was closed as a duplicate of this PR, so it would be great to get this one over the line. Happy to help with the rebase if useful. |
This branch has not been deployed
3 participants
While fuzzing
baseCompilewith a non-throwingonError, I hit a raw internal crash on a few valid-to-parse templates.A
<template>that carries a structural directive which is invalid in its position — a misplacedv-slot/#slot, or av-else/v-else-ifwith no adjacentv-if— has its structural transform report a proper compiler error and bail, but the element node is left in the tree without acodegenNode. The defaultonErrorthrows, so this never surfaces; butonErroris a public option and tooling commonly supplies a collecting (non-throwing) handler. In that case the orphaned node reaches codegen, andgenNodethrowsError: Codegen node is missing for element/if/for node(aTypeErrorin production builds where the assert is stripped) instead of degrading after the already-reported error.Minimal repro:
Other triggers:
<template v-else-if="a">x</template>,<template #slot>x</template>,<div><template #slot>x</template></div>.The fix is in
genNode: when anelement/if/fornode has nocodegenNode(which only happens after such an error was reported and swallowed), emit anullplaceholder rather than throwing. The underlying compiler error is still reported throughonError.Added integration tests in
compile.spec.tscovering the trigger cases and asserting the underlying error is still reported. Fullcompiler-core+compiler-domsuites, typecheck and lint pass locally.Summary by CodeRabbit
Bug Fixes
Tests