chore(xtask): scaffold global-types codegen and resolver builder#10249
Open
minseong0324 wants to merge 1 commit into
Open
chore(xtask): scaffold global-types codegen and resolver builder#10249minseong0324 wants to merge 1 commit into
minseong0324 wants to merge 1 commit into
Conversation
|
Merging this PR will not alter performance
Comparing |
1482ec2 to
30a9e19
Compare
d26c0d6 to
6db64fb
Compare
6db64fb to
220d304
Compare
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_js_type_info/src/globals.rs (1)
298-311:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
Tfor themapcallback parameter.Line 304 types
itemasU, butArray.prototype.mappasses the source element type into the callback. As written, this modelsmapasU -> Uinstead ofT -> U, so inference goes sideways rather quickly.Suggested fix
builder.set_manual_type_data(MAP_CALLBACK_ID_GLOBAL_TYPE_ID, || { TypeData::from(Function { is_async: false, type_parameters: Default::default(), name: Some(Text::new_static(MAP_CALLBACK_ID_NAME)), parameters: [FunctionParameter::Pattern(PatternFunctionParameter { - ty: GLOBAL_U_ID.into(), + ty: GLOBAL_T_ID.into(), bindings: Default::default(), is_optional: false, is_rest: false, })] .into(), return_type: ReturnType::Type(GLOBAL_U_ID.into()), }) });🤖 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_js_type_info/src/globals.rs` around lines 298 - 311, The map callback is currently modeled with its parameter typed as GLOBAL_U_ID which makes map's signature U -> U; update the manual type data in builder.set_manual_type_data for MAP_CALLBACK_ID_GLOBAL_TYPE_ID so the Function's parameter PatternFunctionParameter uses GLOBAL_T_ID.into() for ty (leaving the return_type as GLOBAL_U_ID.into()), i.e. change the parameter type from GLOBAL_U_ID to GLOBAL_T_ID to correctly represent T -> U; adjust any imports or constants if needed.
🤖 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.
Inline comments:
In `@crates/biome_js_type_info/src/globals_ids.rs`:
- Around line 91-97: The helper method
PredefinedGlobalRole::is_sentinel_projection currently omits the HostManual
variant so HostManual rows can be treated as migratable; update
is_sentinel_projection to include Self::HostManual in the matches! arm (i.e.,
change matches!(self, Self::Sentinel | Self::Primitive | Self::Helper) to also
include Self::HostManual) so MIGRATED_PREDEFINED_IDS and callers like
set_manual_type_data() will keep HostManual entries out of migration; adjust the
doc comment to mention HostManual if needed and run tests.
In `@xtask/codegen/src/generate_global_types.rs`:
- Around line 150-172: The code currently uses args.verify for
checkout/SourceOptions.verify but relies on mode for emit/update behavior;
ensure a single source of truth by deriving a single bool (e.g. let verify =
matches!(mode, xtask_glue::Mode::Verify) or similar) and use that for
SourceOptions.verify and any verification logic, and then check that args.verify
(if passed) agrees with that derived value and bail with an error if they
disagree; update run_with_workspace_root to compute and use this unified verify
flag instead of mixing args.verify and mode.
---
Outside diff comments:
In `@crates/biome_js_type_info/src/globals.rs`:
- Around line 298-311: The map callback is currently modeled with its parameter
typed as GLOBAL_U_ID which makes map's signature U -> U; update the manual type
data in builder.set_manual_type_data for MAP_CALLBACK_ID_GLOBAL_TYPE_ID so the
Function's parameter PatternFunctionParameter uses GLOBAL_T_ID.into() for ty
(leaving the return_type as GLOBAL_U_ID.into()), i.e. change the parameter type
from GLOBAL_U_ID to GLOBAL_T_ID to correctly represent T -> U; adjust any
imports or constants if needed.
🪄 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: ae16dbbb-ee3d-44da-a807-08ef15b36dac
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**
📒 Files selected for processing (37)
.github/workflows/autofix.yml.github/workflows/pull_request.ymlCargo.tomlcrates/biome_js_type_info/Cargo.tomlcrates/biome_js_type_info/src/codegen/global_types.rscrates/biome_js_type_info/src/codegen/mod.rscrates/biome_js_type_info/src/globals.rscrates/biome_js_type_info/src/globals_builder.rscrates/biome_js_type_info/src/globals_ids.rscrates/biome_js_type_info/src/lib.rsjustfilextask/codegen/Cargo.tomlxtask/codegen/src/generate_global_types.rsxtask/codegen/src/generate_global_types/collect.rsxtask/codegen/src/generate_global_types/emit.rsxtask/codegen/src/generate_global_types/source.rsxtask/codegen/src/lib.rsxtask/codegen/src/main.rsxtask/codegen/tests/fixtures/global-types/collector.basic.d.tsxtask/codegen/tests/fixtures/global-types/collector.basic.expected.txtxtask/codegen/tests/fixtures/global-types/collector.declare-global.d.tsxtask/codegen/tests/fixtures/global-types/collector.declare-global.expected.txtxtask/codegen/tests/fixtures/global-types/collector.empty-external-module.d.tsxtask/codegen/tests/fixtures/global-types/collector.empty-external-module.expected.txtxtask/codegen/tests/fixtures/global-types/collector.export-unsupported.d.tsxtask/codegen/tests/fixtures/global-types/collector.export-unsupported.expected.txtxtask/codegen/tests/fixtures/global-types/collector.external-module.d.tsxtask/codegen/tests/fixtures/global-types/collector.external-module.expected.txtxtask/codegen/tests/fixtures/global-types/collector.multiple-var.d.tsxtask/codegen/tests/fixtures/global-types/collector.multiple-var.expected.txtxtask/codegen/tests/fixtures/global-types/collector.qualified-namespace.d.tsxtask/codegen/tests/fixtures/global-types/collector.qualified-namespace.expected.txtxtask/codegen/tests/fixtures/global-types/collector.recursive-scope-accounting.d.tsxtask/codegen/tests/fixtures/global-types/collector.recursive-scope-accounting.expected.txtxtask/codegen/tests/fixtures/global-types/collector.unsupported.d.tsxtask/codegen/tests/fixtures/global-types/collector.unsupported.expected.txtxtask/codegen/tests/global_types_codegen.rs
Comment on lines
+91
to
+97
| impl PredefinedGlobalRole { | ||
| /// True when the role belongs to the hand-maintained sentinel projection | ||
| /// (`Sentinel` / `Primitive` / `Helper`) that must never appear in | ||
| /// `MIGRATED_PREDEFINED_IDS`. | ||
| pub(crate) const fn is_sentinel_projection(self) -> bool { | ||
| matches!(self, Self::Sentinel | Self::Primitive | Self::Helper) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Keep HostManual out of the migratable set.
PredefinedGlobalRole::HostManual is documented as “must stay manual”, but this helper excludes it. That leaves rows like FETCH_ID outside the protected set, so a future MIGRATED_PREDEFINED_IDS entry would make set_manual_type_data() skip the hand-written fallback without any invariant barking. Please enforce the non-migratable contract for HostManual too.
🤖 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_js_type_info/src/globals_ids.rs` around lines 91 - 97, The
helper method PredefinedGlobalRole::is_sentinel_projection currently omits the
HostManual variant so HostManual rows can be treated as migratable; update
is_sentinel_projection to include Self::HostManual in the matches! arm (i.e.,
change matches!(self, Self::Sentinel | Self::Primitive | Self::Helper) to also
include Self::HostManual) so MIGRATED_PREDEFINED_IDS and callers like
set_manual_type_data() will keep HostManual entries out of migration; adjust the
doc comment to mention HostManual if needed and run tests.
Comment on lines
+150
to
+172
| pub fn run_with_workspace_root( | ||
| args: GlobalTypesArgs, | ||
| mode: xtask_glue::Mode, | ||
| workspace_root: &Path, | ||
| ) -> anyhow::Result<()> { | ||
| if args.verify && (args.ts_tag.is_some() || args.ts_sha.is_some()) { | ||
| anyhow::bail!( | ||
| "--verify cannot be combined with --ts-tag or --ts-sha; verification always uses the default pin" | ||
| ); | ||
| } | ||
| if args.ts_tag.is_some() != args.ts_sha.is_some() { | ||
| anyhow::bail!("--ts-tag and --ts-sha must be supplied together"); | ||
| } | ||
|
|
||
| let pin = SourcePin::new( | ||
| args.ts_tag.as_deref().unwrap_or(DEFAULT_TYPESCRIPT_TAG), | ||
| args.ts_sha.as_deref().unwrap_or(DEFAULT_TYPESCRIPT_SHA), | ||
| )?; | ||
| let opts = source::SourceOptions { | ||
| offline: args.offline, | ||
| verify: args.verify, | ||
| repo_url_override: None, | ||
| }; |
Contributor
There was a problem hiding this comment.
Use one source of truth for verify mode.
run_with_workspace_root drives checkout validation from args.verify, but emit/update behaviour comes from mode. If those ever drift, a “verify” run can still fetch with overwrite semantics, or vice versa. Please derive both from the same flag, or bail if they disagree.
Possible tidy fix
pub fn run_with_workspace_root(
args: GlobalTypesArgs,
mode: xtask_glue::Mode,
workspace_root: &Path,
) -> anyhow::Result<()> {
- if args.verify && (args.ts_tag.is_some() || args.ts_sha.is_some()) {
+ let verify = matches!(mode, xtask_glue::Mode::Verify);
+ if args.verify != verify {
+ anyhow::bail!("internal error: args.verify and mode must agree");
+ }
+
+ if verify && (args.ts_tag.is_some() || args.ts_sha.is_some()) {
anyhow::bail!(
"--verify cannot be combined with --ts-tag or --ts-sha; verification always uses the default pin"
);
}
if args.ts_tag.is_some() != args.ts_sha.is_some() {
@@
let opts = source::SourceOptions {
offline: args.offline,
- verify: args.verify,
+ verify,
repo_url_override: None,
};🤖 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.rs` around lines 150 - 172, The code
currently uses args.verify for checkout/SourceOptions.verify but relies on mode
for emit/update behavior; ensure a single source of truth by deriving a single
bool (e.g. let verify = matches!(mode, xtask_glue::Mode::Verify) or similar) and
use that for SourceOptions.verify and any verification logic, and then check
that args.verify (if passed) agrees with that derived value and bail with an
error if they disagree; update run_with_workspace_root to compute and use this
unified verify flag instead of mixing args.verify and mode.
Comment on lines
+842
to
+849
| let Ok(owner) = read_acquire_lock_owner(path) else { | ||
| return Ok(false); | ||
| }; | ||
| let Some(pid) = owner | ||
| .split_once(':') | ||
| .and_then(|(pid, _)| pid.parse::<u32>().ok()) | ||
| else { | ||
| return Ok(false); |
Contributor
There was a problem hiding this comment.
Unreadable lock owners never recover.
If a process dies after create_dir() but before writing owner, this branch returns Ok(false) on every poll. The next run then sleeps for ~60 seconds and fails, even though the lock is already orphaned. Please add a reclaim path for missing/corrupt owner files after a bounded grace period.
dyc3
approved these changes
May 20, 2026
Contributor
There was a problem hiding this comment.
nit: our usual convention is to put generated code in a generated folder. replace codegen with generated
| Ok(()) | ||
| } | ||
|
|
||
| #[test] |
Contributor
There was a problem hiding this comment.
all these tests should be at the bottom, in a #[cfg(test)] mod tests {} block.
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
2 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.
This PR was implemented with AI assistance from Codex.
Summary
Foundation for #5977. Adds the global-types codegen scaffold, pinned TypeScript source acquisition, collector coverage, manifest-driven predefined globals, and stricter resolver-builder invariants.
Test Plan
Green CI