Skip to content

fix(inference): handle import cycles - #11008

Merged
ematipico merged 2 commits into
mainfrom
15-handle-inference-import-cycles
Jul 23, 2026
Merged

fix(inference): handle import cycles#11008
ematipico merged 2 commits into
mainfrom
15-handle-inference-import-cycles

Conversation

@ematipico

@ematipico ematipico commented Jul 20, 2026

Copy link
Copy Markdown
Member

Summary

Adds cycle-safe Salsa inference for imported modules. Imports inside the active strongly connected component resolve conservatively while dependencies outside the cycle remain inferred.

Test Plan

Moved cycle coverage to a focused test file and passed all module-graph tests and all-target clippy.

Docs

N/A

This PR was created with AI assistance (OpenCode).

@github-actions

Copy link
Copy Markdown
Contributor

✅ Organic activity

No automation signals detected in the analyzed events.

View full analysis →

This is an automated analysis by AgentScan

@changeset-bot

changeset-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 0e32611

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

@ematipico
ematipico force-pushed the graphite-base/11008 branch from 95c1239 to e9a9f03 Compare July 22, 2026 14:56
@ematipico
ematipico force-pushed the 15-handle-inference-import-cycles branch from 06bfe68 to 0e32611 Compare July 22, 2026 14:56
@ematipico
ematipico changed the base branch from graphite-base/11008 to main July 22, 2026 14:56
@ematipico
ematipico marked this pull request as ready for review July 22, 2026 14:56
@ematipico ematipico closed this Jul 22, 2026
@ematipico ematipico reopened this Jul 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Organic activity

No automation signals detected in the analyzed events.

View full analysis →

This is an automated analysis by AgentScan

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Implemented cycle-aware module type inference using SCC analysis and ImportResolution::CycleFallback. Standard inference explicitly uses full import resolution, while cycle fallback blocks recursive modules and continues resolving non-cyclic dependencies. Updated tests now verify unknown types for cyclic bindings and number inference for an acyclic export.

Possibly related PRs

Suggested labels: A-Type-Inference

Suggested reviewers: denbezrukov

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the main change: handling import cycles in inference.
Description check ✅ Passed The description matches the changeset and describes cycle-safe inference and the test move.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 15-handle-inference-import-cycles

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/biome_module_graph/src/db/type_inference/mod.rs (1)

158-177: 📐 Maintainability & Code Quality | 🔵 Trivial

Consider a short doc comment.

inference_scc next door has a solid rustdoc explaining the cycle-fallback contract, but this function (the actual Salsa cycle_result entry point) has none. A one-liner noting it's the fallback invoked when Salsa detects an import cycle, and that it delegates to inference_scc for blocking, would help future readers who aren't already steeped in Salsa cycle-recovery semantics.

📝 Suggested doc comment
+/// Salsa cycle-recovery fallback for `infer_module_types`, invoked when
+/// inferring `module` would otherwise recurse into itself. Computes the
+/// enclosing strongly connected component and re-resolves `module` with
+/// cyclic imports blocked, leaving acyclic dependencies fully inferred.
 pub(super) fn infer_module_types_cycle_result<'db>(
     db: &'db dyn ModuleDb,
     _id: salsa::Id,
     module: ModuleInfo,
 ) -> Option<InferredModuleTypes<'db>> {

As per coding guidelines: "Write comments only when they convey information not recoverable from code: /// documents contracts... // provides rationale."

🤖 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 `@crates/biome_module_graph/src/db/type_inference/mod.rs` around lines 158 -
177, Add a concise Rustdoc comment above infer_module_types_cycle_result
documenting that it is the Salsa cycle-result fallback invoked when an import
cycle is detected and that it delegates blocking decisions to inference_scc. Do
not alter the function’s behavior.

Source: Coding guidelines

🤖 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 `@crates/biome_module_graph/src/db/type_inference/mod.rs`:
- Around line 158-177: Add a concise Rustdoc comment above
infer_module_types_cycle_result documenting that it is the Salsa cycle-result
fallback invoked when an import cycle is detected and that it delegates blocking
decisions to inference_scc. Do not alter the function’s behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a746e824-fbb2-46f7-b55b-11b883452938

📥 Commits

Reviewing files that changed from the base of the PR and between f0ebdbb and 0e32611.

📒 Files selected for processing (5)
  • crates/biome_module_graph/src/db/queries/type_inference.rs
  • crates/biome_module_graph/src/db/type_inference/mod.rs
  • crates/biome_module_graph/src/db/type_inference/resolver.rs
  • crates/biome_module_graph/tests/spec_tests_v2.rs
  • crates/biome_module_graph/tests/spec_tests_v2/cycles.test.rs

@ematipico
ematipico requested review from a team July 22, 2026 15:31
@ematipico
ematipico merged commit aa87885 into main Jul 23, 2026
22 checks passed
@ematipico
ematipico deleted the 15-handle-inference-import-cycles branch July 23, 2026 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Project Area: project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant