Skip to content

Conversation

@seekskyworld
Copy link

@seekskyworld seekskyworld commented Jan 14, 2026

Issue: #14287

Summary:

  • Treat semicolons in v-on handlers as multi-statement only when expression parsing fails
  • Add regression coverage for function expressions containing semicolons

Motivation:

  • Function expressions with semicolons are valid expressions but were parsed as statements, causing compiler errors

Validation:

  • pnpm exec vitest --project unit packages/compiler-core/tests/transforms/vOn.spec.ts

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation of inline event-handler expressions so semicolons inside function expressions are correctly treated as part of the expression, preventing misclassification of handler code.
  • Tests

    • Added a test covering inline function expressions containing semicolons to ensure correct parsing and behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

📝 Walkthrough

Walkthrough

Adds expression validation utilities and uses them in v-on handler parsing to distinguish single valid expressions from multiple statements; also adds a test covering semicolons inside a function expression in a v-on expression.

Changes

Cohort / File(s) Summary
Expression Validation Utilities
packages/compiler-core/src/utils.ts
Added exported validators: isValidExpressionBrowser(exp: ExpressionNode), isValidExpressionNode(exp: ExpressionNode, context: TransformContext), and isValidExpression(exp: ExpressionNode, context: TransformContext) to validate inline expressions (browser/Node dispatcher).
V-On Handler Processing
packages/compiler-core/src/transforms/vOn.ts
Updated hasMultipleStatements logic to require both a semicolon and that the expression is not a valid expression (isValidExpression) when detecting multiple inline statements.
V-On Transform Tests
packages/compiler-core/__tests__/transforms/vOn.spec.ts
Added test verifying semicolons inside a function expression (function () { ';' }) are treated as part of the expression, not as statement separators.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

scope: compiler, :hammer: p3-minor-bug

Suggested reviewers

  • sxzz
  • baiwusanyu-c
  • KazariEX

Poem

I’m a rabbit checking code with glee,
Semicolons nested where they shouldn’t be free,
Parsers peek in browser and Node,
So inline funcs stay one tidy node,
Hooray — v-on flows carelessly! 🐇✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the main change: introducing a fix that properly handles semicolons in v-on expressions by validating expression syntax before treating them as multi-statement handlers.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a301672 and 004dd75.

📒 Files selected for processing (1)
  • packages/compiler-core/src/utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/src/utils.ts (3)
packages/compiler-core/src/ast.ts (1)
  • ExpressionNode (91-91)
packages/compiler-core/src/transform.ts (1)
  • TransformContext (84-125)
packages/shared/src/general.ts (1)
  • NOOP (8-8)
🔇 Additional comments (3)
packages/compiler-core/src/utils.ts (3)

234-248: LGTM!

The implementation correctly uses new Function() with parentheses wrapping to validate expression syntax. This handles edge cases like object literals and arrow functions that would otherwise be parsed incorrectly. The try/catch pattern is consistent with other validation utilities in this file.


250-270: Verify sourceType: 'module' is intentional.

This function specifies sourceType: 'module' while similar validation functions in this file (isMemberExpressionNode, isFnExpressionNode) don't specify a sourceType, defaulting to 'script'. This could affect parsing of certain expressions differently.

If supporting module-specific syntax in v-on handlers is intended, this is fine. Otherwise, consider removing sourceType: 'module' for consistency:

Suggested change for consistency
 parseExpression(`(${getExpSource(exp)})`, {
-  sourceType: 'module',
   plugins: context.expressionPlugins
     ? [...context.expressionPlugins, 'typescript']
     : ['typescript'],
 })

272-278: LGTM!

The dispatcher pattern is consistent with the existing isMemberExpression and isFnExpression utilities. The type signature correctly includes context for the unified API even though the browser implementation ignores it.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@seekskyworld
Copy link
Author

Added JSDoc for the new isValidExpression helpers to satisfy the docstring coverage check.

@edison1105 edison1105 added scope: compiler 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. labels Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. scope: compiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants