refactor(core): semantic model in collector#9905
Merged
Conversation
|
Merging this PR will not alter performance
Comparing Footnotes
|
Contributor
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExport bookkeeping was changed from start positions to full TextRange values and JSDoc comments were integrated into the semantic layer. Bindings now store export ranges and optional JsdocComment; SemanticModelBuilder records export-node JSDoc by range and SemanticModel exposes range-based binding lookups and export_jsdoc. The module-graph and its collector were simplified to consume an Arc (removing traversal/event plumbing) and moved export/JSDoc responsibilities into the semantic model. JsdocComment representation and its formatter were refactored; formatting/tests adjusted to the new APIs. Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_module_graph/src/js_module_info/visitor.rs (1)
301-337:⚠️ Potential issue | 🟠 MajorCarry the export range for named re-exports.
Line 336 hard-codes
None, so/** ... */ export { Foo as Bar } from "./mod"loses its statement JSDoc after this refactor. Downstream there is nothing left for the semantic-model lookup to resolve.💡 Suggested patch
fn visit_export_named_from_clause( &self, node: JsExportNamedFromClause, collector: &mut JsModuleInfoCollector, ) -> Option<()> { + let export_range = node.syntax().parent().map(|parent| parent.text_trimmed_range()); let module_source = node.source().ok()?; let import_specifier = module_source .as_js_module_source()? .inner_string_text() .ok()?; @@ collector.register_export(JsCollectedExport::Reexport { export_name, reexport: JsReexport { import: JsImport { specifier: import_specifier.clone().into(), resolved_path: resolved_path.clone(), symbol: ImportSymbol::Named(imported_name), }, - export_range: None, + export_range, }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/js_module_info/visitor.rs` around lines 301 - 337, The re-export registration drops the export's source range by passing export_range: None in visit_export_named_from_clause, which loses statement JSDoc for named re-exports; fix it by computing the export range and passing it into JsCollectedExport::Reexport. Locate visit_export_named_from_clause and where collector.register_export(...) builds JsReexport, derive an export_range (prefer the specifier's export_as() range if present, otherwise fall back to the node's overall range/export range) and set export_range: Some(<that range>) instead of None so downstream semantic-model lookups can resolve the JSDoc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_semantic/src/semantic_model/builder.rs`:
- Around line 405-415: The current find_jsdoc walks all ancestors and can pick
up an outer declaration/export JSDoc for an inner binding; change the traversal
so it stops at the binding’s own declaration boundary: when iterating ancestors
in find_jsdoc, if you encounter an AnyJsDeclaration that is an ancestor of the
node, attempt to return its JsdocComment
(JsdocComment::try_from(decl.syntax()).ok()) and then stop (do not continue to
outer ancestors); keep the existing JsExport handling but ensure you only return
export comments if encountered before hitting that first declaration boundary.
This ensures inner bindings don’t inherit JSDoc from outer declarations while
still allowing direct-declaration or preceding-export comments to be returned.
In `@crates/biome_module_graph/src/format_module_graph.rs`:
- Around line 291-294: The JsReexport.export_range metadata is currently ignored
(left behind the TODO) so dumps lose that info; update the formatter in
format_module_graph.rs (the Display/fmt implementation that currently comments
out jsdoc_comment) to include export_range when formatting JsReexport nodes (or
alternatively add an explicit assertion that export_range is present elsewhere),
referencing the JsReexport type and the export_range field so the range is
either printed in the module-graph output or pinned by an assertion to prevent
it from silently disappearing.
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 255-265: The JsOwnExport::Binding branch currently only uses
module.semantic_model.as_binding_by_range(...).and_then(|binding|
binding.jsdoc()), which misses export-site comments and docs for re-exported or
imported bindings; change it to first check binding.export_ranges() and for each
export range call module.semantic_model.export_jsdoc(range).cloned() (returning
the first Some), then if none found and the binding is an import follow the
import through the module graph to the source module and attempt to read its
export-site JSDoc there, and finally fall back to binding.jsdoc(); update the
logic in the JsOwnExport::Binding arm (and reuse
module.semantic_model.as_binding_by_range, binding.export_ranges(), and
module.semantic_model.export_jsdoc(...) helpers) so re-exports and imported
default bindings surface their export comments.
---
Outside diff comments:
In `@crates/biome_module_graph/src/js_module_info/visitor.rs`:
- Around line 301-337: The re-export registration drops the export's source
range by passing export_range: None in visit_export_named_from_clause, which
loses statement JSDoc for named re-exports; fix it by computing the export range
and passing it into JsCollectedExport::Reexport. Locate
visit_export_named_from_clause and where collector.register_export(...) builds
JsReexport, derive an export_range (prefer the specifier's export_as() range if
present, otherwise fall back to the node's overall range/export range) and set
export_range: Some(<that range>) instead of None so downstream semantic-model
lookups can resolve the JSDoc.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86bece2b-13ee-4de2-be1a-1d12925c2755
⛔ Files ignored due to path filters (20)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_module_graph/tests/snapshots/test_export_default_function_declaration.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_export_default_imported_binding.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_export_referenced_function.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_namespace_reexport_type_inference.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_export_type_referencing_imported_type.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_export_types.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_exports.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_generic_return_value.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_generic_return_value_with_multiple_modules.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_import_as_namespace.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_merged_types.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_multiple_reexports.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_nested_function_call_with_namespace_in_return_type.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_promise_export.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_promise_from_imported_function_returning_imported_promise_type.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_promise_from_imported_function_returning_reexported_promise_type.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_recursive_looking_country_info.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_single_reexport.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_type_of_this_in_class_export.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (14)
crates/biome_js_semantic/Cargo.tomlcrates/biome_js_semantic/src/semantic_model/binding.rscrates/biome_js_semantic/src/semantic_model/builder.rscrates/biome_js_semantic/src/semantic_model/model.rscrates/biome_jsdoc_comment/Cargo.tomlcrates/biome_jsdoc_comment/src/format_jsdoc_comment.rscrates/biome_module_graph/src/format_module_graph.rscrates/biome_module_graph/src/js_module_info.rscrates/biome_module_graph/src/js_module_info/binding.rscrates/biome_module_graph/src/js_module_info/collector.rscrates/biome_module_graph/src/js_module_info/scope.rscrates/biome_module_graph/src/js_module_info/visitor.rscrates/biome_module_graph/src/module_graph.rscrates/biome_module_graph/tests/spec_tests.rs
dyc3
approved these changes
Apr 10, 2026
Contributor
dyc3
left a comment
There was a problem hiding this comment.
Kind of a rubber stamp, I'm not entirely up to speed in this area of the codebase.
Comment on lines
40
to
109
| @@ -106,7 +105,6 @@ Imports { | |||
| ``` | |||
| PromisedResult => BindingTypeData { | |||
| Types Module(0) TypeId(4), | |||
| Exported Ranges: (12..26) | |||
| } | |||
| ``` | |||
Member
Author
There was a problem hiding this comment.
No, it's correct. I mentioned in the description
Export ranges have been moved to the semantic model
I updated the printing of the semantic model and added a few snapshots.
chansuke
approved these changes
Apr 11, 2026
6f553e0 to
07eb428
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_jsdoc_comment/src/jsdoc_comment.rs (1)
123-134: Please add one focused test for range preservation.You’re now persisting trivia range, but tests only assert comment detection/collection text. A small assertion that
TryFrom<JsSyntaxToken>preserves the expectedTextRangewould lock this down.Suggested test shape
#[cfg(test)] mod tests { use super::*; use biome_js_parser::{JsParserOptions, parse}; use biome_js_syntax::JsFileSource; + use biome_rowan::AstNode; + #[test] + fn test_try_from_token_preserves_jsdoc_range() { + let source = "/** docs */\nconst a = 1;"; + let root = parse(source, JsFileSource::tsx(), JsParserOptions::default()).syntax(); + let token = root.first_token().expect("expected first token"); + let jsdoc = JsdocComment::try_from(token).expect("expected jsdoc"); + assert_eq!(jsdoc.range, TextRange::new(0.into(), 11.into())); + }As per coding guidelines, "All code changes MUST include appropriate tests: ... and bug fixes require tests that reproduce and validate the fix."
Also applies to: 141-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_jsdoc_comment/src/jsdoc_comment.rs` around lines 123 - 134, Add a focused unit test that constructs a JsSyntaxToken containing a JSDoc trivia and asserts that TryFrom<JsSyntaxToken> for JsdocComment returns a JsdocComment whose stored TextRange equals the trivia's original TextRange; specifically, create a token with known leading_trivia (using a single/multi-line comment text), call JsdocComment::try_from(token) (or TryFrom::try_from), and assert the returned JsdocComment.range() (or the field persisted by Self::new) matches the trivia.text_range() to lock down range preservation for text_is_jsdoc_comment and JsdocComment::new.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_semantic/src/semantic_model/binding.rs`:
- Around line 602-605: The current exports() iterator panics because it indexes
self.data.binding_node_by_start with positions from binding.export_ranges that
may be absent; change the logic to use
self.data.binding_node_by_start.get(&export_start.start()) and a filter_map to
skip missing entries, e.g. iterate
binding.export_ranges.iter().filter_map(|export_start|
self.data.binding_node_by_start.get(&export_start.start()).map(|node|
node.to_node(self.data.to_root().syntax()))), so missing declaration positions
are handled defensively without panicking.
---
Nitpick comments:
In `@crates/biome_jsdoc_comment/src/jsdoc_comment.rs`:
- Around line 123-134: Add a focused unit test that constructs a JsSyntaxToken
containing a JSDoc trivia and asserts that TryFrom<JsSyntaxToken> for
JsdocComment returns a JsdocComment whose stored TextRange equals the trivia's
original TextRange; specifically, create a token with known leading_trivia
(using a single/multi-line comment text), call JsdocComment::try_from(token) (or
TryFrom::try_from), and assert the returned JsdocComment.range() (or the field
persisted by Self::new) matches the trivia.text_range() to lock down range
preservation for text_is_jsdoc_comment and JsdocComment::new.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f68f81f-d4c8-42df-9af1-25220d228a12
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_js_semantic/src/snapshots/format_js.snapis excluded by!**/*.snapand included by**crates/biome_js_semantic/src/snapshots/format_ts.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_namespace_reexport_type_inference.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_resolve_exports.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (8)
crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rscrates/biome_js_semantic/Cargo.tomlcrates/biome_js_semantic/src/format_semantic_model.rscrates/biome_js_semantic/src/semantic_model/binding.rscrates/biome_js_semantic/src/tests/format.rscrates/biome_js_semantic/src/tests/mod.rscrates/biome_jsdoc_comment/src/jsdoc_comment.rscrates/biome_module_graph/src/format_module_graph.rs
✅ Files skipped from review due to trivial changes (1)
- crates/biome_js_semantic/src/tests/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_semantic/Cargo.toml
- crates/biome_module_graph/src/format_module_graph.rs
Contributor
|
Thanks for this! Looks pretty sweet :) |
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
4 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.
Summary
This PR finally finishes the refactor of using the semantic model inside our module graph.
There were still some parts that were ad hoc, and with this PR, now we use the semantic model for everything.
Test Plan
Updated the snapshots, updated unit tests.
Existing tests should still pass (module graph and JS rules)
Docs
N/A
Changeset not needed, it's an internal refactor.