Skip to content

fix(core): improve bindings detection#8918

Merged
ematipico merged 3 commits intonextfrom
fix/false-positives-embeds
Feb 6, 2026
Merged

fix(core): improve bindings detection#8918
ematipico merged 3 commits intonextfrom
fix/false-positives-embeds

Conversation

@ematipico
Copy link
Member

@ematipico ematipico commented Jan 30, 2026

Summary

Closes #8904
Closes #8903

In order to fix the issues, I had to rethink how JS snippets are parsed. Up until now, they were parsed as normal JS files, which turned out be incorrect, because things like {{ duration: 400 }} were incorrectly parserd.

It's safe to assume that JS snippets inside a HTML-ish language should be parsed as an expression, because they must return a value. Hence, I added a new root to the JS language, very much like the root we use for embedded templates. I decided to use a new root to avoid possible conflicts with the semantics and the possible evolution of the feature.

The code has been created via AI, and carefully steerred toward the correct implementation.

Test Plan

Added new tests.

I updated the parsing infra of the JS parser so that we can simulate snippets, using the suffix .inline_expr..

Docs

Not needed

@ematipico ematipico requested a review from dyc3 January 30, 2026 10:42
@changeset-bot
Copy link

changeset-bot bot commented Jan 30, 2026

⚠️ No Changeset found

Latest commit: cecce99

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Jan 30, 2026
@ematipico ematipico force-pushed the fix/false-positives-embeds branch from a14544d to 86f6dff Compare January 30, 2026 10:46
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 30, 2026

Walkthrough

Adds first‑class support for inline/template expressions by renaming JsExpressionSnipped→JsExpressionSnippet and introducing JsExpressionTemplateRoot; parser now parses Vue/Svelte/Astro template expressions as single-expression roots, with new diagnostics for template-expression errors. Formatter and generated code updated to match the renamed node and to format the new template-root. Linter/semantic updates: JsFileSource now exposes template-expression semantics, no_useless_lone_block_statements now uses file_source, and no_unused_variables consults embedded value references to avoid false positives. Tests for Astro, Svelte and Vue linting were added.

Possibly related PRs

Suggested reviewers

  • dyc3
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix(core): improve bindings detection' is somewhat vague and doesn't clearly convey the main change—reworking JS snippet parsing for HTML-like languages. Consider a more specific title like 'fix(core): parse template expressions as expressions instead of statements' to better reflect the core change.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the problem, solution, and approach, directly addressing the linked issues and rationale for the architectural changes.
Linked Issues check ✅ Passed The PR successfully addresses both linked issues: JS snippets are now parsed as expressions (fixing false positives for both noUnusedVariables [#8904] and noUselessLoneBlockStatements [#8903]) via the new JsExpressionTemplateRoot.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective of reworking JS snippet parsing. Typo corrections (JsExpressionSnipped → JsExpressionSnippet) support the core refactoring and are necessary housekeeping.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/false-positives-embeds

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In
`@crates/biome_js_analyze/src/lint/complexity/no_useless_lone_block_statements.rs`:
- Line 147: Remove the stray dbg!(&statement); call in
no_useless_lone_block_statements.rs (the debug print that spams stderr during
lint runs); simply delete that dbg! invocation (or replace it with a proper
trace/log call if structured logging is needed) so the linter no longer emits
debug output for the `statement` variable.
- Around line 150-153: The JsExpressionStatement match arm is inverted: change
the condition on AnyJsStatement::JsExpressionStatement from
"!file_source.is_embedded_source()" to "file_source.is_embedded_source()" so
embedded sources are suppressed (treated as useful) while regular JS still flags
lone expression statements; update the match arm that currently reads
AnyJsStatement::JsExpressionStatement(_) => !file_source.is_embedded_source()
accordingly.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 30, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing fix/false-positives-embeds (cecce99) with next (7e48bd4)

Summary

✅ 58 untouched benchmarks
⏩ 96 skipped benchmarks1

Footnotes

  1. 96 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

dyc3
dyc3 previously requested changes Jan 30, 2026
Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not fix the noUselessLoneBlockStatements issue. I think I know what's happening though.

Given this snippet:

<div {...props} transition:slide={{ duration }}>
	{@render children?.()}
</div>

{ duration } gets parsed as if its a full js doc:

JsModule {
  bom_token: missing (optional),
  interpreter_token: missing (optional),
  directives: JsDirectiveList [],
  items: JsModuleItemList [
    JsBlockStatement {
      l_curly_token: L_CURLY@0..2 "{" [] [Whitespace(" ")],
      statements: JsStatementList [
        JsExpressionStatement {
          expression: JsIdentifierExpression {
            name: JsReferenceIdentifier {
              value_token: IDENT@2..11 "duration" [] [Whitespace(" ")],
            },
          },
          semicolon_token: missing (optional),
        },
      ],
      r_curly_token: R_CURLY@11..12 "}" [] [],
    },
  ],
  eof_token: EOF@12..12 "" [] [],
}

If I make it a little different, { duration: 200 }, it thinks duration: is a label, and I get a bunch of false positives for noConfusingLabels.

I think the correct fix for this is to have some other entry point into the js parser to make it so that these get parsed as a JsObjectExpression (which is what it's supposed to be).

@ematipico
Copy link
Member Author

I think the correct fix for this is to have some other entry point into the js parser to make it so that these get parsed as a JsObjectExpression (which is what it's supposed to be).

Yeah, good catch, I believe we so.

@github-actions github-actions bot added A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-Grit Language: GritQL labels Feb 4, 2026
@ematipico ematipico requested a review from dyc3 February 4, 2026 11:47
@ematipico
Copy link
Member Author

@dyc3 I updated the description of the PR, because I changed completely how the feature should work.

@ematipico ematipico dismissed dyc3’s stale review February 6, 2026 08:45

Feedback addressed

@ematipico
Copy link
Member Author

Going to merge this, but let me know if you have some feedback. The PR addresses the concerns

@ematipico ematipico merged commit b5dd9ce into next Feb 6, 2026
16 checks passed
@ematipico ematipico deleted the fix/false-positives-embeds branch February 6, 2026 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CLI Area: CLI A-Formatter Area: formatter A-Linter Area: linter A-Parser Area: parser A-Project Area: project A-Tooling Area: internal tools L-Grit Language: GritQL L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants