fix(compiler-core): avoid codegen crash on template with invalid structural directive#14955
Open
sarathfrancis90 wants to merge 1 commit into
Open
fix(compiler-core): avoid codegen crash on template with invalid structural directive#14955sarathfrancis90 wants to merge 1 commit into
sarathfrancis90 wants to merge 1 commit 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
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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