chore(xtask): migrate Symbol global types - #11034
Conversation
|
✅ Organic activityNo automation signals detected in the analyzed events. This is an automated analysis by AgentScan |
Merging this PR will not alter performance
Comparing Footnotes
|
211c8bd to
17b8baf
Compare
17b8baf to
422ecfb
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe global-types pipeline now lowers Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
| @@ -15,7 +15,10 @@ const OUTPUT_RELATIVE_PATH: &str = "crates/biome_js_type_info/src/generated/glob | |||
| /// | |||
| /// Must stay ordered by ascending `GlobalTypeId` index so the emitted | |||
| /// `MIGRATED_PREDEFINED_IDS` stays sorted for the runtime `binary_search`. | |||
| const GLOBAL_ID_EMIT_ORDER: [&str; 7] = [ | |||
| const GLOBAL_ID_EMIT_ORDER: [&str; 10] = [ | |||
| "SYMBOL_ID_GLOBAL_TYPE_ID", | |||
| "SYMBOL_DISPOSE_ID_GLOBAL_TYPE_ID", | |||
| "SYMBOL_ASYNC_DISPOSE_ID_GLOBAL_TYPE_ID", | |||
There was a problem hiding this comment.
The comment says that these entries must be ordered, but it doesn't seem true? What is it? We should either:
- remove the "ordered" part
- order them, and add a regression test
There was a problem hiding this comment.
These entries are ordered by ascending GlobalTypeId index, not lexicographically. The index is defined by the row position in PREDEFINED_ID_ROWS
The emitted order is covered by the expected-order test and the strictly-increasing test. It must remain sorted because the runtime uses binary_search
I fixed the comment in commit 3cd8eba
| expect_error_contains( | ||
| compare_lowered_globals(&lowered), | ||
| "generated globals contain 3 entries, expected 7", | ||
| "generated globals contain 3 entries, expected 10", |
There was a problem hiding this comment.
Honestly, I would remove all these hard coded sizes for the arrays we have. Chances are, we would need to add more of them, and updating the numbers is just noise.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xtask/codegen/src/generate_global_types/emit.rs (1)
78-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this rustdoc document the contract, or remove it.
Builds the resolver registration statementsmerely repeats the function’s name and body. Document the useful contract instead — that registrations followGLOBAL_ID_EMIT_ORDERand propagate missing or unlisted-ID errors — or omit the comment.As per coding guidelines, comments and documentation must explain contracts, invariants, module purpose, terminology, or rationale rather than duplicate information recoverable from the code.
Proposed wording
-/// Builds the resolver registration statements. +/// Renders registrations in [`GLOBAL_ID_EMIT_ORDER`], returning an error if +/// lowered globals contain an unlisted ID or omit an expected ID.🤖 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 `@xtask/codegen/src/generate_global_types/emit.rs` around lines 78 - 79, Update the rustdoc for render_registrations to document its contract: registrations follow GLOBAL_ID_EMIT_ORDER and missing or unlisted IDs are propagated as errors. If that contract cannot be documented accurately, remove the redundant rustdoc instead.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 `@xtask/codegen/src/generate_global_types/emit.rs`:
- Around line 78-79: Update the rustdoc for render_registrations to document its
contract: registrations follow GLOBAL_ID_EMIT_ORDER and missing or unlisted IDs
are propagated as errors. If that contract cannot be documented accurately,
remove the redundant rustdoc instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 589e16ea-a7aa-4cff-9a3c-4e98823c832b
📒 Files selected for processing (4)
xtask/codegen/src/generate_global_types/compare.rsxtask/codegen/src/generate_global_types/emit.rsxtask/codegen/src/generate_global_types/lower.rsxtask/codegen/tests/global_types_codegen.rs
💤 Files with no reviewable changes (1)
- xtask/codegen/src/generate_global_types/compare.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- xtask/codegen/tests/global_types_codegen.rs
- xtask/codegen/src/generate_global_types/lower.rs
Summary
Part of #5977.
Migrates
Symbol,Symbol.dispose, andSymbol.asyncDisposeto generated global types while preserving their existing raw and Salsa-backed shapes. This does not change the inference engine.Test Plan
Symboldeclarations.just gen-global-typesis idempotent.Docs
N/A