fix(runtime): clean up test helper allocations - #3
Conversation
💡 Codex ReviewbamTiScript/crates/bamts-runtime/src/builtins/promise.rs Lines 391 to 392 in 3ad8f81  Validate the promise receiver before resolving its species
When bamTiScript/crates/bamts-runtime/src/builtins/promise.rs Lines 410 to 411 in 3ad8f81 Avoid creating an unused promise for every then call
The builtin creates and returns bamTiScript/crates/bamts-runtime/src/lib.rs Lines 1258 to 1262 in 3ad8f81  Charge retained Value vectors by their byte size
The new heap ledger counts each retained bound argument as one byte, while an bamTiScript/crates/bamts-cli/build.rs Lines 512 to 517 in 3ad8f81 Preserve the AOT runtime in cross-built CLI artifacts
For every ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
💡 Codex Review Parse string primitives returned during Date coercion
For one-argument construction with an object whose bamTiScript/crates/bamts-runtime/src/builtins/date.rs Lines 76 to 77 in 79d1261 Invoke observable coercion for Date components
When any multi-argument Date component is an object, Interpret component-based Dates in local time
In environments whose local timezone is not UTC, multi-argument construction is shifted incorrectly because bamTiScript/crates/bamts-runtime/src/builtins/date.rs Lines 210 to 211 in 79d1261 Interpret offset-less date-times in local time
When a date-time string has no Coerce array-like typed-array lengths observably
When an array-like source has an object-valued bamTiScript/crates/bamts-runtime/src/lib.rs Lines 5115 to 5119 in 79d1261 Keep host process globals out of vm sandboxes
When bamTiScript/crates/bamts-runtime/src/builtins/promise.rs Lines 391 to 392 in 79d1261 Validate Promise receivers before species lookup
For a non-Promise receiver, bamTiScript/crates/bamts-compiler/src/program.rs Lines 2060 to 2061 in 79d1261 Preserve star exports from external modules
When an bamTiScript/crates/bamts-runtime/src/builtins/uint8array.rs Lines 54 to 62 in 79d1261 Keep Uint8Array length off instances
Typed-array bamTiScript/crates/bamts-runtime/src/builtins/collections.rs Lines 175 to 176 in 79d1261 Install async-generator return and throw methods
The new async-generator prototype installs only bamTiScript/crates/bamts-runtime/src/lib.rs Lines 8330 to 8333 in 79d1261 Charge queued async-generator requests
When an async generator is parked on an unresolved await, every additional ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bamts-codegen/src/jit.rs (1)
21-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "pinned 30-entry table" doc comment — this is exactly the flaw called out before, and it's still not fixed.
HELPER_COUNTis 34 now (ConsumeFuel = 30,CreateCell = 31,IteratorStep = 32,IteratorResult = 33, confirmed bycodegen_and_native_helper_tables_are_identicaliterating0..bamts_native::HELPER_COUNT). TheUnknownHelperdoc at line 28 still says "30-entry table." You added four helpers and didn't touch the one comment that describes the table size — the previous review told you this exact wording would rot the moment the count changed, and it just did.🐛 Proposed fix
- /// Lowered IR named a runtime helper outside the pinned 30-entry table. + /// Lowered IR named a runtime helper outside the pinned `bamts_native::HELPER_COUNT`-entry table. UnknownHelper { index: u32 },🤖 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/bamts-codegen/src/jit.rs` around lines 21 - 30, Update the UnknownHelper documentation in JitError to remove the stale “30-entry table” wording and describe the runtime helper table using its current or authoritative count symbol, preserving the existing error variant and behavior.
🟡 Other comments (1)
crates/bamts-node/src/lib.rs-22-25 (1)
22-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe doc on a public constant is wrong about which slot it replaces.
AOT_ENTRYPOINT_ENVnever replacesargv[0]— line 631 hard-codes"bamts"there, and the entrypoint lands inargv[1]. Line 587-589 has the mirror-image error: the token is compared againstprocess_args.get(1), i.e. the argument after the executable, not "its first argument". Public docs that contradict the code are worse than no docs.📝 Say what the code does
/// Parent-to-AOT-child launch proof paired with the first private argument. /// -/// Both copies must match before `AOT_ENTRYPOINT_ENV` can replace `argv[0]`. +/// Both copies must match before `AOT_ENTRYPOINT_ENV` can supply `argv[1]`, +/// the logical entrypoint reported to JavaScript. pub const AOT_LAUNCH_TOKEN_ENV: &str = "BAMTS_AOT_LAUNCH_TOKEN";🤖 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/bamts-node/src/lib.rs` around lines 22 - 25, Correct the public documentation for AOT_LAUNCH_TOKEN_ENV to describe the actual argument layout: the executable remains argv[0], the launch token is checked in argv[1], and AOT_ENTRYPOINT_ENV is placed in the entrypoint argument slot rather than replacing argv[0]. Update the nearby argument-handling documentation to use precise wording consistent with the process_args indexing.
🧹 Nitpick comments (4)
crates/bamts-cli/tests/cli.rs (1)
92-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueYou built
command()to stop hand-rolling this, then hand-rolled it anyway.Lines 96 and 105 re-set
current_dirto the same path the helper already configures. Redundant today, and the momentcommand()grows a subdirectory variant this override quietly wins.♻️ Drop the redundant overrides
let jit = project .command() .env_remove("BAMTS_AOT_ENTRYPOINT") .args(["run", "--target", "jit", "main.ts", "--", "first", "second"]) - .current_dir(&project.path) .output() .expect("bamts JIT argv program starts");(and the same for the AOT invocation)
🤖 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/bamts-cli/tests/cli.rs` around lines 92 - 108, Remove the redundant current_dir(&project.path) calls from both the JIT and AOT invocations in the test, relying on the project.command() helper to configure the working directory. Keep the existing command arguments, environment setup, and assertions unchanged.crates/bamts-compiler/src/pipeline.rs (1)
150-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
smallest_containingsilently assumes sorted, non-overlapping siblings.
partition_point+checked_sub(1)is only correct while everychildrenvector is ordered byrange.start()and siblings never overlap. That holds today because the parser hands back source order, but nothing in the type enforces it and the next person who appends a child out of order gets wrong-node attribution, not a panic. A one-line debug assertion when constructingEdgeNodewould pin the invariant.🤖 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/bamts-compiler/src/pipeline.rs` around lines 150 - 165, Add a debug assertion at the EdgeNode construction point verifying each children vector is ordered by range.start() and sibling ranges do not overlap. Preserve smallest_containing’s partition_point-based lookup, and ensure the assertion covers every constructed EdgeNode so violations are detected during development.crates/bamts-node/src/lib.rs (1)
619-635: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueA half-populated transport leaks the launch token into
process.argv.If a child ever receives
AOT_LAUNCH_TOKEN_ENVwith a matchingargv[1]but noAOT_ENTRYPOINT_ENV, the tuple match falls through toNone,first_program_argumentstays1, and the raw token is handed to the program as its first argument.driver.rsalways sets both, so this is unreachable today — which is precisely why nobody will notice when it stops being unreachable.Consume the token whenever it authenticates, independent of whether the entrypoint arrived.
🤖 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/bamts-node/src/lib.rs` around lines 619 - 635, Update the transported-entrypoint parsing around transported_entrypoint so a matching AOT_LAUNCH_TOKEN_ENV and process_args.get(1) always consumes the authenticated token, even when AOT_ENTRYPOINT_ENV is absent. Preserve the entrypoint as optional in that case, but set the argument offset to skip argv[1] whenever authentication succeeds.crates/bamts-compiler/src/rules/semantic/mod.rs (1)
778-796: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueStatic and instance accessors share one
accessor_typesmap, so they can falsely diverge.
accessor_typesis keyed by property name only.static get x(): numberplus instanceset x(v: string)are two unrelated members, but this reports BAMTS-W011. Key on(is_static, name).Same blind spot exists in
ClassFacts::accessorsfor the field-shadowing hazards, but that one at least errs toward reporting a real shape.♻️ Key the map by staticness
- if let Some(previous) = accessor_types.get(&name) + let key = (method.modifiers.is_static, name); + if let Some(previous) = accessor_types.get(&key) && previous != ¤t { self.push(SemanticHazard::DivergentAccessor, member.range()); } - accessor_types.insert(name, current); + accessor_types.insert(key, current);🤖 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/bamts-compiler/src/rules/semantic/mod.rs` around lines 778 - 796, Update the accessor type tracking around accessor_types so its key includes both the member’s staticness and property name, using a tuple such as (is_static, name). Ensure the lookup and insertion use this composite key, preventing unrelated static and instance accessors from being compared while preserving divergence detection within the same scope.
🤖 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.
Outside diff comments:
In `@crates/bamts-codegen/src/jit.rs`:
- Around line 21-30: Update the UnknownHelper documentation in JitError to
remove the stale “30-entry table” wording and describe the runtime helper table
using its current or authoritative count symbol, preserving the existing error
variant and behavior.
---
Other comments:
In `@crates/bamts-node/src/lib.rs`:
- Around line 22-25: Correct the public documentation for AOT_LAUNCH_TOKEN_ENV
to describe the actual argument layout: the executable remains argv[0], the
launch token is checked in argv[1], and AOT_ENTRYPOINT_ENV is placed in the
entrypoint argument slot rather than replacing argv[0]. Update the nearby
argument-handling documentation to use precise wording consistent with the
process_args indexing.
---
Nitpick comments:
In `@crates/bamts-cli/tests/cli.rs`:
- Around line 92-108: Remove the redundant current_dir(&project.path) calls from
both the JIT and AOT invocations in the test, relying on the project.command()
helper to configure the working directory. Keep the existing command arguments,
environment setup, and assertions unchanged.
In `@crates/bamts-compiler/src/pipeline.rs`:
- Around line 150-165: Add a debug assertion at the EdgeNode construction point
verifying each children vector is ordered by range.start() and sibling ranges do
not overlap. Preserve smallest_containing’s partition_point-based lookup, and
ensure the assertion covers every constructed EdgeNode so violations are
detected during development.
In `@crates/bamts-compiler/src/rules/semantic/mod.rs`:
- Around line 778-796: Update the accessor type tracking around accessor_types
so its key includes both the member’s staticness and property name, using a
tuple such as (is_static, name). Ensure the lookup and insertion use this
composite key, preventing unrelated static and instance accessors from being
compared while preserving divergence detection within the same scope.
In `@crates/bamts-node/src/lib.rs`:
- Around line 619-635: Update the transported-entrypoint parsing around
transported_entrypoint so a matching AOT_LAUNCH_TOKEN_ENV and
process_args.get(1) always consumes the authenticated token, even when
AOT_ENTRYPOINT_ENV is absent. Preserve the entrypoint as optional in that case,
but set the argument offset to skip argv[1] whenever authentication succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: f2068046-5c6c-4d8d-a990-3f073a859e71
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/bamts-cli/src/driver.rscrates/bamts-cli/tests/cli.rscrates/bamts-codegen/src/aot.rscrates/bamts-codegen/src/jit.rscrates/bamts-compiler/src/checker.rscrates/bamts-compiler/src/pipeline.rscrates/bamts-compiler/src/rules/mod.rscrates/bamts-compiler/src/rules/semantic/mod.rscrates/bamts-native/src/native_bridge.rscrates/bamts-node/Cargo.tomlcrates/bamts-node/src/lib.rscrates/bamts-runtime/Cargo.tomlcrates/bamts-verification/src/corpus.rscrates/bamts-verification/src/workspace_guard.rs
📜 Review details
🔇 Additional comments (25)
Cargo.toml (1)
30-30: LGTM!crates/bamts-cli/src/driver.rs (2)
400-408: LGTM!
443-482: LGTM!crates/bamts-verification/src/workspace_guard.rs (3)
810-810: LGTM!Also applies to: 819-829
840-874: LGTM!
1743-1777: LGTM!crates/bamts-verification/src/corpus.rs (1)
736-742: LGTM!crates/bamts-compiler/src/rules/semantic/mod.rs (2)
6-6: LGTM!Also applies to: 339-339, 359-370, 808-808
1599-1621: LGTM!crates/bamts-node/Cargo.toml (1)
32-33: LGTM!crates/bamts-runtime/Cargo.toml (1)
3-3: The AI summary claims this file pinsversion = "0.1.2"; the code inherits from the workspace instead. The code is what matters and it's fine — but the summary is describing something that isn't here.crates/bamts-cli/tests/cli.rs (1)
79-91: LGTM!Also applies to: 109-112
crates/bamts-compiler/src/pipeline.rs (2)
393-403: LGTM!Also applies to: 615-700
82-141: 🗄️ Data Integrity & IntegrationNo traversal change is needed here. Static imports and re-exports are registered in
exact, so theirNodeIdalways points to the owning top-level statement. Dynamic imports intentionally use the nearest traversed statement and are not consumed bycollect_program_facts; the missing expression traversal does not affecttype_onlyhandling.> Likely an incorrect or invalid review comment.crates/bamts-node/src/lib.rs (1)
782-839: LGTM!crates/bamts-compiler/src/rules/mod.rs (1)
1284-1291: LGTM!Also applies to: 1485-1518, 1524-1565, 1919-1940
crates/bamts-compiler/src/checker.rs (3)
766-766: LGTM!Also applies to: 815-819, 1070-1070, 1091-1091, 1142-1142, 1416-1416, 2237-2269
1045-1059: LGTM!
1363-1363: LGTM!Also applies to: 1728-1731, 3644-3651
crates/bamts-codegen/src/aot.rs (2)
239-299: Tuple-identity lookup replaces positional trust — past major issue resolved.
define_functionsnow builds a(module_id, function_id) -> FuncIdmap fromunitsup front and looks up each lowered function by identity instead of walkingunitspositionally in lockstep withlowered.modules. Duplicate declarations and missing declarations are both now caught explicitly. This is exactly the fix requested in the earlier consolidated review (aot.rs#L265-L275/jit.rs#L172-L184).Covered by the new
define_functions_rejects_mismatched_declared_identitytest. LGTM.
627-650: LGTM!crates/bamts-native/src/native_bridge.rs (2)
46-51: Safety contract now lives in one place — consistent with the previously requested consolidation.Only one
# Safetyblock shows up in the diff (ahead of the variant table), and the four new helpers (ConsumeFuel/CreateCell/IteratorStep/IteratorResult) carry no per-variant repeats. That matches the earlier ask to hoist the contract once instead of pasting it onto everyNativeHelpervariant. Good — no regression here.
503-509: LGTM!module_id()is a clean accessor and lines up with the new(module_id, function_id)invocation identity used by both JIT and AOT backends.crates/bamts-codegen/src/jit.rs (2)
75-134: Module-identity mismatch and positional-order trust are both closed out now.
invokeresolves the tuple viabinary_search_by_keyand then callsrequire_frame_module_id(frame, entry.module_id)before touching native code — no more blindly trustingframe.module_idagainst the selected entry, which was the past critical finding.compile_loweredadds the missing invariant check (module_id as usize != module_index,function_id as usize != function_index) before declaring anything, on top of the tuple-keyeddeclared_functionsmap from the prior fix. Both halves of the earlier consolidated ask are now in place.Also applies to: 168-259
344-1000: LGTM! Good coverage: duplicate local function IDs across modules, frame/module mismatch rejection, matched program-bytes round-trip, exact fuel boundaries, callback re-entry shared budgets, fuel-bypasses-handler ordering, executable/writable page checks, and the newcompile_lowered_rejects_out_of_order_function_identityregression test for the ordering invariant.
|
@codex review |
|
@coderabbitai ultrareview |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 15 minutes. |
💡 Codex ReviewbamTiScript/crates/bamts-runtime/src/lib.rs Line 1308 in a7952f3 Charge closure capture storage against heap bytes
When bytecode repeatedly creates closures with large capture lists, each bamTiScript/crates/bamts-runtime/src/lib.rs Lines 4934 to 4937 in a7952f3 Keep tombstoned slots within the slot ceiling
With a host-supplied high fuel limit, a program can repeatedly allocate unreachable objects and trigger collection while bamTiScript/crates/bamts-cli/src/driver.rs Lines 585 to 586 in a7952f3 Verify cached runtime archives before linking
When bamTiScript/crates/bamts-runtime/src/builtins/promise.rs Lines 478 to 481 in a7952f3 Resolve Promise.all capabilities without an extra promise hop
When bamTiScript/crates/bamts-runtime/src/builtins/promise.rs Lines 240 to 244 in a7952f3 Check both capability slots before accepting another invocation
A custom promise constructor can first invoke the capability executor with ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Addressed the actionable items from the latest review body.
Addressed:
Addressed: the AOT transport docs now state that the executable or authenticated entrypoint occupies logical
Addressed: a matching private token always consumes
Addressed: every
Addressed: accessor type pairs are keyed by staticness and property name. A regression keeps cross-domain accessors separate while preserving same-domain divergence reports.
Not addressing: Validation: workspace tests passed except one AOT corpus timeout under the full parallel run; that exact differential test passed on immediate rerun. Workspace Clippy passes with warnings denied. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cd22f5157
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let ClassMember::Method(method) = member.data() | ||
| && let Some(member_name) = self.property_name(&method.name) | ||
| { | ||
| methods.insert(member_name.clone()); | ||
| if matches!( | ||
| method.modifier, | ||
| PropertyModifier::Get | PropertyModifier::Set | ||
| ) { | ||
| accessors.insert(member_name); |
There was a problem hiding this comment.
When a base class has only a static member, such as class B { static run() {} } class D extends B { run() {} }, this inserts run into the same set used for instance inheritance checks. The derived instance method is consequently reported as BAMTS-W041, and a base static accessor similarly makes an instance field appear to shadow an inherited accessor. Record staticness in these facts or exclude static members from the instance-member sets.
Useful? React with 👍 / 👎.
| .as_ref() | ||
| .and_then(|name| self.classes.get(self.identifier(name).as_ref())) | ||
| .and_then(|class| class.base.as_ref()) | ||
| .and_then(|base| self.classes.get(base)) | ||
| .cloned(); |
There was a problem hiding this comment.
The selected base facts contain only the immediate base class's own members; the stored ClassFacts::base chain is never traversed. Therefore class B { run() {} } class M extends B {} class D extends M { run() {} } misses BAMTS-W041, and the analogous inherited-accessor case misses the field-shadowing diagnostics. Walk or precompute the full ancestor member set before checking the derived class.
Useful? React with 👍 / 👎.
| let covered = switch | ||
| .cases | ||
| .iter() | ||
| .filter(|case| case.data().test.is_some()) | ||
| .count(); | ||
| if covered < variant_count && !switch.cases.iter().any(|case| case.data().test.is_none()) { |
There was a problem hiding this comment.
Coverage is inferred only from the number of non-default cases, so duplicate or unrelated labels can suppress BAMTS-W063. For example, a two-variant union switched with two case 'a' labels is considered exhaustive even though the 'b' variant remains reachable. Track the distinct union variants matched by each case instead of comparing the raw case count.
Useful? React with 👍 / 👎.
| if matches!(method.as_str(), "toString" | "toFixed") | ||
| && let Some(CallArgument::Expression(argument)) = call.arguments.first() | ||
| && let Expression::Literal(Literal::Number(number)) = argument.data() | ||
| && let Ok(value) = self | ||
| .source | ||
| .token_text(number.data().token()) | ||
| .unwrap_or("") | ||
| .parse::<i32>() | ||
| && ((method == "toString" && !(2..=36).contains(&value)) | ||
| || (method == "toFixed" && !(0..=100).contains(&value))) |
There was a problem hiding this comment.
This emits the default-warning BAMTS-W071 solely from the method name and literal argument, without checking the receiver. Valid calls such as "value".toString(1) or a user-defined formatter.toFixed(101) are therefore diagnosed even though these methods need not implement Number formatting semantics. Resolve or infer the callee's receiver before applying the numeric range checks.
Useful? React with 👍 / 👎.
| if parameter.data().type_annotation.is_none() { | ||
| self.push(SemanticHazard::ImplicitAny, parameter.range()); | ||
| } |
There was a problem hiding this comment.
A missing annotation does not imply an inferred any, but this raises the default-warning BAMTS-W018 for every unannotated parameter. For example, function f(value = 1) { return value; } has an inferred number, and contextually typed callbacks likewise have concrete parameter types, yet both are reported as implicit-any hazards. Consult the semantic model's resolved parameter type and warn only when that type is actually any.
Useful? React with 👍 / 👎.
| Expression::Call(call) => { | ||
| if let Expression::Identifier(identifier) = call.callee.data() { | ||
| self.called_names | ||
| .insert(self.identifier(identifier).into_owned()); |
There was a problem hiding this comment.
Called aliases are collected into one file-wide set keyed only by identifier text. Thus const f = obj.method; function g() { const f = () => {}; f(); } reports the outer assignment as BAMTS-W010 even though the call resolves to the inner binding and the detached method is never invoked. Store the resolved symbol for each call, or otherwise respect lexical scopes, before matching calls to detached assignments.
Useful? React with 👍 / 👎.
| fn visit_statements(&mut self, statements: &[Stmt]) { | ||
| for statement in statements { | ||
| self.visit_statement(statement, false); | ||
| } |
There was a problem hiding this comment.
Entering a block, switch case, or try/catch/finally body routes through visit_statements, which resets in_constructor to false. Consequently class B { constructor() { { this.init(); } } } and equivalent calls inside try or switch bodies miss BAMTS-W038, even though they perform the same virtual dispatch as a direct constructor statement. Pass the current constructor context through nested statement-list traversal.
Useful? React with 👍 / 👎.
| let constructor = species_constructor(machine, this)?; | ||
| let capability = new_promise_capability(machine, constructor)?; |
There was a problem hiding this comment.
Promise.prototype.then must reject a non-Promise receiver before observing its constructor or creating a derived capability, but this calls species_constructor and the selected constructor first; the actual Promise brand check occurs only in the later promise_then call. With an invalid receiver that has a getter or custom species constructor, those user side effects run before the eventual TypeError. Perform the Promise brand check before species lookup and capability construction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/bamts-compiler/src/lint.rs (2)
1889-1907: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winOne root cause: the JavaScript dialect policy lives in hardcoded rule-code strings instead of in the rule table.
level_for_sourcedecides dialect behaviour from amatches!over eleven literal codes plus two copies of a single"BAMTS-W085"exception. Adding a rule therefore gets the wrong policy by default, which is exactly how the newno-withrule lost its declaredDeny.
crates/bamts-compiler/src/lint.rs#L1889-L1907: replace the literal code lists and the"BAMTS-W085"special cases with a per-ruleDialectPolicystored onRuleDefinition, so the registry is the single source of truth.crates/bamts-compiler/src/lint.rs#L1413-L1421: giveBAMTS-W088(no-with) the policy that keeps its declared level in JavaScript, sincewithis legal only in sloppy-mode JavaScript, and add alevel_for_sourceassertion for it to the dialect test at lines 2297-2325.🤖 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/bamts-compiler/src/lint.rs` around lines 1889 - 1907, The JavaScript dialect policy is hardcoded in level_for_source instead of the rule registry. Add a per-rule DialectPolicy field to RuleDefinition, assign BAMTS-W088 the policy that preserves its declared level for JavaScript, and update level_for_source to use that metadata rather than literal rule codes or BAMTS-W085 exceptions. Extend the dialect test assertions to cover BAMTS-W088.
1889-1907: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStop encoding lint policy as a list of magic strings.
javascript_ruleis amatches!over eleven literal codes. W087 was appended even though its group isOpinionated, not the W071-W080 runtime-footgun family the list otherwise represents. Line 1886 and line 1906 then carry two more copies of a single hardcoded code.This design guarantees the bug I flagged on the W088 registration: dialect behaviour lives in a hand-maintained string list instead of in
RuleDefinition, so adding a rule silently gets the wrong policy. Put the policy on the rule.♻️ Proposed refactor: derive the policy from rule metadata
+/// How the JavaScript dialect treats this rule. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DialectPolicy { + /// The declared level applies unchanged in every dialect. + Always, + /// The rule stays active in JavaScript but never above `Warn`. + ClampToWarn, + /// The rule is TypeScript-only. + TypeScriptOnly, +}Store a
dialect_policyon eachRuleDefinition, then reduce the resolver to one lookup:pub fn level_for_source(&self, rule: RuleId, dialect: SourceDialect) -> LintLevel { if dialect == SourceDialect::TypeScript { return self.level(rule); } - if rule.code() == "BAMTS-W085" { - return self.level(rule); - } - let javascript_rule = matches!(rule.code(), "BAMTS-W071" | /* ... */ "BAMTS-W087"); - let control_flow = RULES[rule_index(rule)].group() == RuleGroup::ControlFlow; - let javascript_compatibility = RULES[rule_index(rule)].group() - == RuleGroup::JavaScriptCompatibility - && rule.code() != "BAMTS-W085"; - if javascript_rule || control_flow || javascript_compatibility { - let effective = self.level(rule); - return if effective == LintLevel::Allow { - LintLevel::Allow - } else { - LintLevel::Warn - }; - } - LintLevel::Allow + match RULES[rule_index(rule)].dialect_policy() { + DialectPolicy::Always => self.level(rule), + DialectPolicy::ClampToWarn => match self.level(rule) { + LintLevel::Allow => LintLevel::Allow, + _ => LintLevel::Warn, + }, + DialectPolicy::TypeScriptOnly => LintLevel::Allow, + } }🤖 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/bamts-compiler/src/lint.rs` around lines 1889 - 1907, Move dialect behavior into RuleDefinition by adding a dialect_policy field and assigning the appropriate policy to each rule, including the W071–W080 family and exceptions such as W085, W087, and W088. Replace the javascript_rule, control_flow, and javascript_compatibility magic-string checks with a single lookup of the current rule’s metadata policy via RULES and rule_index(rule). Remove the duplicated hardcoded code comparisons so newly registered rules derive their behavior from RuleDefinition.
🟡 Other comments (4)
crates/bamts-native/src/native_bridge.rs-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo hand-copied helper counts, both stale the moment this PR landed.
HELPER_COUNTis 36 andfrom_u32resolves index 35, yet both doc sites still say 35.HELPER_COUNTis the authority; prose that restates it as a literal is a maintenance tax that nobody pays until it is already wrong.
crates/bamts-native/src/native_bridge.rs#L1-L1: replace "the exact 35" in the module header with a phrasing tied toHELPER_COUNT, not a literal.crates/bamts-native/src/native_bridge.rs#L754-L754: change the section banner "The exact 35 exported C-ABI helpers" the same way, so the two headings cannot drift apart from each other either.🤖 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/bamts-native/src/native_bridge.rs` at line 1, Update the module header at crates/bamts-native/src/native_bridge.rs lines 1-1 and the section banner at lines 754-754 to describe the helper count through the authoritative HELPER_COUNT symbol instead of the literal 35; keep both headings consistent and avoid changing HELPER_COUNT itself.crates/bamts-compiler/src/rules/mod.rs-49-65 (1)
49-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
EscapedReservedWordtokens are left with their raw spelling.The filter at Line 55 admits only
IdentifierandEscapedContextualKeyword. An escaped reserved word always cooks to an owned string, so it never enters the map andSyntaxToken::textkeeps the backslashes. Every rule that comparestoken.textthen fails to match the cooked spelling. An escaped reserved word is a legalIdentifierName, so it reaches member expressions and property keys, for exampleobj.\u{69}f.Add
TokenKind::EscapedReservedWordto the filter. The token kind still distinguishes it, so no rule loses the ability to reject it.♻️ Include escaped reserved words in the cooked map
matches!( token.kind(), - TokenKind::Identifier | TokenKind::EscapedContextualKeyword + TokenKind::Identifier + | TokenKind::EscapedReservedWord + | TokenKind::EscapedContextualKeyword )Separately, keying the map on
(range.start, range.end)re-derives token identity from positions. The token index is unique and cheaper; aVec<Option<String>>parallel tosource.tokens()removes the map and the tuple key.🤖 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/bamts-compiler/src/rules/mod.rs` around lines 49 - 65, Update the cooked-identifier collection in the rules module to include TokenKind::EscapedReservedWord alongside Identifier and EscapedContextualKeyword, preserving token-kind information for rule validation. Also replace the position-keyed BTreeMap with a Vec<Option<String>> aligned to source.tokens(), storing cooked text by token index and updating lookups to use that index.crates/bamts-compiler/src/scanner.rs-1208-1249 (1)
1208-1249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd strict-mode handling for escaped identifier names.
The scanner emits escaped
implements,interface,package,private,protected,public,let, andstaticasIdentifier. It emits escapedyieldas contextual, but the parser rejects it only in generator contexts. The parser has no strict or module state, so module code can acceptimport "x"; let \u{69}mplements = 1;without a diagnostic. Add strict-mode checks for these names, plus module-specificawaithandling, or document the deviation.🤖 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/bamts-compiler/src/scanner.rs` around lines 1208 - 1249, Extend strict-mode reserved-word handling beyond is_unconditional_reserved_word to reject escaped implements, interface, package, private, protected, public, let, static, and generator-context yield identifiers; add module-specific rejection for escaped await. Because the parser lacks strict/module state, introduce or propagate the necessary context through the scanner/parser, or explicitly document the accepted deviation if enforcing these checks is not feasible.crates/bamts-bytecode/src/lib.rs-500-500 (1)
500-500: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe opcode range in this doc comment is wrong.
The stable tags now run 0..=41.
RequireCloseResultencodes as 41 at Line 3058 and decodes at Line 2606, and the round-trip test at Line 3477 asserts 42 cases. A wire-format comment that undercounts the tag space by one is exactly the kind of stale documentation that later gets treated as the contract. Fix the number.📝 Proposed fix
-/// The production instruction algebra. Opcodes 0..=40 are stable wire tags. +/// The production instruction algebra. Opcodes 0..=41 are stable wire tags.🤖 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/bamts-bytecode/src/lib.rs` at line 500, Update the production instruction algebra doc comment to state that stable wire tags span opcodes 0..=41, keeping the surrounding documentation unchanged.
🧹 Nitpick comments (5)
crates/bamts-verification/tests/corpus_differential.rs (1)
230-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate
target/before writing the fixture.
rootis the workspace directory, not the build directory. IfCARGO_TARGET_DIRpoints somewhere else,root/targetmay not exist, andfs::writefails with the useless messagewrite iterator-close fixture. Callfs::create_dir_allon the parent first, or place the fixture in an existing directory.♻️ Proposed change
let path = root.join(&entrypoint); + fs::create_dir_all(path.parent().expect("fixture path has a parent")) + .expect("create fixture directory"); fs::write(🤖 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/bamts-verification/tests/corpus_differential.rs` around lines 230 - 234, Ensure the fixture-writing setup around root, entrypoint, and path creates the destination’s parent directory with fs::create_dir_all before calling fs::write. Preserve the existing fixture path and error context while making the write work when target does not yet exist.crates/bamts-native/src/native_bridge.rs (1)
2956-2991: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe close wrapper's guard has no test.
bamts_iterator_closerejects an out-of-rangecalled_regwithTRAP_INVALID_REGISTERon Lines 1467-1472. Its sibling guard inbamts_iterator_resulthasiterator_result_out_of_range_register_is_fatal_trap. This one has nothing. An untested guard is a guard that silently rots.Add the sibling test.
💚 The missing test, shaped like its neighbour
#[test] fn iterator_close_out_of_range_register_is_fatal_trap() { let mut regs = [Value::UNINITIALIZED; 1]; let mut frame = frame_with(&mut regs); let mut completion = Completion::new(Value::UNDEFINED); let mut ops = Recorder::normal(Value::UNDEFINED); let tag = with_native_ops(&mut ops, || { test_bamts_iterator_close(&mut frame, Value::NULL.to_bits(), 0, 99, &mut completion) }); assert_eq!(tag, CompletionTag::FatalTrap.as_u32()); assert_eq!(completion.value.as_int32(), Some(TRAP_INVALID_REGISTER)); // The dispatcher was never reached. assert_eq!(regs[0], Value::UNINITIALIZED); assert_eq!(ops.last.get(), 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 `@crates/bamts-native/src/native_bridge.rs` around lines 2956 - 2991, Add a sibling test for bamts_iterator_close named iterator_close_out_of_range_register_is_fatal_trap. Invoke it with an invalid called_reg, assert a FatalTrap tag and TRAP_INVALID_REGISTER completion, and verify the register remains uninitialized and no dispatcher operation was recorded.crates/bamts-codegen/src/lib.rs (1)
2282-2294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis ABI test does not check the emitted helper names.
Every sibling test in this file asserts the external name:
u1:32for iterator-step,u1:33for iterator-result,u1:27for get-iterator. This one asserts the constants34and35and two signature strings, then stops.Helper::IteratorClose.external_index() == 34proves the constant, not that lowering emittedu1:34. Wire the wrong index intohelper_refand this test still passes.Also note the signature
(i64, i64, i32, i32, i64) -> i32is shared withIteratorNextandIteratorResult, so it identifies nothing on its own.💚 Pin the imports like the neighbours do
assert_eq!(Helper::IteratorClose.external_index(), 34); assert_eq!(Helper::RequireCloseResult.external_index(), 35); + assert!( + clif.contains("u1:34"), + "iterator-close import missing:\n{clif}" + ); + assert!( + clif.contains("u1:35"), + "require-close-result import missing:\n{clif}" + ); assert!( clif.contains("(i64, i64, i32, i32, i64) -> i32"), "iterator-close helper sig wrong:\n{clif}" );🤖 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/bamts-codegen/src/lib.rs` around lines 2282 - 2294, Strengthen the ABI assertions in the test around lower_one by pinning the emitted helper names through helper_ref, asserting u1:34 for IteratorClose and u1:35 for RequireCloseResult. Keep the existing presence, external_index, and signature checks, and import or reuse the same helper_ref mechanism used by neighboring ABI tests.crates/bamts-cli/tests/cli.rs (1)
332-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an astral escaped identifier to this fixture.
Every escaped name here is a BMP code point, so the surrogate-pair path is untested. Add a name spelled with two fixed-width escapes, for example
const \uD835\uDC65 = 1;used as𝑥, and one spelled with\u{1D465}. Both spellings denote the same identifier, so the test also pins the equivalence.I flagged the underlying decoding gap on
cook_identifier_textincrates/bamts-compiler/src/syntax.rs; this fixture is where the regression test belongs.🤖 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/bamts-cli/tests/cli.rs` around lines 332 - 354, The test function aot_and_jit_share_escaped_identifier_identity only covers BMP escapes. Extend its fixture with an astral identifier represented both by a surrogate-pair escape and a code-point escape, use those equivalent names in the program, and preserve the existing JIT/AOT assertion so both spellings resolve to the same identifier.crates/bamts-compiler/src/rules/semantic/mod.rs (1)
1462-1501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shadowed-intrinsic check instead of writing the same double-negated boolean twice.
Lines 1480-1482 and 1495-1498 both compute "is this identifier the real global, or at least unresolved" using the same pattern:
!model.reference(id).is_some_and(|symbol| model.symbol(symbol).kind() != SymbolKind::IntrinsicValue)This is a double negative wrapped in
is_some_and. Read it twice before you trust it. That is not a good sign for code that decides whether an assignment counts as a CommonJS export. Nobody touches this logic again without re-deriving the truth table from scratch, and the next person who "simplifies" it will invert the polarity by accident.Pull it into one function:
fn is_unshadowed_intrinsic(model: &SemanticModel, id: NodeId) -> bool { !model .reference(id) .is_some_and(|symbol| model.symbol(symbol).kind() != SymbolKind::IntrinsicValue) }Call it at both sites. Same behavior, one place to read, one place to get right.
🤖 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/bamts-compiler/src/rules/semantic/mod.rs` around lines 1462 - 1501, Extract the repeated shadowed-intrinsic predicate into an is_unshadowed_intrinsic helper accepting SemanticModel and NodeId, preserving the existing double-negated behavior. Replace both checks in the CommonJS export logic around module.exports and identifier assignments with calls to this helper, leaving the surrounding export detection unchanged.
🤖 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/bamts-compiler/src/parser.rs`:
- Around line 4460-4466: The arrow parsers currently discard inherited keyword
reservations; update parse_simple_arrow, parse_paren_arrow,
speculate_paren_arrow, speculate_async_paren_arrow, and speculate_generic_arrow
to derive KeywordContext from self.keyword_context while preserving both
await_reserved and yield_reserved flags, adjusting only the async-specific state
as required. Add tests covering nested arrows inside async and generator
contexts to ensure escaped await and yield remain syntax errors.
In `@crates/bamts-compiler/src/program.rs`:
- Around line 2266-2270: Update identifier and metadata_export_name to return
Result<String, ProgramLowerError>, converting identifier_text returning None
into the existing metadata diagnostic instead of panicking. Propagate the Result
through all eight identifier call sites, preserving Some("") token text as valid
and distinguishing it from malformed escapes.
In `@crates/bamts-compiler/src/syntax.rs`:
- Around line 189-233: Update cook_identifier_text in
crates/bamts-compiler/src/syntax.rs#L189-L233 to decode escapes as UTF-16 code
units and combine valid surrogate pairs via String::from_utf16; apply the same
pairing behavior in scan_identifier_escape in
crates/bamts-compiler/src/scanner.rs. Extend the fixture in
crates/bamts-cli/tests/cli.rs#L332-L354 with identifiers spelled using both
\uD835\uDC65 and \u{1D465}, asserting they resolve to the same binding.
In `@crates/bamts-verification/tests/corpus_differential.rs`:
- Around line 405-413: Introduce one shared cleanup-guard helper for generated
fixture paths in the corpus differential tests, and use it in every
write/run/remove fixture test, including the flow around NodeOracle::discover,
BamtsRunner::run_case, and compare_case. The guard must remove the fixture
during Drop so cleanup occurs on both normal completion and unwinding; remove
the duplicated happy-path fs::remove_file calls after adopting it.
---
Outside diff comments:
In `@crates/bamts-compiler/src/lint.rs`:
- Around line 1889-1907: The JavaScript dialect policy is hardcoded in
level_for_source instead of the rule registry. Add a per-rule DialectPolicy
field to RuleDefinition, assign BAMTS-W088 the policy that preserves its
declared level for JavaScript, and update level_for_source to use that metadata
rather than literal rule codes or BAMTS-W085 exceptions. Extend the dialect test
assertions to cover BAMTS-W088.
- Around line 1889-1907: Move dialect behavior into RuleDefinition by adding a
dialect_policy field and assigning the appropriate policy to each rule,
including the W071–W080 family and exceptions such as W085, W087, and W088.
Replace the javascript_rule, control_flow, and javascript_compatibility
magic-string checks with a single lookup of the current rule’s metadata policy
via RULES and rule_index(rule). Remove the duplicated hardcoded code comparisons
so newly registered rules derive their behavior from RuleDefinition.
---
Other comments:
In `@crates/bamts-bytecode/src/lib.rs`:
- Line 500: Update the production instruction algebra doc comment to state that
stable wire tags span opcodes 0..=41, keeping the surrounding documentation
unchanged.
In `@crates/bamts-compiler/src/rules/mod.rs`:
- Around line 49-65: Update the cooked-identifier collection in the rules module
to include TokenKind::EscapedReservedWord alongside Identifier and
EscapedContextualKeyword, preserving token-kind information for rule validation.
Also replace the position-keyed BTreeMap with a Vec<Option<String>> aligned to
source.tokens(), storing cooked text by token index and updating lookups to use
that index.
In `@crates/bamts-compiler/src/scanner.rs`:
- Around line 1208-1249: Extend strict-mode reserved-word handling beyond
is_unconditional_reserved_word to reject escaped implements, interface, package,
private, protected, public, let, static, and generator-context yield
identifiers; add module-specific rejection for escaped await. Because the parser
lacks strict/module state, introduce or propagate the necessary context through
the scanner/parser, or explicitly document the accepted deviation if enforcing
these checks is not feasible.
In `@crates/bamts-native/src/native_bridge.rs`:
- Line 1: Update the module header at crates/bamts-native/src/native_bridge.rs
lines 1-1 and the section banner at lines 754-754 to describe the helper count
through the authoritative HELPER_COUNT symbol instead of the literal 35; keep
both headings consistent and avoid changing HELPER_COUNT itself.
---
Nitpick comments:
In `@crates/bamts-cli/tests/cli.rs`:
- Around line 332-354: The test function
aot_and_jit_share_escaped_identifier_identity only covers BMP escapes. Extend
its fixture with an astral identifier represented both by a surrogate-pair
escape and a code-point escape, use those equivalent names in the program, and
preserve the existing JIT/AOT assertion so both spellings resolve to the same
identifier.
In `@crates/bamts-codegen/src/lib.rs`:
- Around line 2282-2294: Strengthen the ABI assertions in the test around
lower_one by pinning the emitted helper names through helper_ref, asserting
u1:34 for IteratorClose and u1:35 for RequireCloseResult. Keep the existing
presence, external_index, and signature checks, and import or reuse the same
helper_ref mechanism used by neighboring ABI tests.
In `@crates/bamts-compiler/src/rules/semantic/mod.rs`:
- Around line 1462-1501: Extract the repeated shadowed-intrinsic predicate into
an is_unshadowed_intrinsic helper accepting SemanticModel and NodeId, preserving
the existing double-negated behavior. Replace both checks in the CommonJS export
logic around module.exports and identifier assignments with calls to this
helper, leaving the surrounding export detection unchanged.
In `@crates/bamts-native/src/native_bridge.rs`:
- Around line 2956-2991: Add a sibling test for bamts_iterator_close named
iterator_close_out_of_range_register_is_fatal_trap. Invoke it with an invalid
called_reg, assert a FatalTrap tag and TRAP_INVALID_REGISTER completion, and
verify the register remains uninitialized and no dispatcher operation was
recorded.
In `@crates/bamts-verification/tests/corpus_differential.rs`:
- Around line 230-234: Ensure the fixture-writing setup around root, entrypoint,
and path creates the destination’s parent directory with fs::create_dir_all
before calling fs::write. Preserve the existing fixture path and error context
while making the write work when target does not yet exist.
🪄 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: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 664899ff-efc8-40ec-a7b4-c70f624644ef
📒 Files selected for processing (23)
crates/bamts-bytecode/src/lib.rscrates/bamts-cli/tests/cli.rscrates/bamts-codegen/src/aot.rscrates/bamts-codegen/src/jit.rscrates/bamts-codegen/src/lib.rscrates/bamts-compiler/RULES.mdcrates/bamts-compiler/src/checker.rscrates/bamts-compiler/src/lint.rscrates/bamts-compiler/src/lower.rscrates/bamts-compiler/src/parser.rscrates/bamts-compiler/src/pipeline.rscrates/bamts-compiler/src/program.rscrates/bamts-compiler/src/rules/mod.rscrates/bamts-compiler/src/rules/semantic/mod.rscrates/bamts-compiler/src/scanner.rscrates/bamts-compiler/src/script.rscrates/bamts-compiler/src/syntax.rscrates/bamts-compiler/tests/rules.rscrates/bamts-native/src/native_bridge.rscrates/bamts-node/src/lib.rscrates/bamts-runtime/src/lib.rscrates/bamts-runtime/src/native.rscrates/bamts-verification/tests/corpus_differential.rs
📜 Review details
🧰 Additional context used
🔍 Remote MCP DeepWiki, Grep, Tavily, Valyu
Additional review context
- The PR page describes the migration as Program v4, AOT ABI v3, and 34 helpers, while the supplied diff summary reports AOT ABI v4 and 36 helpers. Verify all ABI/version documentation and constants for consistency.
- Review history repeatedly flags an unresolved module-identity mismatch: JIT/native dispatch selects
(module_id, function_id)but does not reconcileShadowFrame.module_id, risking execution with the wrong module context. ProgramDecoderreportedly preallocates vectors from attacker-controlled module/edge/binding/export counts before checking remaining input, creating allocation-amplification risk on tiny malformed payloads.- Runtime issues repeatedly identified include:
Array.fromeagerly drains iterators before invoking the mapper and lacks the required array-like fallback.Uint8Arrayinstances are ordinary objects, so indexed writes/deletes bypass typed-array semantics.decode_hexcan panic on guest-supplied non-ASCII input viafrom_utf8(...).expect(...).Object.definePropertycan overwrite non-configurable/frozen properties.
- The review history also records possible formal-model drift: the Lean bytecode model was flagged as lacking newer opcodes/envelope details despite the production format advancing to v4; verify the model and proof ledger against the actual Rust wire format.
Search limitations: DeepWiki could not index the repository, Tavily was rate-limited, and literal GitHub searches returned no matches.
🔇 Additional comments (56)
crates/bamts-verification/tests/corpus_differential.rs (2)
236-404: LGTM!
414-419: LGTM!crates/bamts-codegen/src/aot.rs (1)
21-21: LGTM!Also applies to: 464-502, 557-617, 628-648
crates/bamts-native/src/native_bridge.rs (2)
1459-1482: LGTM!Also applies to: 1492-1506, 1587-1590
2768-2810: LGTM!Also applies to: 2851-2886
crates/bamts-codegen/src/jit.rs (1)
28-28: LGTM!Also applies to: 328-329, 973-988, 991-1002
crates/bamts-codegen/src/lib.rs (4)
426-434: LGTM!Also applies to: 473-474, 520-521, 564-565, 635-638, 709-716
1492-1517: LGTM!
1892-1893: LGTM!Also applies to: 1956-1957, 2055-2056
2209-2260: LGTM!Also applies to: 3386-3416
crates/bamts-runtime/src/native.rs (5)
129-135: LGTM!Also applies to: 1037-1056
1613-1631: LGTM!
2917-2933: LGTM!
2982-2983: LGTM!
1605-1612: 🗄️ Data Integrity & IntegrationNo issue found. The lowerer emits
RequireCloseResultonly forIteratorCloseMode::Propagate; tests cover sync and async paths.crates/bamts-runtime/src/lib.rs (2)
2990-3030: LGTM!
4790-4838: 🗄️ Data Integrity & IntegrationKeep the current iterator-close lowering.
PreserveAbruptflows to dispatch and uses separate registers fromRequireCloseResult. APropagatefailure transfers to exception handling and does not continue to validation. The verifier does not mark close outputs as initialized on handler edges.> Likely an incorrect or invalid review comment.crates/bamts-compiler/RULES.md (1)
864-884: LGTM!crates/bamts-cli/tests/cli.rs (4)
114-151: LGTM!
153-210: LGTM!
212-279: LGTM!
281-330: LGTM!crates/bamts-compiler/src/scanner.rs (3)
741-753: LGTM!
771-777: LGTM!Also applies to: 872-872, 915-915, 938-938
1411-1431: 🎯 Functional CorrectnessThe escaped-keyword test inputs are correct. Both use
\\uand pass literal escapes to the scanner.> Likely an incorrect or invalid review comment.crates/bamts-node/src/lib.rs (2)
625-651: LGTM!
853-887: LGTM!crates/bamts-compiler/src/rules/mod.rs (1)
1145-1164: LGTM!crates/bamts-bytecode/src/lib.rs (6)
448-473: LGTM!Also applies to: 763-770, 796-797, 828-831, 845-845, 892-893
628-687: LGTM!Also applies to: 1917-1937
2085-2086: LGTM!Also applies to: 2338-2371, 2583-2609, 2634-2640, 3045-3061
1275-1277: LGTM!Also applies to: 1394-1398, 2134-2136, 2197-2199
3296-3296: LGTM!Also applies to: 3466-3477, 3718-3726, 3764-3777, 3779-3847, 3984-3998, 4633-4773, 5127-5160, 5169-5290
53-60: LGTM!Also applies to: 129-130
crates/bamts-compiler/src/pipeline.rs (1)
143-149: LGTM!crates/bamts-compiler/src/checker.rs (4)
16-16: LGTM!Also applies to: 1157-1161, 1307-1307, 1382-1382, 1393-1393, 1405-1405, 1426-1426, 1436-1436, 1479-1479, 1515-1515, 1540-1540, 1569-1569, 1579-1579, 1590-1590, 1618-1618, 1627-1627, 1805-1805, 1851-1851, 1896-1896, 1933-1933, 2202-2202, 2332-2332, 2464-2464
540-540: LGTM!Also applies to: 579-584, 1404-1423, 2332-2345, 2377-2383
1821-1844: LGTM!Also applies to: 1846-1874, 1966-1974
1146-1146: LGTM!Also applies to: 1223-1229, 1659-1683, 1731-1735
crates/bamts-compiler/src/lint.rs (2)
357-357: LGTM!Also applies to: 1404-1412, 2121-2121
2297-2297: LGTM!Also applies to: 2314-2325
crates/bamts-compiler/src/lower.rs (8)
43-52: LGTM!Also applies to: 69-76, 137-146, 156-158, 192-202, 234-243, 284-287, 433-438
572-594: LGTM!Also applies to: 603-627, 641-641, 676-676, 4870-4870, 5607-5607
1349-1349: LGTM!Also applies to: 1389-1447, 1469-1469, 1493-1609
1646-1646: LGTM!Also applies to: 1658-1663, 1677-1681, 1693-1693, 1707-1710, 1719-1719, 1757-1761, 1776-1776, 1792-1804, 1814-1831, 2218-2218, 2269-2272
2114-2201: LGTM!Also applies to: 2358-2372, 2449-2456, 2493-2569
3982-3996: LGTM!Also applies to: 7124-7270
873-876: LGTM!Also applies to: 4114-4114, 4266-4266, 6885-6891, 7328-7331, 7471-7513, 7530-7605, 8385-8546, 8690-8758
1892-1935: 🗄️ Data Integrity & IntegrationNo change needed.
emit_awaitsetsresumeto the next instruction, soIteratorResultreads initializedsettled. The cleanup and handler ranges are correct.crates/bamts-compiler/src/parser.rs (4)
83-83: LGTM!Also applies to: 200-205, 220-220, 254-254, 302-302, 364-364, 574-616
2544-2544: LGTM!Also applies to: 2833-2839, 3542-3542, 3978-3978
2036-2047: LGTM!Also applies to: 3853-3864, 4237-4248
3062-3086: LGTM!Also applies to: 5912-5927
crates/bamts-compiler/src/script.rs (1)
207-230: LGTM!crates/bamts-compiler/src/rules/semantic/mod.rs (1)
167-171: LGTM!The
identifier()helper now decodes escaped identifiers throughidentifier_textinstead of returning raw token text, and every call site listed above switches to it consistently. This closes the gap where\u{78}andxwould previously compare as different names. The static/instance accessor key fix at Lines 793-799, keyed by(method.modifiers.is_static, name), correctly separates static and instance accessor pairs, and the new test at Lines 1638-1654 nails down both the non-divergent and divergent cases.Also applies to: 202-202, 304-304, 325-331, 343-358, 471-471, 629-629, 669-676, 718-719, 756-756, 793-799, 851-851, 905-906, 988-990, 1073-1073, 1184-1184, 1417-1432, 1546-1560, 1638-1654
crates/bamts-compiler/tests/rules.rs (1)
169-198: 🎯 Functional CorrectnessKeep this test as written.
BAMTS-W088has default levelDenyandJavaScriptCompatibilityhandling maps it toWarnfor JavaScript. The visitor emits it forStatement::With, so the assertion is valid without an explicit override.> Likely an incorrect or invalid review comment.
| let keyword_context = KeywordContext { | ||
| await_reserved: is_async, | ||
| yield_reserved: false, | ||
| }; | ||
| let param_start = self.cur_start(); | ||
| let token = self.bump(); | ||
| let name = self.ident_from(token); | ||
| let name = self.with_keyword_context(keyword_context, |this| this.ident_from(token)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Test: every KeywordContext construction. Expect only function-like forms to
# reset both flags; arrow forms must inherit.
ast-grep run --pattern 'KeywordContext { $$$ }' --lang rust crates/bamts-compiler/src/parser.rs
echo '--- with_keyword_context call sites ---'
rg -nP -B6 'with_keyword_context\s*\(' crates/bamts-compiler/src/parser.rsRepository: metaphorics/bamTiScript
Length of output: 6450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- KeywordContext definition and context helpers ---'
rg -n -B8 -A24 'struct KeywordContext|enum KeywordContext|keyword_context|fn with_keyword_context|fn arrow_keyword_context' crates/bamts-compiler/src/parser.rs | head -240
echo '--- Arrow parser implementations ---'
sed -n '4400,4635p' crates/bamts-compiler/src/parser.rs
echo '--- Relevant tests ---'
sed -n '5860,5965p' crates/bamts-compiler/src/parser.rs
echo '--- All KeywordContext construction sites with surrounding function names ---'
python3 - <<'PY'
from pathlib import Path
p = Path("crates/bamts-compiler/src/parser.rs")
lines = p.read_text().splitlines()
for i, line in enumerate(lines):
if "KeywordContext {" in line:
start = max(0, i - 20)
print(f"\n--- lines {i+1}-{min(len(lines), i+12)} ---")
for n in range(start, min(len(lines), i + 12)):
print(f"{n+1}:{lines[n]}")
PYRepository: metaphorics/bamTiScript
Length of output: 33366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Arrow parser call sites and enclosing-context flow ---'
rg -n -B12 -A18 'parse_arrow|parse_simple_arrow|parse_paren_arrow|speculate_.*arrow|parse_assignment_expression|parse_block' crates/bamts-compiler/src/parser.rs | head -360
echo '--- Context restoration and parser test helpers ---'
rg -n -B8 -A20 'fn errors|fn parse_ts|fn parse_text|fn parse_assignment_expression|fn parse_block' crates/bamts-compiler/src/parser.rs | head -300
echo '--- Static verifier for arrow context construction ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("crates/bamts-compiler/src/parser.rs").read_text()
expected = {
"parse_simple_arrow": ("is_async", "false"),
"parse_paren_arrow": ("is_async", "false"),
"speculate_paren_arrow": ("is_async", "false"),
"speculate_async_paren_arrow": ("true", "false"),
"speculate_generic_arrow": ("is_async", "false"),
}
for name, expected_flags in expected.items():
match = re.search(
rf"fn {name}\b.*?(?=\n fn |\n pub fn |\n}}\s*$)",
text,
re.S,
)
if not match:
raise SystemExit(f"missing function: {name}")
body = match.group(0)
context = re.search(
r"let keyword_context = KeywordContext \{\s*"
r"await_reserved:\s*([^,]+),\s*"
r"yield_reserved:\s*([^,]+),\s*\};",
body,
re.S,
)
if not context:
raise SystemExit(f"missing context construction: {name}")
actual = tuple(x.strip() for x in context.groups())
print(f"{name}: await_reserved={actual[0]}, yield_reserved={actual[1]}")
if actual != expected_flags:
raise SystemExit(f"unexpected flags for {name}: {actual}")
print("All five arrow parsers construct contexts without reading self.keyword_context.")
print("The enclosing context is stored in Parser.keyword_context and replaced by with_keyword_context.")
PYRepository: metaphorics/bamTiScript
Length of output: 14787
🌐 Web query:
ECMAScript specification ArrowFunction Yield Await parameters inherited lexical grammar
💡 Result:
In the ECMAScript specification, arrow functions handle the Yield and Await parameters through the language's lexical grammar system, which uses these parameters to conditionally allow or disallow the use of yield and await keywords based on the function's context [1][2]. The specification defines the ArrowFunction production with three parameters: [In, Yield, Await] [1][3]. These parameters are inherited and propagated through the grammar tree [1][4]. 1. Inherited Parameters: When an arrow function is parsed, the Yield and Await parameters determine whether yield and await are treated as keywords or identifiers within the arrow parameters and body [1][2]. Specifically: - ArrowParameters are parsed using the inherited [?Yield, ?Await] parameters [1][2]. - ConciseBody is parsed in a context where Yield and Await are effectively disabled (not allowed) unless the function is explicitly marked as async [1][5]. 2. Syntax Constraints (Static Semantics): The specification enforces strict rules regarding these keywords via static semantics, which define syntax errors if they are used improperly: - It is a Syntax Error if ArrowParameters contains YieldExpression [1][6]. - It is a Syntax Error if ArrowParameters contains AwaitExpression [1][6]. - While AsyncArrowFunction allows await within its body, standard ArrowFunction definitions do not allow yield or await expressions within their ConciseBody [1][7]. If one attempts to use them, the parser relies on the absence of the [+Yield] or [+Await] parameters in the production to trigger a syntax error [1][2]. 3. Async Arrow Functions: For AsyncArrowFunction, the specification uses a different production (AsyncArrowHead) which explicitly sets the [+Await] parameter to allow await expressions within the AsyncConciseBody, while still typically forbidding yield unless nested within a generator function [5][8]. In summary, the Yield and Await grammar parameters act as functional toggles that control the lexical sensitivity of the parser to these keywords, ensuring they are only recognized as control structures when the syntactic context explicitly permits them [2][5].
Citations:
- 1: https://tc39.es/ecma262/2026/multipage/ecmascript-language-functions-and-classes.html
- 2: https://tc39.es/ecma262/multipage/ecmascript-language-functions-and-classes.html
- 3: https://tc39.es/ecma262/2025/multipage/ecmascript-language-functions-and-classes.html
- 4: https://tc39.es/ecma262/2023/multipage/grammar-summary.html
- 5: https://github.com/tc39/proposal-async-await/blob/main/spec/syntax.html
- 6: https://read262.netlify.app/ecmascript-language-functions-and-classes/arrow-function-definitions/
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions
- 8: https://ecma262.com/j/multipage/ecmascript-language-functions-and-classes.html
🏁 Script executed:
#!/bin/bash
set -u
echo '--- All arrow-related definitions and calls ---'
rg -n -B4 -A10 'parse_simple_arrow|parse_paren_arrow|speculate_paren_arrow|speculate_async_paren_arrow|speculate_generic_arrow|parse_arrow_body|parse_arrow' crates/bamts-compiler/src/parser.rs || true
echo '--- Every keyword-context write and use ---'
rg -n -B4 -A8 'keyword_context\s*=|KeywordContext\s*\{|with_keyword_context\s*\(' crates/bamts-compiler/src/parser.rs || true
echo '--- Static classification of all KeywordContext literals ---'
python3 - <<'PY'
from pathlib import Path
import re
lines = Path("crates/bamts-compiler/src/parser.rs").read_text().splitlines()
functions = []
current = "<module>"
for i, line in enumerate(lines, 1):
m = re.search(r"^\s*fn\s+([A-Za-z0-9_]+)\s*\(", line)
if m:
current = m.group(1)
if "KeywordContext {" in line:
block = "\n".join(lines[i-1:min(i+5, len(lines))])
flags = re.search(
r"await_reserved:\s*([^,]+),\s*yield_reserved:\s*([^,]+),",
block,
re.S,
)
print(f"{i}: {current}: {tuple(x.strip() for x in flags.groups()) if flags else 'unparsed'}")
PYRepository: metaphorics/bamTiScript
Length of output: 18154
🌐 Web query:
site:tc39.es/ecma262 ArrowFunction [In, Yield, Await] ArrowParameters ConciseBody ?Yield ?Await
💡 Result:
The provided text, ArrowFunction [In, Yield, Await]: ArrowParameters [?Yield,?Await] [no LineTerminator here] => ConciseBody [?In], is a production rule from the ECMAScript Language Specification [1][2]. It defines the syntax for arrow functions [1][3]. The notation uses a parameter-based grammar system to handle context-sensitive parsing rules [1][4]: 1. [In, Yield, Await]: These are parameters (often called "grammar parameters") that represent the parsing context [1][5]. - In: Indicates whether the 'in' operator is allowed in the expression (e.g., used to prevent ambiguity in for-loop heads) [1][6]. - Yield: Indicates whether the 'yield' keyword is treated as an identifier or as an operator (e.g., active inside generator functions) [5][4]. - Await: Indicates whether the 'await' keyword is treated as an identifier or as an operator (e.g., active inside async functions) [5][4]. 2.?Yield and?Await: The question mark (e.g.,?Yield) means "pass the current value of this parameter through" [1][7]. For example, the arrow function inherits the current Yield and Await context from its surroundings when parsing its ArrowParameters [1][8]. 3. [no LineTerminator here]: This is a lookahead restriction that explicitly prohibits a line terminator between the ArrowParameters and the fat arrow token (=>) [1][7]. 4. ArrowParameters and ConciseBody: These are non-terminal symbols representing the function parameters and the function body, respectively [1][3]. The production ensures that the body of the arrow function is parsed using the appropriate context [1]. In summary, this grammar rule defines that an arrow function's parameters and body are parsed using the same context parameters (Yield/Await) as the environment in which the arrow function is defined, while also imposing a strict requirement that no line break exists before the arrow operator [1][7][3].
Citations:
- 1: https://tc39.es/ecma262/2026/multipage/ecmascript-language-functions-and-classes.html
- 2: https://tc39.es/ecma262/2025/multipage/ecmascript-language-functions-and-classes.html
- 3: https://tc39.es/ecma262/multipage/ecmascript-language-functions-and-classes.html
- 4: https://tc39.es/ecma262/2025/multipage/ecmascript-language-lexical-grammar.html
- 5: https://tc39.es/ecma262/multipage/ecmascript-language-lexical-grammar.html
- 6: https://tc39.es/ecma262/2026/multipage/grammar-summary.html
- 7: https://tc39.es/ecma262/2024/multipage/ecmascript-language-functions-and-classes.html
- 8: https://tc39.es/ecma262/2022/multipage/ecmascript-language-functions-and-classes.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Escaped-identifier handling in expression and binding parsing ---'
rg -n -B12 -A24 'escaped_identifier_is_reserved|ident_from\(|identifier_name_from\(|TokenKind::EscapedContextualKeyword|TokenKind::Identifier' crates/bamts-compiler/src/parser.rs | head -360
echo '--- Parser paths for escaped contextual keywords ---'
ast-grep run --pattern 'self.ident_from($_)' --lang rust crates/bamts-compiler/src/parser.rs | head -120Repository: metaphorics/bamTiScript
Length of output: 15209
Preserve the enclosing keyword context for arrow functions.
All five arrow parsers replace the inherited context with await_reserved: is_async and yield_reserved: false. This lets nested arrows accept escaped yield in generators and escaped await in async functions as identifiers instead of reporting syntax errors.
Derive the context from self.keyword_context and preserve both inherited flags. Apply this in parse_simple_arrow, parse_paren_arrow, speculate_paren_arrow, speculate_async_paren_arrow, and speculate_generic_arrow. Add nested-arrow tests.
🤖 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/bamts-compiler/src/parser.rs` around lines 4460 - 4466, The arrow
parsers currently discard inherited keyword reservations; update
parse_simple_arrow, parse_paren_arrow, speculate_paren_arrow,
speculate_async_paren_arrow, and speculate_generic_arrow to derive
KeywordContext from self.keyword_context while preserving both await_reserved
and yield_reserved flags, adjusting only the async-specific state as required.
Add tests covering nested arrows inside async and generator contexts to ensure
escaped await and yield remain syntax errors.
| fn identifier(file: &SourceFile, node: &crate::syntax::IdentifierNode) -> String { | ||
| file.identifier_text(node.data().token()) | ||
| .expect("parser identifier range belongs to its source") | ||
| .into_owned() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the helper definition and every call site inside the compiler crate.
ast-grep run --pattern 'fn identifier($$$) { $$$ }' --lang rust crates/bamts-compiler/src/program.rs
echo '--- call sites of the local `identifier(` helper ---'
rg -nP -C4 '\bidentifier\s*\(\s*(&?self\.)?file' crates/bamts-compiler/src/program.rs
echo '--- how other modules handle the same None ---'
rg -nP -C3 'identifier_text\s*\(' crates/bamts-compiler/src --type=rustRepository: metaphorics/bamTiScript
Length of output: 161
🏁 Script executed:
set -euo pipefail
echo '--- helper definition and nearby context ---'
rg -n -A18 -B8 'fn identifier\s*\(' crates/bamts-compiler/src/program.rs
echo '--- local helper call sites ---'
rg -n -P -C5 '\bidentifier\s*\(' crates/bamts-compiler/src/program.rs
echo '--- identifier_text implementations and consumers ---'
rg -n -P -C8 'identifier_text\s*\(' crates/bamts-compiler/src --type rust
echo '--- escape cooking behavior ---'
rg -n -A70 -B10 'fn cook_identifier_text|cook_identifier_text\s*\(' crates/bamts-compiler/src/syntax.rsRepository: metaphorics/bamTiScript
Length of output: 50379
🏁 Script executed:
set -euo pipefail
echo '--- program lowering call paths ---'
sed -n '1730,2025p' crates/bamts-compiler/src/program.rs
sed -n '2260,2310p' crates/bamts-compiler/src/program.rs
echo '--- program error types and identifier-related diagnostics ---'
rg -n -A12 -B8 'enum ProgramLowerError|struct ProgramLowerError|missing|malformed_metadata_error|RawBinding|metadata_export_name' crates/bamts-compiler/src/program.rs
echo '--- lowerer identifier handling ---'
sed -n '650,710p' crates/bamts-compiler/src/lower.rs
sed -n '840,890p' crates/bamts-compiler/src/lower.rs
echo '--- complete escape cooker and token APIs ---'
sed -n '185,245p' crates/bamts-compiler/src/syntax.rs
sed -n '1960,2030p' crates/bamts-compiler/src/syntax.rs
rg -n -A8 -B5 'fn is_missing|is_missing\s*\(' crates/bamts-compiler/src --type rustRepository: metaphorics/bamTiScript
Length of output: 50379
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
syntax = Path("crates/bamts-compiler/src/syntax.rs").read_text()
program = Path("crates/bamts-compiler/src/program.rs").read_text()
# Model the two relevant source-level branches without importing or executing
# repository code.
def cook(text):
if "\\" not in text:
return text
chars = iter(text)
out = []
for ch in chars:
if ch != "\\":
out.append(ch)
continue
if next(chars, None) != "u":
return None
first = next(chars, None)
if first is None:
return None
if first == "{":
value = 0
has_digit = False
while True:
digit = next(chars, None)
if digit is None:
return None
if digit == "}":
break
try:
n = int(digit, 16)
except ValueError:
return None
value = value * 16 + n
has_digit = True
if not has_digit or value > 0x10ffff:
return None
out.append(chr(value))
else:
digits = [first] + [next(chars, None) for _ in range(3)]
if any(d is None for d in digits):
return None
try:
value = int("".join(digits), 16)
out.append(chr(value))
except (ValueError, OverflowError):
return None
return "".join(out)
assert 'if token.is_missing() {\n return Some("")' in syntax
assert 'cook_identifier_text(self.token_text(token)?)' in syntax
assert cook("") == ""
assert cook(r"\u{}") is None
assert cook(r"\u{110000}") is None
# Count actual calls in program.rs, excluding the definition itself.
calls = [
m.start() for m in re.finditer(r"\bidentifier\s*\(", program)
]
definition = program.index("fn identifier(")
calls.remove(definition)
print("missing token identifier_text result: Some(\"\")")
print("cook_identifier_text(\\\\u{}): None")
print("cook_identifier_text(\\\\u{110000}): None")
print("local identifier call count:", len(calls))
print("metadata_export_name wraps identifier(...) in Ok(...):",
"Ok(identifier(file, identifier_node))" in program)
PY
echo '--- exact local helper references ---'
rg -n '\bidentifier\s*\(' crates/bamts-compiler/src/program.rsRepository: metaphorics/bamTiScript
Length of output: 277
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
syntax = Path("crates/bamts-compiler/src/syntax.rs").read_text()
program = Path("crates/bamts-compiler/src/program.rs").read_text()
def cook(text):
if "\\" not in text:
return text
chars = iter(text)
out = []
for ch in chars:
if ch != "\\":
out.append(ch)
continue
if next(chars, None) != "u":
return None
first = next(chars, None)
if first is None:
return None
if first == "{":
value = 0
has_digit = False
while True:
digit = next(chars, None)
if digit is None:
return None
if digit == "}":
break
try:
n = int(digit, 16)
except ValueError:
return None
value = value * 16 + n
has_digit = True
if not has_digit or value > 0x10ffff:
return None
out.append(chr(value))
else:
digits = [first] + [next(chars, None) for _ in range(3)]
if any(d is None for d in digits):
return None
try:
value = int("".join(digits), 16)
out.append(chr(value))
except (ValueError, OverflowError):
return None
return "".join(out)
assert 'if token.is_missing() {\n return Some("")' in syntax
assert 'cook_identifier_text(self.token_text(token)?)' in syntax
assert cook("") == ""
assert cook(r"\u{}") is None
assert cook(r"\u{110000}") is None
matches = list(re.finditer(r"\bidentifier\s*\(", program))
calls = [m for m in matches if not program[max(0, m.start()-3):m.start()].endswith("fn ")]
print("missing token identifier_text result: Some(\"\")")
print(r"cook_identifier_text(\u{}): None")
print(r"cook_identifier_text(\u{110000}): None")
print("local identifier references including definition:", len(matches))
print("local identifier call sites:", len(calls))
print("metadata_export_name wraps identifier(...) in Ok(...):",
"Ok(identifier(file, identifier_node))" in program)
PY
echo '--- exact local helper references ---'
rg -n '\bidentifier\s*\(' crates/bamts-compiler/src/program.rsRepository: metaphorics/bamTiScript
Length of output: 1087
Propagate malformed identifier failures instead of panicking.
identifier_text returns None for invalid escapes such as \u{} and \u{110000}. The current .expect aborts compilation. Return a Result<String, ProgramLowerError> and propagate it through all eight call sites and metadata_export_name to the existing metadata diagnostic. Do not treat missing tokens as the None case: token_text returns Some("") for them.
🤖 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/bamts-compiler/src/program.rs` around lines 2266 - 2270, Update
identifier and metadata_export_name to return Result<String, ProgramLowerError>,
converting identifier_text returning None into the existing metadata diagnostic
instead of panicking. Propagate the Result through all eight identifier call
sites, preserving Some("") token text as valid and distinguishing it from
malformed escapes.
| pub(crate) fn cook_identifier_text(text: &str) -> Option<Cow<'_, str>> { | ||
| if !text.contains('\\') { | ||
| return Some(Cow::Borrowed(text)); | ||
| } | ||
|
|
||
| let mut chars = text.chars(); | ||
| let mut cooked = String::with_capacity(text.len()); | ||
| while let Some(character) = chars.next() { | ||
| if character != '\\' { | ||
| cooked.push(character); | ||
| continue; | ||
| } | ||
| if chars.next()? != 'u' { | ||
| return None; | ||
| } | ||
|
|
||
| let first = chars.next()?; | ||
| let code_point = if first == '{' { | ||
| let mut value = 0_u32; | ||
| let mut has_digit = false; | ||
| loop { | ||
| let digit = chars.next()?; | ||
| if digit == '}' { | ||
| break; | ||
| } | ||
| value = value.checked_mul(16)?.checked_add(digit.to_digit(16)?)?; | ||
| has_digit = true; | ||
| } | ||
| if !has_digit { | ||
| return None; | ||
| } | ||
| value | ||
| } else { | ||
| let mut value = first.to_digit(16)?; | ||
| for _ in 1..4 { | ||
| value = value | ||
| .checked_mul(16)? | ||
| .checked_add(chars.next()?.to_digit(16)?)?; | ||
| } | ||
| value | ||
| }; | ||
| cooked.push(char::from_u32(code_point)?); | ||
| } | ||
| Some(Cow::Owned(cooked)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Surrogate-pair identifier escapes are never decoded, and no fixture covers them. cook_identifier_text converts each \u escape with char::from_u32, which rejects lone surrogates, so a legal spelling such as \uD835\uDC65 returns None. The consumers then diverge: crates/bamts-compiler/src/rules/semantic/mod.rs substitutes the empty name, and crates/bamts-compiler/src/program.rs panics through .expect(...). The scanner's scan_identifier_escape validates each escape separately, so it rejects the same spelling before this function runs.
crates/bamts-compiler/src/syntax.rs#L189-L233: buffer decoded values as UTF-16 code units and build the name withString::from_utf16, so a high/low pair combines before conversion. Apply the same pairing rule inscan_identifier_escapeincrates/bamts-compiler/src/scanner.rs.crates/bamts-cli/tests/cli.rs#L332-L354: add an astral escaped identifier to the fixture, spelled once as\uD835\uDC65and once as\u{1D465}, and assert that both spellings denote the same binding.
📍 Affects 2 files
crates/bamts-compiler/src/syntax.rs#L189-L233(this comment)crates/bamts-cli/tests/cli.rs#L332-L354
🤖 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/bamts-compiler/src/syntax.rs` around lines 189 - 233, Update
cook_identifier_text in crates/bamts-compiler/src/syntax.rs#L189-L233 to decode
escapes as UTF-16 code units and combine valid surrogate pairs via
String::from_utf16; apply the same pairing behavior in scan_identifier_escape in
crates/bamts-compiler/src/scanner.rs. Extend the fixture in
crates/bamts-cli/tests/cli.rs#L332-L354 with identifiers spelled using both
\uD835\uDC65 and \u{1D465}, asserting they resolve to the same binding.
| let oracle = NodeOracle::discover(&root).expect("the pinned Node oracle must be available"); | ||
| let bamts = BamtsRunner::new(&root); | ||
| let expected = oracle.run_case(&spec); | ||
| let mut failures = Vec::new(); | ||
| for mode in ExecutionMode::ALL { | ||
| let actual = bamts.run_case(&spec, mode); | ||
| compare_case(&spec.id, mode, &expected, &actual, &mut failures); | ||
| } | ||
| fs::remove_file(path).expect("remove iterator-close fixture"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The fixture leaks whenever anything panics before Line 413.
Cleanup sits on the happy path only. NodeOracle::discover(...).expect(...) at Line 405 panics on any machine without the pinned Node. run_case can panic too. In both cases target/iterator-close-<pid>.js survives the test run. Each run uses a new PID, so these files accumulate, and source_dir: "target" means the corpus tooling scans the same directory. Do not hand-roll cleanup in the success path; use a guard that removes the file on unwind.
♻️ Proposed fix using a drop guard
+ struct Fixture(PathBuf);
+ impl Drop for Fixture {
+ fn drop(&mut self) {
+ let _ = fs::remove_file(&self.0);
+ }
+ }
+ let _fixture = Fixture(path);
let oracle = NodeOracle::discover(&root).expect("the pinned Node oracle must be available");
let bamts = BamtsRunner::new(&root);
let expected = oracle.run_case(&spec);
let mut failures = Vec::new();
for mode in ExecutionMode::ALL {
let actual = bamts.run_case(&spec, mode);
compare_case(&spec.id, mode, &expected, &actual, &mut failures);
}
- fs::remove_file(path).expect("remove iterator-close fixture");The same write/run/remove pattern repeats in the other generated-fixture tests in this file. Fix it with one shared helper instead of copying the guard nine times.
🤖 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/bamts-verification/tests/corpus_differential.rs` around lines 405 -
413, Introduce one shared cleanup-guard helper for generated fixture paths in
the corpus differential tests, and use it in every write/run/remove fixture
test, including the flow around NodeOracle::discover, BamtsRunner::run_case, and
compare_case. The guard must remove the fixture during Drop so cleanup occurs on
both normal completion and unwinding; remove the duplicated happy-path
fs::remove_file calls after adopting it.
1 participant
Summary by cubic
Switches the engine to the canonical
Programcontainer (v4) with AOT ABI v4, adds a strictness lint surface with an explain UX, enforces W^X JIT memory with strict code/data separation, and cleans up runtime test helper allocations. Also adds exact ECMAScript UTF‑16 strings and a precise, non‑moving GC, resolves and lowers whole program graphs with classic script compilation, completes Node parity for async iteration, timers, and microtasks, preserves Node AOT stdout and logical argv, and tightens compiler lowering with escaped identifiers, labeled control flow, nondecimal bigint literals,debuggererasure, and nested finally completion fixes.New Features
bamts-compiler: strictness rules with explain; whole‑program resolver/checker; classic‑script compile; escaped identifiers; labeled control flow; nondecimal bigint;debuggererasure; reportdebugger/with.bamts-codegen: W^X host JIT memory with page‑separated code/data; AOT ABI v4.bamts-bytecode: canonicalProgramenvelope v4; exactEcmaString.Bug Fixes
bamts-compiler: route nestedfinallycompletions; classify classic script syntax errors.bamts-node/bamts-cli: preserve AOT stdout on failure; keep logical AOT argv intact.Written for commit 5cd22f5. Summary will update on new commits.