feat(checker): split checker modules and preserve generic call types - #15
feat(checker): split checker modules and preserve generic call types#15metaphorics wants to merge 263 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This PR has 69,314 reviewable changed lines after ignored/generated files are excluded, above cubic's default 50,000-changed-line automatic review limit. The raw diff is 75,326 lines before ignored/generated files are excluded. Most of the diff comes from:
Comment |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR rewrites the bytecode format to v4 with UTF-16 exact strings via Estimated code review effort: 5 (Critical) | ~180 minutes ChangesBytecode/Codegen/Native ABI
If this looks like a lot to review, that is because it is a lot. Don't skim it. Compiler frontend
Runtime
Node/CLI/Facade
Verification/CI
Sequence Diagram(s)sequenceDiagram
participant CLI as bamts-cli driver
participant Facade as bamts facade
participant Compiler as bamts-compiler program
participant Codegen as bamts-codegen
participant Native as bamts-native ABI
participant Runtime as bamts-runtime/node
CLI->>Facade: resolve_project(entrypoint)
Facade->>Compiler: ProgramLoader.load(entrypoint)
Compiler->>Compiler: lower_program -> ExecutableProgram
Facade->>Codegen: compile_jit / compile_aot(Program<Verified>)
Codegen->>Native: bind helper ABI (module_id, function_id)
Codegen->>Runtime: invoke(module_id, function_id, frame)
Runtime-->>CLI: exit_code / output
Nothing fancy, but it shows the actual data flow instead of pretending this PR is "just a refactor." 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 68
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 (7)
crates/bamts-runtime/src/builtins/array.rs (1)
884-922: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAll three iterator factories materialize the whole array just to throw the result away.
Lines 888, 902, and 916 call
elements(machine, this)?and discard the value. The only purpose is receiver validation, butelementsbuilds a fullVec<Value>of every element first. Forarr.keys()on a million-element array that is a million-element allocation, immediately dropped, before the iterator has yielded anything.
collection_slotincollections.rsshows the right shape: check the heap entry brand, return the slot index, allocate nothing. Add the equivalent for arrays and use it at all three sites.🤖 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-runtime/src/builtins/array.rs` around lines 884 - 922, Add an array-specific receiver-validation helper matching collections::collection_slot: verify the heap entry is an array and return its slot/index without materializing elements. Replace the discarded elements(machine, this)? calls in keys_iterator, values_iterator, and entries_iterator with this lightweight validation while preserving the existing iterator construction.crates/bamts-compiler/src/parser.rs (2)
5185-5194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComments claim diagnostics that the code no longer emits.
Line 5256 says "diagnose and record a missing element rather than dropping it." No diagnostic is emitted; only the
Missingnode is pushed. Line 5187 has a bare blank line where the compound-target diagnostic used to be. Both sites now defer reporting to the checker, which is a defensible change, but the comments were left behind describing the old behavior.A comment that describes code that no longer exists is worse than no comment. Fix them.
♻️ Proposed fix
ArrayElement::Spread(_) => { - // A rest element has no array-target slot; diagnose and - // record a missing element rather than dropping it. - + // A rest element has no array-target slot. Record a missing + // element so the checker can report it; the parser stays silent. elements.push(AssignmentArrayElement::Missing(MissingNode::new(Also applies to: 5255-5262
🤖 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 5185 - 5194, Update the stale comments near the compound-target fallback and the missing-element handling to match the current behavior: remove references to emitting or recording diagnostics, and delete the obsolete blank/commented diagnostic wording. Preserve the existing MissingNode construction and defer reporting to the checker.
6203-6237: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
parse_function_type_signaturetakes an error range it never reads, and one caller builds a six-arm match to feed it.The parameter is
_error_range. It is discarded. Lines 6203-6210 compute aPropertyNamerange through a full match, and lines 6157 and 6166 passself.cur().range(), all to produce a value the function throws away. This is dead work on the hot path of every type-member parse, and it leaves a parameter that looks meaningful to the next reader.Delete the parameter and the match, or wire the range into a real diagnostic. Do not leave a half-finished signature in the tree.
♻️ Proposed cleanup
- let error_range = match &name { - PropertyName::Identifier(n) => n.range(), - PropertyName::Private(n) => n.range(), - PropertyName::String(n) => n.range(), - PropertyName::Number(n) => n.range(), - PropertyName::Computed(n) => n.range(), - PropertyName::Missing(_) => self.cur().range(), - }; - let function = self.parse_function_type_signature(false, error_range); + let function = self.parse_function_type_signature(false);- fn parse_function_type_signature( - &mut self, - constructor: bool, - _error_range: TextRange, - ) -> FunctionType { + fn parse_function_type_signature(&mut self, constructor: bool) -> FunctionType {🤖 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 6203 - 6237, Remove the unused _error_range parameter from parse_function_type_signature and update every caller, including the type-member parsing branch and the call sites passing self.cur().range(). Delete the PropertyName range match that only constructs this discarded argument, leaving function-type parsing behavior unchanged.crates/bamts-runtime/src/builtins/regexp.rs (1)
57-75: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winEvery
new RegExp(...)compiles the pattern twice.Line 57 calls
compile(machine, &pattern, &flags)?purely to validate and throws the compiledRegexaway. Line 69 then callsRegex::compile(&pattern, &flags).expect("validated")again, only to read.flags().canonical().Pattern compilation is the expensive part of constructing a RegExp. Doing it twice doubles the cost of every construction, and code that builds regexes inside a loop pays for it on every iteration. The
.expect("validated")also encodes an assumption that two compiles of the same input always agree — true today, and pointless to rely on when the first result is right there.♻️ Proposed fix
- compile(machine, &pattern, &flags)?; + let regex = compile(machine, &pattern, &flags)?; + let canonical_flags = regex.flags().canonical(); let mut properties = PropertyMap::default(); for (name, value, writable) in [ ( "source", allocate_string(machine, canonical_source(&pattern))?, false, ), ( "flags", - allocate_string( - machine, - Regex::compile(&pattern, &flags) - .expect("validated") - .flags() - .canonical(), - )?, + allocate_string(machine, canonical_flags)?, false, ),🤖 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-runtime/src/builtins/regexp.rs` around lines 57 - 75, Reuse the compiled Regex returned by compile in the RegExp construction flow instead of discarding it and recompiling for the flags property. Update the compile call and the flags allocation to read canonical flags from that single result, removing the redundant Regex::compile and expect("validated") while preserving validation and property behavior.crates/bamts-compiler/src/lint.rs (1)
1886-1915: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe JavaScript severity clamp is keyed on a hard-coded rule code, so
BAMTS-W088is silently downgraded.level_for_sourcecarves out exactly one code,BAMTS-W085, from the clamp.BAMTS-W088(no-with) is the secondDenyrule inRuleGroup::JavaScriptCompatibilityand nobody updated the list, sowithin a.jsfile resolves toWarninstead ofDeny— in the only dialect wherewithcan appear. The test enshrines the one carved-out code and never checks the rule class.
crates/bamts-compiler/src/lint.rs#L1886-L1915: replace theBAMTS-W085early return and the duplicatedrule.code() != "BAMTS-W085"condition with a severity-based carve-out: return the effective level unchanged when it isDenyorForbidand the group isJavaScriptCompatibility.crates/bamts-compiler/src/lint.rs#L1413-L1421: no registry change is needed once the clamp is fixed; confirmno-withresolves toDenyunderSourceDialect::JavaScriptafter the fix.crates/bamts-compiler/src/lint.rs#L2314-L2325: add an assertion thatlevel_for_source(rule("no-with"), SourceDialect::JavaScript)isLintLevel::Deny, so the nextDenyrule added to that group cannot regress silently.🤖 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 1886 - 1915, Update crates/bamts-compiler/src/lint.rs:1886-1915 in level_for_source to remove both hard-coded BAMTS-W085 carve-outs and preserve Deny/Forbid severities for JavaScriptCompatibility rules while continuing to clamp lesser levels to Warn. In crates/bamts-compiler/src/lint.rs:1413-1421, make no registry change; confirm the existing no-with rule resolves to Deny for SourceDialect::JavaScript. In crates/bamts-compiler/src/lint.rs:2314-2325, add an assertion that level_for_source(rule("no-with"), SourceDialect::JavaScript) returns LintLevel::Deny.crates/bamts-compiler/src/scanner.rs (1)
1933-1958: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win190 lines of new JSX span scanning, zero tests for it.
This test exercises
scan_jsx_text,scan_jsx_identifier, andscan_jsx_attribute_string— the three pre-existing primitives. The new entry pointscan_jsx_spanand its five helpers are not called by any test in this file.scanner_is_total_over_arbitrary_inputsat Line 1973 does not reach them either, because it only drives the default pass.The untested paths are the ones that matter: nested elements,
<>fragments,<Foo.Bar>and<ns:Foo>names,{expr}containers with nested object literals and templates, and every malformed-input exit. The depth bookkeeping inscan_jsx_spanand the brace bookkeeping inscan_jsx_expression_tokensare exactly the kind of code that is wrong until proven otherwise.Add span-level cases, including the totality-and-tiling assertion this file already applies to the default pass.
💚 Suggested cases to add
#[test] fn jsx_span_tiles_nested_elements_fragments_and_containers() { for source in [ "<div />", "<div></div>", "<><a/><b/></>", "<Foo.Bar ns:attr=\"x\" onClick={() => ({a: 1})}>text</Foo.Bar>", "<div>{`x${y}z`}</div>", // Malformed: each must terminate and must not corrupt later tokens. "<div>", "<div>{`x${y", "</div>", "<div / bar>", ] { let text = SourceText::new(source).expect("test source fits the per-file budget"); let mut scanner = Scanner::new(SourceId::new(0), ScriptKind::TypeScriptReact, &text); let tokens = scanner.scan_jsx_span(); let mut cursor = 0usize; for token in &tokens { assert_eq!(token.range().start().get(), cursor, "gap in {source:?}"); assert!(!token.range().is_empty(), "no progress in {source:?}"); cursor = token.range().end().get(); } } }🤖 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 1933 - 1958, Add span-level tests that invoke Scanner::scan_jsx_span for nested elements, fragments, member and namespaced JSX names, expression containers with nested objects/templates, and malformed inputs. Assert tokens are non-empty and tile the source without gaps, matching the existing totality-and-tiling coverage pattern, and ensure each case terminates safely.crates/bamts-runtime/src/regexp.rs (1)
316-362: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftThe matcher materializes every backtracking path in memory at once. This is ReDoS with an out-of-memory amplifier.
match_nodereturnsVec<State>— the complete set of states reachable at that node, not a lazy stream.match_sequencebuilds on that: for aRepeat,levels[count]holds every state achievable aftercountrepetitions, and all levels are retained simultaneously until Line 350 starts testing the remainder.For a pattern with
kequivalent alternatives under a star,levels[n]holdsk^nstates. EachStateowns a heap-allocatedcaptures: Vec<Option<Range<usize>>>. So:new RegExp("(a|a|a)*$").test("a".repeat(20))builds roughly 3^20 states, each with its own allocation, before a single one is checked against
$. A conventional backtracking engine explores depth-first and holds O(depth) state; it would be slow. This one exhausts memory and takes the process down.Line 351 makes the constant factor worse:
for candidate in levels[count].clone()deep-copies the entire level vector, including every captures allocation, on each iteration of the count loop.Line 328 also sets the unbounded-repeat limit to
input.len() + min + 1, so the level count scales with input length as well.Two things are needed. First, make the matcher lazy — return an iterator, or restructure to explicit depth-first search with a continuation, so memory is proportional to expression depth rather than to the number of paths. Second, until that lands, impose a hard step budget and fail the match when it is exceeded, so a hostile pattern degrades to an error instead of an OOM.
Add a regression test with a known-exponential pattern and a short deadline.
🤖 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-runtime/src/regexp.rs` around lines 316 - 362, Refactor match_sequence and the match_node call chain to explore Repeat paths depth-first/lazily instead of retaining all states in levels, and avoid cloning complete candidate levels; preserve greedy and non-greedy ordering and zero-progress protection. Until lazy traversal is implemented, add a hard match-step budget that aborts with an error when exceeded, and add a regression test covering an exponential pattern with a short deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 1e9b83d8-58bb-4e75-bc6d-21dd6bde5359
⛔ Files ignored due to path filters (12)
Cargo.lockis excluded by!**/*.lockcorpus/projects/citty/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/defu/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/destr/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/hookable/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/ohash/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/pathe/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/perfect-debounce/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/rou3/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/tiny-invariant/yarn.lockis excluded by!**/yarn.lock,!**/*.lockcorpus/projects/ufo/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (152)
.gitignore.outline/sdd/task-10-report.md.outline/sdd/task-14-report.md.outline/sdd/task-7-fix-report.md.outline/sdd/task-7-report.md.outline/sdd/task-9-report.mdCargo.tomlLICENSEcorpus/bamts.tomlcorpus/projects/tslib/test/package.jsoncorpus/specs/citty.tomlcorpus/specs/defu.tomlcorpus/specs/dot-prop.tomlcorpus/specs/hookable.tomlcorpus/specs/ohash.tomlcorpus/specs/p-map.tomlcorpus/specs/pathe.tomlcorpus/specs/rou3.tomlcorpus/specs/tiny-invariant.tomlcorpus/specs/tslib.tomlcorpus/specs/valita.tomlcorpus/specs/yocto-queue.tomlcrates/bamts-bytecode/Cargo.tomlcrates/bamts-bytecode/src/lib.rscrates/bamts-bytecode/src/program.rscrates/bamts-bytecode/src/string.rscrates/bamts-cli/Cargo.tomlcrates/bamts-cli/build.rscrates/bamts-cli/src/args.rscrates/bamts-cli/src/diagnostics.rscrates/bamts-cli/src/driver.rscrates/bamts-cli/src/main.rscrates/bamts-cli/tests/cli.rscrates/bamts-codegen/Cargo.tomlcrates/bamts-codegen/src/aot.rscrates/bamts-codegen/src/jit.rscrates/bamts-codegen/src/jit_memory.rscrates/bamts-codegen/src/lib.rscrates/bamts-compiler/Cargo.tomlcrates/bamts-compiler/RULES.mdcrates/bamts-compiler/src/bin/generate_rule_reference.rscrates/bamts-compiler/src/checker.rscrates/bamts-compiler/src/checker/binder.rscrates/bamts-compiler/src/checker/inference.rscrates/bamts-compiler/src/checker/intrinsic_environment.rscrates/bamts-compiler/src/checker/jsx.rscrates/bamts-compiler/src/checker/narrowing.rscrates/bamts-compiler/src/checker/relations.rscrates/bamts-compiler/src/emitter.rscrates/bamts-compiler/src/enum_plan.rscrates/bamts-compiler/src/lib.rscrates/bamts-compiler/src/lint.rscrates/bamts-compiler/src/literal.rscrates/bamts-compiler/src/lower.rscrates/bamts-compiler/src/namespace_plan.rscrates/bamts-compiler/src/parser.rscrates/bamts-compiler/src/pipeline.rscrates/bamts-compiler/src/program.rscrates/bamts-compiler/src/project.rscrates/bamts-compiler/src/rules/mod.rscrates/bamts-compiler/src/rules/semantic/coercions.rscrates/bamts-compiler/src/rules/semantic/control_flow.rscrates/bamts-compiler/src/rules/semantic/enums.rscrates/bamts-compiler/src/rules/semantic/flow_safety.rscrates/bamts-compiler/src/rules/semantic/functions.rscrates/bamts-compiler/src/rules/semantic/intrinsics.rscrates/bamts-compiler/src/rules/semantic/members.rscrates/bamts-compiler/src/rules/semantic/mod.rscrates/bamts-compiler/src/rules/semantic/modules.rscrates/bamts-compiler/src/rules/semantic/object_types.rscrates/bamts-compiler/src/scanner.rscrates/bamts-compiler/src/script.rscrates/bamts-compiler/src/source.rscrates/bamts-compiler/src/syntax.rscrates/bamts-compiler/src/telemetry.rscrates/bamts-compiler/src/warning.rscrates/bamts-compiler/tests/corpus_parse.rscrates/bamts-compiler/tests/rules.rscrates/bamts-native/Cargo.tomlcrates/bamts-native/src/lib.rscrates/bamts-native/src/native_bridge.rscrates/bamts-node/Cargo.tomlcrates/bamts-node/src/lib.rscrates/bamts-node/src/timers.rscrates/bamts-runtime/Cargo.tomlcrates/bamts-runtime/src/builtins/array.rscrates/bamts-runtime/src/builtins/collections.rscrates/bamts-runtime/src/builtins/date.rscrates/bamts-runtime/src/builtins/json.rscrates/bamts-runtime/src/builtins/mod.rscrates/bamts-runtime/src/builtins/number.rscrates/bamts-runtime/src/builtins/object.rscrates/bamts-runtime/src/builtins/promise.rscrates/bamts-runtime/src/builtins/regexp.rscrates/bamts-runtime/src/builtins/string.rscrates/bamts-runtime/src/builtins/symbol.rscrates/bamts-runtime/src/builtins/test_support.rscrates/bamts-runtime/src/builtins/timers.rscrates/bamts-runtime/src/builtins/uint8array.rscrates/bamts-runtime/src/external_modules.rscrates/bamts-runtime/src/gc.rscrates/bamts-runtime/src/host_objects.rscrates/bamts-runtime/src/intrinsics.rscrates/bamts-runtime/src/lib.rscrates/bamts-runtime/src/native.rscrates/bamts-runtime/src/regexp.rscrates/bamts-runtime/src/vm.rscrates/bamts-verification/Cargo.tomlcrates/bamts-verification/src/bin/perf_budget.rscrates/bamts-verification/src/bin/ts_conformance.rscrates/bamts-verification/src/check_cells.rscrates/bamts-verification/src/corpus.rscrates/bamts-verification/src/facets.rscrates/bamts-verification/src/ledger.rscrates/bamts-verification/src/lib.rscrates/bamts-verification/src/main.rscrates/bamts-verification/src/oracle_pins.rscrates/bamts-verification/src/perf.rscrates/bamts-verification/src/suite.rscrates/bamts-verification/src/ts_ledger.rscrates/bamts-verification/src/workspace_guard.rscrates/bamts-verification/tests/corpus_differential.rscrates/bamts-verification/tests/inspect_2darrays.rscrates/bamts/Cargo.tomlcrates/bamts/src/lib.rsdocs/solutions/architecture-patterns/exact-ecmascript-utf16-strings.mdformal/lean/Bamti/Bytecode/Model.leanformal/lean/Bamti/Bytecode/Verify.leanformal/lean/Bamti/JitLifecycle.leannpm/artifacts/cli-darwin-arm64/README.mdnpm/artifacts/cli-darwin-arm64/package.jsonnpm/artifacts/cli-darwin-x64/README.mdnpm/artifacts/cli-darwin-x64/package.jsonnpm/artifacts/cli-linux-arm64/README.mdnpm/artifacts/cli-linux-arm64/package.jsonnpm/artifacts/cli-linux-x64/README.mdnpm/artifacts/cli-linux-x64/package.jsonnpm/artifacts/cli-win32-x64/README.mdnpm/artifacts/cli-win32-x64/package.jsonnpm/bamti-cli/index.jsnpm/bamti-cli/package.jsonnpm/test/bamti-cli.test.mjspackage.jsonperf/benchmarks.tomlperf/budgets.tomlperf/hosts/bh1.tomlproof/completeness-ledger.jsonproof/lean-assumptions.jsonvendor/sources.tomlverification/diagnostic-code-map.jsonverification/manifest.lock.jsonverification/ts-conformance-ledger.schema.json
📜 Review details
🧰 Additional context used
🪛 LanguageTool
npm/artifacts/cli-linux-arm64/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # bamti-cli-linux-arm64 Linux arm64 artifact package for `bamti-...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
npm/artifacts/cli-darwin-x64/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # bamti-cli-darwin-x64 macOS x64 artifact package for `bamti-cl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
npm/artifacts/cli-darwin-arm64/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # bamti-cli-darwin-arm64 macOS arm64 artifact package for `bamti-...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.outline/sdd/task-7-report.md
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... No project-wide checks. - No commit. - No lasting source edits (none needed; all ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
crates/bamts-compiler/RULES.md
[style] ~630-~630: The adverb ‘never’ is usually put before the verb ‘assert’.
Context: ...d alternative: Handle every variant and assert never in the default branch. - Silence: `-A e...
(ADVERB_WORD_ORDER)
docs/solutions/architecture-patterns/exact-ecmascript-utf16-strings.md
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... type requires Unicode scalar values. - When bytecode or native artifacts must prese...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...gs across processes or architectures. - When string, regular-expression, JSON, or pr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~90-~90: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... depends on UTF-16 code-unit offsets. - When source maps or diagnostics must report ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔍 Remote MCP Valyu
Relevant review context
- The reviewed change is open PR
#15, titled “feat(checker): split checker modules and preserve generic call types”, based on commitf4206d6. It splits checker logic into binder, relations, inference, narrowing, JSX, and intrinsic-environment modules. - Generic signature interning intentionally preserves parameter names and type parameters while retaining structural deduplication; generic/non-generic distinctions remain significant.
- Intrinsic
Objectis identified bySymbolId, avoiding false nominal matches from user-definedinterface Object. A follow-up commit limited nominal intrinsic handling toObject; treating every intrinsic nominally broke structural targets such asRecord<string, unknown>. - Reported verification: workspace checks, formatting, clippy, 666 compiler tests, 223 verification tests, and 78,513 ledger checks passed.
- Five corpus failures—
hookable,pathe,tiny-invariant,tslib, andvalita—are reported as pre-existing across Interpreter/JIT/AOT.citty,ohash, andis-plain-objreportedly pass after the checker changes. - PR
#14was the predecessor implementing the generic-call, intrinsic-Object, parameter-name, and post-body return-inference fixes; PR#15carries that work ontomainalongside the module split.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (42)
crates/bamts-bytecode/src/lib.rs (1)
2143-2172: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Aliasing rules are inconsistent across the two-write opcodes.
IteratorResultis new here and got no check.
IteratorCloserejectsresult == called.DisposeCapturerejectsmethod == kind. Both are two-write opcodes, and both are correct to reject aliasing, because with a shared destination the second write clobbers the first and the ISA never states which write happens first.
IteratorResultwritesdoneandvalueand has no such check. Neither doesIteratorNext. So a module withIteratorResult { done: r1, value: r1, result: r2 }verifies. The definite-initialization pass happily marksr1initialized, and the meaning ofr1afterwards depends entirely on the order the interpreter, the JIT, and the AOT backend each picked. Three backends, three chances to disagree, and a "verified" certificate saying it is fine.Pick one rule and apply it to all four opcodes. Either reject aliased outputs everywhere, or specify the write order in the module docs and drop the two checks you already added.
🔧 Proposed fix — extend the existing rule
Instruction::IteratorResult { done, value, result, } => { check_register(done)?; check_register(value)?; check_register(result)?; + if done == value { + return Err(instruction_error( + function_index, + pc, + VerifyErrorKind::AliasedIteratorResultOutputs { register: done }, + )); + } }Apply the same arm to
Instruction::IteratorNextand add the matchingVerifyErrorKindvariant plus itsDisplaymessage.🤖 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` around lines 2143 - 2172, Apply the existing aliased-output rejection rule consistently to the iterator write opcodes: add a check in the Instruction::IteratorResult arm for done == value, and in Instruction::IteratorNext for its two output registers. Introduce the corresponding VerifyErrorKind variants and Display messages, following the existing IteratorClose and DisposeCapture patterns.crates/bamts-compiler/src/checker/binder.rs (2)
2958-2995: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated parameter-lowering block. Extract it.
Lines 2958-2995 and
signature_typeat lines 5733-5770 contain the same ~35 lines:thisskip, annotation resolution, rest detection, optional detection, and the four-level binding-pattern name match witharg{idx}fallbacks. The two copies are already byte-identical, which means the next fix lands in one of them and quietly diverges.Extract one helper and call it from both sites.
♻️ Suggested helper
fn lower_parameter( &mut self, index: usize, parameter: &'src ParameterNode, scope: ScopeId, ) -> Option<FunctionParameter> { if self.is_this_parameter(parameter) { return None; } let data = parameter.data(); let type_id = match &data.type_annotation { Some(annotation) => self.resolve_type(&annotation.data().type_node, scope), None => self.types.any(), }; let rest = matches!(data.binding.data(), BindingPattern::Rest(_)); let optional = data.optional || data.initializer.is_some(); let name = self .binding_identifier_name(&data.binding) .unwrap_or_else(|| format!("arg{index}")); Some(FunctionParameter::new(name, type_id, optional, rest)) }🤖 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/checker/binder.rs` around lines 2958 - 2995, Extract the duplicated parameter-lowering logic into a shared Binder helper, such as lower_parameter, returning None for this parameters and Some(FunctionParameter) otherwise. Reuse it from the function-parameter block and signature_type, preserving annotation resolution, rest and optional detection, binding-pattern name fallbacks, and arg{index} naming.
5846-5871: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Explicit type arguments are bound in occurrence order, not declaration order. This is wrong.
dedupedis built by walking the signature's parameter types and then the return type. That is occurrence order. Line 5862 then indexesexplicitby position indeduped.For
function f<T, U>(u: U, t: T): T,collect_type_parameter_symbolsyields[U, T]. A callf<number, string>(...)therefore bindsU := numberandT := string. TypeScript bindsT := numberandU := string. Every argument check and the call's return type are then computed against swapped types.
FunctionSignature::type_parameters()already carries the declaration order — this PR added it. Use it. Note thatexplicit_callable_signatureat lines 4372-4380 already does the right thing by walking the declared list; this path just was not updated to match.Separately:
collect_type_parameter_symbolsalready dedups at line 4417, so thededupedloop at lines 5851-5856 is dead work. The same duplicate loop exists ininferred_function_signatureat lines 5904-5909.🐛 Proposed fix
- let mut inference_symbols = Vec::new(); - for parameter in signature.parameters() { - self.collect_type_parameter_symbols(parameter.type_id(), &mut inference_symbols); - } - self.collect_type_parameter_symbols(signature.return_type(), &mut inference_symbols); - let mut deduped: Vec<SymbolId> = Vec::new(); - for sym in inference_symbols { - if !deduped.contains(&sym) { - deduped.push(sym); - } - } + let mut deduped: Vec<SymbolId> = signature.type_parameters().to_vec(); + if deduped.is_empty() { + // Fall back to occurrence order only when the signature carries no + // declared type-parameter list. + for parameter in signature.parameters() { + self.collect_type_parameter_symbols(parameter.type_id(), &mut deduped); + } + self.collect_type_parameter_symbols(signature.return_type(), &mut deduped); + } if deduped.is_empty() { return Some(signature.clone()); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut deduped: Vec<SymbolId> = signature.type_parameters().to_vec(); if deduped.is_empty() { // Fall back to occurrence order only when the signature carries no // declared type-parameter list. for parameter in signature.parameters() { self.collect_type_parameter_symbols(parameter.type_id(), &mut deduped); } self.collect_type_parameter_symbols(signature.return_type(), &mut deduped); } if deduped.is_empty() { return Some(signature.clone()); } let mut inferred = Vec::new(); for (index, symbol) in deduped.iter().enumerate() { let type_id = explicit .get(index) .copied() .unwrap_or_else(|| self.types.any()); inferred.push(InferredTypeArgument::new( *symbol, type_id, InferenceProvenance::Explicit, )); }🤖 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/checker/binder.rs` around lines 5846 - 5871, Update the explicit type-argument binding in the signature inference path to iterate `signature.type_parameters()` in declaration order, matching `explicit_callable_signature`, rather than using occurrence-ordered `deduped`. Remove the redundant deduplication loop because `collect_type_parameter_symbols` already deduplicates, and apply the same cleanup to `inferred_function_signature`.crates/bamts-compiler/src/checker/inference.rs (1)
261-273: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
instantiatesilently dropsreadonlyon object properties.
PropertyType::newproduces a non-readonly property. Thereadonlyflag of the source property is never carried over. Anyreadonlymember of a generic object type loses that flag the moment the type is instantiated.
TypeTable::wideninbinder.rsat lines 812-825 gets this right with.with_readonly(property.readonly). This path does not. The result is thatreadonlysurvives widening but not instantiation, which is exactly the kind of inconsistency that makes a rule likeBAMTS-W012 readonly-alias-mutationfire or not fire depending on whether a generic sat in the middle.🐛 Proposed fix
Type::ObjectType(properties) => { let properties: Vec<PropertyType> = properties .iter() .map(|property| { PropertyType::new( property.name(), property.optional(), self.instantiate(table, property.type_id()), ) + .with_readonly(property.readonly()) }) .collect(); table.object_type(properties) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Type::ObjectType(properties) => { let properties: Vec<PropertyType> = properties .iter() .map(|property| { PropertyType::new( property.name(), property.optional(), self.instantiate(table, property.type_id()), ) .with_readonly(property.readonly()) }) .collect(); table.object_type(properties) }🤖 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/checker/inference.rs` around lines 261 - 273, Update the object-property mapping in instantiate to preserve each source property's readonly flag, matching the behavior in TypeTable::widen. Construct the instantiated PropertyType with the existing name, optionality, and instantiated type, then apply the source property’s readonly state before collecting the properties.crates/bamts-compiler/src/checker/jsx.rs (1)
340-357: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the
.expectwith recovery. A compiler must not panic on source input.Line 346 asserts that every declared type parameter resolves in
scope. That holds today becausebind_type_parametersat line 315 declares every entry of the same list into the same scope. It holds by construction, not by contract.
declarehas several paths that do not insert under the name you looked up — merge paths, conflict paths, and the empty-name path whenidentifier_textreturns the default for a missing identifier node. This file is reached from JSX/TSX parsing, which is exactly where malformed input arrives. A panic here takes down the whole compilation instead of degrading one element toany, which is the documented recovery contract stated at the top of this file: "Every lookup failure degrades to anany-typed element."Honor that contract here too.
🛡️ Proposed fix
for parameter in &list.parameters { let data = parameter.data(); let name = self.identifier_text(&data.name).into_owned(); - let symbol = self.scopes[scope.get() as usize] - .type_binding(&name) - .expect("type parameter bound above"); + let Some(symbol) = self.scopes[scope.get() as usize].type_binding(&name) else { + continue; + }; let mut inference_parameter = InferenceParameter::new(symbol);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut inference_parameters = Vec::with_capacity(list.parameters.len()); for parameter in &list.parameters { let data = parameter.data(); let name = self.identifier_text(&data.name).into_owned(); let Some(symbol) = self.scopes[scope.get() as usize].type_binding(&name) else { continue; }; let mut inference_parameter = InferenceParameter::new(symbol); if let Some(constraint) = &data.constraint { inference_parameter = inference_parameter.with_constraint(self.resolve_type(constraint, scope)); } if let Some(default) = &data.default { inference_parameter = inference_parameter.with_default(self.resolve_type(default, scope)); } inference_parameters.push(inference_parameter); }🤖 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/checker/jsx.rs` around lines 340 - 357, Replace the expect-based lookup in the type-parameter inference loop with recovery that handles a missing binding without panicking. When type_binding cannot find the name, degrade that inference parameter or element according to this file’s existing any-typed recovery contract, while preserving constraint/default resolution for successfully resolved symbols.crates/bamts-compiler/src/enum_plan.rs (3)
387-402: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Four
#[expect(clippy::too_many_arguments)]in one file is not a lint problem. It is a design problem.
build_with_importstakes eleven parameters.evaluatetakes twelve, and it recurses, so every one of those twelve is copied through at each nesting level. Suppressing the lint four times does not make the signatures readable; it just silences the messenger.Every one of these functions threads the same bundle:
model,source,entries,symbol_to_entry,names_by_enum,values,imported_member_uses,imported_values,source_id,diagnostics. Put that bundle in a struct and pass&mutit. Theexpectattributes then disappear, andevaluate's recursive calls shrink from twelve arguments to three.Also applies to: 418-434, 753-770, 957-970
🤖 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/enum_plan.rs` around lines 387 - 402, Introduce a shared mutable context struct containing model, source, entries, symbol_to_entry, names_by_enum, values, imported_member_uses, imported_values, source_id, and diagnostics, then update build_with_imports, evaluate, and the other flagged functions plus evaluate’s recursive calls to accept a context reference instead of threading these values individually. Remove all four clippy::too_many_arguments expectations while preserving the existing behavior and mutation flow.
608-610: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Unparenthesized
||/&&mix, sixteen lines below an identical condition that is parenthesized.Line 592 writes
(const_enum || binding.ambient) && is_nonfinite(&value). Line 609 writesconst_enum || binding.ambient && entry.initializer.is_some(). Rust precedence makes the second oneconst_enum || (binding.ambient && ...), which is almost certainly what you meant — but a reader scanning both guards will assume they share a shape and misread it. Add the parentheses.♻️ Make the grouping explicit
Evaluated::Runtime - if const_enum || binding.ambient && entry.initializer.is_some() => + if const_enum || (binding.ambient && entry.initializer.is_some()) =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Evaluated::Runtime if const_enum || (binding.ambient && entry.initializer.is_some()) => {🤖 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/enum_plan.rs` around lines 608 - 610, Update the guard in the Evaluated::Runtime match arm to parenthesize const_enum || binding.ambient before applying the entry.initializer.is_some() condition, making its grouping explicit and consistent with the nearby guard.
926-950: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
C042/C043 fire for operators that are not arithmetic.
The guard is
binary.operator != BinaryOperator::Add. That is every non-Addoperator, not the arithmetic and bitwise set. Writeenum E { A = "x" < "y" ? 1 : 2 }-style code — or any relational or equality comparison with a string constant operand inside an enum initializer — and you get "The left-hand side of an arithmetic operation must be of type 'any', 'number', 'bigint' or an enum type." There is no arithmetic operation.binary_valuealready routes those operators toEvaluated::Runtimeat line 1047, so the diagnostic is both wrong and redundant.
binary_valueat lines 994-1005 already enumerates the correct operator set. Reuse it.🐛 Restrict the check to arithmetic and bitwise operators
- if binary.operator != BinaryOperator::Add { + if matches!( + binary.operator, + BinaryOperator::Subtract + | BinaryOperator::Multiply + | BinaryOperator::Divide + | BinaryOperator::Remainder + | BinaryOperator::Exponentiate + | BinaryOperator::LeftShift + | BinaryOperator::SignedRightShift + | BinaryOperator::UnsignedRightShift + | BinaryOperator::BitAnd + | BinaryOperator::BitOr + | BinaryOperator::BitXor + ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// `+` is the only binary operator that accepts string operands; // `-`, `*`, etc. require numeric/bigint/enum operands. if matches!( binary.operator, BinaryOperator::Subtract | BinaryOperator::Multiply | BinaryOperator::Divide | BinaryOperator::Remainder | BinaryOperator::Exponentiate | BinaryOperator::LeftShift | BinaryOperator::SignedRightShift | BinaryOperator::UnsignedRightShift | BinaryOperator::BitAnd | BinaryOperator::BitOr | BinaryOperator::BitXor ) { let left_error = matches!(left, Evaluated::Constant(EnumScalar::String(_))); let right_error = matches!(right, Evaluated::Constant(EnumScalar::String(_))); if left_error { diagnostics.push(error( source_id, ENUM_ARITHMETIC_LEFT_NOT_NUMBER, binary.left.range(), ENUM_ARITHMETIC_LEFT_NOT_NUMBER_MESSAGE, )); } if right_error { diagnostics.push(error( source_id, ENUM_ARITHMETIC_RIGHT_NOT_NUMBER, binary.right.range(), ENUM_ARITHMETIC_RIGHT_NOT_NUMBER_MESSAGE, )); } if left_error || right_error { return Evaluated::Invalid; } }🤖 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/enum_plan.rs` around lines 926 - 950, Restrict the string-operand diagnostic guard in the enum initializer evaluation flow to the arithmetic and bitwise operators supported by binary_value, rather than every operator except BinaryOperator::Add. Reuse the existing operator set or helper used by binary_value, preserving relational and equality operators so they continue to produce Evaluated::Runtime without C042/C043 diagnostics.crates/bamts-native/src/native_bridge.rs (1)
1877-1943: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
bamts_dispose_capture(index 39) has no ABI signature assertion. Add it.Walk the block: 36, 37, then straight to 40. Index 39 is missing.
bamts_dispose_captureis an exported#[unsafe(no_mangle)] extern "C"helper whose signature(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32is now completely unpinned. Change the argument order or a parameter width and nothing in this crate complains — you find out when JIT-generated code corrupts a register index at runtime.This is precisely the failure this block exists to prevent, and it got lost because index 38 was appended at the bottom instead of in sequence. Put 38 back in order so the next gap is visible.
🐛 Proposed fix
const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_to_object; // 36 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_import_dynamic; // 37 +const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_import_meta; // 38 +const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = + bamts_dispose_capture; // 39 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_suppress_error; // 40 @@ const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_with_has_binding; // 45 -const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_import_meta; // 38📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, u64, *mut Completion) -> u32 = bamts_binary; // 2 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_create_object; // 3 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_create_array; // 4 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, *mut Completion) -> u32 = bamts_create_closure; // 5 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_get_property; // 6 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_set_property; // 7 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_delete_property; // 8 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_call; // 9 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_construct; // 10 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_import; // 11 const _: unsafe extern "C" fn(*mut ShadowFrame, u64) -> u32 = bamts_truthy; // 12 (no out) const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_resume_value; // 13 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, u32, *mut Completion) -> u32 = bamts_define_accessor; // 14 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_load_global; // 15 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, *mut Completion) -> u32 = bamts_store_global; // 16 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_typeof_global; // 17 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_this; // 18 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_arguments; // 19 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_new_target; // 20 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_array_push; // 21 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_array_extend; // 22 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_object_spread; // 23 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_set_prototype; // 24 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_create_private_name; // 25 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u32, *mut Completion) -> u32 = bamts_create_regexp; // 26 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, *mut Completion) -> u32 = bamts_get_iterator; // 27 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_iterator_next; // 28 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, *mut Completion) -> u32 = bamts_export; // 29 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_consume_fuel; // 30 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_create_cell; // 31 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_iterator_step; // 32 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_iterator_result; // 33 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_iterator_close; // 34 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_require_close_result; // 35 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_to_object; // 36 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_import_dynamic; // 37 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_import_meta; // 38 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_dispose_capture; // 39 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_suppress_error; // 40 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_construct_with_new_target; // 41 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_define_data_property; // 42 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u32, *mut Completion) -> u32 = bamts_load_own_descriptor_slot; // 43 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, u32, *mut Completion) -> u32 = bamts_define_own_descriptor_slot; // 44 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_with_has_binding; // 45🤖 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 1877 - 1943, Add the missing ABI signature assertion for bamts_dispose_capture at index 39, using the specified parameters (*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32. Reorder the existing bamts_load_import_meta assertion (index 38) before it so the sequence remains ordered and the gap is visible.crates/bamts-node/src/lib.rs (1)
37-47: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect compile_classic_script and ScriptSource to see whether an exact UTF-16 name can be passed. set -euo pipefail fd -t f 'script.rs' crates/bamts-compiler/src --exec ast-grep outline {} --items all echo '--- compile_classic_script definition ---' ast-grep run --lang rust --pattern 'pub fn compile_classic_script($$$) -> $RET { $$$ }' crates/bamts-compiler/src echo '--- ScriptSource definition and name field type ---' rg -nP --type=rust -C 6 '\bstruct\s+ScriptSource\b' crates/bamts-runtime/src echo '--- other from_utf16_lossy call sites in non-test code ---' rg -nP --type=rust -C 3 'from_utf16_lossy' cratesRepository: metaphorics/bamTiScript
Length of output: 2067
🌐 Web query:
Search the public GitHub repository metaphorics/bamTiScript for crates/bamts-compiler/src and crates/bamts-runtime/src definitions of compile_classic_script and ScriptSource.💡 Result:
In the metaphorics/bamTiScript GitHub repository, the definitions for compile_classic_script and ScriptSource are located in the following crates: compile_classic_script This function is defined in the bamts-compiler crate, within the crates/bamts-compiler/src/script.rs module [1][2]. It is designed for filesystem-free classic-script compilation [1]. The signature is [3]: pub fn compile_classic_script( source: &[u16], resource_name: &str,) -> Result<Program, ScriptCompileError> It compiles exact UTF-16 source code into a one-module verified classic-script program without performing filesystem access, project resolution, type checking, or lossy UTF-16 conversion [1][3]. ScriptSource This struct is defined in the bamts-runtime crate, within the crates/bamts-runtime/src/lib.rs module [4]. It represents the exact source of a single classic script [5][4]. The definition is [5]: pub struct ScriptSource<'a> { pub source: &'a [u16], pub name: &'a [u16], } The fields source and name preserve UTF-16 code units verbatim, including unpaired surrogates [5][4]. It is notably used as an argument in the CompileProvider trait's compile_script method [4][6].
Citations:
- 1: https://docs.rs/bamts-compiler/latest/src/bamts_compiler/script.rs.html
- 2: https://docs.rs/crate/bamts-compiler/latest/source/src/lib.rs
- 3: https://docs.rs/bamts-compiler/latest/bamts_compiler/script/fn.compile_classic_script.html
- 4: https://docs.rs/bamts-runtime/latest/src/bamts_runtime/lib.rs.html
- 5: https://docs.rs/bamts-runtime/latest/bamts_runtime/struct.ScriptSource.html
- 6: https://docs.rs/bamts-runtime/latest/bamts_runtime/trait.CompileProvider.html
🏁 Script executed:
#!/bin/bash set -euo pipefail for url in \ "https://api.github.com/repos/metaphorics/bamTiScript/contents/crates/bamts-compiler/src" \ "https://api.github.com/repos/metaphorics/bamTiScript/contents/crates/bamts-runtime/src" \ "https://api.github.com/repos/metaphorics/bamTiScript/git/trees/HEAD?recursive=1" do echo "--- $url ---" curl -LfsS "$url" | head -c 20000 echo doneRepository: metaphorics/bamTiScript
Length of output: 2038
🌐 Web query:
Inspect the source of bamts-compiler::compile_classic_script and its callers, plus the exact-ecmascript-utf16-strings architecture document, to determine whether resource_name reaches language-visible diagnostics and whether any exact UTF-16 compiler entry point exists.💡 Result:
The function bamts_compiler::compile_classic_script takes a resource_name: &str argument [1][2]. Within this function, resource_name is processed via normalized_module_name [1]. If a parsing or assembly error occurs, the resulting diagnostics include information mapped from the source text and potentially the module name context; however, resource_name is primarily used to determine the module naming in the compiled program output rather than being directly exposed as a language-visible diagnostic message [1]. There is no dedicated "exact UTF-16" compiler entry point beyond compile_classic_script itself. The documentation explicitly identifies compile_classic_script as the entry point that accepts exact UTF-16 source (provided as a &[u16]) and performs compilation without lossy UTF-16 conversion [1][2]. The architecture utilizes EcmaString::from_units to handle the UTF-16 input, maintaining the integrity of the data during the compilation pipeline [1].
Citations:
- 1: https://docs.rs/bamts-compiler/latest/src/bamts_compiler/script.rs.html
- 2: https://docs.rs/bamts-compiler/latest/bamts_compiler/script/fn.compile_classic_script.html
🌐 Web query:
Retrieve the current source code for bamts-compiler/src/script.rs, especially normalized_module_name, parser error construction, Program module naming, and all uses of resource_name. Determine exactly whether resource_name appears in SyntaxError messages, stack frames, or vm.Script diagnostics.💡 Result:
In the bamts-compiler crate, the file src/script.rs provides an entrypoint for classic-script compilation [1]. The primary function is compile_classic_script, which takes a resource_name argument [1][2]. Here are the details regarding your request: normalized_module_name: This function is defined in src/script.rs as a helper to normalize the provided resource_name [1]. It is used to resolve the module name during script compilation, falling back to a default constant (DEFAULT_MODULE_NAME) if the normalization returns None [1]. Program module naming: The module name is derived from the resource_name through normalized_module_name [1]. If the resulting name equals DEFAULT_MODULE_NAME, the script is handled as a single-module Program [1]. resource_name usage: The variable resource_name is passed into compile_classic_script [1][2]. Within the function, it is exclusively used as an argument for normalized_module_name to determine the module identifier [1]. SyntaxError messages, stack frames, and vm.Script diagnostics: Based on the source code for src/script.rs, resource_name is not used in SyntaxError messages, stack frames, or vm.Script diagnostics [1]. The error handling logic in src/script.rs, specifically functions like map_lower_error and map_program_error, relies on position-based diagnostics (using line and column numbers derived from SourceText) rather than the resource_name string [1]. top_results: [1][2]
Citations:
- 1: https://docs.rs/bamts-compiler/latest/src/bamts_compiler/script.rs.html
- 2: https://docs.rs/bamts-compiler/latest/bamts_compiler/script/fn.compile_classic_script.html
Pass the exact script name to the compiler
compile_classic_scriptaccepts&strand usesresource_nameto derive the module name.String::from_utf16_lossyreplaces unpaired surrogates before that boundary. Extend the compiler API to accept the exact UTF-16 name, or add an exact-name entry point.🤖 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 37 - 47, Update compile_script and the compiler API behind compile_classic_script so the exact UTF-16 source.name reaches module-name derivation without String::from_utf16_lossy conversion. Add an exact-name entry point or equivalent UTF-16 parameter while preserving existing compilation behavior for valid names.crates/bamts-runtime/src/builtins/collections.rs (3)
526-564: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
One root cause: the collection entry lifecycle was designed for tombstoning and implemented as physical removal.
CollectionEntrycarries aliveflag and both traversal paths filter on it, but the only deletion path removes the entry from the vector outright. Soliveis permanentlytrue, the filters never reject anything, and every delete must renumber the index because the entry indices shifted. Making deletion tombstone the entry fixes the dead flag and eliminates the full index rebuild in one change.
crates/bamts-runtime/src/builtins/collections.rs#L526-L564: replaceentries.remove(entry_index)and the whole-collection index rebuild withentries[entry_index].live = falseplus removal of that one index bucket; change*size -= 1tochecked_sub.crates/bamts-runtime/src/builtins/collections.rs#L271-L283: keep thefind(|entry| entry.live)filter, which becomes meaningful once deletion tombstones, and makeindex.getskip non-live entries.crates/bamts-runtime/src/builtins/collections.rs#L888-L897: inappend_collection_entry, reuse or compact tombstoned slots so the entries vector does not grow without bound under repeated insert-and-delete.crates/bamts-runtime/src/builtins/collections.rs#L947-L960: keep thelivefilter incollection_nextfor the same reason, soforEachskips entries deleted during traversal.📍 Affects 1 file
crates/bamts-runtime/src/builtins/collections.rs#L526-L564(this comment)crates/bamts-runtime/src/builtins/collections.rs#L271-L283crates/bamts-runtime/src/builtins/collections.rs#L888-L897crates/bamts-runtime/src/builtins/collections.rs#L947-L960🤖 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-runtime/src/builtins/collections.rs` around lines 526 - 564, Change map_delete_for in crates/bamts-runtime/src/builtins/collections.rs:526-564 to tombstone the matched entry by setting live to false, remove only its index bucket, and decrement size with checked_sub; eliminate the full index rebuild. In the same file at 271-283, retain the live filter and make index.get skip tombstoned entries; at 888-897, update append_collection_entry to reuse or compact tombstoned slots; and at 947-960, retain collection_next’s live filter so traversal skips deleted entries.
566-596: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
map_clearandset_clearare the same thirty lines twice.The only difference between lines 566-596 and lines 727-757 is
CollectionKind::MapversusCollectionKind::Set. Everything else — the destructure, theremovedcapture, the three field resets, thechecked_mulrefund — is character-for-character identical.You already have the pattern for this:
map_has_for,map_get_for, andmap_delete_forall take anexpected: CollectionKindparameter and are shared between the strong and weak variants.set_hasat line 697 delegates tomap_has_for. Do the same here.♻️ Extract the shared body
+fn collection_clear<H: Host>( + machine: &mut Machine<'_, H>, + this: Value, + expected: CollectionKind, +) -> Result<BuiltinOutcome, EvalFailure> { + let slot = collection_slot(machine, this, expected)?; + let removed = { + let HeapEntry::Collection { + entries, + index, + size, + .. + } = &mut machine.heap[slot] + else { + unreachable!("collection brand was checked") + }; + let removed = *size; + entries.clear(); + index.clear(); + *size = 0; + removed + }; + machine.refund_slot( + slot, + removed + .checked_mul(crate::CollectionEntry::BYTES + crate::CollectionIndex::ENTRY_BYTES) + .expect("collection entry charge fits heap limits"), + ); + Ok(BuiltinOutcome::Value(Value::UNDEFINED)) +}
map_clearandset_clearthen become one-line delegations.Also applies to: 727-757
🤖 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-runtime/src/builtins/collections.rs` around lines 566 - 596, Extract the duplicated clear implementation from map_clear and set_clear into a shared helper accepting an expected CollectionKind, following the existing map_*_for pattern. Have each public clear function delegate to that helper with CollectionKind::Map or CollectionKind::Set, while preserving the existing entry resets, size result, and slot refund behavior.
973-1008: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Two problems: a linear registry scan on the hot path, and a catch-all that silently rejects future heap variants.
Lines 978-982 scan every value in
intrinsics.symbol_registryon every single weak-collection key validation.WeakMap.setis called in a loop by real code. A program that callsSymbol.fora few thousand times turns every subsequentweakMap.set(sym, v)into a few-thousand-element scan. Store aregistered: boolflag onHeapEntry::Symbol, or keep a reverse set of registered symbol values.Lines 983-1001 enumerate eighteen variants as valid, then line 1001 is
_ => false. That default is backwards. Every heap variant listed is an object; the ones that must be rejected are the primitives. When someone adds a nineteenth object-like variant, it silently becomes an invalidWeakMapkey and nobody finds out until a conformance test fails. Drop the_arm so the compiler forces the decision, or invert the match to list the invalid variants explicitly.🤖 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-runtime/src/builtins/collections.rs` around lines 973 - 1008, Update require_weak_key to avoid scanning intrinsics.symbol_registry for every symbol key by adding or reusing O(1) registration metadata on HeapEntry::Symbol, while preserving rejection of registered symbols. Replace the catch-all _ => false match arm with an exhaustive classification of heap variants, or explicitly match only primitive variants as invalid, so newly added object-like variants require a deliberate compiler-checked decision.crates/bamts-runtime/src/builtins/date.rs (2)
46-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Date()withoutnewmust return the human-readable date string, not ISO-8601.The specification says the call form returns
ToDateString(systemUTCTime).ToDateStringproduces the"Mon Jan 01 2024 00:00:00 GMT+0000 (Coordinated Universal Time)"form. This code returnsiso_string(...), soDate()yields"2024-01-01T00:00:00.123Z".Any script that does string matching on
Date()output, or that relies onDate() !== new Date().toISOString(), gets the wrong answer. The test at lines 476-491 asserts the ISO output, so it locks the divergence in rather than catching it.The fallback
"Invalid Date"at line 48 is correct for the invalid case — keep that and change the valid case.Also applies to: 475-491
🤖 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-runtime/src/builtins/date.rs` around lines 46 - 53, Update the non-constructing branch of the Date builtin to format the current time with the human-readable ToDateString representation instead of iso_string, while preserving the existing "Invalid Date" fallback. Update the associated Date() test to assert the human-readable output and ensure it no longer expects ISO-8601 formatting.
81-82: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check whether intrinsics.global reads the mutable globals map, and whether # BuiltinTable already stores prototypes the way array/iterator do. set -euo pipefail rg -n -C 10 'fn global\s*\(' crates/bamts-runtime/src/intrinsics.rs rg -n -C 4 'fn set_constructor_prototype|fn array_prototype|fn iterator_prototype|date_prototype' crates/bamts-runtime/src/intrinsics.rs rg -n -C 6 'new_target|NewTarget' crates/bamts-runtime/src --type=rust | head -40Repository: metaphorics/bamTiScript
Length of output: 2067
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- cwd ---' pwd printf '%s\n' '--- repository state ---' git status --short 2>&1 | head -40 || true printf '%s\n' '--- target files ---' if [ -f crates/bamts-runtime/src/intrinsics.rs ]; then wc -l crates/bamts-runtime/src/intrinsics.rs crates/bamts-runtime/src/builtins/date.rs sed -n '1,220p' crates/bamts-runtime/src/intrinsics.rs printf '%s\n' '--- date builtin ---' sed -n '1,180p' crates/bamts-runtime/src/builtins/date.rs else printf '%s\n' 'local target files unavailable' printf '%s\n' '--- public raw files ---' for url in \ https://raw.githubusercontent.com/metaphorics/bamTiScript/main/crates/bamts-runtime/src/intrinsics.rs \ https://raw.githubusercontent.com/metaphorics/bamTiScript/master/crates/bamts-runtime/src/intrinsics.rs \ https://raw.githubusercontent.com/metaphorics/bamTiScript/main/crates/bamts-runtime/src/builtins/date.rs \ https://raw.githubusercontent.com/metaphorics/bamTiScript/master/crates/bamts-runtime/src/builtins/date.rs do printf '\n--- %s ---\n' "$url" curl -L --fail --silent --show-error --max-time 15 "$url" | head -260 || true done fiRepository: metaphorics/bamTiScript
Length of output: 2038
🌐 Web query:
Search the public web and GitHub for repository metaphorics/bamTiScript and the files crates/bamts-runtime/src/builtins/date.rs and crates/bamts-runtime/src/intrinsics.rs. Return direct source links or the relevant source text.💡 Result:
The repository metaphorics/bamTiScript is a Rust-based project associated with the "BamTS" framework, which includes a TypeScript/JavaScript compiler frontend and execution runtime [1][2][3][4]. The project hosts several crates under this repository, including bamts-runtime, which is described as providing the "execution runtime and built-in intrinsics for BamTS" [3][4]. While the repository metaphorics/bamTiScript exists and hosts these crates [4][5], the specific file paths crates/bamts-runtime/src/builtins/date.rs and crates/bamts-runtime/src/intrinsics.rs are not publicly indexed or accessible via direct source links on the open web at this time. The crates themselves (such as bamts-runtime v0.1.2) are published on crates.io and documented on docs.rs, but the source code for the specified internal modules is not exposed in the rendered documentation or public search index [3][4]. Repository link: https://github.com/metaphorics/bamTiScript [1][4]
Citations:
- 1: https://crates.io/crates/bamts
- 2: https://crates.io/crates/bamts-compiler
- 3: https://crates.io/crates/bamts-runtime
- 4: https://docs.rs/crate/bamts-runtime/latest/source/Cargo.toml
- 5: https://docs.rs/crate/bamts-compiler/latest/source/Cargo.toml
🔍 Additional research query:
Search public GitHub for metaphorics/bamTiScript, specifically crates/bamts-runtime/src/builtins/date.rs and crates/bamts-runtime/src/intrinsics.rs. Determine how intrinsics.global, constructor_prototype, date prototypes, and new.target are implemented.💡 Result:
Findings
intrinsics.global:Intrinsicsstores globals in aBTreeMap<String, Value>. Duringinitialize, builtins and host objects populate it.global(name)simply returns a copied value from that map:self.globals.get(name).copied(). It is not a global-object lookup.
constructor_prototype:BuiltinTable::set_constructor_prototyperequires the constructor to be aHeapEntry::NativeFunction, then inserts a non-writable, non-enumerable, non-configurable"prototype"data property pointing to the supplied prototype object.Date prototype:
date::installcreates an ordinary object whose prototype is the sharedObject.prototype.- It installs the native
Dateconstructor and attaches that object throughset_constructor_prototype.getTime,valueOf, andtoISOStringare installed as properties on the Date prototype.- Static
Date.nowis installed directly on the constructor.- Constructed Date instances are ordinary objects with the Date prototype and a private
"\0Date.value"property containing milliseconds.
new.target: Neither inspected file contains a directnew.targetvalue or implementation. Native constructors receive only a booleanconstructingargument throughBuiltinHandler/call_builtin.Dateuses that flag: when false,Date()returns a date string; when true, it allocates a Date object. Thus, in these files, construction context is represented byconstructing: bool, not by an exposednew.targetobject. The broader user-defined-function implementation is outside the two requested files.Source files:
intrinsics.rs,date.rs.
🔍 Additional research query:
Retrieve the exact source text and line context from https://github.com/metaphorics/bamTiScript/blob/main/crates/bamts-runtime/src/intrinsics.rs and https://github.com/metaphorics/bamTiScript/blob/main/crates/bamts-runtime/src/builtins/date.rs. Also locate the construct/call_builtin implementation that invokes native constructors and handlesnew.target, subclassing, or instance prototypes. Explain whetherdelete globalThis.Dateor reassignment changesmachine.intrinsics.global("Date"), and whethernew MyDate() instanceof MyDatecan be true.💡 Result:
Retrieved source and conclusions
intrinsics.rsRelevant current-
mainlines:// crates/bamts-runtime/src/intrinsics.rs:13–15 pub(crate) enum BuiltinOutcome { Value(Value), Call { callee: Value, this_value: Value, argument_start: usize, }, } pub(crate) type BuiltinHandler = fn(&mut Machine<'_, H>, this: Value, args: &[Value], constructing: bool) -> Result;// intrinsics.rs:43–45 pub(crate) fn set_constructor_prototype( &mut self, heap: &mut [HeapEntry], constructor: Value, prototype: Value, ) { let index = heap_index(constructor); let HeapEntry::NativeFunction { properties, .. } = &mut heap[index] else { panic!("builtin constructor is a native function"); }; properties.insert( crate::PropertyKey::Named("prototype".to_owned()), crate::Property::Data { value: prototype, writable: false, enumerable: false, configurable: false, }, ); }// intrinsics.rs:49–57 pub(crate) struct Intrinsics { globals: BTreeMap<String, Value>, // ... } pub(crate) fn initialize(heap: &mut Vec<HeapEntry>) -> Self { // ... let mut globals = BTreeMap::new(); // ... builtins::install(heap, &mut globals, &mut builtins); crate::host_objects::install(heap, &mut globals, &mut builtins); // ... } pub(crate) fn global(&self, name: &str) -> Option<Value> { self.globals.get(name).copied() }// intrinsics.rs:71 pub(crate) fn call_builtin( &mut self, id: BuiltinId, this_value: Value, arguments: &[Value], constructing: bool, ) -> Result { let handler = self.intrinsics.builtins.get(id).handler; let previous = self.current_builtin_id.replace(id); let outcome = handler(self, this_value, arguments, constructing); self.current_builtin_id = previous; outcome }Source: intrinsics.rs
builtins/date.rs// crates/bamts-runtime/src/builtins/date.rs:7–9 pub(super) fn install( heap: &mut Vec<HeapEntry>, globals: &mut BTreeMap<String, Value>, builtins: &mut BuiltinTable, ) { let prototype = super::super::ordinary_prototype(heap, builtins.object_prototype()); let constructor = install_function(heap, builtins, "Date", 7, constructor); builtins.set_constructor_prototype(heap, constructor, prototype); // ... globals.insert("Date".to_owned(), constructor); }// date.rs:11 fn constructor( machine: &mut Machine<'_, H>, _this: Value, args: &[Value], constructing: bool, ) -> Result { let milliseconds = if let Some(value) = args.first().copied() { value_number(machine.to_number(value)?) } else { machine.host.now_ms() as f64 }; if !constructing { return Ok(BuiltinOutcome::Value(allocate_string( machine, iso_string(milliseconds) .unwrap_or_else(|| "Invalid Date".to_owned()), )?)); } let constructor = machine.intrinsics.global("Date").expect("Date installed"); let prototype = machine.get_named_property(constructor, "prototype")?; // Creates an ordinary object whose [[Prototype]] is Date.prototype. // ... }Source: date.rs
Behavioral answers
delete globalThis.Datedoes not changemachine.intrinsics.global("Date"). The intrinsic is stored separately inIntrinsics.globals, andglobal()only reads thatBTreeMap.- Reassigning
globalThis.Datelikewise does not update that intrinsic map. It changes the exposed global property, but the Date native handler explicitly retrieves the original intrinsic withmachine.intrinsics.global("Date").- The native-constructor dispatch receives only
constructing: bool; the shown implementation has nonew.targetparameter or handling.- For a normal JavaScript subclass,
new MyDate()therefore creates an object whose prototype is the intrinsicDate.prototype, notMyDate.prototype. Consequently,new MyDate() instanceof MyDateis normally false.- It can be true if
MyDate.prototypeis made identical to, or placed in the prototype chain of, the object’s actual prototype—for example, by aliasing or manually changing prototypes. That would be prototype manipulation, not nativenew.target-aware subclassing.
Honor
new.targetwhen constructingDateinstances. The constructor always uses the intrinsicDate.prototype, sonew MyDate() instanceof MyDateisfalse. Use the effective constructor’sprototype, withDate.prototypeas fallback.🤖 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-runtime/src/builtins/date.rs` around lines 81 - 82, Update the Date construction logic around the intrinsic constructor and prototype lookup to use the effective new.target constructor’s “prototype” property, falling back to the intrinsic Date.prototype when absent or invalid. Ensure instances created through subclasses, such as MyDate, use that constructor’s prototype while preserving the existing Date fallback behavior.crates/bamts-verification/src/check_cells.rs (2)
851-878: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The diagnostics oracle throws away the file, so cross-unit position collisions pass.
collect_facet_diagnosticsflattens every reached unit into one flat list keyed by(line, character, category, severity, code). The unit is never recorded.expected_facet_diagnostics(lines 908-916) dropsErrorsDiagnostic::unitin exactly the same way, even though the parser went to the trouble of capturing it at line 635.Consequence: for a multi-unit case, an error the compiler emits at
a.ts(3,5)compares equal to a baseline row atb.ts(3,5). The comparator reports parity. The module header at lines 1-9 claims this machinery is "the canonical diagnostics union" andexecute_types_checkadvertises "never a false pass". This is a false pass, and it is the only oracle guarding S2.Carry the unit through both sides and compare it. The baseline prints the unit basename, so compare against
unit.virtual_path's basename.🤖 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/src/check_cells.rs` around lines 851 - 878, Preserve the originating unit in both collect_facet_diagnostics and expected_facet_diagnostics instead of flattening diagnostics without file identity. Populate the baseline side from ErrorsDiagnostic::unit by comparing its basename with unit.virtual_path’s basename, and include that unit identity in the diagnostic comparison key so identical positions and metadata from different units cannot match.
966-979: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
baseline_groupsrescans the entire suite index once per cell.The doc comment at lines 431-432 says "Build the stem/suffix groups once from the snapshot index". Then line 968 calls it per cell, and
execute_types_check(line 1409) andexecute_symbols_check(line 1743) do the same. Every call walksindex.entriesin full and allocates twoStrings per baseline row.With the ledger sizes this PR reports, that is a full index scan multiplied by the cell count.
resolve_errors_baselinesthen piles oncase_inputs_with_stem, which is another full index scan, plusbaseline_owner, which reads a blob from disk per candidate.Build the groups once and hand them to the executors through
CheckContext, which already exists for exactly this purpose.🤖 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/src/check_cells.rs` around lines 966 - 979, Move the single `baseline_groups(&snapshot.index)` construction out of per-cell execution and store the resulting groups in `CheckContext`; update the relevant setup path to build them once, then pass or reuse them in the executors including the flows around `execute_types_check`, `execute_symbols_check`, and the shown cell logic. Replace each per-cell `baseline_groups` call with the context-provided groups while preserving `resolve_errors_baselines` behavior.crates/bamts-verification/src/perf.rs (2)
751-776: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Two of the four budgets are wired to constants. They gate nothing.
Line 751 reads
VmHWMfrom/proc/self/statusonce, after every repeat has finished, and then stuffs the same scalar intop50,p95, andp99.VmHWMis the high-water mark of the whole harness process and it never decreases. It includes suite materialization, JSON parsing, and every allocation the runner made before the benchmark started. Three identical numbers are not a distribution, sorss.p95inevaluate_budgetscan never say anything thatrss.p50did not already say.Line 776 is worse:
artifact_bytes: 0, always.check_abs_rel("artifact.p50", 0.0, baseline, …)passes unconditionally.The module doc at the top sells RSS and artifact bytes as real budgets and never says they are stubs.
Quantiles::zero()already exists as the explicit placeholder in this file, so the vocabulary for "not measured yet" is right there and unused.Either measure per-repeat RSS deltas and the real emitted artifact size, or mark both as unmeasured and make
evaluate_budgetsskip them the way it already skips a missing baseline. Do not ship a gate that always returns green.🤖 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/src/perf.rs` around lines 751 - 776, Replace the fabricated RSS quantiles and constant artifact_bytes in the measurement result with explicit unmeasured values using Quantiles::zero() and the appropriate zero artifact representation. Update evaluate_budgets to skip these metrics when they are unmeasured, matching its existing missing-baseline behavior, so RSS and artifact checks cannot report false passing gates.
814-818: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A baseline with no
totalphase silently disables every wall-time budget.
load_baselinefalls back toQuantiles::zero()when the parsedMeasureResulthas no"total"key.check_ratiothen doesif base <= 0.0 { return Ok(()) }forwall.p50,wall.p95, andwall.p99. So a truncated, hand-edited, or schema-drifted baseline file does not fail the run. It passes the run.This fails open on the one metric the harness exists to protect. The file parsed as JSON, so the operator gets no signal at all.
require_phase_keysalready exists in this module and does exactly the right check. Call it on the loaded baseline and returnPerfErrorCode::NoBaselinewhen the key is absent.🐛 Proposed fix to reject a baseline missing `total`
+ require_phase_keys(&base.phases)?; let wall_ms = base .phases .get("total") .copied() - .unwrap_or_else(Quantiles::zero); + .ok_or_else(|| { + PerfError::new( + PerfErrorCode::NoBaseline, + format!("{}: baseline has no `total` phase", path.display()), + ) + })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require_phase_keys(&base.phases)?; let wall_ms = base .phases .get("total") .copied() .ok_or_else(|| { PerfError::new( PerfErrorCode::NoBaseline, format!("{}: baseline has no `total` phase", path.display()), ) })?;🤖 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/src/perf.rs` around lines 814 - 818, Update load_baseline to validate the loaded baseline with require_phase_keys, requiring the "total" phase before extracting wall_ms. If the phase is absent, return PerfErrorCode::NoBaseline instead of falling back to Quantiles::zero(), while preserving the existing behavior for valid baselines.crates/bamts-verification/src/suite.rs (1)
1052-1077: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Two directory walks call
entry.metadata()wherefs::symlink_metadatais required.entry.metadata()follows symlinks, sofile_type().is_symlink()on its result is alwaysfalseand every symlink is silently treated as whatever it points at.walk_extracted_treeandmerge_compiler_license_noticealready usefs::symlink_metadatacorrectly; these two did not get the memo.
crates/bamts-verification/src/suite.rs#L1052-L1077: indirectory_size, replaceentry.metadata()at Line 1062 withfs::symlink_metadata(entry.path())and hoist the symlink rejection above theis_dirbranch, so the guard at Lines 1064-1069 becomes reachable and the walk stops escaping the AOT scratch directory or looping on a symlink cycle.crates/bamts-verification/src/suite.rs#L2248-L2268: insingle_archive_root, replaceentry.metadata()at Line 2252 withfs::symlink_metadata(entry.path()), so a symlink pointing at a directory is not counted as the single archive root.📍 Affects 1 file
crates/bamts-verification/src/suite.rs#L1052-L1077(this comment)crates/bamts-verification/src/suite.rs#L2248-L2268🤖 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/src/suite.rs` around lines 1052 - 1077, Update crates/bamts-verification/src/suite.rs#L1052-L1077 in directory_size to use fs::symlink_metadata(entry.path()), and reject symlinks before branching on is_dir so symlink targets cannot be traversed. Also update crates/bamts-verification/src/suite.rs#L2248-L2268 in single_archive_root to use fs::symlink_metadata(entry.path()), ensuring symlinks to directories are not treated as the single archive root.crates/bamts-verification/src/ts_ledger.rs (1)
789-803: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
to_stringmakes four full copies of the ledger. The PR description says that ledger is 169 MB.Count them:
ledger.clone()— a deep clone of everyEntry, everyString, everyVec.serde_json::to_value(&sorted)— a second full tree asserde_json::Value, which is far larger per node than the typed struct.to_string_pretty(&value)— the whole document as oneString.canonicalize_json—text.lines()into aVec<&str>, thenjoin("\n")allocates the entire document a second time as aString.Peak resident memory is several times the artifact size, and
to_vecthen callsinto_byteson top of that. For a 169 MB output this is a multi-gigabyte spike inside a CI job.The clone at Line 791 exists only so
sort_and_recomputecan run on an owned value. Take&mut TsLedger, or sort aVecof indices. Theto_string_pretty+ line-rejoin pass exists only to strip trailing whitespace and\r;serde_jsonpretty output never emits either, so that entire step is copying 169 MB to remove characters that are not there.🤖 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/src/ts_ledger.rs` around lines 789 - 803, Refactor `to_string` to avoid cloning the full ledger and duplicating the serialized document: accept or otherwise operate on a mutable `TsLedger` so `sort_and_recompute` runs in place, then serialize directly to the final JSON string without the `to_string_pretty` plus `canonicalize_json` copy-and-rejoin path. Preserve sorting, validation, key ordering, and canonical output behavior, and update callers such as `to_vec` to match the ownership changes.crates/bamts-verification/src/workspace_guard.rs (1)
1307-1319: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
require_enabled_featurehardcodes"bamts-codegen", and the two blocks below it exist because of that.The function takes a
closure, amode, and afeature.modeis used only in the error string. The package is nailed to"bamts-codegen"at Line 1310, so the helper can check exactly one package's features and nothing else.The consequence is visible immediately. Lines 1224-1234 hand-roll the same lookup for
bamts-native. Lines 1241-1251 hand-roll it again forbamts-codegenunder a different root. Two open-coded copies of a helper, sitting directly beneath the helper, because the helper cannot take a package name.Add the parameter. Then the two blocks below collapse into two calls.
♻️ Proposed refactor to parameterize the package
-fn require_enabled_feature(closure: &ResolvedClosure, mode: &str, feature: &str) -> Result<()> { - let active = closure - .package_features - .get("bamts-codegen") - .ok_or_else(|| workspace_error("codegen closure lacks bamts-codegen features"))?; - if !active.contains(feature) { - return Err(workspace_error(format!( - "{mode} metadata closure does not enable bamts-codegen feature `{feature}`" - ))); - } - - Ok(()) -} +fn require_enabled_feature( + closure: &ResolvedClosure, + mode: &str, + package: &str, + feature: &str, +) -> Result<()> { + let active = closure + .package_features + .get(package) + .ok_or_else(|| workspace_error(format!("{mode} closure lacks `{package}` features")))?; + if !active.contains(feature) { + return Err(workspace_error(format!( + "{mode} metadata closure does not enable `{package}` feature `{feature}`" + ))); + } + + Ok(()) +}Lines 1224-1234 and 1241-1251 then become:
require_enabled_feature(&closure, "bamts-codegen/host-jit", "bamts-native", "jit-entry")?; require_enabled_feature(&closure, "bamts/host-jit", "bamts-codegen", "host-jit")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn require_enabled_feature( closure: &ResolvedClosure, mode: &str, package: &str, feature: &str, ) -> Result<()> { let active = closure .package_features .get(package) .ok_or_else(|| workspace_error(format!("{mode} closure lacks `{package}` features")))?; if !active.contains(feature) { return Err(workspace_error(format!( "{mode} metadata closure does not enable `{package}` feature `{feature}`" ))); } Ok(()) }🤖 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/src/workspace_guard.rs` around lines 1307 - 1319, Parameterize require_enabled_feature with a package-name argument and use it for the package_features lookup and error context as needed. Replace the duplicated bamts-native and bamts-codegen feature-check blocks beneath it with calls using the respective package names and existing closure, mode, and feature values shown in the review.crates/bamts-verification/tests/corpus_differential.rs (1)
1291-1349: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This 55-line block is copy-pasted about fourteen times in one file.
Spawn
node_modules/.bin/tsc, pass the same fourteen arguments, assertstatus.success(), build aCaseSpecwith the same eight constant fields, clone it with a swapped entrypoint, loopExecutionMode::ALL, remove two files, assertfailures.is_empty(). Identical every time. The argument array alone —--target es2022,--module commonjs,--strict false,--esModuleInterop,--skipLibCheck,--rootDir target,--outDir target/decorator-oracles,--allowJs,--checkJs false— appears fourteen times verbatim, with a--lib es2022,dom,esnext.disposablevariant appearing five more times.Bumping the target to
es2023is a fourteen-site edit. Miss one and you get a test comparing against a differently-transpiled oracle, which will look like a runtime bug and will be debugged as one.The drift has already started. Every other test joins failures with
"\n\n"; the one at Lines 849-853 spells the same separator as a literal string spanning four source lines. Nobody wrote that on purpose. It is what copy-paste does to a file this size.Extract one helper and let the tests state only what differs: the source, the extension, and the oracle flavour.
♻️ Sketch of the extraction
struct TranspiledCase { spec: CaseSpec, actual_spec: CaseSpec, _fixtures: FixtureGuard, } /// Writes `source`, transpiles it with the pinned `tsc` flags, and returns the /// oracle spec plus the direct-source spec. Fixtures are removed on drop. fn transpiled_case( root: &Path, label: &str, extension: &str, out_dir: &str, extra_tsc_args: &[&str], source: &str, ) -> TranspiledCase { /* … */ } /// Runs every execution mode against the Node oracle and asserts parity. fn assert_parity(root: &Path, case: &TranspiledCase, what: &str) { /* … */ }Each test then reads:
let case = transpiled_case(&root, "class-decorators", "js", "decorator-oracles", &[], SOURCE); assert_parity(&root, &case, "class decorator");🤖 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 1291 - 1349, Extract the repeated TypeScript transpilation, CaseSpec construction, execution-mode comparison, fixture cleanup, and failure assertion from the duplicated tests into shared helpers such as TranspiledCase, transpiled_case, and assert_parity. Centralize the common tsc arguments and CaseSpec defaults while allowing each test to provide only its source, label, extension, output directory, and oracle-specific arguments; ensure fixture cleanup remains automatic and parity failures use the consistent separator.formal/lean/Bamti/Bytecode/Model.lean (1)
93-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
abbrev Program := FunctionmakesProgrammean "one function body" in a file that also definesProgramEnvelope. Rename it.You now have three things in one namespace:
Program(a singleFunction),ProgramModule(a module plus linkage), andProgramEnvelope(the actual v4 program). ThenprogramCanonicalat line 417 aliasesfunctionCanonical, and it sits eleven lines aboveenvelopeCanonical.Everything downstream —
instructionBoundaries,targets,backEdgeTargets,entryPoints,nextPc— takesProgram, meaning one function body. Anyone stating a new theorem "about programs" will writeProgram, get a single function, and it will typecheck and prove something far weaker than they think. In a formal model the names are the load-bearing part. This one actively lies.Rename the alias to
BodyorFunctionBody, renameprogramCanonicalto match, and leaveProgrameither unused or bound toProgramEnvelope.Also applies to: 417-417
🤖 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 `@formal/lean/Bamti/Bytecode/Model.lean` around lines 93 - 99, Rename the single-function alias `Program` to `FunctionBody` (or `Body`) and update all downstream symbols that currently use it, including `instructionBoundaries`, `targets`, `backEdgeTargets`, `entryPoints`, and `nextPc`. Rename `programCanonical` to match the function-body terminology, and reserve `Program` for `ProgramEnvelope` or leave it unused; do not alter `ProgramModule` or `ProgramEnvelope` semantics.formal/lean/Bamti/Bytecode/Verify.lean (1)
79-85: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 5 \ 'formatVersion|programMagicBytes|moduleMagicBytes|createCell|CreateCell|BMTPC|BMTBC' \ crates/bamts-bytecode formal || trueRepository: metaphorics/bamTiScript
Length of output: 2067
🌐 Web query:
Search the public GitHub repository metaphorics/bamTiScript for formal/lean/Bamti/Bytecode/Verify.lean and the production Rust bytecode constants for formatVersion, programMagicBytes, moduleMagicBytes, and CreateCell.💡 Result:
The public GitHub repository for the BamTS project is metaphorics/bamTiScript [1][2]. The formal model is maintained under the path formal/lean/Bamti/Bytecode/Model.lean [3] and the verification of the production bytecode is performed against the logic in Bytecode/Verify.lean [4][3]. The production Rust bytecode constants for the project are defined within the bamts-bytecode crate as follows [4][5]: - formatVersion: The constant PROGRAM_VERSION is defined as 3, representing the sole supported program-envelope version [6][5]. - programMagicBytes: The constant PROGRAM_MAGIC is [7] (BMTPC\0\0\1) [5][8]. - moduleMagicBytes: The constant MAGIC is [7] (BMTBC\0\0\1) [4][9][5]. - CreateCell: This is an instruction in the production instruction algebra (Instruction::CreateCell), which is lowered by the bamts_codegen crate [10][11]. Note that CreateCell is an instruction opcode used to create new objects/cells rather than a magic byte constant [10][11].
Citations:
- 1: https://crates.io/crates/bamts
- 2: https://docs.rs/crate/bamts-bytecode/latest/source/Cargo.toml
- 3: https://docs.rs/crate/bamts-bytecode/latest
- 4: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/
- 5: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/index.html
- 6: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.PROGRAM_VERSION.html
- 7: https://github.com/harbor-framework/frontier-bench/blob/main/tasks/lean-midpoint-proof/tests/verify.lean
- 8: https://docs.rs/bamts-bytecode/latest/src/bamts_bytecode/program.rs.html
- 9: https://docs.rs/bamts-bytecode/latest/src/bamts_bytecode/lib.rs.html
- 10: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/enum.Instruction.html
- 11: https://docs.rs/bamts-codegen/latest/bamts_codegen/
🏁 Script executed:
#!/bin/bash set -euo pipefail base='https://raw.githubusercontent.com/metaphorics/bamTiScript' for path in \ formal/lean/Bamti/Bytecode/Verify.lean \ formal/lean/Bamti/Bytecode/Model.lean \ crates/bamts-bytecode/src/lib.rs \ crates/bamts-bytecode/src/program.rs do echo "===== $path =====" curl -fsSL "$base/HEAD/$path" | \ rg -n -C 4 \ 'formatVersion|programMagicBytes|moduleMagicBytes|PROGRAM_VERSION|PROGRAM_MAGIC|MAGIC|CreateCell|create_cell|createCell' || true doneRepository: metaphorics/bamTiScript
Length of output: 2038
🌐 Web query:
Search exact source definitions in metaphorics/bamTiScript: "PROGRAM_VERSION" "PROGRAM_MAGIC" "formatVersion" "format_v4_createCell_tag".💡 Result:
In the metaphorics/bamTiScript project, specifically within the
bamts-bytecodecrate, the identifiers refer to the following technical definitions: PROGRAM_VERSION: A constant defined aspub const PROGRAM_VERSION: u8 = 3;, representing the sole supported version of the program-envelope [1][2]. PROGRAM_MAGIC: A constant defined aspub const PROGRAM_MAGIC: [u8; 8] = [66, 77, 84, 80, 67, 0, 0, 1];(corresponding to the ASCII/byte sequenceBMTPC\0\0\1), which serves as the canonical identifier for a whole-program container [1]. formatVersion: This refers to theFORMAT_VERSIONconstant, defined aspub const FORMAT_VERSION: u8 = 3;, which denotes the sole supported wire format version [3]. format_v4_createCell_tag: There is no direct definition for this specific string in the public technical documentation forbamts. The project utilizes a NaN-boxed value representation with specific tag codes (e.g.,TAG_BOOLEAN,TAG_HEAP_REF,TAG_HOLE,TAG_INT32,TAG_NULL,TAG_UNDEFINED,TAG_UNINITIALIZED) defined inbamts_native[4]. It is possible that "format_v4_createCell_tag" refers to an internal implementation detail, a generated symbol, or a concept outside the current stable API documentation [4][5].Citations:
- 1: https://docs.rs/bamts-bytecode/latest/src/bamts_bytecode/program.rs.html
- 2: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.PROGRAM_VERSION.html
- 3: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.FORMAT_VERSION.html
- 4: https://docs.rs/bamts-native/latest/bamts_native/
- 5: https://docs.rs/crate/bamts-native/latest
🌐 Web query:
Open the published bamts-bytecode Rust source for program.rs, lib.rs, and the Instruction encoder. Extract the exact definitions of PROGRAM_VERSION, FORMAT_VERSION, PROGRAM_MAGIC, module MAGIC, and the opcode value or encoding for Instruction::CreateCell. Also identify the crate version and repository commit used by the documentation.💡 Result:
The crate version for bamts-bytecode is 0.1.0 [1][2]. The documentation does not explicitly specify a repository commit hash, but the associated repository is https://github.com/metaphorics/bamTiScript [1]. The requested constant definitions are as follows: PROGRAM_VERSION: pub const PROGRAM_VERSION: u8 = 3; [3] FORMAT_VERSION: pub const FORMAT_VERSION: u8 = 3; [4] PROGRAM_MAGIC: pub const PROGRAM_MAGIC: [u8; 8] = [1]; [5] (This represents BMTPC\0\0\1) [5] MAGIC (module magic): pub const MAGIC: [u8; 8] = [1]; [6] (This represents BMTBC\0\0\1) [6][7] Instruction::CreateCell: The Instruction algebra defines opcodes 0..=36 as stable wire tags [6][8]. While the source code uses an enum for Instruction, the opcode value is implicitly determined by the order of variants in the enum [6]. The instruction is defined as: CreateCell { dst: Register } [6]
Citations:
- 1: https://docs.rs/crate/bamts-bytecode/latest/source/Cargo.toml
- 2: https://docs.rs/crate/bamts-bytecode/latest
- 3: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.PROGRAM_VERSION.html
- 4: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.FORMAT_VERSION.html
- 5: https://docs.rs/bamts-bytecode/latest/src/bamts_bytecode/program.rs.html
- 6: https://docs.rs/bamts-bytecode/latest/src/bamts_bytecode/lib.rs.html
- 7: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.MAGIC.html
- 8: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/enum.Instruction.html
Align the theorem with the production wire format.
bamts-bytecodedefinesPROGRAM_VERSIONandFORMAT_VERSIONas3, but this theorem hard-codesformatVersion = 4. It proves a Lean model that does not match production. Use a shared/generated source or an executable Rust/Lean cross-check, and validateCreateCellagainst the production opcode mapping.🤖 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 `@formal/lean/Bamti/Bytecode/Verify.lean` around lines 79 - 85, Update theorem format_v4_createCell_tag to derive the format version and magic bytes from the shared/generated production definitions instead of hard-coding formatVersion = 4, and ensure it reflects the production v3 values. Validate encodeOpcode .createCell against the production opcode mapping through the existing Rust/Lean cross-check or generated source, preserving the theorem’s conjunction structure.formal/lean/Bamti/JitLifecycle.lean (1)
343-356: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
ProviderStepis a partial relation.JitStep, in this same file, is total. The gap is exactly the buggy-caller cases.Look at
JitStepabove: every action has a paired rejected constructor with the negated guard —queueRejected,finalizeRejected,publishRejected, and so on. Callpublishin a bad phase and the model tells you it is rejected and the artifact goes to.failed.
ProviderStephas no such pairs. There is no rule forallocatefrom.Executable, none forallocatefrom.Freed, none forfinalizefrom.Executable. Those transitions are not rejected by the model; they are simply absent from it.That weakens what the W^X theorems actually say.
provider_never_writable_executableproves you cannot derive a writable phase from an executable one. It does not prove that a realallocatecall against a finalized provider gets refused. For a memory-protection property, "the model has no rule for it" and "the implementation rejects it" are very different claims, and only the second one keepsmmapfrom handing back a W+X page.Add the rejected constructors so the relation is total, matching the
JitSteppattern you already established twelve lines up. Then a theorem stating "allocate from.Executableis rejected" becomes provable rather than vacuous.🔧 Proposed fix
inductive ProviderStep : ProviderPhase → ProviderAction → ProviderResult → Prop where | allocateAccepted : ProviderStep .Writable .allocate (.accepted .Writable) + | allocateRejected (p : ProviderPhase) (h : p ≠ .Writable) : + ProviderStep p .allocate (.rejected p) | finalizeAccepted : ProviderStep .Writable .finalize (.accepted .Executable) | finalizeFailed : ProviderStep .Writable .finalize (.rejected .Freed) + | finalizeRejected (p : ProviderPhase) (h : p ≠ .Writable) : + ProviderStep p .finalize (.rejected p) | freeAccepted (p : ProviderPhase) (h : p = .Writable ∨ p = .Executable) : ProviderStep p .free (.accepted .Freed) | freeIdempotent : ProviderStep .Freed .free (.accepted .Freed)The existing proofs need the new cases added, and
provider_trace_exclusivityneeds arms for them. Both rejected transitions keep the phase unchanged, so the invariant still holds.🤖 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 `@formal/lean/Bamti/JitLifecycle.lean` around lines 343 - 356, Add rejected ProviderStep constructors for every invalid action/phase combination, following the total JitStep pattern and preserving the current phase in each rejection. Update affected proofs to handle the new constructors, especially provider_trace_exclusivity, while retaining existing accepted-transition behavior.npm/bamti-cli/package.json (1)
25-30: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail for package in \ bamti-cli-darwin-arm64 \ bamti-cli-darwin-x64 \ bamti-cli-linux-arm64 \ bamti-cli-linux-x64 \ bamti-cli-win32-x64 do echo "== $package ==" npm view "$package" version dist-tags.latest maintainers --json doneRepository: metaphorics/bamTiScript
Length of output: 2067
🌐 Web query:
Check the public npm registry for bamti-cli-darwin-arm64, bamti-cli-darwin-x64, bamti-cli-linux-arm64, bamti-cli-linux-x64, and bamti-cli-win32-x64. Report whether each package exists, its latest version, and its maintainers or publisher.💡 Result:
As of August 7, 2026, the following packages are not found on the public npm registry: bamti-cli-darwin-arm64 bamti-cli-darwin-x64 bamti-cli-linux-arm64 bamti-cli-linux-x64 bamti-cli-win32-x64 Searches of the public npm registry returned no matches for these specific package names. Other similarly named packages (such as bami-cli or
@knfs-tech/bamimi-cli) exist but are distinct from the requested packages.Citations:
- 1: https://www.npmjs.com/package/bami-cli
- 2: https://www.npmjs.com/package/@knfs-tech/bamimi-cli
- 3: https://www.npmjs.com/package/bambu-cli
- 4: https://registry.npmjs.org/%40tauri-apps%2Fcli-linux-arm64-gnu
- 5: https://www.npmjs.com/package/@apm-cli/linux-arm64
Claim all five artifact package names before publishing
bamti-cli.None of the five unscoped packages exists on the public npm registry. An unrelated publisher can claim these names before release. Publish all five under the release account and keep the resolver and optional dependencies aligned.
📍 Affects 3 files
npm/bamti-cli/package.json#L25-L30(this comment)npm/bamti-cli/index.js#L9-L13npm/artifacts/cli-win32-x64/package.json#L2-L2🤖 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 `@npm/bamti-cli/package.json` around lines 25 - 30, Claim and publish all five unscoped artifact packages under the release account before publishing bamti-cli, then keep npm/bamti-cli/package.json optionalDependencies and npm/bamti-cli/index.js resolver names aligned with the artifact package name in npm/artifacts/cli-win32-x64/package.json; update each affected site as needed to use the claimed names consistently.vendor/sources.toml (1)
19-33: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Confirm reproducibility of the pinned GitHub archives by fetching twice. set -euo pipefail url="https://github.com/microsoft/typescript-go/archive/2bd066d87f5bafd315be9f40889d0a60b9e58e0b.tar.gz" for attempt in 1 2; do curl -sL "$url" | sha256sum done echo "expected 5ccb47dbb3f68cd0da58b71e6f445eee36a50bd3e9f9f330cc23a97b88500119"Repository: metaphorics/bamTiScript
Length of output: 2067
🌐 Web query:
GitHub autogenerated source archives deterministic tar.gz reproducible archive format codeload archive commit byte stable💡 Result:
GitHub-autogenerated source archives are not guaranteed to be deterministic or byte-for-byte stable over time [1][2][3]. While they are generated using the
git archivecommand [1][4], GitHub does not make explicit guarantees that these archives will maintain identical binary output across different requests or platform updates [2][3][5]. Key points regarding GitHub source archives include: Generation and Stability: GitHub generates these archives on-demand [1][3]. Because the underlying software stack—including thegitversion and compression settings—can be updated by GitHub, the byte layout (and therefore the checksum/hash) of a generated archive can change even if the source code contents remain identical [2][3][6]. GitHub has explicitly stated that they do not intend for these autogenerated archives to be used as a reliable, byte-stable method for software distribution where cryptographic verification (e.g., checksums) is required [3][5]. Codeload: Archives are frequently served viacodeload.github.com[5][7]. This service functions as an on-demand generator, and its output is subject to the same lack of long-term bit-for-bit stability as other GitHub-generated archives [7][8]. Recommendations for Reproducibility: To ensure reproducible builds or long-term integrity, GitHub and industry experts recommend the following alternatives: 1. Use the Archives REST API with a specific commit ID: Requesting an archive via the API using a full commit SHA ensures that the file contents inside the archive remain consistent, as commit IDs are immutable [1][2]. However, the outer archive container itself may still vary if compression or archival settings change [2]. 2. Use Release Assets: For projects where checksum stability is mandatory (e.g., for security or package management), maintainers should manually generate release tarballs, sign them, and upload them as release assets [2][3][5]. Unlike autogenerated archives, release assets are preserved as-is and remain byte-stable once uploaded [3]. In summary, GitHub's autogenerated archives should not be relied upon for processes that require exact, persistent, byte-for-byte reproducibility across different time periods [2][3][8].Citations:
- 1: https://docs.github.com/en/repositories/working-with-files/using-files/downloading-source-code-archives
- 2: https://github.blog/open-source/git/update-on-the-future-stability-of-source-code-archives-and-hashes/
- 3: github/docs#31141
- 4: https://github.com/github/docs/blob/main/content/repositories/working-with-files/using-files/downloading-source-code-archives.md
- 5: keybase/client#10800
- 6: https://lists.reproducible-builds.org/pipermail/rb-general/2022-October/002712.html
- 7: https://lists.openwrt.org/pipermail/openwrt-devel/2018-August/019347.html
- 8: openwrt/packages#6748
🌐 Web query:
GitHub REST API tarball archive generated archive checksum reproducible codeload.github.com archive commit💡 Result:
GitHub-generated tarball and zipball archives (including those retrieved via the REST API) are not guaranteed to have stable, reproducible checksums [1][2]. Because these archives are generated on-demand and cached, GitHub reserves the right to change compression settings, environment configurations, or internal generation processes, which can alter the byte-for-byte output of the archive [1][3][2]. Key considerations regarding archive reproducibility include: 1. Archive Instability: GitHub explicitly states that checksums for automatically generated source archives may change [2]. While the contents (the file tree) remain identical for a given commit, the compressed archive (the
.tar.gzor.zipfile itself) may have a different byte layout over time due to updates in the underlying generation software [1][3][4]. 2. Role of the REST API: You can use the GitHub REST API (GET /repos/{owner}/{repo}/tarball/{ref}) to request an archive for a specific commit ID [5]. While using a specific commit ID ensures you will always retrieve the same file contents, it does not guarantee that the resulting archive's checksum will remain stable across different generation requests [1][6]. 3. Recommended Practices for Reproducibility: If your workflow requires stable, reproducible checksums (e.g., for security verification or deterministic build systems), GitHub strongly recommends that you do not rely on these on-demand source code archives [1][2]. Instead, you should: - Upload your own pre-built archives as Release Assets [6][3][2]. Release assets are stored as-is and are guaranteed to remain immutable [2]. - If you must use automatically generated archives, you should accept that their checksums are not reliable for long-term verification [6][3]. For those needing to verify the contents of a repository at a specific point in time without relying on GitHub-generated archives, cloning the repository and running git archive locally is a standard way to maintain control over the archive generation process [7][3].Citations:
- 1: https://docs.github.com/en/repositories/working-with-files/using-files/downloading-source-code-archives
- 2: https://github.blog/changelog/2023-01-30-git-archive-checksums-may-change/
- 3: github/docs#31141
- 4: https://github.com/github/docs/blob/main/content/repositories/working-with-files/using-files/downloading-source-code-archives.md
- 5: https://docs.github.com/en/rest/repos/contents
- 6: https://github.blog/open-source/git/update-on-the-future-stability-of-source-code-archives-and-hashes/
- 7: https://lists.reproducible-builds.org/pipermail/rb-general/2021-October/002422.html
🔍 Additional research query:
Search GitHub documentation and public repository metadata for whether /archive/<commit>.tar.gz source archives are byte-stable, and identify the resolved commits for microsoft/typescript-go 2bd066d87f5bafd315be9f40889d0a60b9e58e0b and microsoft/TypeScript 4d4f005c8541e0255a9d8791205fdce326e462bc.💡 Result:
Finding
/archive/<commit>.tar.gzarchives are not guaranteed byte-stable. GitHub guarantees that a commit archive will contain the same extracted file contents, provided the commit remains available and the repository name is unchanged. However, compression settings may change, altering the compressed archive’s bytes and checksum.For byte-stable distribution, use an uploaded GitHub release asset rather than an autogenerated source archive.
Resolved commits
Repository Requested SHA Resolved metadata microsoft/typescript-go2bd066d87f5bafd315be9f40889d0a60b9e58e0bExact commit; merge message “Merge branch 'main' into ts7-release”, dated 2026-07-08; associated with tag typescript/v7.0.2; parentsb4171c3e...and52168999....microsoft/TypeScript4d4f005c8541e0255a9d8791205fdce326e462bcExact commit; merge message “Merge branch 'main' into tsgo-port”; parents 0ab90cf...and637d574....Archive URLs:
https://github.com/microsoft/typescript-go/archive/2bd066d87f5bafd315be9f40889d0a60b9e58e0b.tar.gz https://github.com/microsoft/TypeScript/archive/4d4f005c8541e0255a9d8791205fdce326e462bc.tar.gz
Do not use GitHub-generated archive digests as long-term pins.
GitHub may change compression settings for
/archive/<sha>.tar.gzwithout changing the extracted commit contents. Store a maintainer-controlled release asset, or create the archive locally after verifying the commit. Two matching downloads only test current output; they cannot prove future byte stability.🤖 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 `@vendor/sources.toml` around lines 19 - 33, Replace the GitHub-generated archive URLs and digests for the typescript-7-compiler and typescript-7-suite sources with maintainer-controlled, byte-stable release assets, or locally generated archives created after verifying each referenced commit. Keep the existing commit pins and ensure the recorded SHA-256 digests correspond to the new stable artifacts.crates/bamts-compiler/src/emitter.rs (2)
906-911: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Find whether EmitOptions carries a module-target/interop setting that # should gate the `export =` lowering, and locate consumers of the emitted JavaScript. set -euo pipefail rg -n -C 8 'struct EmitOptions|impl EmitOptions|enum EmitMode' crates/bamts-compiler/src/emitter.rs rg -n -C 4 'module.exports|esModuleInterop|CommonJS|commonjs' crates/bamts-compiler/src --type=rustRepository: metaphorics/bamTiScript
Length of output: 2067
🌐 Web query:
GitHub metaphorics bamTiScript crates/bamts-compiler/src/emitter.rs💡 Result:
The file src/emitter.rs is a component of the bamts-compiler crate, which is the TypeScript/JavaScript compiler frontend for the BamTS project [1][2]. The BamTS source code is hosted in the metaphorics/bamTiScript repository on GitHub [3][1]. Within the bamts-compiler crate, the emitter module is responsible for the deterministic emission of AST (Abstract Syntax Tree) to JavaScript and declaration files [4]. It is integrated into the compiler's frontend pipeline, which follows a sequence of scanning, parsing, checking, and optional emitting [5]. The emitter stage is invoked within the pipeline to process the recovered tree, and it operates regardless of whether diagnostics were produced by preceding stages [5]. The bamts-compiler crate (version 0.1.0) was created on July 30, 2026, and is licensed under the MIT license [1]. It depends on the bamts-bytecode crate and is part of a larger ecosystem that includes runtime, codegen, and native bridge components [1][6][7][8].
Citations:
- 1: https://crates.io/crates/bamts-compiler
- 2: https://docs.rs/crate/bamts-compiler/latest/source/src/emitter.rs
- 3: https://crates.io/crates/bamts
- 4: https://docs.rs/bamts-compiler/latest/bamts_compiler/
- 5: https://docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs
- 6: https://crates.io/crates/bamts-runtime
- 7: https://crates.io/crates/bamts-codegen
- 8: https://crates.io/crates/bamts-native
🌐 Web query:
site:docs.rs/crate/bamts-compiler/latest/source/src/emitter.rs "ExportDeclaration::Assignment" "EmitOptions"💡 Result:
The term ExportDeclaration::Assignment does not appear to be part of the official TypeScript compiler API or standard TypeScript language specifications. The combination of ExportDeclaration, Assignment, and EmitOptions appears in the source code of bamts-compiler, a specific third-party TypeScript/JavaScript compiler frontend [1]. In that context, these terms are identifiers used within the emitter module of that specific library [1].
Citations:
🌐 Web query:
site:github.com/metaphorics/bamTiScript "module.exports" "ExportDeclaration::Assignment"💡 Result:
No exact match was found for that GitHub search query. The closest results reference TypeScript/Babel handling of
ExportDeclaration,ExportAssignment, andmodule.exports, but not themetaphorics/bamTiScriptrepository specifically. (gist.github.com)Citations:
🏁 Script executed:
#!/bin/bash set -euo pipefail url='https://docs.rs/crate/bamts-compiler/latest/source/src/emitter.rs' curl -LfsS "$url" -o /tmp/bamts-emitter.html python3 - <<'PY' from html.parser import HTMLParser from pathlib import Path class P(HTMLParser): def __init__(self): super().__init__() self.in_code=False self.parts=[] def handle_starttag(self, tag, attrs): if tag in ('code','pre'): self.in_code=True def handle_endtag(self, tag): if tag in ('code','pre'): self.in_code=False def handle_data(self, data): if self.in_code: self.parts.append(data) text=''.join(P().feed(Path('/tmp/bamts-emitter.html').read_text()) or []) Path('/tmp/bamts-emitter.txt').write_text(text) PY rg -n -C 12 'ExportDeclaration|module\.exports|EmitOptions|emit_expression_prec' /tmp/bamts-emitter.txt | head -240Repository: metaphorics/bamTiScript
Length of output: 2038
🔍 Additional research query:
Use Tavily or DeepWiki to retrieve the current source ofcrates/bamts-compiler/src/emitter.rsfrommetaphorics/bamTiScript, includingEmitOptions,ExportDeclaration::Assignment, and allmodule.exportsemission paths. Also identify whether the compiler has a module-target or interop option and how tests invoke the emitter.💡 Result:
Retrieval status
- The repository is public, and
maincontainscrates/bamts-compiler/src/emitter.rs. GitHub reports 3,898 lines / 137 KB.- Current source links:
- The compiler test directory currently exposes
corpus_parse.rs.I could not retrieve the file body or verify
EmitOptions,ExportDeclaration::Assignment,module.exportspaths, module-target/interop options, or emitter invocation details: DeepWiki has not indexed the repository, and Tavily requests were blocked by its usage limit.
🌐 Web query:
https://raw.githubusercontent.com/metaphorics/bamTiScript/main/crates/bamts-compiler/src/emitter.rs💡 Result:
The file located at the specified GitHub path is part of the bamts-compiler crate, which serves as the TypeScript/JavaScript compiler frontend for the BamTS project [1][2][3]. BamTS is a compiler and runtime framework [4] that provides tools for compiling and executing TypeScript/JavaScript code, featuring a bytecode format (identified by the magic bytes BMTBC\0\0\1) [5] and a Node.js host compatibility layer [6]. The bamts-compiler crate is one of several components in the BamTS ecosystem, which also includes: - bamts: A high-level facade crate that provides the main entry point for compiling and running programs [7]. - bamts-bytecode: Defines the bytecode specification [4][5]. - bamts-codegen: Provides backends for JIT/AOT code generation [8]. - bamts-runtime: Contains the runtime environment [4][7]. - bamts-node: Offers a Node.js-compatible host environment [6]. The project was created recently, with initial versions (0.1.0) published to crates.io around July 30, 2026 [4][2]. The emitter module within the compiler crate is responsible for the final stages of the compilation pipeline, translating the compiler's intermediate representations or syntax trees into the project's native bytecode format [4][9].
Citations:
- 1: https://docs.rs/crate/bamts-compiler/latest
- 2: https://crates.io/crates/bamts-compiler
- 3: https://docs.rs/crate/bamts-compiler/0.1.0
- 4: https://crates.io/crates/bamts
- 5: https://docs.rs/bamts-bytecode/latest/bamts_bytecode/constant.MAGIC.html
- 6: https://docs.rs/crate/bamts-node/latest
- 7: https://docs.rs/bamts/latest/bamts/
- 8: https://docs.rs/crate/bamts-codegen/latest/source/Cargo.toml
- 9: https://docs.rs/bamts-compiler/latest/bamts_compiler/syntax/type.Stmt.html
🔍 Additional research query:
Use Valyu to fetch and analyze https://raw.githubusercontent.com/metaphorics/bamTiScript/main/crates/bamts-compiler/src/emitter.rs. Return the definitions and call sites forEmitOptions,ExportDeclaration::Assignment,module.exports, and any module-target or interop configuration.💡 Result:
Result
Valyu could not extract the source contents. It only confirmed that the file exists at the requested path and contains 3,898 lines; therefore, I cannot reliably return definitions or call sites for:
EmitOptionsExportDeclaration::Assignmentmodule.exports- module-target or interop configuration
The raw-file and GitHub API extraction attempts failed, while the GitHub HTML page exposed only file metadata.
🌐 Web query:
"pub struct EmitOptions" "bamts-compiler"💡 Result:
In the bamts-compiler crate (version 0.1.0), EmitOptions is a struct defined in the emitter module [1] used to configure the emission process [2]. It is primarily utilized within the compiler's pipeline to determine the output type of the compilation process [2]. The FrontendMode enum provides methods to instantiate specific EmitOptions [2]: - EmitOptions::javascript is used when the compilation target is JavaScript [2]. - EmitOptions::declaration is used when the target is a declaration file [2]. The emitter uses these options to perform deterministic AST-to-JavaScript or AST-to-declaration emission [3]. While the exact internal fields of EmitOptions are defined in the crate source (specifically in src/emitter.rs) [4], it acts as a configuration parameter passed to the emitter::emit function along with the parsed product [2]. This crate is distinct from other similarly named tools like BAML's compiler [5] or the main bamts crate [6]. It is part of the BamTS project, a TypeScript/JavaScript compiler frontend [7].
Citations:
- 1: https://docs.rs/bamts-compiler/latest/bamts_compiler/all.html
- 2: https://docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs
- 3: https://docs.rs/bamts-compiler/latest/bamts_compiler/
- 4: https://docs.rs/crate/bamts-compiler/latest/source/src/emitter.rs
- 5: https://github.com/BoundaryML/baml/blob/13112c3a/baml_language/crates/baml_compiler_emit/src/lib.rs
- 6: https://docs.rs/bamts/latest/src/bamts/lib.rs.html
- 7: https://crates.io/crates/bamts-compiler
🌐 Web query:
"ExportDeclaration::Assignment" "bamTiScript"💡 Result:
In the context of the BamTiScript (BamTS) project, ExportDeclaration::Assignment refers to a variant of the ExportDeclaration enumeration within the bamts-compiler crate [1][2]. Specifically, the bamts-compiler syntax definition includes an ExportDeclaration enum that represents different ways of exporting values, where the Assignment variant is defined to hold an expression [1]: pub enum ExportDeclaration { Named(ExportNamedDeclaration), All(ExportAllDeclaration), Default(ExportDefaultDeclaration), Assignment(Box), } This structure is part of the compiler's abstract syntax tree (AST) for parsing and processing TypeScript/JavaScript code [3]. It is distinct from the related concept of an "Export Assignment" (export =) in TypeScript [4], which corresponds to a different construct, often handled in other AST representations (such as ts-morph) as a separate ExportAssignment node [5][6]. BamTiScript (BamTS) is a compiler and runtime framework for TypeScript/JavaScript, published on crates.io, consisting of several components including a compiler (bamts-compiler), runtime (bamts-runtime), and code generation backend (bamts-codegen) [7][3][8].
Citations:
- 1: https://docs.rs/bamts-compiler/latest/src/bamts_compiler/syntax.rs.html
- 2: https://docs.rs/bamts-compiler/latest/bamts_compiler/syntax/enum.NodeKind.html
- 3: https://crates.io/crates/bamts-compiler
- 4: https://docs.syntblaze.com/typescript/modules/export-assignment
- 5: https://github.com/dsherret/ts-morph/blob/latest/packages/ts-morph/src/compiler/ast/module/ExportAssignment.ts
- 6: https://ts-morph.com/details/exports
- 7: https://crates.io/crates/bamts
- 8: https://crates.io/crates/bamts-codegen
🌐 Web query:
"module.exports" "bamTiScript"💡 Result:
BamTiScript refers to a project hosted on GitHub (github.com/metaphorics/bamTiScript) that provides a TypeScript/JavaScript compiler, runtime, and framework [1][2][3]. The query module.exports bamTiScript appears to be a reference to a Node.js-style CommonJS export pattern often used to expose functionality from a script. While the codebase is primarily written in Rust—utilizing crates such as bamts-compiler, bamts-runtime, and bamts-native—it includes a bamts-node crate designed to provide Node.js host environment compatibility [1][2][3]. In such an integration, a JavaScript file or module within that ecosystem might use module.exports to expose the BamTiScript functionality or a specific script instance to the surrounding Node.js runtime [3]. The project focuses on features such as verified classic-script compilation, a deterministic register interpreter for bytecode, and support for JIT/AOT execution [4][1][5]. You can find technical documentation for the various components (compiler, runtime, native ABI) on docs.rs under the bamts crate prefix [6][4][7][5].
Citations:
- 1: https://crates.io/crates/bamts
- 2: https://crates.io/crates/bamts-compiler
- 3: https://crates.io/crates/bamts-node
- 4: https://docs.rs/bamts-compiler/latest/bamts_compiler/script/index.html
- 5: https://docs.rs/bamts-runtime/latest/bamts_runtime/
- 6: https://docs.rs/bamts-compiler/latest/src/bamts_compiler/script.rs.html
- 7: https://docs.rs/bamts-native/latest/bamts_native/
🌐 Web query:
"EmitOptions {" "bamts_compiler"💡 Result:
The term EmitOptions within the context of the bamts-compiler crate refers to a configuration structure used by the compiler's emitter module [1][2]. In bamts-compiler (a TypeScript/JavaScript compiler frontend for BamTS) [3][4], EmitOptions is defined in the crate::emitter module [1]. It is used to specify how the compiler should generate output during the compilation pipeline [1]. For instance, it is used by the FrontendMode to determine whether to perform a check-only operation or to emit specific types of artifacts, such as JavaScript or declaration files [1]. It is important to distinguish this from EmitOptions in the.NET ecosystem (Microsoft.CodeAnalysis.Emit namespace) [5], which is a well-known class used in the Roslyn compiler infrastructure to control compilation emit behavior (e.g., debug information format, metadata-only emit, etc.) [6][7]. They are unrelated structures belonging to different programming languages and project ecosystems [4][5].
Citations:
- 1: https://docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs
- 2: https://docs.rs/crate/bamts-compiler/latest/source/src/emitter.rs
- 3: https://docs.rs/crate/bamts-compiler/latest
- 4: https://crates.io/crates/bamts-compiler
- 5: https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.emit?view=roslyn-dotnet-5.0.0
- 6: https://source.dot.net/Microsoft.CodeAnalysis/Emit/EmitOptions.cs.html
- 7: https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.compilation.emit?view=roslyn-dotnet-4.13.0
🌐 Web query:
site:docs.rs/bamts-compiler/latest/src/bamts_compiler/emitter.rs "struct EmitOptions"💡 Result:
No exact match was found for that query. The closest Docs.rs result is
hiker::backends::EmitOptions, defined with a singlemodule: Stringfield. (docs.rs)Citations:
🌐 Web query:
site:docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs "EmitOptions::javascript"💡 Result:
EmitOptions::javascript is used to request JavaScript emission of a recovered/checked syntax tree; the mode that returns Some(EmitOptions::javascript) corresponds to “JavaScript”, while “Check” returns None (check-only, no emit). [1] In the provided code, JavaScript emission “erases the type annotation” (the test asserts the emitted JS does not contain the string "number"), whereas the “Declaration” mode retains the type annotation (the test asserts the declaration output contains "number"). [1] So, in this crate’s context, EmitOptions::javascript configures the emitter to produce JavaScript output with type information removed/erased. [1]
Citations:
🌐 Web query:
"EmitOptions::javascript" "FrontendMode" bamts💡 Result:
In the context of the bamts-compiler (a TypeScript/JavaScript compiler frontend for BamTS), EmitOptions::javascript is a method used to configure the compiler to produce JavaScript output [1]. The FrontendMode enum defines the desired product of the compiler frontend, and EmitOptions::javascript is specifically associated with the FrontendMode::JavaScript variant [1]. When this mode is selected, the compiler emits runtime JavaScript code with type-only syntax (such as TypeScript type annotations) erased [1]. As seen in the implementation, the FrontendMode::JavaScript mode uses this method as follows [1]: impl FrontendMode { const fn emit_options(self) -> Option { match self { Self::Check => None, Self::JavaScript => Some(EmitOptions::javascript), Self::Declaration => Some(EmitOptions::declaration), } } } The bamts-compiler is a Rust-based tool (version 0.1.0) designed for managing TypeScript/JavaScript frontend orchestration, lowering ASTs to canonical bytecode, and performing semantic analysis [2][3].
Citations:
- 1: https://docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs
- 2: https://docs.rs/bamts-compiler/latest/bamts_compiler/
- 3: https://crates.io/crates/bamts-compiler
🌐 Web query:
"export default " "ExportDeclaration::Assignment" Rust💡 Result:
The phrase "export default" and the specific syntax structure "export default AssignmentExpression" are not features of the Rust programming language [1][2]. Instead, these terms belong to the ECMAScript (JavaScript) specification, which defines how modules export bindings [2]. In the context of Rust, you may encounter these terms in the following scenarios: 1. AST Parsers and Compilers: If you are building or working with a JavaScript parser written in Rust (such as those used in web browsers or JavaScript runtimes), you will define data structures—often called "Abstract Syntax Trees" (ASTs)—to represent JavaScript syntax [3][4]. In these implementations, you might find Rust enums or structs named ExportDeclaration, which include a variant or field to handle the AssignmentExpression associated with an export default statement [3][5][4]. 2. JavaScript Runtimes in Rust: When using Rust to embed a JavaScript engine (like Deno's use of V8), you may need to interact with JavaScript modules [6]. You do not write export default in your Rust code, but your Rust code must be able to resolve and retrieve the "default" export from a JavaScript module if you are executing JS within your application [6]. If you are seeing "export default AssignmentExpression" inside Rust source code, it is almost certainly part of a parser or transpiler implementation that is processing JavaScript code [3][4].
Citations:
- 1: https://tc39.es/proposal-module-declarations/
- 2: https://tc39.es/ecma262/2024/multipage/ecmascript-language-scripts-and-modules.html
- 3: https://docs.rs/tishlang_ast/latest/tishlang_ast/enum.ExportDeclaration.html
- 4: https://code.moldybits.net/moldybits/rust_browser/src/branch/fix/es-modules-fifth-review-3-4/_bmad-output/implementation-artifacts/3-4-es-modules.md
- 5: https://docs.rs/tishlang_ast/latest/src/tishlang_ast/ast.rs.html
- 6: denoland/deno#17837
🌐 Web query:
"ExportDeclaration::Assignment(expression)" bamts💡 Result:
The term ExportDeclaration::Assignment does not refer to a standard part of the ECMAScript (JavaScript) specification, but rather appears to be a internal representation or identifier used within specific compilers, transpilers, or abstract syntax tree (AST) tooling [1][2][3][4]. In the context of JavaScript ASTs and compiler design: 1. Abstract Syntax Trees (ASTs): Tools such as Esprima, Closure Compiler, and various Rust-based JavaScript engines (like Boa) define their own internal structures to represent export declarations [1][5][2][6]. While the ECMAScript specification classifies exports into types like ExportAllDeclaration, ExportDefaultDeclaration, and ExportNamedDeclaration, individual tools may use more granular or internal names like Assignment to distinguish specific node types during parsing or transformation [5][2][6]. 2. TypeScript and Tooling: In TypeScript-related tooling (e.g., ts-morph, TypeScript compiler internals), ExportAssignment is a distinct node kind used to handle legacy export assignment syntax (e.g., export =...), which is separate from standard ES6 export declarations [3][4]. The syntax ExportDeclaration::Assignment may appear in source code or documentation of these compilers as an internal path, namespace, or enumeration member [1][2][4]. 3. "bamts": This is likely a typographical error or a context-specific abbreviation not associated with standard programming language specifications or widely recognized AST tooling. In summary, ExportDeclaration::Assignment is not a language feature but an implementation-specific construct used by compiler infrastructure to manage how exports are parsed, validated, and transformed into target code [1][2][4].
Citations:
- 1: https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/parsing/parser/Parser.java
- 2: https://docs.rs/boa_ast/latest/src/boa_ast/declaration/export.rs.html
- 3: dsherret/ts-morph@f2b346b
- 4: https://github.com/Microsoft/TypeScript/blob/v2.4.2/src/compiler/transformers/ts.ts
- 5: https://docs.esprima.org/en/latest/syntax-tree-format.html
- 6: https://docs.esprima.org/en/4.0/syntax-tree-format.html
🌐 Web query:
TypeScript compiler export = target ES2015 TS1203 export assignment cannot be used when targeting ECMAScript modules official💡 Result:
The error TS1203: Export assignment cannot be used when targeting ECMAScript modules occurs because the CommonJS-style export syntax (export =...) is incompatible with the ECMAScript module (ESM) specification [1][2]. TypeScript's export assignment (export =...) and import assignment (import x = require(...)) are designed specifically to mimic the CommonJS module.exports and require behavior [2]. Because the ECMAScript module standard does not support this specific pattern, the TypeScript compiler prevents its use when the module system is set to an ESM target (such as ES2015, ES2020, ES2022, ESNext, Node16, NodeNext, etc.) [2][3][4]. To resolve this error, you should replace the CommonJS-style exports with standard ECMAScript module syntax [1][2]: 1. If you are exporting a single value or function, use export default instead: // Change this: export = myFunction; // To this: export default myFunction; 2. If you are exporting multiple items, use named exports: // Change this: export = { foo, bar }; // To this: export { foo, bar }; Alternatively, if you are working in a project that requires keeping CommonJS-style syntax (for example, in a legacy codebase), you must set the compiler option "module" to "commonjs" in your tsconfig.json file [1][4]. If you are working in a modern environment and need to interoperate with existing CommonJS modules, you may benefit from setting "esModuleInterop": true in your tsconfig.json, which allows you to use standard ESM import/export syntax while maintaining compatibility with CommonJS dependencies [5].
Citations:
- 1: https://stackoverflow.com/questions/55701403/export-assignment-cannot-be-used-when-targeting-ecmascript-modules
- 2: https://www.typescriptlang.org/docs/handbook/modules/reference
- 3: https://www.typescriptlang.org/docs/handbook/modules/reference.html
- 4: https://www.typescriptlang.org/tsconfig/module.html
- 5: microsoft/TypeScript#32150
🌐 Web query:
TypeScript compiler output export = module.exports CommonJS official documentation💡 Result:
In TypeScript, the export = syntax is a mechanism specifically designed to support the legacy CommonJS module pattern, providing a direct analog to module.exports [1][2]. Key characteristics and usage: - Functionality: When using export =, you are instructing the TypeScript compiler to set the module.exports object to the specified value (such as a class, function, interface, or object) [1][2]. - Import Syntax: Modules exported using export = must be imported using the TypeScript-specific import identifier = require("module") syntax [1][2]. - Purpose: This syntax is used to model traditional CommonJS or AMD workflows where a module exports a single, specific object rather than a collection of named exports [2][3]. - Modern Context: While still supported, the TypeScript handbook recommends using ECMAScript Module (ESM) syntax where possible [4]. For projects specifically targeting CommonJS, modern best practices suggest using compiler options like verbatimModuleSyntax [5][4]. This setting enforces the use of CommonJS-style imports and exports (import x = require and export =) in files emitted as CommonJS, ensuring predictable output and avoiding issues related to interop flags like esModuleInterop [5][4][6]. When the compiler is configured to output CommonJS (e.g., via module settings like commonjs, node16, or nodenext), the export = statement is transpiled directly to a module.exports assignment [1][7]. In newer TypeScript versions (5.4+), the --module preserve option also maintains this mapping, preserving the CommonJS-style import and export statements in the output [1][7].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/modules/reference
- 2: https://github.com/microsoft/TypeScript-Handbook/blob/master/pages/Modules.md
- 3: microsoft/TypeScript#7185
- 4: https://www.typescriptlang.org/docs/handbook/modules/theory.html
- 5: https://www.typescriptlang.org/docs/handbook/modules/appendices/esm-cjs-interop.html
- 6: https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html
- 7: https://www.typescriptlang.org/tsconfig/module.html
🌐 Web query:
site:docs.rs/crate/bamts-compiler/latest/source "export =" bamts💡 Result:
The term bamts refers to BamTS, a project described as a TypeScript/JavaScript compiler frontend [1][2]. In the context of this codebase, specifically within its compiler and bytecode modules, Export is a data structure used to represent export declarations and their associated sources within the module system [3]. The expression export = bamts is not a standard language feature of TypeScript or JavaScript, nor is it a documented syntax or configuration directive for the BamTS compiler itself. Given the project structure, it is likely that this syntax snippet was either a misunderstanding of TypeScript's CommonJS-compatible export assignment syntax (export =...) or a specific, non-standard usage within internal code or testing files for the bamts-compiler project [4][5]. BamTS is implemented in Rust and focuses on compiling TypeScript/JavaScript, providing functionality for parsing, linting, and lowering code [2]. Its internal modules, such as syntax.rs and program.rs, handle the representation of various JavaScript/TypeScript constructs, including export statements (ExportDeclaration) [3][6].
Citations:
- 1: https://docs.rs/crate/bamts-compiler/latest/source/
- 2: https://docs.rs/crate/bamts-compiler/latest/source/src/lib.rs
- 3: https://docs.rs/crate/bamts-compiler/latest/source/src/program.rs
- 4: https://docs.rs/crate/bamts-compiler/latest/source/RULES.md
- 5: https://docs.rs/crate/bamts-compiler/latest/source/tests/corpus_parse.rs
- 6: https://docs.rs/crate/bamts-compiler/latest/source/src/syntax.rs
🌐 Web query:
site:docs.rs/crate/bamts-compiler/latest/source/src "module.exports" bamts💡 Result:
The term "bamts" refers to a TypeScript/JavaScript compiler project, typically associated with the Rust-based crate "bamts-compiler" [1][2]. The string "module.exports = bamts" (or similar variations) is not a standard or documented export pattern for any widely recognized public library or official tool named "bamts". In the context of the bamts-compiler project, which is written in Rust, this syntax would not be relevant as it is a characteristic pattern of CommonJS modules in Node.js, whereas Rust projects do not use
module.exports[1][3]. If you encountered this in a codebase, it is likely a local alias, a specific configuration for a private tool, or a misunderstanding of a project's module system. Because bamts-compiler is a developer tool for compiling JavaScript/TypeScript (often into bytecode), it operates independently of standard Node.js module export mechanisms [4][5][2].Citations:
- 1: https://docs.rs/crate/bamts-compiler/latest/source/src/lib.rs
- 2: https://docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs
- 3: https://docs.rs/crate/bamts-compiler/latest/source/src/source.rs
- 4: https://docs.rs/crate/bamts-compiler/latest/source/src/program.rs
- 5: https://docs.rs/crate/bamts-compiler/latest/source/src/script.rs
🌐 Web query:
site:docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs "module" "FrontendMode"💡 Result:
FrontendMode is an enumeration defined within the bamts-compiler crate, which is used to specify the desired output for the compilation of a source module [1]. The enum defines three distinct modes that determine how the frontend processes the input [1]: - Check: Performs scanning, parsing, and type-checking without producing any emitted output [1]. - JavaScript: Performs the same initial steps as Check and additionally emits runtime JavaScript with type-only syntax erased [1]. - Declaration: Performs the initial steps and additionally emits a TypeScript declaration file [1]. The FrontendMode is utilized by functions such as compile_program_frontend to govern the compilation process for program modules [1]. [1]
Citations:
Preserve
export =module semantics
EmitOptionshas no module-target or interop setting. This unconditional lowering changes the CommonJS-style single module value into an ESM default export. TypeScript rejectsexport =for ESM targets and emitsmodule.exports = xfor CommonJS targets. Preservemodule.exports = x, or add an explicit module-target decision. The parse-only test does not catch this module-shape change.🤖 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/emitter.rs` around lines 906 - 911, Update the ExportDeclaration::Assignment branch in the emitter to preserve export = as CommonJS module.exports assignment rather than unconditionally emitting an ESM default export. Since EmitOptions lacks a module-target decision, emit the existing CommonJS shape or introduce an explicit target-based choice; ensure tests cover emitted module shape, not only parsing.
1034-1050: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
emit_enum_scalarwrites past the indentation machinery and corrupts layout.Every other output path goes through
self.raw, which flushespending_indent. Line 1045 callswrite!(self.out, "{value}")directly. When a numeric const-enum use is the first thing emitted on a fresh line,pending_indentis still set, the leading spaces are never written, and the followingrawcall thinks the indent was already flushed.Reachable path:
emit_block→newline()setspending_indent→emit_statement→emit_expression_statement→emit_expression_prec→emit_expression_inner→ line 1449emit_enum_scalar. A const-enum reference used as a bare expression statement inside any indented block loses its indentation.Line 1063-1065 in
emit_enum_stringhas the sameself.out.pushshortcut. It happens to be safe only becauseself.raw("\"")runs first. Do not rely on that.🐛 Route the numeric branch through `raw`
} else { - write!(self.out, "{value}").expect("writing to a String cannot fail"); + let mut text = String::new(); + write!(text, "{value}").expect("writing to a String cannot fail"); + self.raw(&text); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn emit_enum_scalar(&mut self, scalar: &EnumScalar) { match scalar { EnumScalar::Number(value) => { let value = value.to_f64(); if value.is_nan() { self.raw("NaN"); } else if value == f64::INFINITY { self.raw("Infinity"); } else if value == f64::NEG_INFINITY { self.raw("-Infinity"); } else { let mut text = String::new(); write!(text, "{value}").expect("writing to a String cannot fail"); self.raw(&text); } } EnumScalar::String(value) => self.emit_enum_string(value), } }🤖 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/emitter.rs` around lines 1034 - 1050, Update emit_enum_scalar so finite numeric values are emitted through self.raw rather than writing directly to self.out, ensuring pending indentation is flushed consistently. Preserve the existing NaN and infinity representations, and leave emit_enum_string unchanged except where needed to maintain the same indentation behavior.crates/bamts-compiler/src/parser.rs (1)
3968-3969: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
jsx_spansis not rolled back, so a speculated-and-discarded JSX rescan still suppresses lexical diagnostics.
rollbackrestorescursor,prev_end,diagnostics,next_node_id, and replays the token journal. It does not touchjsx_spans.rescan_jsx_spanpushes here unconditionally, and JSX parsing is reachable from speculation:speculate_paren_arrowandspeculate_generic_arrowboth parse parameter initializers, which reachparse_primary_expression, which reachesparse_jsx.When that speculation rolls back, the token splice is undone but the span entry survives.
parsethen drops every default-pass lexical diagnostic starting inside a region that was never committed as JSX. Errors vanish from a source that has none of the JSX the parser briefly imagined.Note the regex path does not have this problem: it recomputes
regex_spansfrom the final token stream at line 157, so a rolled-back rescan leaves no trace. Either recordjsx_spans.len()inParserCheckpointand truncate inrollback, or derive JSX spans from the committed token stream the same way.🐛 Proposed fix
struct ParserCheckpoint { cursor: usize, prev_end: usize, diagnostics: usize, next_node_id: u32, journal: usize, + jsx_spans: usize, }fn checkpoint(&self) -> ParserCheckpoint { ParserCheckpoint { cursor: self.cursor, prev_end: self.prev_end, diagnostics: self.diagnostics.len(), next_node_id: self.next_node_id, journal: self.journal.len(), + jsx_spans: self.jsx_spans.len(), } }self.cursor = checkpoint.cursor; self.prev_end = checkpoint.prev_end; self.diagnostics.truncate(checkpoint.diagnostics); self.next_node_id = checkpoint.next_node_id; + self.jsx_spans.truncate(checkpoint.jsx_spans);🤖 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 3968 - 3969, Ensure speculative JSX rescans do not survive rollback: extend ParserCheckpoint and the rollback logic to capture the current jsx_spans length and truncate jsx_spans back to that checkpoint value when speculation is discarded. Update the checkpoint creation and rollback paths, preserving committed JSX spans while removing entries added by rescan_jsx_span during failed speculation.crates/bamts-compiler/src/pipeline.rs (1)
144-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Binary search correctness rests on a
debug_assertthat never runs in release.
smallest_containingusespartition_pointonnode.range.start(). That is only valid ifchildrenis sorted by start position. The only thing enforcing it is thedebug_assert!at line 144. In a release build an out-of-order child list produces a wrongNodeIdwith no warning, and the caller cannot tell.The
Tryarm concatenates block, handler, and finalizer, and theSwitcharm flat-maps case consequents. Both happen to be source-ordered today. The next statement kind added to this match will not be checked by anything.Sort the vector before returning it. The lists are tiny and this runs once per statement.
🛡️ Proposed fix
- debug_assert!( - children - .windows(2) - .all(|pair| pair[0].range.end() <= pair[1].range.start()), - "edge node children must be source-ordered and non-overlapping" - ); + let mut children = children; + // `smallest_containing` binary-searches this list, so the order is a + // correctness requirement, not a convention. + children.sort_by_key(|node| node.range.start());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut children = children; // `smallest_containing` binary-searches this list, so the order is a // correctness requirement, not a convention. children.sort_by_key(|node| node.range.start()); EdgeNode { id, range: statement.range(), children, } } fn node_for(&self, range: crate::source::TextRange) -> Option<crate::syntax::NodeId> { self.exact .get(&range) .copied() .or_else(|| Self::smallest_containing(&self.roots, range)) } fn smallest_containing( nodes: &[EdgeNode], range: crate::source::TextRange, ) -> Option<crate::syntax::NodeId> { let index = nodes.partition_point(|node| node.range.start() <= range.start()); let node = nodes.get(index.checked_sub(1)?)?; (node.range.end() >= range.end()) .then(|| Self::smallest_containing(&node.children, range).unwrap_or(node.id)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/bamts-compiler/src/pipeline.rs` around lines 144 - 173, Sort the children vector by each child’s range start before constructing and returning the EdgeNode in the surrounding node-building logic. Keep the existing debug_assert as validation, but make ordering unconditional so smallest_containing can safely use partition_point in release builds, including for concatenated Try arms and flattened Switch consequents.crates/bamts-compiler/src/program.rs (3)
1306-1308: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm the JSX AST carries nested expressions that the collector skips. set -euo pipefail fd -t f 'syntax.rs' crates/bamts-compiler/src | while IFS= read -r file; do echo "== $file" ast-grep outline "$file" --items all --match 'Jsx' --view expanded || true rg -n -C 4 'Jsx(Element|Fragment|SelfClosingElement|Attribute|Child|ExpressionContainer)' "$file" done # Show every place JSX nodes are traversed (or skipped) in the compiler. rg -n -C 3 'Expression::JsxElement' crates/bamts-compiler/srcRepository: metaphorics/bamTiScript
Length of output: 2067
🏁 Script executed:
set -euo pipefail base='https://raw.githubusercontent.com/metaphorics/bamTiScript' for ref in main master; do echo "== ref: $ref" for path in \ crates/bamts-compiler/src/program.rs \ crates/bamts-compiler/src/syntax.rs \ crates/bamts-compiler/src/rules/mod.rs do url="$base/$ref/$path" status=$(curl -L -sS -o /tmp/review-file -w '%{http_code}' "$url") echo "$status $path" if [ "$status" = 200 ]; then wc -l /tmp/review-file case "$path" in *program.rs) sed -n '980,1040p;1270,1325p' /tmp/review-file ;; *) rg -n -C 5 'Jsx(Element|Fragment|SelfClosingElement|Attribute|Child|ExpressionContainer)|enum Expression|struct .*Jsx' /tmp/review-file || true ;; esac fi done doneRepository: metaphorics/bamTiScript
Length of output: 2038
🌐 Web query:
metaphorics bamTiScript crates/bamts-compiler/src/program.rs DynamicEdgeCollector JSX💡 Result:
The metaphorics/bamTiScript project is a TypeScript/JavaScript compiler frontend and runtime framework [1][2]. Within the
bamts-compilercrate, theprogram.rsmodule manages whole-program loading and canonical module identity [3]. While the provided documentation forbamts-compilerand the relatedbamts-bytecodecrate does not explicitly mention aDynamicEdgeCollectoror specific JSX-handling logic within the accessibleprogram.rssource [4][5][3], the framework is architected to compile TypeScript/JavaScript to verified canonical bytecode [3]. The compilation pipeline, managed in thepipelinemodule, follows a fixed scan-parse-check-emit process [6]. For scripts and module graphs, the compiler resolves dependencies and performs semantic analysis [3][7]. Given the framework's focus on canonical module identity and linkage [5][3], any dynamic collection of edges or JSX transformation would typically be integrated within thechecker,lower(which handles AST-to-bytecode conversion), or thepipelineorchestration [3][6].Citations:
- 1: https://crates.io/crates/bamts-compiler
- 2: https://crates.io/crates/bamts
- 3: https://docs.rs/bamts-compiler/latest/bamts_compiler/
- 4: https://docs.rs/crate/bamts-compiler/latest/source/src/lib.rs
- 5: https://docs.rs/bamts-bytecode/latest/src/bamts_bytecode/program.rs.html
- 6: https://docs.rs/crate/bamts-compiler/latest/source/src/pipeline.rs
- 7: https://docs.rs/crate/bamts/latest/source/src/lib.rs
🌐 Web query:
site:docs.rs/crate/bamts-compiler/latest/source/src/program.rs "DynamicEdgeCollector"💡 Result:
No indexed results were found for:
site:docs.rs/crate/bamts-compiler/latest/source/src/program.rs "DynamicEdgeCollector"The symbol may not be present in the latest
bamts-compilersource, or the page may not be indexed by search engines.
🌐 Web query:
site:github.com/metaphorics/bamTiScript "Expression::JsxElement"💡 Result:
No indexed GitHub result was found for the exact query:
site:github.com/metaphorics/bamTiScript "Expression::JsxElement"The repository or symbol may be private, deleted, renamed, or not indexed by search. The results only surfaced unrelated JSX/TypeScript pages. (github.com)
Citations:
🌐 Web query:
site:github.com/metaphorics/bamTiScript "ModuleEdgeKind::TypeOnly"💡 Result:
No indexed GitHub results were found for:
site:github.com/metaphorics/bamTiScript "ModuleEdgeKind::TypeOnly"The repository or symbol may be private, unindexed, renamed, or absent.
🔍 Additional research query:
Use Tavily or another available source to retrieve the raw contents of https://docs.rs/crate/bamts-compiler/latest/source/src/program.rs and locate DynamicEdgeCollector::scan_expression, the token-window fallback around lines 1000-1030, and the handling of ModuleEdgeKind::TypeOnly. Also retrieve any public source for the syntax Expression JSX variants.💡 Result:
Retrieved findings
Tavily was unavailable due to its usage limit; Valyu successfully retrieved both files.
program.rsThe file defines:
pub enum ModuleEdgeKind { StaticRuntime, TypeOnly, DynamicRuntime, }
DynamicEdgeCollector::scan_expressionrecursively traverses templates, arrays, objects, functions, classes, calls, members, operators, assignments, and TypeScript wrappers.The dynamic-import arm is:
Expression::Import(value) => { if let Expression::Literal(Literal::String(literal)) = value.source.data() { self.push_literal_edge(ModuleEdgeKind::DynamicRuntime, literal); } self.scan_expression(&value.source); if let Some(options) = &value.options { self.scan_expression(options); } }The token-window fallback near the requested region scans three non-comment tokens:
for window in tokens.windows(3) { if window[0].kind() != TokenKind::KwImport || window[1].kind() != TokenKind::LParen || window[2].kind() != TokenKind::StringLiteral || edges.iter().any(|edge| edge.range == window[2].range()) { continue; } // ... edges.push(UnresolvedEdge { kind: ModuleEdgeKind::TypeOnly, specifier: Arc::from(specifier), range: window[2].range(), }); }Thus, this fallback classifies a token-shaped
import("...")occurrence asTypeOnly, while the ASTExpression::Importpath classifies literal dynamic imports asDynamicRuntime.
TypeOnlyedges are excluded from runtime closure and executable wire edges:.filter(|edge| edge.kind() != ModuleEdgeKind::TypeOnly)They remain available through compiler provenance and
type_only_edges().JSX expression variants
BamTS’s public
syntax.rsExpressionenum contains no JSX variants. It includesImport,Template,Array,Object, and other JavaScript/TypeScript forms, but notJSXElementorJSXFragment.For comparison, Oxc’s public AST defines:
Expression::JSXElement(Box<'a, JSXElement<'a>>), Expression::JSXFragment(Box<'a, JSXFragment<'a>>),Its parser constructs
JSXFragmentfor<>...</>andJSXElementfor<Tag>...</Tag>.
🔍 Additional research query:
Search public sources for the BamTS syntax crate and the definitions of Expression::JsxElement, Expression::JsxFragment, Expression::JsxSelfClosingElement, including whether their payloads contain expression containers or attribute values and whether DynamicEdgeCollector is expected to traverse them. Distinguish the current published source from the revision containing these variants.💡 Result:
Result
I could not locate a public crate or repository definitively named BamTS syntax. Searches for
BamTS,DynamicEdgeCollector, and the exactExpression::Jsx*spellings produced no matching public source; theBamTSresults were unrelated projects.Closest relevant public sources
Variant Public AST evidence Expression::JsxElementNot found with this exact casing. Oxc currently uses Expression::JSXElement.Expression::JsxFragmentNot found with this exact casing. Oxc currently uses Expression::JSXFragment.Expression::JsxSelfClosingElementNo public definition found in the searched Rust sources. Oxc represents self-closing JSX as JSXElementwith no closing element.Oxc’s current public parser emits only
Expression::JSXElementorExpression::JSXFragment; its JSX attribute parser can produce expression containers, nested elements, or fragments.Payload contents
- JSX elements: opening-element attributes, children, and—when non-self-closing—a closing element.
- JSX fragments: children only; fragments have no attributes.
- Self-closing elements: attributes but no children; in Oxc they are represented as a
JSXElementwhose closing element is absent.- Attribute values: may be string values, expression containers, nested JSX elements, or fragments.
- Children: may include text, nested elements/fragments, and expression containers.
Traversal expectation
There is no public
DynamicEdgeCollectorimplementation to verify. However, analogous public visitors explicitly traverseJSXElementandJSXFragment, including JSX attributes and children; therefore a collector intended to find dynamic references should traverse:
- element attributes and spread attributes;
- expression containers in attribute values;
- element and fragment children;
- nested JSX recursively.
Current source vs. variant-containing revision
The public current source I could verify uses the Oxc-style model:
Expression::JSXElement Expression::JSXFragmentIt does not expose a separate
Expression::JsxSelfClosingElement. Consequently, the three lower-camel-case variants appear to belong to an unpublished, alternate, or revision-specific AST definition—not the currently verifiable published source.
Traverse expressions inside JSX nodes.
DynamicEdgeCollector::scan_expressionreturns without visiting JSX attributes, children, or expression containers. Thus<Foo onClick={() => import("./dep")} />is classified by the token fallback asTypeOnly, notDynamicRuntime.TypeOnlyedges are excluded from runtime closure and executable wire edges. Traverse JSX contents before applying the fallback.🤖 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 1306 - 1308, Update DynamicEdgeCollector::scan_expression so JSX elements, fragments, and self-closing elements traverse their attributes, children, and expression containers before fallback classification. Ensure imports or other runtime expressions nested in JSX are collected as DynamicRuntime rather than TypeOnly.
1569-1578: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
parse_hexoverflows on long\u{...}escapes.
value * 16 + digitis unchecked. A specifier such asimport "./\u{FFFFFFFFFF}"feeds more than eight hex digits into this fold. Debug builds panic on the multiply. Release builds wrap silently and produce a wrong code point. The compiler must not panic on attacker-supplied source text. Use checked arithmetic and returnNoneon overflow.🐛 Proposed fix
fn parse_hex(bytes: &[u8]) -> Option<u32> { if bytes.is_empty() { return None; } bytes.iter().try_fold(0_u32, |value, byte| { char::from(*byte) .to_digit(16) - .map(|digit| value * 16 + digit) + .and_then(|digit| value.checked_mul(16)?.checked_add(digit)) }) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn parse_hex(bytes: &[u8]) -> Option<u32> { if bytes.is_empty() { return None; } bytes.iter().try_fold(0_u32, |value, byte| { char::from(*byte) .to_digit(16) .and_then(|digit| value.checked_mul(16)?.checked_add(digit)) }) }🤖 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 1569 - 1578, Update parse_hex to use checked arithmetic for each hexadecimal digit and return None when value * 16 + digit overflows u32, preserving the existing None result for empty or invalid input and preventing panics or wrapped code points on overly long escapes.
2523-2562: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
star_export_originscaches path-dependent results.The result depends on the
visitedset that the caller passed in. Line 2530 truncates traversal when a(module, name)pair is already on the current path. Line 2560 then caches that truncated result unconditionally, keyed only by(module_index, name).In a cyclic star graph the cached entry can be empty or partial purely because of the order in which the outer loop at line 2441 reached it. A later, independent query reads the poisoned entry and sees fewer origins than exist. Fewer origins changes the
candidates.len() == 1decision at line 2481, so an ambiguous export can be materialized as unambiguous, or a real export can be dropped from the linked program.The current cyclic test uses a two-module cycle where the truncation is symmetric, so it does not expose this. Track whether the traversal was truncated and skip the cache write when it was.
🐛 Sketch of the fix
- if !visited.insert((module_index, name.to_owned())) { - return BTreeSet::new(); - } + if !visited.insert((module_index, name.to_owned())) { + // Signal truncation to the caller so it does not cache a partial result. + return BTreeSet::new(); + }Thread a
truncated: &mut bool(or return(BTreeSet<_>, bool)) through the recursion. Set it when line 2530 short-circuits, propagate it through thecandidates.extend(...)loop at line 2551, and guard thecache.insertat line 2560 on!truncated.🤖 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 2523 - 2562, Update star_export_origins to track whether traversal was truncated by the visited-path guard, propagating that state through recursive star traversal. Only insert results into cache when the current traversal completed without truncation; preserve the existing candidate collection and cache lookup behavior otherwise.crates/bamts-compiler/src/rules/mod.rs (2)
1223-1231: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constructor parameters skip the shared signature checks.
ClassMember::Constructorcallsflag_parameter_countdirectly and then only walks parameter initializers. It never callsvisit_callable_signature.ClassMember::Methodreaches that function throughvisit_function, so it does get the full set.The result is inconsistent coverage.
class C { m(xs: number[]) {} }fires W059.class C { constructor(xs: number[]) {} }does not. The same gap applies to W085 for TypeScript parameter types in JavaScript, and to parameter decorators, which constructors use more than any other member.You wrote
visit_callable_signatureto remove exactly this kind of duplication, per its own doc comment at line 1561. Use it here.♻️ Proposed change
ClassMember::Constructor(constructor) => { - flag_parameter_count(member.range(), constructor.parameters.len(), findings); - for parameter in &constructor.parameters { - if let Some(initializer) = ¶meter.data().initializer { - visit_expression(initializer, script_kind, findings); - } - } + visit_callable_signature( + member.range(), + &constructor.parameters, + None, + None, + script_kind, + findings, + ); visit_statement_list(&constructor.body.data().statements, script_kind, findings); }Confirm the constructor node carries no
return_typeortype_parameters; if it does, pass them instead ofNone.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ClassMember::Constructor(constructor) => { visit_callable_signature( member.range(), &constructor.parameters, None, None, script_kind, findings, ); visit_statement_list(&constructor.body.data().statements, script_kind, findings); }🤖 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 1223 - 1231, Update the ClassMember::Constructor branch to call visit_callable_signature for the constructor parameters, passing the constructor’s available signature fields and None for return_type/type_parameters only if the node lacks them; retain flag_parameter_count only if it is not already performed by that helper, and continue visiting parameter initializers and the constructor body.
1537-1558: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
W065 false-positives on functions that return from every switch case.
block_contains_value_returndescends intoStatement::Switchcases at line 1632, so a switch full ofreturnstatements sets the first condition totrue.can_complete_normallyhas noStatement::Switcharm and falls through to_ => trueat line 1681, so the second condition is alsotrue.The result: this ordinary and exhaustive function is flagged as having "a reachable path without a returned value".
function f(x: 1 | 2) { switch (x) { case 1: return "a"; default: return "b"; } }The two helpers must agree on which statements they understand. Either add a
Statement::Switcharm tocan_complete_normallythat returnsfalsewhen adefaultclause exists and no clause completes normally, or stop descending into switch cases inblock_contains_value_return. The first option is correct; the second only suppresses the noise.🐛 Proposed fix in `can_complete_normally`
Statement::Try(statement) => {Add before the
_ => truearm:Statement::Switch(statement) => { // A switch completes normally unless it has a default clause and // no clause (and no break) can fall out of it. let has_default = statement .cases .iter() .any(|case| case.data().test.is_none()); if !has_default { return true; } statement.cases.iter().any(|case| { case.data() .consequent .last() .is_none_or(can_complete_normally) }) }Note this still treats
breakinside a case as non-completing via theStatement::Breakarm, which over-reports completion for the enclosing switch. Verify against the W065 and W067 rule 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/rules/mod.rs` around lines 1537 - 1558, Update can_complete_normally to handle Statement::Switch before its fallback arm, treating switches without a default clause as normally completing and switches with a default as completing only when a case’s consequent can complete normally. Ensure break statements are handled consistently with the existing logic, then verify the W065 and W067 rule tests.crates/bamts-compiler/src/rules/semantic/mod.rs (2)
408-469: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The call-collection walk is missing half the statement kinds, so W072 and W010 fire falsely.
collect_calls_statementhandlesVariable,Expression,Block,Function,Class,If,Return, exported declarations,Declare, andNamespace. It ignoresFor,ForIn,ForOf,While,DoWhile,Switch,Try,Labeled,Throw,With, andExport(Default).visit_statementwalks all of them.The consequence is not cosmetic.
sorted_object_keysandcalled_namesstay empty for anything inside a loop or atryblock, whilevisit_callstill runs there. So this code reports BAMTS-W072 even though the keys are sorted:for (const _ of xs) { Object.keys({ b: 1, "2": 2 }).sort(); }The documented near miss at Line 1757 only passes because the test puts the call at the top level. Two independent traversals over the same tree will keep drifting apart. Either extend
collect_calls_statementto the full statement set, or fold the call collection intovisit_statementas a pre-pass so there is exactly one walker to keep correct.🤖 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 408 - 469, Extend collect_calls_statement to traverse every statement kind handled by visit_statement, including For, ForIn, ForOf, While, DoWhile, Switch, Try, Labeled, Throw, With, and default exports. Recurse through each statement’s expressions, bodies, cases, handlers, and nested declarations so calls inside control-flow constructs contribute to sorted_object_keys and called_names. Prefer reusing existing collection helpers and preserve current behavior for already-supported statements.
1492-1514: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two spellings of the same shadow check, one of them a double negative, neither tested.
Line 1494 reads
!model.reference(...).is_some_and(|symbol| kind != IntrinsicValue). Line 1509 readsmodel.reference(...).is_some_and(|symbol| kind != IntrinsicValue || !matches!(name, "module" | "exports"))and thencontinues. Both mean "this identifier is not a user-declared shadow of the CommonJS globals", written with opposite polarity and different structure. The second also duplicates the name discrimination that thematchat Line 1515 already performs.These two branches decide whether a file is classified as CommonJS, which in turn drives BAMTS-W037 and BAMTS-W086 across module boundaries. No test in this file declares a local
const module = ...orlet exports = ..., so the exact branch these checks exist for is unverified.Extract one predicate and add the shadowing near miss to the test suite.
♻️ Proposed helper
/// Returns whether `identifier` denotes the host-provided CommonJS global /// rather than a user declaration that shadows it. fn is_commonjs_global(model: &SemanticModel, identifier: &IdentifierNode) -> bool { model .reference(identifier.id()) .is_none_or(|symbol| model.symbol(symbol).kind() == SymbolKind::IntrinsicValue) }🤖 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 1492 - 1514, Extract a shared is_commonjs_global predicate for IdentifierNode references, treating unresolved identifiers and IntrinsicValue symbols as CommonJS globals while rejecting user declarations. Replace both existing shadow checks before the exports handling and object-name matching with this helper, remove the duplicated name condition, and add tests covering local const/let declarations that shadow module or exports and verify the resulting CommonJS classification.crates/bamts-compiler/tests/rules.rs (1)
271-290: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This
filter_mapswallows broken program examples instead of failing.
source.resolves_to()declares that this example source must link to another module. Two lines later, the?onspecifiersilently drops the edge when noImportorExportstatement is found. The program then gets checked with no module edge, the rule under test never exercises cross-module resolution, and the assertion at line 211 may still pass for the wrong reason. Test infrastructure that hides its own misconfiguration is worse than no test.Second problem:
find_maptakes the first import or export statement. Any example with more than one module statement wires the edge to whichever one happens to come first and ignores the rest.Make the missing-specifier case loud:
💚 Fail instead of dropping the edge
- let specifier = file.statements().iter().find_map(|statement| { - matches!( - statement.data(), - Statement::Import(_) | Statement::Export(_) - ) - .then_some(statement.id()) - })?; + let specifier = file + .statements() + .iter() + .find_map(|statement| { + matches!( + statement.data(), + Statement::Import(_) | Statement::Export(_) + ) + .then_some(statement.id()) + }) + .expect("a source declaring resolves_to must contain an import or export");🤖 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/tests/rules.rs` around lines 271 - 290, Update the edge construction around the sources filter_map so a source that resolves to another module must have exactly one Import or Export statement. Replace the optional specifier lookup with an assertion or explicit failure for a missing specifier, and reject multiple matching statements instead of selecting the first; keep the existing ResolvedModuleEdge construction for the valid single-statement case.crates/bamts-runtime/src/builtins/object.rs (2)
292-356: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
ValidateAndApplyPropertyDescriptoris missing entirely: non-configurable properties can be redefined, andObject.freezeis advisory.
into_propertymerges the requested fields over the current descriptor and hands the result todefine_descriptor. Neither function ever consultscurrent.configurable. ECMA-262 §10.1.6.3 spends most of its length on exactly that check, and none of it is here.The consequences are not subtle:
const o = {}; Object.defineProperty(o, "x", { value: 1, configurable: false, writable: false }); Object.defineProperty(o, "x", { value: 2 }); // spec: TypeError. here: succeeds Object.defineProperty(o, "x", { configurable: true }); // spec: TypeError. here: succeeds Object.defineProperty(o, "x", { get() { return 3; } });// spec: TypeError. here: succeeds Object.freeze(o); Object.defineProperty(o, "x", { value: 4 }); // spec: TypeError. here: succeeds
mark_frozenincrates/bamts-runtime/src/builtins/mod.rsclearswritableandconfigurableon every property, anddefine_descriptorin that same file validates extensibility, typed-array indices, and arraylength— and nothing else. SoObject.freezestops ordinary assignment and does not stopdefineProperty. Every piece of JavaScript that freezes a prototype or a config object to establish an invariant now has no invariant.Note the file already reaches for
machine.own_descriptor(target, &key)?at line 470 to fetchcurrent. The value is right there. Validate it before merging.Implement the standard check: reject when
currentexists,current.configurableis false, and the request changes the descriptor kind, setsconfigurableto true, flipsenumerable, setswritablefrom false to true, or changesvalueon a non-writable data property (using SameValue).🛡️ Sketch of the missing validation
fn apply_property_descriptor<H: Host>( machine: &mut Machine<'_, H>, target: Value, key: PropertyKey, descriptor: PropertyDescriptor, ) -> Result<(), EvalFailure> { if define_array_length_descriptor(machine, target, &key, descriptor)? { return Ok(()); } let current = machine.own_descriptor(target, &key)?; + validate_property_descriptor(current.as_ref(), descriptor)?; machine.define_descriptor(target, key, descriptor.into_property(current)) } + +/// ECMA-262 §10.1.6.3 ValidateAndApplyPropertyDescriptor, validation half. +fn validate_property_descriptor( + current: Option<&Property>, + descriptor: PropertyDescriptor, +) -> Result<(), EvalFailure> { + let Some(current) = current else { + return Ok(()); + }; + if current.configurable() { + return Ok(()); + } + if descriptor.configurable == Some(true) { + return Err(type_error("Cannot redefine property")); + } + if descriptor + .enumerable + .is_some_and(|enumerable| enumerable != current.enumerable()) + { + return Err(type_error("Cannot redefine property")); + } + match current { + Property::Data { writable, value, .. } => { + if descriptor.is_accessor() { + return Err(type_error("Cannot redefine property")); + } + if !*writable { + if descriptor.writable == Some(true) { + return Err(type_error("Cannot redefine property")); + } + if descriptor.value.is_some_and(|next| next != *value) { + return Err(type_error("Cannot redefine property")); + } + } + } + Property::Accessor { getter, setter, .. } => { + if descriptor.is_data() { + return Err(type_error("Cannot redefine property")); + } + if descriptor.getter.is_some_and(|next| Some(next) != *getter) + || descriptor.setter.is_some_and(|next| Some(next) != *setter) + { + return Err(type_error("Cannot redefine property")); + } + } + } + Ok(()) +}The
SameValuecomparison for numbers needs+0/-0andNaNhandling; do not use rawValueequality for the value check without confirming its semantics.Also applies to: 461-472
🤖 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-runtime/src/builtins/object.rs` around lines 292 - 356, Add ValidateAndApplyPropertyDescriptor checks in define_descriptor after fetching current with machine.own_descriptor and before into_property: reject non-configurable descriptors when the request changes data/accessor kind, sets configurable true, flips enumerable, makes a non-writable data property writable, or changes its value using SameValue semantics, including +0/-0 and NaN handling. Propagate the TypeError and preserve existing extensibility, typed-array, and array-length validation.
904-915: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A bound function always gets
%Function.prototype%, ignoring the target's prototype.ECMA-262 §20.2.3.2 step 2:
Let proto be ? Target.[[GetPrototypeOf]](), and step 3 creates the bound function with that prototype. This allocates a bareHeapEntry::NativeFunctionwith no prototype field, so it inherits the default.class C {} class D extends C {} Object.getPrototypeOf(D.bind(null)) // spec: C. here: Function.prototypeThe test at line 2246 asserts
Some(machine.intrinsics.function_prototype)and therefore locks the wrong behavior in. Subclass-aware code that walks the prototype chain of a bound constructor gets the wrong answer, andD.bind(null).call === C.callstyle checks break.Read the target's prototype and pass it through.
🤖 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-runtime/src/builtins/object.rs` around lines 904 - 915, The bound-function creation path around NativeCallable::Bound must read the target’s [[GetPrototypeOf]] result and assign that prototype to the newly allocated function, rather than relying on the default %Function.prototype%. Update the related test assertion to expect the target prototype, preserving the existing behavior for ordinary functions.
|
@coderabbitai ultrareview |
|
✅ Action performedFull review finished. |
|
@coderabbitai ultrareview |
|
|
There was a problem hiding this comment.
Actionable comments posted: 65
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 (9)
crates/bamts-cli/src/args.rs (1)
734-745: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
explainis order-dependent, and you already declared order-independence three lines below.Line 736 tests
mode, the mode parsed so far, noteffective_mode. Sobamts BAMTS-W017 explainstoresBAMTS-W017inentrypoint, reaches line 760 withexplain_rule == None, and reports "missing rule name for explain subcommand". The user supplied the rule name. The CLI says they did not.The comment you wrote at line 765 says the entrypoint constraint is checked "after the effective mode is resolved so the constraint is order-independent". Apply the same rule to
explaininstead of contradicting it two statements later.🐛 Proposed fix: resolve the rule after the effective mode
if effective_mode == Mode::Explain && explain_rule.is_none() && !help && !version { + if let Some(first) = entrypoint.take() { + if let Some(second) = extra_inputs.first() { + return Err(ArgsError::UnexpectedArgument { arg: second.clone() }); + } + explain_rule = Some(first); + } else { + return Err(ArgsError::MissingExplainRule); + } + } + if effective_mode == Mode::Explain && explain_rule.is_none() && !help && !version { return Err(ArgsError::MissingExplainRule); }
entrypointandexplain_rulemust bemutfor this.Also applies to: 760-762
🤖 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/src/args.rs` around lines 734 - 745, Update the argument handling around the entrypoint and explain validation to resolve the effective mode before assigning positional arguments. Make entrypoint and explain_rule mutable, then transfer the captured entrypoint into explain_rule when the effective mode is Mode::Explain, preserving duplicate-argument and missing-rule validation so explain accepts the rule regardless of argument order.crates/bamts-compiler/src/lint.rs (1)
1886-1915: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
BAMTS-W088can never reach its declaredDeny, and one guard here is dead.Two problems in the same block.
First, line 1906.
rule.code() != "BAMTS-W085"is unreachable-condition padding. Line 1886 already returned for W085 before this expression is ever evaluated. Delete it.Second, and this one matters:
no-with(W088) is registered asJavaScriptCompatibilitywith default levelDeny. Thejavascript_compatibilitybranch clamps every rule in that group down toWarnunder a JavaScript source. Awithstatement only appears in JavaScript input. So theDenyyou declared at line 1417 is dead in the only dialect that can trigger it. You exempted W085 by name to keep itDeny; W088 needs the same treatment, or its declared default is a lie.Confirm which behaviour you want. If W088 must stay
Deny, exempt it alongside W085 and add a test assertion next to the W085 one at line 2314.🐛 Proposed fix
- if rule.code() == "BAMTS-W085" { + if matches!(rule.code(), "BAMTS-W085" | "BAMTS-W088") { 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"; + let javascript_compatibility = + RULES[rule_index(rule)].group() == RuleGroup::JavaScriptCompatibility;🤖 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 1886 - 1915, Update the lint-level logic around the javascript_compatibility branch: remove the redundant BAMTS-W085 comparison, and exempt BAMTS-W088 alongside BAMTS-W085 so its JavaScriptCompatibility rule retains its declared Deny level. Add a corresponding assertion beside the existing W085 test to verify W088 remains Deny.crates/bamts-runtime/src/builtins/string.rs (1)
832-854: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
normalizevalidates the form, then returns the input untouched.Lines 838-848 carefully reject anything that is not
NFC,NFD,NFKC, orNFKD. Lines 850-853 then ignore the form entirely and hand back the original string."e\u0301".normalize("NFC").length // spec: 1. here: 2 "e\u0301".normalize("NFC") === "é" // spec: true. here: falseA function that throws on invalid input and produces wrong output on valid input is worse than one that throws on everything. The argument validation actively signals "this works", and callers will believe it. Equality checks and dictionary lookups downstream will be silently wrong.
Either implement the four normalization forms, or throw for every form and document
normalizeas unimplemented. Do not do this.🤖 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-runtime/src/builtins/string.rs` around lines 832 - 854, Update normalize to perform the requested Unicode normalization for each accepted form—NFC, NFD, NFKC, and NFKD—before allocating and returning the result, using the existing string/text and runtime facilities visible around normalize. Preserve the current RangeError validation for unsupported forms and the default behavior when the form is undefined.crates/bamts-runtime/src/builtins/regexp.rs (1)
32-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe constructor ignores
_constructingand drops the pattern-identity rule.The parameter is named
_constructingand never read. ECMA-262 §22.2.4.1 step 1: whenRegExpis called as a function,patternis already a RegExp, andflagsisundefined, the abstract operation returnspatternitself.const re = /x/; RegExp(re) === re // spec: true. here: false, a fresh objectLine 88 also hardcodes
machine.intrinsics.regexp_prototype(), soclass R extends RegExp {}produces an instance with the base prototype. That is the sameNewTargetgap flagged incrates/bamts-runtime/src/builtins/promise.rs.🤖 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-runtime/src/builtins/regexp.rs` around lines 32 - 47, Update constructor to honor _constructing and implement the RegExp pattern-identity rule: when called without construction, given an existing RegExp pattern, and flags are undefined, return that same value. Also propagate the constructor’s NewTarget when selecting the resulting object’s prototype instead of always using machine.intrinsics.regexp_prototype(), preserving derived RegExp subclass prototypes.crates/bamts-runtime/src/builtins/symbol.rs (2)
125-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Symbol().descriptionreturns""where it must returnundefined.Line 131 collapses the absent-description case with
unwrap_or_default(), soHeapEntry::Symbolstores an emptyEcmaString. The accessor at lines 198-202 then allocates that empty string and hands it back.Symbol().description // spec: undefined. here: "" Symbol().description === undefined // spec: true. here: false Symbol("").description === "" // both: true — now indistinguishable from the aboveECMA-262 §20.4.3.2 returns the
[[Description]]slot, and that slot holdsundefinedwhen no argument is passed. You have flattened two distinct states into one, so no caller can tell them apart any more.
toStringat line 205 happens to produce the right text for both cases, which is why the tests did not catch it.The field must become
Option<EcmaString>. Line 131 keeps theOptionfromtranspose()instead of defaulting it, and the accessor returnsValue::UNDEFINEDforNone.Run this to size the
HeapEntry::Symbolchange:#!/bin/bash # Description: Find the HeapEntry::Symbol description field and every construction/read site. set -eu echo '--- HeapEntry::Symbol declaration ---' rg -nP --type=rust -B2 -A4 'Symbol\s*\{\s*$' crates/bamts-runtime/src/lib.rs echo '--- all construction and destructuring sites ---' rg -nP --type=rust -C3 'HeapEntry::Symbol' crates/bamts-runtime/srcAlso applies to: 198-202
🤖 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-runtime/src/builtins/symbol.rs` around lines 125 - 135, Change HeapEntry::Symbol’s description field to Option<EcmaString> and update every construction, read, and destructuring site accordingly. In the Symbol builtin, preserve the Option returned by transpose() instead of calling unwrap_or_default(); update the description accessor to return Value::UNDEFINED for None and allocate the string only for Some, while keeping Symbol("") behavior unchanged.
138-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Symbol.keyForis missing, andvalueOfrejects a boxed receiver.Line 37 installs
forand nothing else. ECMA-262 §20.4.2.6 pairs it withSymbol.keyFor, which reverses the same registry you already keep inmachine.intrinsics.symbol_registry. The lookup is a scan of a map you already own.Separately,
value_ofat line 231 callssymbol_description(machine, this), which at lines 183-188 demands thatthisdecode straight toHeapEntry::Symbol. A boxed Symbol wrapper does not:Object(Symbol("x")).valueOf() // spec: the symbol. here: TypeError Object(Symbol("x")).toString() // spec: "Symbol(x)". here: TypeError
ThisSymbolValuein §20.4.3 accepts both a Symbol and an Object with a[[SymbolData]]slot.textincrates/bamts-runtime/src/builtins/string.rsat line 103 already callsunbox_primitive_or_selffor exactly this reason. Do the same here.Also applies to: 225-233
🤖 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-runtime/src/builtins/symbol.rs` around lines 138 - 177, Implement Symbol.keyFor alongside the existing Symbol.for registration, scanning machine.intrinsics.symbol_registry in reverse to return the registered key or undefined. Update symbol_description and the valueOf/toString paths to use unbox_primitive_or_self, accepting both direct Symbols and boxed Symbol objects while preserving TypeError for non-Symbol receivers.crates/bamts-runtime/src/builtins/number.rs (2)
320-326: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
(255).toString(undefined)throwsRangeError. It must return"255".
args.first().copied().unwrap_or(Value::int32(10))only supplies the default when the argument list is empty. An explicitundefinedisSome(Value::UNDEFINED), soto_numberyieldsNaN,NaN as u32saturates to0, and the(2..=36)guard throws.§21.1.3.6 step 2 handles this before any range check: if
radixisundefined,radixMVis 10. This matters becauseundefinedis exactly what an omitted forwarded argument looks like —arr.map(n => n.toString(base))with an undefinedbase, or any wrapper that passes options through.🐛 Proposed fix
- let radix = - value_number(machine.to_number(args.first().copied().unwrap_or(Value::int32(10)))?) as u32; + let argument = args.first().copied().unwrap_or(Value::UNDEFINED); + let radix = if argument == Value::UNDEFINED { + 10 + } else { + value_number(machine.to_number(argument)?) as 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-runtime/src/builtins/number.rs` around lines 320 - 326, Update the radix initialization in the number toString implementation to treat an explicit Value::UNDEFINED the same as an omitted argument, defaulting radix to 10 before numeric conversion and range validation. Preserve the existing conversion and 2–36 validation for all other radix values so Number::toString(undefined) returns the decimal representation.
337-351: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
radix_stringtruncates the fractional part, so(0.5).toString(2)returns"0".Line 340 does
n.abs().trunc() as u128and the function never looks at the remainder. §21.1.3.6.1 requires the fractional digits after a.separator. Node returns"0.1".The
as u128cast is the second problem: any magnitude aboveu128::MAXsaturates silently, so very large finite numbers print a fixed garbage string instead of their radix expansion.Emit the fractional digits by repeated multiplication by the radix, and bound the digit count so an inexact binary fraction terminates.
🤖 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-runtime/src/builtins/number.rs` around lines 337 - 351, Update radix_string to preserve and emit the fractional portion after the integer digits, using repeated multiplication by radix and a bounded digit count so values such as 0.5 produce the correct radix representation without infinite loops. Replace the u128-based integer conversion with magnitude handling that does not silently saturate values above u128::MAX, while preserving sign and radix digit formatting.crates/bamts-runtime/src/regexp.rs (1)
316-362: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftThe matcher materializes every backtracking state into
Vecs. This is an unbounded memory-exhaustion path from any script regex.Look at what
match_sequencedoes for aRepeat. Line 327 seedslevels. Each iteration takes every state inlevels[count]and pushes every state thatmatch_nodeproduced for it. Then lines 349-354 runmatch_sequence(rest, ...)for every candidate at every count and collect all results intoresult.Nothing is lazy.
match_nodereturnsVec<State>, andStateownscaptures: Vec<Option<Range<usize>>>. So the classic pathological pattern does not merely spin the CPU — it allocates:/(a+)+$/.test("a".repeat(30) + "X"); /(x+x+)+y/.test("x".repeat(30));A conventional backtracking engine explores those states one at a time on a stack and burns CPU. This one holds every partial state, with its capture vector, in memory simultaneously.
Two things make it worse than ordinary ReDoS:
- These
Vecs are Rust-side allocations. They are never passed throughcharge_slot, soLimits::max_heap_bytesdoes not see them. The runtime accounts for individual property insertions to the byte and then lets a regex allocate without limit. The whole ledger discipline inbuiltins/mod.rsis bypassed here.limitat line 328 ismax.unwrap_or(input.len() + min + 1), so an unbounded quantifier scales the level count with the input length.The lookbehind path at lines 256-275 compounds it: it restarts the body matcher at every code point from 0 to the current position, and each restart can produce its own state explosion.
This needs a step budget threaded through
match_node, decremented per state produced, returning a catchable error when exhausted — or a rewrite to a lazy backtracker that keeps one state live. A budget is the smaller change and closes the hole. Either way the allocation must be bounded before this ships, becauseRegExpsources reach this from ordinary guest code.🤖 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-runtime/src/regexp.rs` around lines 316 - 362, Add a regex matching step budget and thread it through match_sequence, match_node, and the lookbehind matching path, decrementing it whenever a state is produced or explored. Propagate a catchable exhaustion error through the matcher instead of continuing once the budget is depleted, while preserving existing greedy, lazy, and capture behavior; ensure all regex entry points initialize the budget from the runtime limits.
♻️ Duplicate comments (11)
crates/bamts-cli/src/driver.rs (2)
1349-1353: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test still asserts Linux-only behaviour with no
cfggate.
peak_rss_kbreads/proc/self/status. On macOS and Windows it returnsNoneand.expect("VmHWM is readable on this Linux host")panics. The test name says "on_linux". The attribute does not. Every non-Linux developer and CI runner gets a red test for no reason.💚 Proposed fix
+ #[cfg(target_os = "linux")] #[test] fn peak_rss_kb_reads_proc_status_on_linux() { let rss = peak_rss_kb().expect("VmHWM is readable on this Linux host"); assert!(rss > 0, "peak RSS should be positive"); }Gate the
peak_rss_kbimport at line 1106 the same way, or the other platforms trade a failing test for an unused-import warning.🤖 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/src/driver.rs` around lines 1349 - 1353, Add a Linux-only cfg gate to the peak_rss_kb_reads_proc_status_on_linux test and apply the same gate to its peak_rss_kb import, preventing non-Linux builds from compiling the platform-specific test and unused import.
972-1032: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftThis project-resolution block is still a near-copy of
crates/bamts/src/lib.rs::compile_source_file.Both perform the same sequence: canonicalize the entrypoint, pick a fallback root,
discover_project, canonicalizeproject.root(), buildProjectRoot, read the config or default to"{}",ProjectConfig::parse,ProgramLoader::new(...).load(...). The differences are the error types and the fallback-root rule. That second difference is the whole problem: the facade and the CLI can resolve different roots for the same file and then report different diagnostics for the same program.Move the resolution into one shared function that returns the
ResolvedProgram, and let each caller map the error type.🤖 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/src/driver.rs` around lines 972 - 1032, Extract the shared project-resolution sequence from load_program_frontend and compile_source_file into one function returning ResolvedProgram, including entrypoint canonicalization, fallback-root selection, project discovery, root/config parsing, and ProgramLoader loading. Make both callers use this function and map its errors into their respective error types; preserve one consistent fallback-root rule so the CLI and facade resolve identical roots and diagnostics.crates/bamts-cli/build.rs (1)
159-185: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCargo fingerprint metadata is still a hard failure. This was already flagged and is still not fixed.
node_rlib_from_fingerprintreturnsErrwhen the file is unreadable, when the JSON does not parse, when a dependency tuple is not four elements, or when nobamts_nodedependency is present.select_node_rlibline 135 turns every one of those into a panic. The.fingerprintJSON is an undocumented Cargo implementation detail. Cargo is free to add a field, change the tuple arity, or rename the file, and this build script then refuses to build at all — even though the compatible-candidate scan below it would have produced the correct answer.The same over-strictness appears twice more in
exact_node_rlib: line 306 hard-errors on a directory namedbamts-node-, and lines 324-331 require the stamp to be exactly 16 lowercase hex characters. A trailing newline from a future Cargo version makes every stamp fail the shape check, which then reports "no stamp matches" and panics.Treat unavailable or unrecognized metadata as absent and fall through to the scan. Keep the hard error only for the one case where the metadata is valid and names an artifact that is missing.
🛡️ Proposed fix: degrade to the fallback scan instead of panicking
match node_rlib_from_fingerprint(build_fingerprint, dependencies) { Ok(Some(candidate)) => return Some(candidate), - Ok(None) => {} - Err(error) => { - panic!("could not resolve bamts-node from Cargo fingerprint metadata: {error}") - } + Ok(None) => {} + Err(FingerprintError::Unavailable(reason)) => { + cargo_line(&format!( + "warning=ignoring unusable Cargo fingerprint metadata: {reason}" + )); + } + Err(FingerprintError::MissingArtifact(error)) => { + panic!("could not resolve bamts-node from Cargo fingerprint metadata: {error}") + } }Split the error type so read, parse, shape, and stamp-scan failures map to
Unavailable, and only theexact_node_rlib"selected missing rlib" case maps toMissingArtifact.🤖 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/build.rs` around lines 159 - 185, Make node_rlib_from_fingerprint treat unreadable, malformed, shape-incompatible, or missing bamts_node fingerprint metadata as unavailable and let select_node_rlib continue to the compatible-candidate scan. Update exact_node_rlib to ignore unrecognized bamts-node- directories and normalize valid stamp contents before validation, while preserving a hard error only when valid metadata selects a missing rlib artifact. Split or map errors accordingly so only that missing-artifact case propagates as fatal.crates/bamts-runtime/src/builtins/promise.rs (2)
386-392: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Promise.prototype.thenstill runsspecies_constructorbefore the brand check.Line 391 still reaches
Get(this, "constructor")andGet(constructor, @@species)on an arbitrary receiver. TheIsPromisecheck required by ECMA-262 §27.2.5.4 step 2 is still absent.finallyat line 521 guards its receiver;thendoes not.🤖 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-runtime/src/builtins/promise.rs` around lines 386 - 392, Add the required IsPromise brand check in the Promise.prototype.then implementation before calling species_constructor, returning the existing TypeError failure for arbitrary receivers. Ensure species_constructor and new_promise_capability are reached only after this validation, matching the receiver guard already used by finally.
327-336: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe Promise constructor still ignores
NewTarget.Line 327 still calls
machine.create_promise()unconditionally. Subclass instances still get the base prototype. Nothing changed since the last round.🤖 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-runtime/src/builtins/promise.rs` around lines 327 - 336, Update the Promise constructor flow around create_promise to use the constructor’s NewTarget when creating the promise, ensuring subclass instances receive the appropriate prototype; preserve the existing resolver creation, executor invocation, rejection handling, and returned promise behavior.crates/bamts-runtime/src/gc.rs (1)
317-322: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
runtime_indexstill underflows on slot 0.Line 321 is unchanged. Debug builds panic on the subtraction. Release builds wrap to
usize::MAXand survive only becausemark_indexhappens to bail on the length check andis_marked_valuehappens to bail onmarks.get.RuntimeErrorKind::InvalidRuntimeHeapReferenceat line 608 shows the runtime already treats slot 0 as a reachable invalid state. Usechecked_sub(1).🤖 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-runtime/src/gc.rs` around lines 317 - 322, Update runtime_index to use checked_sub(1) when converting id.slot() to the runtime index, so slot 0 returns None instead of underflowing while valid runtime-heap slots retain their existing behavior.crates/bamts-runtime/src/builtins/uint8array.rs (1)
18-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Symbol.iteratoris still absent fromUint8Array.prototype.Lines 21-31 install
constructor,join, andSymbol.toStringTag. The iterator is still not there, sofor...of, spread, and destructuring still fail on everyUint8Arrayinstance.string.rsline 62 andarray.rsboth install one; this prototype is the odd one out.🤖 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-runtime/src/builtins/uint8array.rs` around lines 18 - 33, Update the Uint8Array prototype initialization around the existing join and Symbol.toStringTag setup to install the standard Symbol.iterator method, reusing the established iterator implementation and installation pattern from string.rs or array.rs. Ensure Uint8Array instances support for-of iteration, spread, and destructuring while preserving the existing constructor, join, and toStringTag properties.crates/bamts-runtime/src/builtins/regexp.rs (1)
58-87: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
sourceandflagsare still own non-configurable data properties.Lines 59-87 still install them on the instance. ECMA-262 §22.2.6 defines both as accessors on
RegExp.prototype; onlylastIndexis an own data property. The tests you added at lines 484-529 document the consequence in their own comment: the literalCreateRegExppath owns nosourceproperty, so.sourceneeds a read-time fallback that the constructor path does not need. You wrote a test to pin the workaround instead of removing the cause.Two construction routes for the same regex still produce two different own-property sets.
🤖 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-runtime/src/builtins/regexp.rs` around lines 58 - 87, Update the RegExp construction logic around the properties map so instances only define the writable non-configurable lastIndex own data property; remove the own source and flags data properties and rely on the RegExp.prototype accessors for those values. Ensure both literal/CreateRegExp and constructor-created regexes expose source and flags consistently without read-time fallback workarounds.crates/bamts-runtime/src/builtins/string.rs (1)
992-1018: 🚀 Performance & Scalability | 🟠 Major | ⚖️ Poor tradeoffThe string iterator still materializes the whole string before returning.
Lines 998-1012 still build a
Vec<EcmaString>of every code point, then aValueper piece, then an array. No preflight was added.preflight_string_allocationsits at line 745 in this same file and is wired intopadandrepeat; this path still charges the machine slot by slot on its way to failing.🤖 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-runtime/src/builtins/string.rs` around lines 992 - 1018, Update string_iterator to preflight the full allocation before constructing any code-point pieces or values, reusing preflight_string_allocation with the appropriate total allocation size. Preserve the existing code-point iteration and iterator result, but ensure allocation failure occurs before partial slot-by-slot allocation rather than during the Vec/array construction.crates/bamts-runtime/src/builtins/symbol.rs (1)
20-20: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Symbol.hasInstanceis still exposed and still ignored.Line 20 creates the symbol and line 42 publishes it on the constructor. Lines 26-32 register every other well-known symbol in
BuiltinTable;has_instanceis the one omission, because nothing consults it.instanceofstill runs the plain prototype-chain check. A user handler is silently discarded.Either wire the dispatch or stop advertising the property.
Also applies to: 42-42
🤖 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-runtime/src/builtins/symbol.rs` at line 20, Resolve the unimplemented Symbol.hasInstance contract in the symbol initialization and constructor publication flow: either register and dispatch has_instance through the same BuiltinTable mechanism as the other well-known symbols so instanceof consults user handlers, or remove its creation and publication from the relevant symbol setup. Do not leave Symbol.hasInstance exposed while instanceof ignores it.crates/bamts-runtime/src/external_modules.rs (1)
829-840: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
decode_hexstill panics on non-ASCII input. Untouched since the last review.Line 830 still measures
text.len()in bytes, line 834 still splits raw bytes withchunks_exact(2), and line 836 still calls.expect("hex input is a string")on a pair that was never established to be a character boundary.
hash.update("aéb", "hex")still aborts the process. Bytes[0x61, 0xC3, 0xA9, 0x62], length 4, guard passes, first chunk[0x61, 0xC3]is not valid UTF-8.The path got wider, not narrower. Line 725 now runs the input through
EcmaString::to_utf8_lossy, so a lone surrogate becomes U+FFFD — three non-ASCII bytes — and lands here too.🐛 Proposed fix
fn decode_hex(text: &str) -> Result<Vec<u8>, EvalFailure> { - if !text.len().is_multiple_of(2) { + if !text.is_ascii() || !text.len().is_multiple_of(2) { return Err(type_error("invalid hexadecimal hash input")); } text.as_bytes() .chunks_exact(2) .map(|pair| { - let digits = std::str::from_utf8(pair).expect("hex input is a string"); + let digits = std::str::from_utf8(pair) + .map_err(|_| type_error("invalid hexadecimal hash input"))?; u8::from_str_radix(digits, 16).map_err(|_| type_error("invalid hexadecimal hash input")) }) .collect() }🤖 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-runtime/src/external_modules.rs` around lines 829 - 840, Update decode_hex to validate and process hexadecimal characters without splitting UTF-8 byte sequences or using expect. Reject any non-ASCII or non-hex character with the existing type_error, while preserving successful decoding for valid even-length hexadecimal input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 91a0aea5-4c08-4c2d-a81f-c8d2ede50e4c
⛔ Files ignored due to path filters (12)
Cargo.lockis excluded by!**/*.lockcorpus/projects/citty/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/defu/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/destr/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/hookable/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/ohash/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/pathe/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/perfect-debounce/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/rou3/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlcorpus/projects/tiny-invariant/yarn.lockis excluded by!**/yarn.lock,!**/*.lockcorpus/projects/ufo/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (152)
.gitignore.outline/sdd/task-10-report.md.outline/sdd/task-14-report.md.outline/sdd/task-7-fix-report.md.outline/sdd/task-7-report.md.outline/sdd/task-9-report.mdCargo.tomlLICENSEcorpus/bamts.tomlcorpus/projects/tslib/test/package.jsoncorpus/specs/citty.tomlcorpus/specs/defu.tomlcorpus/specs/dot-prop.tomlcorpus/specs/hookable.tomlcorpus/specs/ohash.tomlcorpus/specs/p-map.tomlcorpus/specs/pathe.tomlcorpus/specs/rou3.tomlcorpus/specs/tiny-invariant.tomlcorpus/specs/tslib.tomlcorpus/specs/valita.tomlcorpus/specs/yocto-queue.tomlcrates/bamts-bytecode/Cargo.tomlcrates/bamts-bytecode/src/lib.rscrates/bamts-bytecode/src/program.rscrates/bamts-bytecode/src/string.rscrates/bamts-cli/Cargo.tomlcrates/bamts-cli/build.rscrates/bamts-cli/src/args.rscrates/bamts-cli/src/diagnostics.rscrates/bamts-cli/src/driver.rscrates/bamts-cli/src/main.rscrates/bamts-cli/tests/cli.rscrates/bamts-codegen/Cargo.tomlcrates/bamts-codegen/src/aot.rscrates/bamts-codegen/src/jit.rscrates/bamts-codegen/src/jit_memory.rscrates/bamts-codegen/src/lib.rscrates/bamts-compiler/Cargo.tomlcrates/bamts-compiler/RULES.mdcrates/bamts-compiler/src/bin/generate_rule_reference.rscrates/bamts-compiler/src/checker.rscrates/bamts-compiler/src/checker/binder.rscrates/bamts-compiler/src/checker/inference.rscrates/bamts-compiler/src/checker/intrinsic_environment.rscrates/bamts-compiler/src/checker/jsx.rscrates/bamts-compiler/src/checker/narrowing.rscrates/bamts-compiler/src/checker/relations.rscrates/bamts-compiler/src/emitter.rscrates/bamts-compiler/src/enum_plan.rscrates/bamts-compiler/src/lib.rscrates/bamts-compiler/src/lint.rscrates/bamts-compiler/src/literal.rscrates/bamts-compiler/src/lower.rscrates/bamts-compiler/src/namespace_plan.rscrates/bamts-compiler/src/parser.rscrates/bamts-compiler/src/pipeline.rscrates/bamts-compiler/src/program.rscrates/bamts-compiler/src/project.rscrates/bamts-compiler/src/rules/mod.rscrates/bamts-compiler/src/rules/semantic/coercions.rscrates/bamts-compiler/src/rules/semantic/control_flow.rscrates/bamts-compiler/src/rules/semantic/enums.rscrates/bamts-compiler/src/rules/semantic/flow_safety.rscrates/bamts-compiler/src/rules/semantic/functions.rscrates/bamts-compiler/src/rules/semantic/intrinsics.rscrates/bamts-compiler/src/rules/semantic/members.rscrates/bamts-compiler/src/rules/semantic/mod.rscrates/bamts-compiler/src/rules/semantic/modules.rscrates/bamts-compiler/src/rules/semantic/object_types.rscrates/bamts-compiler/src/scanner.rscrates/bamts-compiler/src/script.rscrates/bamts-compiler/src/source.rscrates/bamts-compiler/src/syntax.rscrates/bamts-compiler/src/telemetry.rscrates/bamts-compiler/src/warning.rscrates/bamts-compiler/tests/corpus_parse.rscrates/bamts-compiler/tests/rules.rscrates/bamts-native/Cargo.tomlcrates/bamts-native/src/lib.rscrates/bamts-native/src/native_bridge.rscrates/bamts-node/Cargo.tomlcrates/bamts-node/src/lib.rscrates/bamts-node/src/timers.rscrates/bamts-runtime/Cargo.tomlcrates/bamts-runtime/src/builtins/array.rscrates/bamts-runtime/src/builtins/collections.rscrates/bamts-runtime/src/builtins/date.rscrates/bamts-runtime/src/builtins/json.rscrates/bamts-runtime/src/builtins/mod.rscrates/bamts-runtime/src/builtins/number.rscrates/bamts-runtime/src/builtins/object.rscrates/bamts-runtime/src/builtins/promise.rscrates/bamts-runtime/src/builtins/regexp.rscrates/bamts-runtime/src/builtins/string.rscrates/bamts-runtime/src/builtins/symbol.rscrates/bamts-runtime/src/builtins/test_support.rscrates/bamts-runtime/src/builtins/timers.rscrates/bamts-runtime/src/builtins/uint8array.rscrates/bamts-runtime/src/external_modules.rscrates/bamts-runtime/src/gc.rscrates/bamts-runtime/src/host_objects.rscrates/bamts-runtime/src/intrinsics.rscrates/bamts-runtime/src/lib.rscrates/bamts-runtime/src/native.rscrates/bamts-runtime/src/regexp.rscrates/bamts-runtime/src/vm.rscrates/bamts-verification/Cargo.tomlcrates/bamts-verification/src/bin/perf_budget.rscrates/bamts-verification/src/bin/ts_conformance.rscrates/bamts-verification/src/check_cells.rscrates/bamts-verification/src/corpus.rscrates/bamts-verification/src/facets.rscrates/bamts-verification/src/ledger.rscrates/bamts-verification/src/lib.rscrates/bamts-verification/src/main.rscrates/bamts-verification/src/oracle_pins.rscrates/bamts-verification/src/perf.rscrates/bamts-verification/src/suite.rscrates/bamts-verification/src/ts_ledger.rscrates/bamts-verification/src/workspace_guard.rscrates/bamts-verification/tests/corpus_differential.rscrates/bamts-verification/tests/inspect_2darrays.rscrates/bamts/Cargo.tomlcrates/bamts/src/lib.rsdocs/solutions/architecture-patterns/exact-ecmascript-utf16-strings.mdformal/lean/Bamti/Bytecode/Model.leanformal/lean/Bamti/Bytecode/Verify.leanformal/lean/Bamti/JitLifecycle.leannpm/artifacts/cli-darwin-arm64/README.mdnpm/artifacts/cli-darwin-arm64/package.jsonnpm/artifacts/cli-darwin-x64/README.mdnpm/artifacts/cli-darwin-x64/package.jsonnpm/artifacts/cli-linux-arm64/README.mdnpm/artifacts/cli-linux-arm64/package.jsonnpm/artifacts/cli-linux-x64/README.mdnpm/artifacts/cli-linux-x64/package.jsonnpm/artifacts/cli-win32-x64/README.mdnpm/artifacts/cli-win32-x64/package.jsonnpm/bamti-cli/index.jsnpm/bamti-cli/package.jsonnpm/test/bamti-cli.test.mjspackage.jsonperf/benchmarks.tomlperf/budgets.tomlperf/hosts/bh1.tomlproof/completeness-ledger.jsonproof/lean-assumptions.jsonvendor/sources.tomlverification/diagnostic-code-map.jsonverification/manifest.lock.jsonverification/ts-conformance-ledger.schema.json
📜 Review details
🧰 Additional context used
🪛 LanguageTool
npm/artifacts/cli-darwin-x64/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # bamti-cli-darwin-x64 macOS x64 artifact package for `bamti-cl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
npm/artifacts/cli-darwin-arm64/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # bamti-cli-darwin-arm64 macOS arm64 artifact package for `bamti-...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
.outline/sdd/task-7-report.md
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... No project-wide checks. - No commit. - No lasting source edits (none needed; all ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/solutions/architecture-patterns/exact-ecmascript-utf16-strings.md
[style] ~88-~88: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... type requires Unicode scalar values. - When bytecode or native artifacts must prese...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...gs across processes or architectures. - When string, regular-expression, JSON, or pr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~90-~90: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... depends on UTF-16 code-unit offsets. - When source maps or diagnostics must report ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
npm/artifacts/cli-linux-arm64/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # bamti-cli-linux-arm64 Linux arm64 artifact package for `bamti-...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
crates/bamts-compiler/RULES.md
[style] ~630-~630: The adverb ‘never’ is usually put before the verb ‘assert’.
Context: ...d alternative: Handle every variant and assert never in the default branch. - Silence: `-A e...
(ADVERB_WORD_ORDER)
🔍 Remote MCP DeepWiki, Grep, Valyu
Additional review context
- TypeScript’s inference is performed before assignability checking; generic inference matches source and target types structurally, including function signatures and return types. Inference priorities and variance affect which candidates are retained. This is relevant when reviewing
inference.rsand post-body return inference. - Contextual typing is applied to function parameters and literals based on surrounding signatures, including call arguments and JSX-like expressions. Changes to
binder.rs,jsx.rs, andnarrowing.rsshould preserve this ordering. - Repository-specific external inspection was inconclusive: DeepWiki could not index
metaphorics/bamTiScript, and literal GitHub searches returned no matching implementation snippets., - The repository itself is publicly listed under
metaphorics/bamTiScript, confirming the target project identity, but the search did not expose PR#15’s source or review discussion.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (49)
crates/bamts-bytecode/src/lib.rs (1)
2147-2190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
You forbid aliased outputs on two of the four two-write opcodes and stay silent on the other two. Pick one rule.
IteratorCloserejectsresult == called.DisposeCapturerejectsmethod == kind.IteratorResultacceptsdone == valueandIteratorNextalways has. SoIteratorResult { done: r0, value: r0, .. }verifies, and whichever of the two writes the runtime performs last silently wins.
IteratorNextpredates this change, so its permissiveness is at least explicable.IteratorResultis new in this commit and copies the hole deliberately or by accident — the code does not say which. Either add the same alias rejection, or write down why the two-write opcodes split into "aliasing is a verify error" and "aliasing is fine".Confirm the runtime write order for the
done/valuepair before deciding:#!/bin/bash # Find the interpreter/codegen handlers for the two-write iterator opcodes. rg -n -C6 'IteratorResult|IteratorNext' --type=rust -g '!crates/bamts-bytecode/**'🤖 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` around lines 2147 - 2190, Resolve the inconsistent aliasing policy for iterator two-output instructions by inspecting the runtime handlers for IteratorResult and IteratorNext to confirm done/value write order, then apply the chosen rule consistently. If aliasing is invalid, add the corresponding verification error for IteratorResult’s done and value registers; otherwise remove the selective IteratorClose/DisposeCapture rejection or document the intentional distinction in the verifier.crates/bamts-codegen/src/jit_memory.rs (1)
387-396: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not cast a
*constderived from&selfinto*mutand then write through it.
region::Allocation::as_ptr::<T>(&self) -> *const T. You take that pointer, cast awayconst, and hand it to Cranelift, which writes generated machine code through it. Under Stacked Borrows that write is through a pointer whose provenance came from a shared borrow. Miri will flag it, and the module is#[allow(unsafe_code)]precisely because it is supposed to be the one place that gets this right.
region::Allocationexposesas_mut_ptr::<T>(&mut self). Use it.allocationis a localmutbinding at this point, so this costs one keyword.🛡️ Proposed fix
- let allocation = + let mut allocation = region::alloc(rounded, region::Protection::READ_WRITE).map_err(io::Error::other)?; - let pointer = allocation.as_ptr::<u8>() as *mut u8; + let pointer = allocation.as_mut_ptr::<u8>(); let mapping_kind = MappingKind::from_request(kind);#!/bin/bash # Confirm the region 3.0.2 Allocation API surface: as_ptr vs as_mut_ptr receivers. set -eu fd -t d '^region-3\.0\.2$' ~/.cargo/registry/src 2>/dev/null | head -1 | while read -r dir; do rg -n -B 4 -A 6 'fn as_ptr|fn as_mut_ptr|unsafe impl (Send|Sync) for Allocation' "$dir/src" done🤖 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_memory.rs` around lines 387 - 396, Update the allocation handling in the method containing the local allocation to bind it as mutable and call region::Allocation::as_mut_ptr::<u8>() instead of as_ptr::<u8>() followed by a const-to-mut cast. Keep the existing MappingKind, liveness recording, mapping storage, and returned pointer behavior unchanged.crates/bamts-codegen/src/jit.rs (1)
283-299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
require_finalizedpanics inside a function that returnsResult. Fix that.
compile_loweredreturnsResult<JitProgram, JitError>, and then reaches for an assertion to decide whether it may publish. Iffinalize_definitionsever returnsOkwithout driving the provider toWxPhase::Executable, this aborts the process. The comment concedes the point: "infallible in practice". "In practice" is not a guarantee, and the caller is the CLI driver, which will die without a diagnostic instead of reporting a compilation failure.The receipt is a real invariant. Enforce it with a real error.
🛡️ Proposed fix
- // Publication requires the receipt. `finalize_definitions` returned `Ok`, so - // the provider reached `Executable` only after every owned mapping - // transitioned to its exact final protection; `require_finalized` is then - // infallible in practice. No receipt exists in `Writable` or `Freed`, so a - // partially-finalized module (which errors above) can never publish. - let receipt = memory.require_finalized(); + // Publication requires the receipt. No receipt exists in `Writable` or + // `Freed`, so a partially-finalized module can never publish. + let receipt = memory + .try_finalized() + .ok_or_else(|| JitError::InvalidLoweredModule( + "host JIT finalization did not reach executable memory".to_string(), + ))?;Add the fallible accessor next to
require_finalizedinjit_memory.rs:/// The publication receipt, or `None` outside [`WxPhase::Executable`]. pub(crate) fn try_finalized(&self) -> Option<FinalizedMemory> { (self.phase() == WxPhase::Executable).then_some(FinalizedMemory { _private: () }) }Keep
require_finalizedfor the tests that assert the panic.#!/bin/bash # Does cranelift-jit always call the memory provider's `finalize` from # `finalize_definitions`, including when nothing new was compiled? set -eu fd -t d 'cranelift-jit-0.134.2' ~/.cargo/registry/src 2>/dev/null | head -1 | while read -r dir; do rg -n -C 12 'fn finalize_definitions|fn finalize\b' "$dir/src" || true done rg -n -C 6 'finalize_definitions' crates/bamts-codegen/src🤖 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 283 - 299, Replace the infallible memory receipt assertion in compile_lowered with the fallible try_finalized accessor, returning an appropriate JitError when the provider is not executable. Add try_finalized alongside require_finalized in the memory implementation, preserving require_finalized for panic-based tests and keeping successful publication unchanged.crates/bamts-compiler/src/checker/binder.rs (7)
377-391: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Parameter names are stored but excluded from
Eq/Hash, so interning silently discards them.
TypeTable::internkeys onType, andType::Functionhashes throughFunctionSignature→FunctionParameter.nameis excluded from bothPartialEqandHash. Therefore(a: number) => voidand(b: number) => voidintern to the sameTypeId, and the surviving type carries whichever names were interned first.This is fine while names are decoration. It is a correctness bug the moment anything renders them. The PR description claims the change "preserves generic call types by adding parameter names ... while retaining structural interning". You cannot have both. Pick one: either drop
namefrom the interned type and carry it beside theTypeId, or include it inEq/Hashand accept the extra interning entries.Confirm nothing renders these names today:
#!/bin/bash # Find every read of FunctionParameter::name and every place it reaches output. rg -nP -C 5 '\bFunctionParameter\b' --type=rust rg -nP -C 5 '\.parameters\(\)[\s\S]{0,200}?\.name\(\)' --type=rust # Declaration emit / baseline emitters that could print a signature. fd -e rs . crates/bamts-compiler/src --exec rg -nP -l 'fn .*(emit|render|display|format).*signature'🤖 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/checker/binder.rs` around lines 377 - 391, Resolve the interning contract for FunctionParameter::name: either remove names from the interned Type representation and store them alongside the TypeId, or include name in FunctionParameter::eq and FunctionParameter::hash so signatures with different parameter names receive distinct TypeIds. Confirm current consumers before choosing, and preserve name rendering correctness.
2278-2282: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Linear token scan per statement makes namespace binding quadratic.
is_dotted_namespace_tailwalks the entire token vector of the file.bind_namespacecalls it once per body statement, andbind_namespace_membercalls it again per statement. On a declaration file with a large namespace body this is O(tokens × statements).Build the lookup once. A
HashSet<Utf16Pos>of the start offsets of everyDottoken, computed atBinderconstruction, turns this into a hash probe.Separately, the predicate is a positional proxy, not a syntactic one: it asks whether some
Dottoken starts at the same offset as the statement. Any future change to token ranges breaks it silently. Prefer asking theNamespaceNameshape directly if the parser records it.🤖 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/checker/binder.rs` around lines 2278 - 2282, Replace the full-token scan in is_dotted_namespace_tail with a Binder-level HashSet<Utf16Pos> containing Dot token start offsets, initialized once during Binder construction and queried by the predicate. Also prefer checking the parsed NamespaceName shape directly when available, preserving the existing dotted-tail behavior without relying solely on matching statement and token positions.
3120-3157: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Thirty-eight lines of parameter-lowering logic copied verbatim into two functions.
Lines 3120-3157 here and lines 5863-5900 in
signature_typeare the same code: the sameis_this_parameterskip, the same annotation-or-anyresolution, the samerest/optionalderivation, and the same four-deepBindingPatternmatch with the sameformat!("arg{idx}")fallbacks.Two copies of a nested match this shaped will diverge. Extract one
fn function_parameter(&mut self, idx: usize, parameter: &'src ParameterNode, scope: ScopeId) -> FunctionParameterand call it from both loops.🤖 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/checker/binder.rs` around lines 3120 - 3157, Extract the duplicated parameter-lowering logic into a shared Checker method named function_parameter, accepting idx, parameter, and scope and returning FunctionParameter. Use it in both the current loop and the signature_type loop, preserving the existing this-parameter filtering and all type, optional, rest, name, and fallback behavior.
3616-3626: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Base-class members are lost when the base is declared after the derived class.
Inheritance reads
self.class_instance_types.get(&base_symbol). That map is filled inresolve_class_body, andresolve_statementswalks statements in source order. So a forward reference sees nothing:class D extends B { } // resolved first: no entry for B yet class B { fromB: number; } // fills class_instance_types[B] afterwards new D().fromB; // PROPERTY_DOES_NOT_EXISTTypeScript accepts this. The checker reports a false
PROPERTY_DOES_NOT_EXIST.Resolve the base class on demand, the same way
resolve_type_symbolmemoizes named types withTypeState::InProgressto break cycles. Aclass_instance_type_of(symbol)entry point that forces the base's class body before reading the map would fix it and would also give you cycle detection forclass A extends B/class B extends A.Verify the ordering assumption and check whether a forward-declared base is already handled elsewhere:
#!/bin/bash # Every writer and reader of the class instance map. rg -nP -C 6 'class_instance_types' crates/bamts-compiler/src # Any existing test covering a forward-declared base class. rg -nP -C 4 'extends\s+B\b' crates/bamts-compiler/src --type=rust🤖 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/checker/binder.rs` around lines 3616 - 3626, Add a class_instance_type_of(symbol) resolution entry point and use it when retrieving the base instance in the inheritance member collection, forcing resolve_class_body for forward-declared bases before reading class_instance_types. Mirror resolve_type_symbol’s InProgress memoization to detect cyclic inheritance, while preserving the existing base-property merge behavior.
5616-5622: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Quoted property names are unquoted by trimming characters, not by decoding the literal.
trim_matchesstrips every leading and trailing quote character and performs no escape processing. Two consequences:
- Escapes survive.
{ "\u0041": 1 }yields the key\u0041, notA. The same property written both ways becomes two distinct members, and a lookup through one spelling reportsPROPERTY_DOES_NOT_EXIST.- Repeated quote characters are over-trimmed. The literal
"''"yields an empty key.
crate::literal::string_valueis already imported at line 67 and is used correctly for namespace string names at line 2166. Use it here too.🐛 Proposed fix
PropertyName::String(string) => { - let text = self.text(string.data().token()); - Some( - text.trim_matches(|c| c == '"' || c == '\'' || c == '`') - .to_owned(), - ) + let text = self.text(string.data().token()); + Some(string_value(text)?.to_utf8_lossy().into_owned()) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.PropertyName::String(string) => { let text = self.text(string.data().token()); Some(string_value(text)?.to_utf8_lossy().into_owned()) }🤖 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/checker/binder.rs` around lines 5616 - 5622, Update the PropertyName::String branch to decode the quoted literal with the imported string_value helper instead of trimming quote characters from the raw text. Preserve the resulting decoded string as the property key so escapes resolve consistently and repeated quote characters remain part of the literal content.
5976-6001: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Explicit type arguments are bound in the wrong order.
signature.type_parameters()already holds the declared type parameters in declaration order. You ignore it and instead rebuild an order by walking parameter types first and the return type second. Those two orders are not the same order.Take
function f<T, U>(u: U, t: T): T. Walking parameters yieldsU, thenT. Sodeduped == [U, T]. A callf<number, string>(x, y)then assignsU := numberandT := string. Both type arguments are swapped. Every generic whose type parameters are not first mentioned in declaration order is mis-instantiated.Use the declared list. It is the only correct positional basis.
🐛 Proposed fix
let explicit = self.resolve_type_arguments(Some(type_args), scope); - let mut inference_symbols = Vec::new(); - for parameter in signature.parameters() { - self.collect_type_parameter_symbols(parameter.type_id(), &mut inference_symbols); - } - self.collect_type_parameter_symbols(signature.return_type(), &mut inference_symbols); - let mut deduped: Vec<SymbolId> = Vec::new(); - for sym in inference_symbols { - if !deduped.contains(&sym) { - deduped.push(sym); - } - } - if deduped.is_empty() { + let declared: Vec<SymbolId> = signature.type_parameters().to_vec(); + if declared.is_empty() { return Some(signature.clone()); } let mut inferred = Vec::new(); - for (index, symbol) in deduped.iter().enumerate() { + for (index, symbol) in declared.iter().enumerate() {The declaration order is populated at lines 3107-3119 and 5594-5606, so it is available on every signature the binder builds.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let declared: Vec<SymbolId> = signature.type_parameters().to_vec(); if declared.is_empty() { return Some(signature.clone()); } let mut inferred = Vec::new(); for (index, symbol) in declared.iter().enumerate() { let type_id = explicit .get(index) .copied() .unwrap_or_else(|| self.types.any()); inferred.push(InferredTypeArgument::new( *symbol, type_id, InferenceProvenance::Explicit, )); }🤖 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/checker/binder.rs` around lines 5976 - 6001, In the generic type-argument binding logic, replace the inferred symbol order built from parameter and return types with the declaration-ordered list from signature.type_parameters(). Preserve the existing deduplication/empty handling as appropriate, and map each explicit argument by index to the corresponding declared type parameter so signatures such as f<T, U> retain T-then-U binding.
6043-6047: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Type-parameter constraints and defaults are dropped on every ordinary call.
InferenceParameter::new(*symbol)records no constraint and no default.inference.rsdocuments that an inferred candidate failing itsextendsconstraint is replaced by the constraint, and that an unreached parameter falls back to its default. Neither rule can ever fire here, because the context never learns the bounds.
jsx.rslines 347-355 builds the same context correctly withwith_constraintandwith_default. The ordinary call path does not.The root cause is upstream:
resolve_type_parameter_boundsat lines 3204-3221 resolves each constraint and default and immediately discards theTypeIdwithlet _ = .... Record those per type-parameter symbol, then feed them in here.Effect today:
function f<T extends string>(x: T): Tcalled asf(1)infersT := 1instead ofstring, and the argument check passes.🤖 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/checker/binder.rs` around lines 6043 - 6047, Preserve resolved constraints and defaults from resolve_type_parameter_bounds instead of discarding their TypeIds, keyed by type-parameter symbol. In the ordinary call path building inference_parameters, initialize each InferenceParameter with the corresponding with_constraint and with_default values, matching the JSX context setup, while retaining unconstrained or default-less parameters unchanged.crates/bamts-compiler/src/checker/inference.rs (1)
261-273: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Instantiation silently clears
readonlyon every object property.
PropertyType::newdefaultsreadonlytofalse. Rebuilding the property list here therefore drops the flag.interface Box<T> { readonly value: T }instantiated asBox<number>produces a mutablevalue.
TypeTable::wideninbinder.rslines 831-844 gets this right with.with_readonly(property.readonly). This site does not. Any rule that readsreadonly—readonly-alias-mutation,prefer-readonly-array— loses its evidence the moment a generic is instantiated.🐛 Proposed fix
.map(|property| { PropertyType::new( property.name(), property.optional(), self.instantiate(table, property.type_id()), ) + .with_readonly(property.readonly()) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Type::ObjectType(properties) => { let properties: Vec<PropertyType> = properties .iter() .map(|property| { PropertyType::new( property.name(), property.optional(), self.instantiate(table, property.type_id()), ) .with_readonly(property.readonly()) }) .collect(); table.object_type(properties) }🤖 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/checker/inference.rs` around lines 261 - 273, Preserve each property’s readonly flag when rebuilding object types in the Type::ObjectType instantiation branch. Update the PropertyType construction to carry through property.readonly, matching the existing TypeTable::widen behavior, while leaving name, optionality, and instantiated type handling unchanged.crates/bamts-compiler/src/checker/jsx.rs (2)
311-330: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
A generic factory allocates a fresh scope and re-resolves its whole signature on every JSX usage.
Lines 313-317 create a new
ScopeKind::Functionscope and re-bind the factory's type parameters each time the component appears. Lines 321-330 then re-resolve every parameter annotation and the return annotation in that scope.Scopes and symbols are append-only.
new_scopepushes toself.scopes;bind_type_parameterspushes aSymbol, asymbol_type, and atype_stateper type parameter. Use a generic component two hundred times and the frozenSemanticModelcarries two hundred copies of its type-parameter scope.The re-resolution is worse than the memory.
resolve_type_referencepushes intosymbol_referencesatbinder.rsline 5118, andsymbol_referencesfeeds the S2.symbolsbaseline emitter. Every extra usage therefore appends duplicate reference records to the baseline output. That is an output-contract change, not just waste.Memoize the resolved factory signature per symbol and instantiate from the cached signature.
Confirm the duplicate-record effect against the baseline emitter:
#!/bin/bash # Who consumes symbol_references, and does it deduplicate? rg -nP -C 8 'symbol_references' crates/bamts-compiler/src # Confirm resolve_type_reference records on every call. ast-grep run --pattern 'fn resolve_type_reference($$$) { $$$ }' --lang rust crates/bamts-compiler/src/checker/binder.rs🤖 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/checker/jsx.rs` around lines 311 - 330, Memoize each generic factory’s resolved signature per symbol instead of rebuilding it for every JSX usage. Update the flow around type-parameter scope creation, parameter resolution, and declared_return in the relevant checker method to cache the resolved signature—including type parameters, parameter types, and return type—and reuse it when instantiating the same factory, avoiding repeated new_scope, bind_type_parameters, and symbol-reference recording.
488-496: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Spreading an
any-typed value produces a false "attributes not assignable".The spread arm only merges when the spread type is
Type::ObjectType. Anything else contributes nothing at all, and the synthesized props object stays empty.const props = something as any; const x = <div {...props} />; // JSX.IntrinsicElements: div: { id: string }
type_of_exprgivesany, theif letfails,propertiesstays empty, andcheck_jsx_props_assignablethen reports every required target property as missing. TypeScript accepts this. Spreading ananyorunknownprops bag is routine in TSX.Treat an opaque spread as "this element's props are unknowable" and skip the assignability check for that element, the same way
check_jsx_props_assignablealready short-circuits on an opaque target at lines 511-516.🤖 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/checker/jsx.rs` around lines 488 - 496, Update the JsxAttributeItem::Spread handling to detect opaque spread types such as any or unknown and mark the element’s props as unknowable, causing check_jsx_props_assignable to skip validation for that element. Preserve the existing property-merging behavior for Type::ObjectType spreads and use the same opaque-target short-circuit semantics already present in check_jsx_props_assignable.crates/bamts-compiler/src/checker/relations.rs (1)
252-269: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
strictNullChecksrejectsnullandundefinedagainst union targets. That is wrong.The source-side
Null/Undefinedarms sit before the target-side union arm. Forrelates(null, string | null, StrictNull)the match stops at line 261, seestoisType::Union(_), and returnsfalse. UnderstrictNullChecks,nullis assignable to any union that containsnull. The same defect hitsundefinedagainststring | undefined, which is the single most common nullable annotation in real TypeScript.The doc comment even says the intent: "they only flow to types that explicitly include them". The code never looks inside the union to find out.
Distribute over the target union before applying the nullish rules.
🐛 Proposed fix: let a union target be inspected first
(Type::Null | Type::Undefined, _) if strictness == Strictness::Assignable => { !matches!(to, Type::Never) } + // A union target must be searched before the nullish rules decide: + // `null` flows into `string | null` under strict null checks. + (Type::Null | Type::Undefined, Type::Union(targets)) + if strictness == Strictness::StrictNull => + { + targets + .iter() + .any(|member| self.relates(source, *member, strictness)) + } (Type::Undefined, _) if strictness == Strictness::StrictNull => { matches!(to, Type::Any | Type::Unknown | Type::Void | Type::Undefined) }Add a test for
null -> string | nullandundefined -> string | undefinedunderassignable_with_strict_null. The existing suite never exercisesStrictNullat all, which is how this got through.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Null/undefined are assignable to any type in non-strict mode, // but not to `never`. Under strict null checks they only flow to // types that explicitly include them. (Type::Null | Type::Undefined, _) if strictness == Strictness::Assignable => { !matches!(to, Type::Never) } // A union target must be searched before the nullish rules decide: // `null` flows into `string | null` under strict null checks. (Type::Null | Type::Undefined, Type::Union(targets)) if strictness == Strictness::StrictNull => { targets .iter() .any(|member| self.relates(source, *member, strictness)) } (Type::Undefined, _) if strictness == Strictness::StrictNull => { matches!(to, Type::Any | Type::Unknown | Type::Void | Type::Undefined) } (Type::Null, _) if strictness == Strictness::StrictNull => { matches!(to, Type::Any | Type::Unknown | Type::Null) } (Type::Union(sources), _) => sources .iter() .all(|member| self.relates(*member, target, strictness)), (_, Type::Union(targets)) => targets .iter() .any(|member| self.relates(source, *member, strictness)),🤖 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/checker/relations.rs` around lines 252 - 269, Update the relation matching in the checker around self.relates so target-side Type::Union is handled before the strict-null Type::Null and Type::Undefined arms, allowing nullish sources to match any union member they are assignable to. Preserve the existing non-union nullish rules, and add assignable_with_strict_null coverage for null to string | null and undefined to string | undefined.crates/bamts-compiler/src/emitter.rs (2)
904-911: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
export = xis rewritten toexport default x. Those are not the same module.
export =declares a CommonJS-style single export.export defaultdeclares an ES module named export calleddefault. A consumer doingconst m = require('./mod')gets the value withexport =and gets{ default: value }withexport default. Declaration mode at line 2427 still printsexport =, so the.d.tsand the.jsnow describe different module shapes.The PR objectives do not mention this conversion. State the intended interop contract, or gate the rewrite on the CommonJS/ESM mode the pipeline already tracks (
ProgramCheckOptions::commonjs()incrates/bamts-compiler/src/pipeline.rs).🤖 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/emitter.rs` around lines 904 - 911, Update the ExportDeclaration::Assignment emission in emitters to preserve the intended CommonJS/ESM interop contract: do not unconditionally emit export default for export = assignments. Use the pipeline’s tracked ProgramCheckOptions::commonjs() mode to emit the matching CommonJS form when applicable, while keeping declaration output consistent with the generated JavaScript.
1163-1167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parameter decorators are emitted into JavaScript. That output is not JavaScript.
emit_params_jsnow printsparameter.decoratorsinline, so a TypeScript constructor lowers to:constructor(`@constructorParameterFirst` parameter) {}Parameter decorators exist only in TypeScript's legacy
experimentalDecoratorsmode. They are not in the ECMAScript decorators proposal and no JavaScript engine parses them. The round-trip test at lines 3996-4112 only re-parses the output with this crate's own parser, so it proves nothing about runnability.Class, method, field, and accessor decorators are defensible as passthrough. Parameter decorators are not. Either erase them in
EmitMode::JavaScript, or lower them to the runtime calls TypeScript emits. Emitting them raw guarantees aSyntaxErrorat load.🐛 Minimal fix: erase parameter decorators in JavaScript output
first = false; - self.emit_decorators_inline(¶meter.decorators); self.emit_pattern(¶meter.binding);🤖 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/emitter.rs` around lines 1163 - 1167, Update emit_params_js so parameter.decorators are omitted when emitting in EmitMode::JavaScript, while preserving the existing decorator emission for TypeScript output. Ensure generated JavaScript constructors and methods contain only valid parameter syntax.crates/bamts-compiler/src/enum_plan.rs (1)
466-466: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Two
HashMapindex panics on a table the same function elsewhere treats as fallible.Line 466 does
member_symbols[&member_id]and line 557 doesmember_symbols[&member_id]again. Both panic if the binder did not record a symbol for that member. Yet line 637 reads the same map with.get(&member_id).copied()and handlesNoneby settingauto = None. Pick one contract. If a missing symbol is impossible, line 637's defensive branch is dead code that hides the invariant. If it is possible, lines 466 and 557 crash the compiler on malformed input.The
.expect("bound enum member has an entry")at line 558 is fine becausesymbol_to_entryis built in this function. Themember_symbolsindexing is not; that map is caller-supplied.🛡️ Proposed fix: make the caller-supplied lookup fallible
- let symbol = member_symbols[&member_id]; + let Some(&symbol) = member_symbols.get(&member_id) else { + continue; + };- let entry = &entries[*symbol_to_entry - .get(&member_symbols[&member_id]) - .expect("bound enum member has an entry")]; + let entry = match member_symbols + .get(&member_id) + .and_then(|symbol| symbol_to_entry.get(symbol)) + { + Some(index) => &entries[*index], + None => { + plans + .get_mut(&binding.declaration_id) + .expect("every enum declaration has a plan") + .push(EnumMemberPlan::Invalid); + continue; + } + };Also applies to: 556-558
🤖 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/enum_plan.rs` at line 466, Make the caller-supplied member_symbols lookups in the enum planning function fallible at both member_id access sites, matching the existing member_symbols.get(...).copied() handling near the auto calculation. Preserve the established missing-symbol behavior by propagating or applying the same None handling instead of indexing the map and panicking; keep the symbol_to_entry expect unchanged.crates/bamts-compiler/src/rules/semantic/mod.rs (1)
128-128: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
variable_typesis a flat, unscoped name-to-type map.Only
visit_functionsaves and restores shadowed entries. Block scopes,forbindings, catch clauses, class bodies, and nested arrow bodies all write into the same global map keyed by bare identifier text. Two sibling functions declaringconst value: Userandconst value: Paymentleave whichever ran last in the map, and every downstream rule (visit_switch,visit_member,json_unserializable) reads that stale entry.The result is cross-scope false positives and false negatives in W008, W016, W045, W063, and W073. The tests all use single-scope one-liners, so none of them can catch it.
This needs a scope stack, not more save/restore special cases.
Also applies to: 638-722
🤖 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` at line 128, Replace the flat variable_types map with a scope stack so bindings are isolated by lexical scope. Update declaration handling and scope visitors, including visit_function, block/for/catch/class scopes, and nested arrow bodies, to push and pop scopes; resolve reads in visit_switch, visit_member, and json_unserializable from the nearest active scope. Remove function-only save/restore special cases while preserving shadowing and sibling-scope isolation.crates/bamts-compiler/tests/rules.rs (1)
20-32: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Nothing asserts that rule codes and slugs are unique.
assert_complete_metadatachecks that each field is non-empty. It never checks that two rules do not share a code.rule_by_codeincrates/bamts-compiler/src/rules/semantic/mod.rsline 39 does.expect("semantic rule code must be registered")and takes the first match, so a duplicate registration silently shadows a rule and its diagnostics vanish. The per-rule contract test at line 98 would still pass, because the shadowed rule's trigger fires under the shadowing rule's identical code.This is a two-line check that closes the hole.
💚 Proposed addition
for rule in &RULES { assert_complete_metadata(rule); } + + let mut codes = std::collections::HashSet::new(); + let mut slugs = std::collections::HashSet::new(); + for rule in &RULES { + assert!(codes.insert(rule.code()), "duplicate rule code {}", rule.code()); + assert!(slugs.insert(rule.slug()), "duplicate rule slug {}", rule.slug()); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.#[test] fn every_registered_rule_has_complete_metadata_and_current_reference() { assert_eq!(RULES.len(), 88, "the adopted catalog has exactly 88 rules"); assert_eq!( REFERENCE, rule_reference(), "regenerate RULES.md from RULES" ); for rule in &RULES { assert_complete_metadata(rule); } let mut codes = std::collections::HashSet::new(); let mut slugs = std::collections::HashSet::new(); for rule in &RULES { assert!(codes.insert(rule.code()), "duplicate rule code {}", rule.code()); assert!(slugs.insert(rule.slug()), "duplicate rule slug {}", rule.slug()); } }🤖 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/tests/rules.rs` around lines 20 - 32, Extend every_registered_rule_has_complete_metadata_and_current_reference to assert that all rule codes and slugs in RULES are unique, alongside the existing metadata checks. Use sets or equivalent collection-based assertions so duplicate registrations fail the test rather than allowing rule_by_code to shadow an earlier rule.crates/bamts-native/src/native_bridge.rs (1)
1877-1943: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Index 39 has no signature assertion, and the list is out of order at the end.
This block is the compile-time pin for the C ABI. Every helper from 2 through 45 gets an assertion except one:
bamts_dispose_capture(39). Its signature can drift from(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32and nothing in the workspace will complain, even though it writes a caller-supplied register index into the frame. That is exactly the helper you least want unpinned.Line 1943 also parks index 38 after index 45. That is how 39 went missing in the first place: an unordered ledger hides its own holes.
Add the assertion and put 38 back where it belongs.
🛡️ Proposed fix
const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_import_dynamic; // 37 +const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_import_meta; // 38 +const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = + bamts_dispose_capture; // 39 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_suppress_error; // 40 @@ const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_with_has_binding; // 45 -const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_import_meta; // 38📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, u64, *mut Completion) -> u32 = bamts_binary; // 2 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_create_object; // 3 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_create_array; // 4 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, *mut Completion) -> u32 = bamts_create_closure; // 5 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_get_property; // 6 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_set_property; // 7 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_delete_property; // 8 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_call; // 9 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_construct; // 10 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_import; // 11 const _: unsafe extern "C" fn(*mut ShadowFrame, u64) -> u32 = bamts_truthy; // 12 (no out) const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_resume_value; // 13 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, u32, *mut Completion) -> u32 = bamts_define_accessor; // 14 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_load_global; // 15 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, *mut Completion) -> u32 = bamts_store_global; // 16 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_typeof_global; // 17 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_this; // 18 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_arguments; // 19 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_new_target; // 20 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_array_push; // 21 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_array_extend; // 22 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_object_spread; // 23 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_set_prototype; // 24 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_create_private_name; // 25 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u32, *mut Completion) -> u32 = bamts_create_regexp; // 26 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, *mut Completion) -> u32 = bamts_get_iterator; // 27 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_iterator_next; // 28 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, u64, *mut Completion) -> u32 = bamts_export; // 29 const _: unsafe extern "C" fn(*mut ShadowFrame, u32, *mut Completion) -> u32 = bamts_consume_fuel; // 30 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_create_cell; // 31 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_iterator_step; // 32 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_iterator_result; // 33 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_iterator_close; // 34 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_require_close_result; // 35 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_to_object; // 36 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, *mut Completion) -> u32 = bamts_import_dynamic; // 37 const _: unsafe extern "C" fn(*mut ShadowFrame, *mut Completion) -> u32 = bamts_load_import_meta; // 38 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32 = bamts_dispose_capture; // 39 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_suppress_error; // 40 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_construct_with_new_target; // 41 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, *mut Completion) -> u32 = bamts_define_data_property; // 42 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u32, *mut Completion) -> u32 = bamts_load_own_descriptor_slot; // 43 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, u64, u32, *mut Completion) -> u32 = bamts_define_own_descriptor_slot; // 44 const _: unsafe extern "C" fn(*mut ShadowFrame, u64, u64, *mut Completion) -> u32 = bamts_with_has_binding; // 45🤖 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 1877 - 1943, Complete the ABI assertion ledger by adding the compile-time signature assertion for bamts_dispose_capture at index 39, using (*mut ShadowFrame, u64, u32, u32, *mut Completion) -> u32. Move the existing bamts_load_import_meta assertion (index 38) before the new index 39 entry so the assertions remain ordered through index 45.crates/bamts-verification/src/check_cells.rs (1)
1506-1510: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Two nested linear scans per symbol. This is quadratic, and you run it over the whole TypeScript conformance suite.
Line 1506 calls
symbol_decl_positiononce per symbol. Each call doestokens.iter().position(...)at line 1597 — a full scan of the unit's token stream. For keyword-led declarations, line 1605 then loops callingprev_significant_token, which does its ownrpositionover the token prefix at line 1624.So the cost is O(symbols × tokens) at best, and the keyword walk adds another O(tokens) factor per step. Symbol count and token count both grow with file size, so
.symbolsemission is quadratic in file size, executed once per case, across the full suite. The.typesemitter next door is linear. There is no reason this one is not.Build the start-position index once per unit.
🔧 Proposed fix
+ // Significant token indices by UTF-16 start position, built once. + let significant: Vec<usize> = (0..tokens.len()) + .filter(|&i| !tokens[i].is_missing() && !is_trivia_token(tokens[i].kind())) + .collect(); + let by_start: std::collections::HashMap<Utf16Pos, usize> = significant + .iter() + .enumerate() + .map(|(rank, &index)| (tokens[index].range().start(), rank)) + .collect(); let decl_positions: Vec<Option<(usize, usize)>> = model .symbols() .iter() - .map(|symbol| symbol_decl_position(tokens, source, symbol)) + .map(|symbol| symbol_decl_position(tokens, &significant, &by_start, source, symbol)) .collect();
symbol_decl_positionthen resolves the identifier in O(1) throughby_startand walks left oversignificantby rank, soprev_significant_tokendisappears entirely.Also applies to: 1588-1627
🤖 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/src/check_cells.rs` around lines 1506 - 1510, Build a per-unit start-position index and significant-token sequence before mapping symbols, then pass those indexes into symbol_decl_position. Replace its tokens.iter().position lookup with O(1) by_start resolution and replace prev_significant_token calls with rank-based leftward traversal over significant, removing the repeated scans while preserving declaration-position results.crates/bamts-verification/src/perf.rs (1)
751-776: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The RSS and artifact metrics are fabricated, and the budget policy gates on them anyway.
Three separate problems in fourteen lines:
read_peak_rss_bytesreadsVmHWMfrom/proc/self/status. That is the high-water mark of the harness process, monotonic since process start. It includes every allocation the test binary made beforemeasurewas ever called. It does not describe the measured slice.- The same single sample is written into
p50,p95, andp99. A one-sample distribution is not a distribution.check_abs_rel("rss.p95", …)at line 923 then compares a number that is p95 of nothing.artifact_bytes: 0is hardcoded, whileArtifactPolicyandcheck_abs_rel("artifact.p50", …)at line 931 exist to gate it.Today
evaluate_budgetsreturns early because S0 carries no baseline, so none of this fires. That is exactly the problem. The moment someone drops a baseline JSON in place, two of the three budget families start comparing invented numbers against real thresholds, and nobody will know why the gate never catches anything — or why it suddenly does.Either measure these per repeat, or stop emitting them as measured values. If they are placeholders, say so in the type and skip the corresponding budget checks until they are real.
🩹 Minimum honest change: gate the placeholder metrics explicitly
let rss = read_peak_rss_bytes().unwrap_or(0); - let rss_bytes = Quantiles { - p50: rss as f64, - p95: rss as f64, - p99: rss as f64, - }; + // `VmHWM` is the harness process high-water mark, not a per-repeat sample + // of the measured slice. Until the runner samples RSS around each repeat, + // record the single observation on p50 only and leave the tail quantiles + // at zero so `check_abs_rel` cannot compare an invented p95. + let rss_bytes = Quantiles { + p50: rss as f64, + p95: 0.0, + p99: 0.0, + };The real fix samples RSS around each
run_suite_with_telemetrycall and feedsQuantiles::from_samples, the same waywall_samplesalready works at line 731.🤖 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/src/perf.rs` around lines 751 - 776, Replace the fabricated RSS and artifact values in the measurement flow with explicit placeholder handling: mark these metrics as unavailable in the relevant result type and update evaluate_budgets so ArtifactPolicy and RSS checks are skipped when they are unavailable. Do not emit the harness-wide read_peak_rss_bytes sample as p50/p95/p99 or hardcode artifact_bytes as a measured value; preserve existing budget checks for real metrics.crates/bamts-verification/src/suite.rs (1)
938-948: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two
let _ = (...)blocks that do nothing, with comments claiming they do something. Both sites bind a tuple of unused symbols to_purely to silence dead-code and unused-import warnings, and both carry a comment asserting a verification benefit that the statement cannot deliver. Evaluating a constant proves nothing about drift. Coercingdrain_stream::<std::io::Empty>to a function pointer and discarding it does not pin a process convention. A future reader will trust these comments and skip writing the check that is actually missing.
crates/bamts-verification/src/suite.rs#L938-L948: delete the tuple and the "Keep corpus seams referenced" comment. If the corpus seams are genuinely unused until the execute backends are wired, mark the imports#[allow(unused_imports)]with aTODOnaming the slice that will consume them, so the compiler stops lying on your behalf.crates/bamts-verification/src/suite.rs#L1969-L1978: delete the tuple and the "Touch constants so pin drift ... is obvious" comment. If you want the pin document checked against the module constants, assert it — compare eachexpectedfield against its constant, or dropPinDocument::expected()and build the comparison from the constants directly.📍 Affects 1 file
crates/bamts-verification/src/suite.rs#L938-L948(this comment)crates/bamts-verification/src/suite.rs#L1969-L1978🤖 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/src/suite.rs` around lines 938 - 948, The discarded tuple at crates/bamts-verification/src/suite.rs:938-948 must be removed along with its misleading comment; if those corpus seams remain unused, mark the relevant imports with #[allow(unused_imports)] and a TODO identifying the future execute-backend consumer. Also remove the tuple and “Touch constants...” comment at crates/bamts-verification/src/suite.rs:1969-1978, and replace them with real assertions comparing each PinDocument::expected() field against its corresponding module constant, or construct the comparison directly from those constants.crates/bamts-verification/src/ts_ledger.rs (1)
319-327: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Nothing rejects duplicate entries, and that quietly defeats NO_SILENT_SKIP.
validateiterates entries, checks sorting, and checks totals. It never checks thatidis unique, or that(input, facet)appears once. Two identical rows validate cleanly.validate_sortedcannot catch them either — identical sort keys compare equal.This is not theoretical.
enforce_no_silent_skipincrates/bamts-verification/src/suite.rsat lines 978-984 collects results into aBTreeSet<(String, String)>keyed by(entry_id, backend). Duplicate ledger rows collapse to one set member. So a ledger with a duplicated included row produces one executed cell, and the guard that exists specifically to prove no cell was skipped reports success. The whole point of this ledger is that a cell cannot vanish. Right now one can.Add the uniqueness check where the other structural rules live.
🔒 Proposed fix
for (index, entry) in self.entries.iter().enumerate() { entry.validate(index)?; } + let mut seen_ids: BTreeSet<&str> = BTreeSet::new(); + let mut seen_cells: BTreeSet<(&str, Facet)> = BTreeSet::new(); + for entry in &self.entries { + if !seen_ids.insert(entry.id.as_str()) { + return Err(schema_error(format!("duplicate entry id `{}`", entry.id))); + } + if !seen_cells.insert((entry.input.as_str(), entry.facet)) { + return Err(schema_error(format!( + "duplicate cell for input `{}` facet `{}`", + entry.input, + entry.facet.as_str() + ))); + } + } + self.validate_sorted()?;Add a test that a duplicated row is rejected. Do you want me to open an issue to track the corresponding
enforce_no_silent_skiphardening?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for (index, entry) in self.entries.iter().enumerate() { entry.validate(index)?; } let mut seen_ids: BTreeSet<&str> = BTreeSet::new(); let mut seen_cells: BTreeSet<(&str, Facet)> = BTreeSet::new(); for entry in &self.entries { if !seen_ids.insert(entry.id.as_str()) { return Err(schema_error(format!("duplicate entry id `{}`", entry.id))); } if !seen_cells.insert((entry.input.as_str(), entry.facet)) { return Err(schema_error(format!( "duplicate cell for input `{}` facet `{}`", entry.input, entry.facet.as_str() ))); } } self.validate_sorted()?; self.validate_totals()?; Ok(()) }🤖 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/src/ts_ledger.rs` around lines 319 - 327, Update the ledger validation method around validate_sorted and validate_totals to reject duplicate entries by enforcing uniqueness of both entry id and the (input, facet) pair before validation succeeds. Add a focused test demonstrating that a duplicated row causes validation to fail, while preserving the existing sorting and totals checks.crates/bamts-verification/src/workspace_guard.rs (2)
240-245: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Your own crates are exempt from the version policy you just wrote.
validate_registry_versionat line 840 rejects wildcards, tildes, comparator ranges, leading zeros, and anything that is notX.Y[.Z]or=X.Y[.Z]. Fifty lines of policy.Then this function, for workspace-internal dependencies, checks only that the string is non-empty and contains no whitespace.
version = "*"passes.version = ">=0.1"passes.version = "~1.2"passes.A workspace guard that enforces strict pinning on third-party crates and shrugs at its own is not a guard. Reuse the validator.
🔧 Proposed fix
require_exact_string(attributes, "path", expected_path, &context)?; let version = required_string(attributes, "version", &context)?; - if version.is_empty() || version.chars().any(char::is_whitespace) { - return Err(workspace_error(format!( - "{context}: `version` must be a non-empty version requirement" - ))); - } - Ok(()) + validate_registry_version(name, version, &context) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require_exact_string(attributes, "path", expected_path, &context)?; let version = required_string(attributes, "version", &context)?; validate_registry_version(name, version, &context)🤖 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/src/workspace_guard.rs` around lines 240 - 245, Update the version validation in the workspace-internal dependency path to reuse validate_registry_version instead of only checking for emptiness and whitespace. Preserve the existing workspace_error context and ensure invalid wildcards, ranges, tildes, leading zeros, and unsupported formats are rejected consistently with registry dependencies.
1307-1319: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
require_enabled_featureignores which package you asked about.The signature reads
(closure, mode, feature). The body hardcodesclosure.package_features.get("bamts-codegen").modeis a free-form label used only in the error string.Look at the call sites. Line 1123 passes
"bamts-cli". Line 1221 passes"bamts-codegen/host-jit". One is a package name, one is a feature spec, and neither selects what gets looked up. Both queries hitbamts-codegenregardless.Today the assertions happen to be about codegen features, so the results are right by coincidence. The next person who writes
require_enabled_feature(&closure, "bamts-node", "script-compiler")will get abamts-codegenlookup and either a baffling error or a false pass. This is the identical defect already flagged incrates/bamts-verification/src/oracle_pins.rs— a parameter that pretends to be general over logic that is not.Take the package explicitly and keep the label for the message.
🔧 Proposed fix
-fn require_enabled_feature(closure: &ResolvedClosure, mode: &str, feature: &str) -> Result<()> { +fn require_enabled_feature( + closure: &ResolvedClosure, + mode: &str, + package: &str, + feature: &str, +) -> Result<()> { let active = closure .package_features - .get("bamts-codegen") - .ok_or_else(|| workspace_error("codegen closure lacks bamts-codegen features"))?; + .get(package) + .ok_or_else(|| workspace_error(format!("{mode} closure lacks `{package}` features")))?; if !active.contains(feature) { return Err(workspace_error(format!( - "{mode} metadata closure does not enable bamts-codegen feature `{feature}`" + "{mode} metadata closure does not enable `{package}` feature `{feature}`" ))); } Ok(()) }Call sites become, for example:
require_enabled_feature(&closure, "bamts-cli", "bamts-codegen", "aot")?; require_enabled_feature(&closure, "bamts-codegen/host-jit", "bamts-codegen", "host-jit")?;The two ad-hoc
package_features.get(...)blocks at lines 1224 and 1241 then collapse into this helper too.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn require_enabled_feature( closure: &ResolvedClosure, mode: &str, package: &str, feature: &str, ) -> Result<()> { let active = closure .package_features .get(package) .ok_or_else(|| workspace_error(format!("{mode} closure lacks `{package}` features")))?; if !active.contains(feature) { return Err(workspace_error(format!( "{mode} metadata closure does not enable `{package}` feature `{feature}`" ))); } Ok(()) }🤖 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/src/workspace_guard.rs` around lines 1307 - 1319, Update require_enabled_feature to accept an explicit package name separate from the display label, and use that package argument for the package_features lookup instead of hardcoding "bamts-codegen". Update all call sites, including the checks near the existing package_features.get blocks, to pass the intended package and feature while preserving mode for error messages; consolidate those duplicate checks through the helper.crates/bamts-verification/tests/corpus_differential.rs (1)
1291-1316: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fifteen tests hard-depend on
node_modules/.bin/tscwith no guard, and leak fixtures when it is absent.This block spawns
node_modules/.bin/tscand.expect("run TypeScript decorator oracle")on the spawn result. If dependencies are not installed, every one of these decorator, namespace, and resource-management tests panics with a rawNotFoundio error rather than a message that tells the developer to runnpm ci.The panic also happens before
fs::remove_file(source_path)at line 1342, so each failed run leaves a stray fixture intarget/.The pinned Node oracle already has a discovery seam —
NodeOracle::discoverreturns aResult. Thetscoracle has none. Give it the same treatment, or at minimum make the failure message name the missing tool and the fix.This repeats verbatim in
class_decorator_initializer_state_matches_tsc_oracle_in_every_execution_mode,invalid_class_decorator_return_matches_tsc_oracle_in_every_execution_mode,standard_member_decorators_match_tsc_oracle_in_every_execution_mode,merged_namespace_function_matches_tsc_oracle_in_every_execution_mode, and ten more. The identical fourteen-line.args([...])array is copy-pasted every time. Extract onefn transpile_with_tsc(root: &Path, source: &Path, out_dir: &str, extra_lib: Option<&str>) -> Result<(), String>helper and call it.🤖 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 1291 - 1316, Extract the repeated TypeScript compilation command into a transpile_with_tsc helper returning Result<(), String>, and update all affected decorator, namespace, and resource-management tests to use it. Handle missing node_modules/.bin/tsc through the Result with an actionable message directing developers to run npm ci, while ensuring each test removes its generated source fixture even when compilation fails.crates/bamts/src/lib.rs (2)
190-197: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Hardcoding
javascript_compatibility: trueis a policy decision baked into a public API.
compile_source_fileis the only compile entrypoint this crate exposes, and it forces JavaScript compatibility on for every caller. The CLI derives this flag from arguments (lower_options(args)incrates/bamts-cli/src/driver.rs), and the verification corpus decides it per case withcase_requires_javascript_compatibility. So the facade and the CLI can lower the same project differently, and a library user has no way to get strict lowering.Take the options as a parameter and keep
compile_source_fileas the defaulted wrapper.Proposed shape
pub fn compile_source_file_with( path: impl AsRef<Path>, options: LowerOptions, ) -> Result<bamts_compiler::program::ExecutableProgram> { /* current body, `options` passed through */ } pub fn compile_source_file( path: impl AsRef<Path>, ) -> Result<bamts_compiler::program::ExecutableProgram> { compile_source_file_with(path, LowerOptions { javascript_compatibility: true }) }🤖 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/src/lib.rs` around lines 190 - 197, Update the compile facade by introducing a configurable compile_source_file_with function that accepts LowerOptions and passes them to lower_program, then make compile_source_file a defaulted wrapper using javascript_compatibility: true. Preserve the existing return type and error mapping while allowing callers to request strict lowering.
222-231: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
run_programthrows away the runtime outcome. Output and exit code are wrong.The result of
bamts_runtime::runis discarded by?. Onlyhost.stdout()andhost.exit_code()reachProgramOutput.Both other consumers in this workspace merge the outcome:
crates/bamts-cli/src/driver.rs(lines 471-479) appendsoutcome.stdouttohost.stdout()and prefersoutcome.exit_codewhenhost.exit_code() == 0.crates/bamts-verification/src/corpus.rs(lines 940-948) does the same.So the facade silently drops runtime-produced stdout and reports exit code
0for a program that exits nonzero through the runtime outcome. Two of three call sites agree; this one is the odd one out. Fix it here, not by patching the callers.Proposed fix
- bamts_runtime::run( + let outcome = bamts_runtime::run( executable.wire(), &mut host, &bamts_runtime::Limits::default(), )?; - Ok(ProgramOutput { - stdout: host.stdout().to_vec(), - exit_code: host.exit_code(), - }) + let mut stdout = host.stdout().to_vec(); + stdout.extend_from_slice(&outcome.stdout); + let exit_code = if host.exit_code() == 0 { + outcome.exit_code + } else { + host.exit_code() + }; + Ok(ProgramOutput { stdout, exit_code })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let outcome = bamts_runtime::run( executable.wire(), &mut host, &bamts_runtime::Limits::default(), )?; let mut stdout = host.stdout().to_vec(); stdout.extend_from_slice(&outcome.stdout); let exit_code = if host.exit_code() == 0 { outcome.exit_code } else { host.exit_code() }; Ok(ProgramOutput { stdout, exit_code })🤖 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/src/lib.rs` around lines 222 - 231, Update run_program to capture the outcome returned by bamts_runtime::run instead of discarding it with ?. Merge outcome.stdout after host.stdout(), and use outcome.exit_code when host.exit_code() is zero; otherwise preserve the host exit code. Return these merged values in ProgramOutput.verification/diagnostic-code-map.json (1)
377-380: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the invalid JSON string.
Line 378 does not escape the quotes around
./f1. The JSON parser terminatesevidencebefore the module specifier. Escape both quotes.Proposed fix
- "evidence": "Import declaration conflicts with a local declaration -> TS2440 via tsc --noEmit on \"import {f} from "./f1"; function f() {}\".", + "evidence": "Import declaration conflicts with a local declaration -> TS2440 via tsc --noEmit on \"import {f} from \"./f1\"; function f() {}\".",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."bamtsCode": "BAMTS-C041", "evidence": "Import declaration conflicts with a local declaration -> TS2440 via tsc --noEmit on \"import {f} from \"./f1\"; function f() {}\".", "status": "mapped", "tsCode": 2440🤖 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 `@verification/diagnostic-code-map.json` around lines 377 - 380, Update the evidence value for BAMTS-C041 in the diagnostic code map to escape both double quotes surrounding ./f1, keeping the resulting file valid JSON and the message content unchanged.crates/bamts-compiler/src/checker/narrowing.rs (1)
396-407: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
type_atprojects the declared root, so root narrowing never reaches sibling members.The frame walk looks up the exact
FlowKey. If no frame holds that key, the fallback projectsself.facts.declared, not the narrowed root. Afterapply_guarddiscriminatesshapeto the circle variant,type_at(flow, shape.radius)still projects the declaredShapeunion.property_typethen returnsNonebecause the square variant has noradius, and the access becomes untrackable exactly where narrowing was supposed to make it trackable.Project the effective root type instead of the declared one: resolve the longest tracked prefix of the key, then project the remaining segments.
🐛 Proposed fix: project from the nearest tracked ancestor key
pub fn type_at(&mut self, flow: FlowNodeId, key: &FlowKey) -> Option<TypeId> { - let mut current = Some(flow); - while let Some(id) = current { - let frame = &self.facts.frames[id.index()]; - if let Some(ty) = frame.facts.get(key) { - return Some(*ty); - } - current = frame.parent; - } - let declared = self.facts.declared.get(&key.root_symbol()).copied()?; - self.project(declared, key.path()) + // Longest tracked prefix first: an exact fact, else the nearest + // refined ancestor, else the declared root. + for split in (0..=key.path().len()).rev() { + let prefix = FlowKey { + root: key.root_symbol(), + path: key.path()[..split].to_vec().into_boxed_slice(), + }; + let mut current = Some(flow); + while let Some(id) = current { + let frame = &self.facts.frames[id.index()]; + if let Some(ty) = frame.facts.get(&prefix).copied() { + return self.project(ty, &key.path()[split..]); + } + current = frame.parent; + } + } + let declared = self.facts.declared.get(&key.root_symbol()).copied()?; + self.project(declared, key.path()) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub fn type_at(&mut self, flow: FlowNodeId, key: &FlowKey) -> Option<TypeId> { // Longest tracked prefix first: an exact fact, else the nearest // refined ancestor, else the declared root. for split in (0..=key.path().len()).rev() { let prefix = FlowKey { root: key.root_symbol(), path: key.path()[..split].to_vec().into_boxed_slice(), }; let mut current = Some(flow); while let Some(id) = current { let frame = &self.facts.frames[id.index()]; if let Some(ty) = frame.facts.get(&prefix).copied() { return self.project(ty, &key.path()[split..]); } current = frame.parent; } } let declared = self.facts.declared.get(&key.root_symbol()).copied()?; self.project(declared, key.path()) }🤖 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/checker/narrowing.rs` around lines 396 - 407, Update type_at to resolve the effective root type from the longest tracked ancestor FlowKey before projecting the remaining path. Preserve the exact-key frame lookup, then walk key prefixes toward key.root_symbol() and use the nearest tracked type plus only the untracked suffix; fall back to the declared root when no ancestor is tracked.crates/bamts-compiler/src/lint.rs (1)
358-369: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Half the "clean" examples prove nothing.
const safe: number = 1;is not a clean counterexample formethod-parameter-bivariance. It is an unrelated statement. It demonstrates only that the rule does not fire on a line that has nothing to do with the rule. Roughly forty entries in this registry do the same thing: W006, W009, W010, W011, W012, W013, W014, W015, W017, W019, W020, W021, W022, W024, W025, W027, and on and on.You wrote a
sound_alternativefield for each of these rules. W001 says "Use a function-property callback with a contravariant parameter." The clean example should be exactly that, so the executable contract proves the advice you print to users actually silences the rule. Right now the catalog documents one fix and tests a different, empty one. W003, W016, W040, W041, W068, W069, and W070 already do this correctly, so the pattern exists; the rest were filled in with a placeholder and left.Replace each placeholder clean case with the source form named in that rule's
sound_alternative.🤖 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 358 - 369, Replace unrelated clean-example placeholders such as `const safe: number = 1;` throughout the lint rule registry with executable source that implements each rule’s documented `sound_alternative`, starting with the `BAMTS-W001` entry and its function-property callback using a contravariant parameter. Apply the same mapping to the other affected rule entries, preserving the existing warning examples and ensuring each clean example actually silences its associated rule.crates/bamts-compiler/src/literal.rs (2)
96-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
radix_valueaccumulates inf64and produces the wrong number.
value = value * radix + digitinf64rounds on every single addition. ECMAScript specifies the mathematical value of the digit sequence, rounded to a double exactly once. Once the accumulator passes 2^53 those per-step roundings stack up and you hand back a value that is off by one or more ulp from the correct result. The decimal path is fine because you delegate to Rust's correctly-roundedparse; the hex, octal, and binary paths do not get that courtesy.This is a silently wrong constant baked into the bytecode. No diagnostic, no crash, just a number that is not the number the programmer wrote.
Accumulate exactly in an integer and convert once.
🐛 Proposed fix
fn radix_value(digits: &str, radix: u32) -> Option<f64> { - let mut value = 0.0_f64; - let mut any = false; - for character in digits.chars().filter(|character| *character != '_') { - let digit = character.to_digit(radix)?; - value = value * f64::from(radix) + f64::from(digit); - any = true; - } - any.then_some(value) + let mut exact = 0_u128; + let mut overflowed = false; + let mut value = 0.0_f64; + let mut any = false; + for character in digits.chars().filter(|character| *character != '_') { + let digit = character.to_digit(radix)?; + any = true; + match exact + .checked_mul(u128::from(radix)) + .and_then(|scaled| scaled.checked_add(u128::from(digit))) + { + Some(next) if !overflowed => exact = next, + _ => { + if !overflowed { + overflowed = true; + value = exact as f64; + } + value = value * f64::from(radix) + f64::from(digit); + } + } + } + if !any { + return None; + } + Some(if overflowed { value } else { exact as f64 }) }The
u128path covers every literal a human will ever write and converts once, correctly rounded. Past 128 bits the value is already far beyondf64precision, so the fallback is harmless.🤖 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/literal.rs` around lines 96 - 105, Update radix_value to accumulate digits in an integer type such as u128, preserving underscore filtering, radix validation, and the empty-input None result; convert the completed integer to f64 only once before returning it, with an appropriate fallback for values exceeding the integer capacity.
111-132: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
\u{D800}may panic the compiler.Line 128 calls
push_code_point(value)for anyvalue <= 0x10_FFFF, and asserts with.expect("a bounded code point is representable"). That range includes the surrogate blockD800..=DFFF.\u{D800}is perfectly legal ECMAScript source and must produce the single UTF-16 unit0xD800.The sibling call at line 54 is justified with "a Rust char is a Unicode scalar", which strongly suggests
push_code_pointis a scalar-value API that refuses surrogates. If it is, legal user source panics the compiler instead of compiling. Your own test file covers\uD800(the non-brace path, which usespush_unitand is fine) and\u{1F603}. It does not cover\u{D800}. That is precisely the case that would blow up.Confirm the contract of
EcmaStringBuilder::push_code_point. If it rejects surrogates, route theD800..=DFFFrange throughpush_unitand add the missing test.Separately, when
value > 0x10_FFFFthe guard at line 127 drops the entire escape and emits nothing at all. Silent deletion is the worst recovery option available.#!/bin/bash # Description: Determine whether EcmaStringBuilder::push_code_point accepts lone surrogates. set -euo pipefail fd -t f 'string.rs' crates/bamts-bytecode | while IFS= read -r file; do echo "===== $file" rg -n -C 12 'fn push_code_point|fn push_unit|surrogate|D800|0xDFFF|0x10_FFFF|0x10FFFF' "$file" done # Structural view of the builder API surface. ast-grep outline crates/bamts-bytecode/src/string.rs --items all🤖 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/literal.rs` around lines 111 - 132, Update the braced escape handling in the literal parser to confirm EcmaStringBuilder::push_code_point’s scalar-value contract and route surrogate values D800..=DFFF through push_unit instead of push_code_point, preserving \u{D800} as one UTF-16 unit without panicking. Add coverage for the braced surrogate case, and replace the current value > 0x10_FFFF silent-drop behavior with the parser’s established invalid-escape recovery behavior.crates/bamts-compiler/src/namespace_plan.rs (1)
319-327: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
An unexported nested namespace is recorded as a runtime export of its parent.
The first match arm handles
export <declaration>and is correct. The second arm accepts a bareStatement::Namespacewith noexportin front of it, on the sole condition that its parent symbol matches. Nesting is not exporting.namespace A { namespace B { export const x = 1 } // B is local to A }TypeScript emits
Bas a local binding insideA's closure and never assignsA.B = B.A.Bdoes not exist. This arm routes thatBstraight into the arm at line 382, which records it withExportStorage::LocalAndPropertyand puts it inplan.exports— the list that drives property assignment on the container.Note also that the exported case is already covered:
export namespace B {}arrives asExport(Named(Declaration(inner))), matches the first arm, and reaches line 382 anyway. So this arm adds nothing for correct input and produces a wrong export for incorrect input. Delete it, or gate it on the enclosing statement actually being an export.#!/bin/bash # Description: Check how plan.exports drives container property assignment during lowering. set -euo pipefail rg -nP -C 10 '\bexports\s*\(\)|NamespaceExport|ExportStorage::(LocalAndProperty|Property)' --type=rust -g '!**/namespace_plan.rs'🤖 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/namespace_plan.rs` around lines 319 - 327, Remove the bare Statement::Namespace match arm in the namespace-planning match; only explicitly exported namespace declarations should reach the export-recording path. Preserve the first export-declaration arm and ensure unexported nested namespaces are not added to plan.exports or assigned as parent container properties.crates/bamts-compiler/src/parser.rs (3)
2118-2134: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This six-line block is copy-pasted seven times.
Build
KeywordContext, parse type parameters, parse the parameter list inside the context, parse the return type outside it, parse the body inside it. Identical at lines 2118-2134, 4344-4356, 4729-4744, 4952-4972, 4986-4994, 5024-5036, 5052-5064, and 5092-5104.Eight copies of one rule. When the rule changes — and it will, see the arrow
yield_reservedproblem — you get to find all eight. Or you find seven and ship the eighth.Extract a helper that takes
is_asyncandis_generatorand returns(type_parameters, parameters, return_type, body). The call sites that differ only in their missing-body handling can pass that in.🤖 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 2118 - 2134, Extract the repeated function-signature parsing logic into a shared helper accepting is_async, is_generator, and a callback for missing-body handling, returning type_parameters, parameters, return_type, and body. Centralize KeywordContext creation, contextual parameter parsing, non-contextual return-type parsing, and body parsing in this helper, then replace the duplicated flows near the parser’s function/arrow parsing call sites while preserving each site’s distinct missing-body behavior.
3910-3934: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Every JSX construct copies the entire rest of the file.
Line 3923:
SourceText::new(self.source.as_str()[start_byte..].to_owned()). That is a fresh heap allocation and memcpy of every byte from the current<to end of file, plus a fullSourceTextindex build (checkpoints and line starts) over all of it — and then you use the first few hundred bytes and throw the rest away.Do that once per JSX expression in a React file. A 200 KB component with 200 JSX expressions copies and indexes roughly 20 MB. The indexing is the expensive part:
SourceText::from_arcwalks every character to build boundary checkpoints.The regex rescan next door does not do this; it slices
&self.source.as_str()[start_byte..]and walks it directly with no allocation. The JSX path needs aSourceTextonly becauseScanner::newdemands one.Either give the scanner an entry point that takes a
&strplus a base offset, or bound the copied tail. Copying the whole file per element is not a strategy.#!/bin/bash # Description: Inspect Scanner::new and scan_jsx_span to see whether a borrowed-str entry point is feasible. set -euo pipefail ast-grep outline crates/bamts-compiler/src/scanner.rs --items all rg -nP -C 10 'fn new\s*\(|fn scan_jsx_span|SourceText' crates/bamts-compiler/src/scanner.rs🤖 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 3910 - 3934, Update rescan_jsx_span and the scanner entry point used by Scanner::new/scan_jsx_span to avoid constructing a full SourceText for the entire remaining file on each JSX rescan. Prefer a borrowed string slice with a base offset; otherwise limit the copied fragment to the maximum span needed while preserving token ranges and JSX scanning behavior.
5255-5262: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rest elements in assignment destructuring are silently dropped.
[a, ...rest] = arr;is ordinary, legal JavaScript. It arrives here asArrayElement::Spread, and you convert it toAssignmentArrayElement::Missing— with no diagnostic.The comment directly above says "diagnose and record a missing element rather than dropping it." There is no diagnostic. There is a blank line at 5258 where one used to be. So the code does exactly the thing the comment promises it does not do: it drops the element, quietly, and hands lowering a target that is missing a binding the programmer wrote.
Two acceptable outcomes. Either the assignment-target AST grows a rest slot and this works, or it emits a diagnostic saying the form is unsupported. Producing a silently wrong target is not one of them.
The same stale-comment-plus-deleted-diagnostic pattern sits at line 5187.
#!/bin/bash # Description: Check whether AssignmentArrayElement has a rest representation and how Missing is lowered. set -euo pipefail rg -nP -C 8 'enum AssignmentArrayElement|AssignmentArrayPattern' --type=rust rg -nP -C 6 'AssignmentArrayElement::Missing' --type=rust -g '!**/parser.rs'🤖 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 5255 - 5262, The ArrayElement::Spread branch currently converts assignment rest elements into Missing without reporting an error. Either add a rest representation to AssignmentArrayElement and preserve the binding through lowering, or emit the appropriate unsupported-form diagnostic before recording the missing element; also inspect the analogous handling near the other stale comment and ensure it does not silently discard rest targets.crates/bamts-compiler/src/pipeline.rs (2)
409-430: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A module edge that fails to map to a node is silently discarded.
Line 425:
specifier: nodes.node_for(edge.range())?. Inside afilter_map, that?means "drop this edge and say nothing."The resolver already decided this module depends on that module. If
node_forreturnsNone, the index and the resolver disagree about the source, and the checker is then handed an incomplete dependency graph. It will not report an error, because from its point of view the edge never existed. The user gets an unresolved import, or a type that resolves toany, with no explanation anywhere.
node_forfalls back tosmallest_containing, which returnsNoneonly when no root statement contains the range — meaning the resolver found a specifier outside every statement. That should be impossible. So make it loud:debug_assert!at minimum, and decide deliberately what release builds do.Separately, lines 413-417 do a linear
findoverfilesfor every module, giving O(M²) with an.expectto cover the failure. Both slices are built in the same order —parsedat line 327 iteratesprogram.modules()— sozipthem and both the quadratic scan and theexpectdisappear.🐛 Proposed fix
fn resolved_checker_edges( program: &ResolvedProgram, files: &[Recovered<SourceFile>], ) -> Vec<ResolvedModuleEdge> { program .modules() .iter() - .flat_map(|module| { - let source = files - .iter() - .find(|file| file.product().source_id() == module.source_id()) - .expect("resolved module has one parsed source") - .product(); + .zip(files) + .flat_map(|(module, file)| { + let source = file.product(); + debug_assert_eq!( + source.source_id(), + module.source_id(), + "parsed files must follow the resolved module order" + ); let nodes = SourceEdgeNodeIndex::new(source); module.dependencies().iter().filter_map(move |edge| { let ModuleTarget::Local(to) = edge.target() else { return None; }; + let specifier = nodes.node_for(edge.range()); + debug_assert!( + specifier.is_some(), + "a resolved dependency specifier must lie inside a statement" + ); Some(ResolvedModuleEdge { from: module.source_id(), - specifier: nodes.node_for(edge.range())?, + specifier: specifier?, to: *to, }) }) }) .collect() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.program .modules() .iter() .zip(files) .flat_map(|(module, file)| { let source = file.product(); debug_assert_eq!( source.source_id(), module.source_id(), "parsed files must follow the resolved module order" ); let nodes = SourceEdgeNodeIndex::new(source); module.dependencies().iter().filter_map(move |edge| { let ModuleTarget::Local(to) = edge.target() else { return None; }; let specifier = nodes.node_for(edge.range()); debug_assert!( specifier.is_some(), "a resolved dependency specifier must lie inside a statement" ); Some(ResolvedModuleEdge { from: module.source_id(), specifier: specifier?, to: *to, }) }) }) .collect()🤖 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 409 - 430, Update the module-edge construction around ResolvedModuleEdge to avoid silently dropping edges when SourceEdgeNodeIndex::node_for returns None: assert the resolver/index invariant and choose an explicit release-build behavior that preserves graph completeness. Also replace the per-module files.iter().find(...).expect lookup with a positional zip between program.modules() and the correspondingly ordered files collection, eliminating the quadratic scan and expect.
754-774: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
These duration assertions will flake.
totals.get(Phase::Scan) > Duration::ZEROfor a source that readsconst n: number = 1;. Scanning twenty characters takes nanoseconds. WhetherInstant::now()observes a nonzero delta depends entirely on the platform's clock granularity, and on Windows the default timer resolution is around 15 ms. A sub-microsecond phase can and will measure exactly zero.You have written a test that asserts the machine is slow. It will pass on your laptop and fail on someone's CI runner at 3 a.m., and whoever gets that build will spend an hour deciding it is "just flaky" — which is how real failures start getting ignored.
Assert what you actually mean: that the phase was recorded. If the telemetry API cannot distinguish "recorded zero" from "never recorded", give it a call count. Falling back to a larger workload only moves the flake threshold; it does not remove it.
#!/bin/bash # Description: Check whether TelemetryTotals exposes a per-phase call count as well as a duration. set -euo pipefail ast-grep outline crates/bamts-compiler/src/telemetry.rs --items all rg -nP -C 6 'fn get|struct .*Totals|count' crates/bamts-compiler/src/telemetry.rs🤖 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 754 - 774, Replace the duration-based `> Duration::ZERO` assertions in the telemetry test with assertions that verify each phase was recorded, including `Phase::Scan`, `Parse`, `Check`, and `Emit`. Inspect `TelemetryTotals` and its `get` API in `telemetry.rs` for the existing per-phase call-count mechanism; if available, assert those counts instead of elapsed durations while preserving the total wall-time assertion as appropriate.crates/bamts-compiler/src/program.rs (3)
1305-1308: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
JSX subtrees are skipped, so dynamic imports inside JSX get misclassified as type-only.
scan_expressionbails out onJsxElement,JsxFragment, andJsxSelfClosingElement. Children and attribute expressions are never traversed.import('./x')inside a.tsxbody is therefore never pushed asModuleEdgeKind::DynamicRuntime.The token fallback at lines 1013-1030 then picks up the same string literal and labels it
TypeOnly. That is not a cosmetic mislabel:
resolve_edgeswitches toResolutionFlavor::Typesand prefers a.d.tscandidate.runtime_modulesdrops the target from the eager closure.collect_raw_modulefilters type-only edges out of the wire program, while lowering still emitsImportDynamicfor the same specifier.Traverse JSX children and attribute expressions, or stop letting the token fallback claim every unclaimed
import(as type-only.Run this to confirm the lowering side still emits a dynamic-import instruction for a specifier that has no runtime edge:
#!/bin/bash # Find JSX expression containers in the syntax model and the dynamic-import lowering path. fd -e rs . crates/bamts-compiler/src | xargs rg -n -C4 'JsxElement|JsxExpression|JsxAttribute' -g '!**/tests/**' | head -80 rg -n -C6 'ImportDynamic' crates/bamts-compiler/src rg -n -C6 'MissingRuntimeEdge' crates/bamts-compiler/src🤖 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 1305 - 1308, The scan_expression handling for Expression::JsxElement, Expression::JsxFragment, and Expression::JsxSelfClosingElement currently skips nested expressions, allowing the token fallback to misclassify JSX dynamic imports as TypeOnly. Traverse JSX children and attribute expression containers through the existing expression scanner so import('./x') is recorded as ModuleEdgeKind::DynamicRuntime; preserve the existing token fallback for imports outside JSX.
1569-1578: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
parse_hexoverflows on long\u{...}escapes.
value * 16 + digitis uncheckedu32arithmetic, and the\u{...}branch at line 1548 feeds it every hex digit up to the closing brace. There is no length cap.Two outcomes, both bad. A debug build panics on overflow and takes the compiler down on attacker-supplied source. A release build wraps and can land on a different valid code point, which silently rewrites a module specifier before
push_code_pointever gets a chance to reject it. Do not hand the wire format a specifier that came out of an integer wrap.🐛 Proposed fix
fn parse_hex(bytes: &[u8]) -> Option<u32> { if bytes.is_empty() { return None; } bytes.iter().try_fold(0_u32, |value, byte| { char::from(*byte) .to_digit(16) - .map(|digit| value * 16 + digit) + .and_then(|digit| value.checked_mul(16)?.checked_add(digit)) }) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fn parse_hex(bytes: &[u8]) -> Option<u32> { if bytes.is_empty() { return None; } bytes.iter().try_fold(0_u32, |value, byte| { char::from(*byte) .to_digit(16) .and_then(|digit| value.checked_mul(16)?.checked_add(digit)) }) }🤖 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 1569 - 1578, Update parse_hex to use checked u32 accumulation for each hexadecimal digit, returning None when value * 16 + digit overflows. Preserve the existing None result for empty or invalid input so oversized \u{...} escapes are rejected before reaching push_code_point.
2523-2562: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The cycle-pruned result is memoized. That cache is unsound.
Line 2530 returns an empty set when
(module_index, name)is already on thevisitedpath. Line 2560 then caches whatever partial set the traversal produced under that samevisitedpath, and every later query reuses it with a differentvisitedset.
expand_star_exportsacts oncandidates.len() == 1. An under-populated cache entry makes an ambiguous name look unique, so the linker materializes an export that TypeScript would omit. The same entry can also drop a legitimate re-export. Both call sites (lines 2463 and 2495) share onecache, so the selection pass reads entries built during the discovery pass.Either key the cache on the pruning context, or only cache entries whose traversal completed without hitting the
visitedguard.
program_lowering_terminates_cyclic_star_reexports_without_pollutionat line 3559 does not catch this. It uses a two-module cycle where both names resolve locally.Run this to see whether any test covers a cyclic star graph in which a name is reachable through two distinct origins:
#!/bin/bash rg -n -B2 -A25 'fn program_lowering_.*star' crates/bamts-compiler/src/program.rs🤖 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 2523 - 2562, Update star_export_origins so cycle-pruned results are not reused across different traversal contexts: either include the visited/pruning context in the cache key or cache only results from traversals that never hit the visited guard. Ensure the shared cache used by expand_star_exports cannot turn ambiguous cyclic re-exports into unique candidates or discard valid origins, and add coverage for a cyclic star graph reachable through distinct origins.crates/bamts-compiler/src/rules/semantic/modules.rs (1)
6-53: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rule identities are bare strings in both semantic-rule files.
lint.rsprivatizesRuleDefinition's fields specifically so that "a code or slug cannot exist without the rest of its metadata." Both files then handemita&'static strliteral, which discards that guarantee: a typo compiles, and renumbering a rule inRULESsilently desynchronizes the emitted code from the registry that owns it. One fix covers both — take the identity from the registry instead of retyping it.
crates/bamts-compiler/src/rules/semantic/modules.rs#L6-L53: replace the six literals"BAMTS-W028","BAMTS-W031","BAMTS-W032","BAMTS-W035","BAMTS-W037", and"BAMTS-W086"with registry lookups, and changeemitto accept aRuleIdor&'static RuleDefinition.crates/bamts-compiler/src/rules/semantic/object_types.rs#L6-L37: apply the same change to"BAMTS-W008","BAMTS-W009","BAMTS-W015", and"BAMTS-W016", and extend it to the remainingrules/semantic/*.rsmodules that follow this pattern.📍 Affects 2 files
crates/bamts-compiler/src/rules/semantic/modules.rs#L6-L53(this comment)crates/bamts-compiler/src/rules/semantic/object_types.rs#L6-L37🤖 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/modules.rs` around lines 6 - 53, Update emit and the semantic rule modules so diagnostics obtain their codes from the registry’s RuleId or RuleDefinition instead of bare string literals. In crates/bamts-compiler/src/rules/semantic/modules.rs lines 6-53, replace the six listed codes; in crates/bamts-compiler/src/rules/semantic/object_types.rs lines 6-37, replace BAMTS-W008, W009, W015, and W016; apply the same pattern to remaining matching rules/semantic/*.rs files while preserving each rule’s registry metadata.crates/bamts-runtime/src/builtins/array.rs (1)
179-189: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Array.fromnow rejects every array-like that lacksSymbol.iterator.
machine.iterable_values(source)is the only path. ECMA-262 §23.1.2.1 selectsGetMethod(items, @@iterator)first and falls back to the array-like path when that method isundefined. SoArray.from({ length: 2 })must produce[undefined, undefined], andArray.from({ length: 2, 0: "a", 1: "b" })must produce["a", "b"]. This code throws instead.The test at lines 1077-1086 asserts the throw. That test locks in the wrong behavior; it must assert the length-based result.
A second, smaller deviation sits in the same function. The iterator is drained completely before the first mapper call. The spec interleaves
IteratorStepand the mapper call, and it performsIteratorClosewhen the mapper throws. A mapper that throws on element 0 here has already consumed the whole iterator.Add the array-like fallback, and interleave the mapper with iteration.
🤖 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-runtime/src/builtins/array.rs` around lines 179 - 189, Update the Array.from implementation around iterable_values to first use the iterator when Symbol.iterator is present, otherwise fall back to the array-like length and indexed properties, preserving undefined for missing elements. Refactor iteration so each value is mapped immediately as it is consumed rather than draining the iterator first, and close the iterator if the mapper throws. Update the existing test that expects a throw for { length: 2 } to assert the length-based result, including indexed values.crates/bamts-runtime/src/builtins/collections.rs (2)
87-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
WeakMapandWeakSetprototypes have noSymbol.toStringTag, butMapandSetdo.Look twelve lines up.
install_mapandinstall_setboth calldefine_to_string_tag.install_weak_mapandinstall_weak_setdo not. ECMA-262 §24.3.3.5 and §24.4.3.5 require"WeakMap"and"WeakSet"with{ [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true }— the same shape you already produce for the strong collections.The result:
Object.prototype.toString.call(new WeakMap())returns"[object Object]". Every other engine returns"[object WeakMap]". Feature-detection and debug output both break.🐛 Proposed fix
define_data(heap, prototype, name, function); } + let tag = super::super::push(heap, HeapEntry::String(EcmaString::from_utf8("WeakMap"))); + define_to_string_tag(heap, prototype, builtins.symbol_to_string_tag(), tag); globals.insert(EcmaString::from_utf8("WeakMap"), constructor); }Apply the same two lines to
install_weak_setwith"WeakSet".🤖 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-runtime/src/builtins/collections.rs` around lines 87 - 124, Update install_weak_map and install_weak_set to call define_to_string_tag on their prototypes with "WeakMap" and "WeakSet" respectively, matching install_map and install_set and preserving the required property attributes.
546-562: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Every single delete rebuilds the entire hash index. That is O(n) per delete.
entries.remove(entry_index)shifts the tail, so all indices after the hole change. The code answers this by discardingindexand re-hashing every surviving entry, callingcollection_key_hashn times.Delete n entries one at a time and you pay O(n²) hashes plus O(n²) memmove. A 100k-entry
Mapdrained in a loop — the shape every LRU cache and everyfor (const k of map.keys()) map.delete(k)produces — does 5 billion hash operations. That is not a micro-optimization concern; it is a quadratic blowup on the most ordinaryMapusage there is.The
CollectionEntrystruct already carries alive: bool, anditerator_nextandcollection_nextalready filter on it. Use it. Tombstone the entry, decrementsize, remove only that key fromindex, and compact when the dead fraction crosses a threshold. Then delete is amortized O(1) and the iterator logic you already wrote keeps working unchanged.Note that tombstoning also removes the index-invalidation problem entirely, because surviving entries keep their positions.
🤖 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-runtime/src/builtins/collections.rs` around lines 546 - 562, Update the collection deletion logic around entries.remove and index rebuilding to tombstone the targeted CollectionEntry by setting live to false, decrement size, and remove only its key from the existing index. Stop shifting entries, rehashing survivors, and rebuilding CollectionIndex on each delete; retain the existing iterator_next and collection_next live-entry filtering, and add compaction only when the dead-entry fraction crosses an appropriate threshold.crates/bamts-runtime/src/builtins/date.rs (1)
57-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The one-argument path skips
ToPrimitive, so anything that is not a literal string heap entry goes straight to number coercion.Trace it. Line 68 calls
machine.string_value(value). That succeeds only forHeapEntry::String. Everything else falls to line 71 andcoerce_number_observable.ECMA-262 §21.4.2.1 step 4b is explicit: after the
[[DateValue]]check, letv = ToPrimitive(value); then branch on whethervis a String.So these all produce
Invalid Datehere and correct dates everywhere else:new Date(new String("2024-01-01")); new Date({ toString() { return "2024-01-01"; } }); new Date(["2024-01-01"]);The array case is the one that will actually bite, because
ToPrimitiveon a one-element array yields its string form. CallToPrimitiveonce, branch on the result, and coerce the number from the same primitive rather than from the original object.🤖 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-runtime/src/builtins/date.rs` around lines 57 - 72, Update the one-argument path in the date constructor around copied_time to apply ToPrimitive once after the HeapEntry::Date check. Branch on whether that primitive is a string and parse it with parse_iso_date; otherwise pass the same primitive to value_number and coerce_number_observable, preserving the existing time_clip behavior.crates/bamts-runtime/src/builtins/json.rs (1)
136-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The
spacebranch treats every heap value as a string, soJSON.stringify({}, null, {})throws.
Some(Decoded::HeapRef(_))funnels everything intomachine.to_string(...). For an ordinary objectto_stringgoes throughto_primitive, which throwsTypeErrorin this runtime. ECMA-262 §25.5.2.1 steps 5-7 say the opposite: only a[[NumberData]]wrapper takes the numeric path, only a[[StringData]]wrapper takes the string path, and any other value leavesgapas the empty string.Two visible defects follow:
JSON.stringify(value, null, {})throws instead of returning compact JSON.JSON.stringify(value, null, new Number(4))indents with the string"4"instead of four spaces.The first one is the dangerous one. A
TypeErrorescapingJSON.stringifyfor a harmless third argument is not a formatting nit.Unbox first, then branch on the unboxed primitive's type, and fall through to
EcmaString::default()for everything that is neither a number nor a string.🤖 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-runtime/src/builtins/json.rs` around lines 136 - 149, Update the space handling branch in the JSON.stringify implementation to unbox heap values before selecting indentation behavior. Apply the numeric path only to unboxed numbers, the string path only to unboxed strings, and leave gap as EcmaString::default() for ordinary objects and all other values; preserve the existing length limits.
Type-parameter symbols were collected by walking parameter types then the return type, and the caller's explicit arguments were indexed by position in that list. For `f<T, U>(u: U, t: T)` the occurrence order is `[U, T]`, so `f<number, string>(...)` bound them swapped and every argument check and the call's return type were computed against the wrong types. The declared list is authoritative and is now used directly. Also extracts the parameter-lowering block that `signature_type` and the function-declaration binding site held as byte-identical copies.
`readonly` survived widening but not instantiation, so whether a readonly-alias rule fired depended on a generic sitting in the middle.
…meter A compiler must not panic on source input. The lookup now degrades to a diagnostic, which is the recovery contract the rest of the file documents.
Three defects in the dependency scan: `parse_hex` multiplied unchecked, so a specifier carrying more than eight hex digits panicked in debug and wrapped in release. Attacker-supplied source text must not do either. The JSX arms of the dynamic-edge collector returned without visiting attributes or children, so `import()` inside JSX was classified by the token fallback as type-only. `star_export_origins` cached results truncated by its own visit set, keyed without the path that truncated them. Truncated results are no longer cached.
`jsx_spans` was not restored on rollback, so a speculated-and-discarded JSX rescan still suppressed lexical diagnostics. It now truncates alongside `diagnostics` and `journal`. Also drops a parameter `parse_function_type_signature` never read together with the six-arm match one caller built to feed it, and corrects comments that claimed diagnostics the parser no longer emits.
`emit_enum_scalar` wrote straight to the sink, so `pending_indent` was never flushed and an inlined const-enum member lost its leading indentation. The two direct writes in `emit_enum_string` were only incidentally safe for the same reason and now go through the same path.
The binary search over edge children assumed sortedness that only a `debug_assert` enforced, so release builds searched a possibly-unsorted list. The children are now sorted where they are built, which costs one sort per statement and makes the search sound in every profile.
C042/C043 fired for relational, equality, `in` and `instanceof` operators, which are not arithmetic and cannot produce the condition the diagnostics describe. Also parenthesizes a `||`/`&&` guard to match the grouping its parenthesized sibling sixteen lines above already spells out.
`can_complete_normally` had no `Switch` arm and fell through to `true`, so W065 fired on a function whose every case returns.
The walk handled about half the statement kinds and ignored the rest behind a wildcard, so calls inside loops, `switch`, `try`, labels and default exports were invisible and W072/W010 fired falsely. The match is now exhaustive, so adding a statement kind is a compile error rather than a silent gap. Also collapses two spellings of the same CommonJS-shadow check, one of them a double negative, into a single named predicate.
The clamp was keyed on a hard-coded rule code, so any other deny-level JavaScript-compatibility rule was silently downgraded to a warning in `.js` sources. It now keys on the effective level and the rule's group.
The JSX span scanner had no test asserting the gap-free tiling and forward progress the default pass already asserts.
A `filter_map` dropped examples whose source declared a resolution but carried no import or export, so a malformed fixture silently reduced coverage.
Script input reached an `expect`, so a hostile string aborted the whole runtime instead of throwing. Non-ASCII input is now rejected at the guard and an invalid pair surfaces as a TypeError.
Slot zero cannot occur — `SlotId` holds a `NonZeroU32` and `from_parts` rejects zero — but the code subtracted one unconditionally. The invariant is now named and the subtraction is checked, so a future violation cannot turn into a wrapped index.
`options` was validated and discarded, so a caller-supplied `timeout` was silently ignored. The runtime has no enforcement point for it, and silently accepting a safety-relevant option is worse than refusing it.
![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: 5164733490
ℹ️ 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".
Finish the checker, runtime, native Node API, AOT/JIT validation, corpus gates, formal bounds, package publishing, and platform release paths. Resolve all open review contracts and harden Windows native artifact staging against local cache substitution.
💡 Codex ReviewbamTiScript/crates/bamts-compiler/src/checker/binder.rs Lines 10680 to 10684 in 28f31fa  Include variadic spreads in generic inference
When an array is spread into a call, bamTiScript/crates/bamts-compiler/src/checker/binder.rs Lines 11153 to 11155 in 28f31fa Preserve tuple positions when checking rest arguments
For a tuple-typed rest parameter, Compare every tuple-rest position in function relations
The comparison bound counts syntactic parameters, so a tuple-typed rest parameter contributes only one position even though ℹ️ 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". |
# Conflicts: # Cargo.lock # Cargo.toml # corpus/specs/valita.toml # crates/bamts-bytecode/src/lib.rs # crates/bamts-bytecode/src/program.rs # crates/bamts-bytecode/src/string.rs # crates/bamts-cli/build.rs # crates/bamts-cli/src/args.rs # crates/bamts-cli/src/diagnostics.rs # crates/bamts-cli/src/driver.rs # crates/bamts-cli/src/main.rs # crates/bamts-cli/tests/cli.rs # crates/bamts-codegen/Cargo.toml # crates/bamts-codegen/src/aot.rs # crates/bamts-codegen/src/jit.rs # crates/bamts-codegen/src/jit_memory.rs # crates/bamts-codegen/src/lib.rs # crates/bamts-compiler/Cargo.toml # crates/bamts-compiler/RULES.md # crates/bamts-compiler/src/checker.rs # crates/bamts-compiler/src/checker/intrinsic_environment.rs # crates/bamts-compiler/src/emitter.rs # crates/bamts-compiler/src/enum_plan.rs # crates/bamts-compiler/src/lint.rs # crates/bamts-compiler/src/literal.rs # crates/bamts-compiler/src/lower.rs # crates/bamts-compiler/src/namespace_plan.rs # crates/bamts-compiler/src/parser.rs # crates/bamts-compiler/src/pipeline.rs # crates/bamts-compiler/src/program.rs # crates/bamts-compiler/src/rules/mod.rs # crates/bamts-compiler/src/rules/semantic/mod.rs # crates/bamts-compiler/src/scanner.rs # crates/bamts-compiler/src/script.rs # crates/bamts-compiler/tests/rules.rs # crates/bamts-native/src/native_bridge.rs # crates/bamts-node/src/lib.rs # crates/bamts-node/src/timers.rs # crates/bamts-runtime/src/builtins/array.rs # crates/bamts-runtime/src/builtins/collections.rs # crates/bamts-runtime/src/builtins/date.rs # crates/bamts-runtime/src/builtins/json.rs # crates/bamts-runtime/src/builtins/mod.rs # crates/bamts-runtime/src/builtins/number.rs # crates/bamts-runtime/src/builtins/object.rs # crates/bamts-runtime/src/builtins/promise.rs # crates/bamts-runtime/src/builtins/regexp.rs # crates/bamts-runtime/src/builtins/string.rs # crates/bamts-runtime/src/builtins/symbol.rs # crates/bamts-runtime/src/builtins/test_support.rs # crates/bamts-runtime/src/builtins/timers.rs # crates/bamts-runtime/src/builtins/uint8array.rs # crates/bamts-runtime/src/external_modules.rs # crates/bamts-runtime/src/gc.rs # crates/bamts-runtime/src/host_objects.rs # crates/bamts-runtime/src/intrinsics.rs # crates/bamts-runtime/src/lib.rs # crates/bamts-runtime/src/native.rs # crates/bamts-runtime/src/regexp.rs # crates/bamts-runtime/src/vm.rs # crates/bamts-verification/src/corpus.rs # crates/bamts-verification/src/workspace_guard.rs # crates/bamts-verification/tests/corpus_differential.rs # crates/bamts/Cargo.toml # crates/bamts/src/lib.rs # formal/lean/Bamti/Bytecode/Model.lean # formal/lean/Bamti/Bytecode/Verify.lean # formal/lean/Bamti/JitLifecycle.lean # npm/test/bamti-cli.test.mjs # package-lock.json # package.json # proof/completeness-ledger.json # verification/manifest.lock.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45f28e620b
ℹ️ 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".
| const packagedBinary = join(packageDirectory, "bin", artifact.binary); | ||
| const directory = artifactDirectoryName(fixed.package); | ||
| const packageDirectory = join(stagingRoot, directory); | ||
| await cp(join(npmRoot, "artifacts", directory), packageDirectory, { |
There was a problem hiding this comment.
In the checked .github/workflows/cli-release-build.yml “Package CLI leaf” step, every target reaches this copy with directory derived from an unscoped package name such as bamti-cli-linux-x64, but the repository-wide artifact search shows the checked-in templates are under npm/artifacts/cli-linux-x64 (and equivalent cli-* directories for the other targets). Consequently cp raises ENOENT for all five matrix jobs before any leaf can be packed; preserve the existing package-to-template directory mapping instead of treating the package name as the directory name.
Useful? React with 👍 / 👎.
| Object.freeze({ | ||
| selector: "linux-x64", | ||
| target: "x86_64-unknown-linux-gnu", | ||
| package: "@bamti/cli-linux-x64", |
There was a problem hiding this comment.
When the CLI release reaches the preflight step in .github/workflows/cli-release-build.yml, package-platform.mjs generates release-table rows using the unscoped names from npm/bamti-cli/index.js (for example bamti-cli-linux-x64), matching the facade's optional dependencies and leaf manifests. This separate preflight target table instead requires @bamti/cli-linux-x64, so validateCliReleaseTable rejects every otherwise valid generated table with “wrong package”; align this table with the package identities actually emitted by the builder.
Useful? React with 👍 / 👎.
| stagedBinaries.set(stagedPath, dirname(stagedPath)); | ||
| return stagedPath; |
There was a problem hiding this comment.
When callers use the exported resolveBinary() API directly rather than run(), each successful call creates a new temporary directory and executable here, records it in the module-level map, and returns without any public cleanup path or process-exit cleanup. cleanupStaged() is private and is invoked only by run()'s child handlers, so tools that repeatedly resolve and spawn the binary themselves leak both memory in stagedBinaries and files under the system temporary directory; expose a disposal mechanism or otherwise clean direct resolutions after their lifetime.
Useful? React with 👍 / 👎.
| This is the reclamation claim; it holds only under the narrower hypothesis | ||
| `p = .Writable`, whereas `provider_finalization_failure_never_writable` holds | ||
| for any starting phase. -/ | ||
| theorem provider_finalization_failure_frees (q : ProviderPhase) |
There was a problem hiding this comment.
Any formal-gate run after this theorem change fails before it can validate the proofs: proof/lean-assumptions.json was not updated, so its source_sha256 still describes the previous Lean sources and its public theorem list still names the removed Bamti.provider_finalization_failure_reclaims; the newly generated verification/manifest.lock.json also retains Bamti/JitLifecycle.lean::provider_finalization_failure_reclaims. validate_lean_assumptions recomputes the digest and requires every inventoried theorem to occur exactly once, so regenerate both the assumptions evidence and formal catalog for the new provider_finalization_failure_frees theorem.
Useful? React with 👍 / 👎.
1 participant
Carries the local checker work onto
main, and fixes every checker false positive that made the corpus differential red.Checker module split
checker.rswas one file. It becomes an entry-point module overbinder(scope tree, symbol table, type table),relations(assignability, subtyping, variance),inference(type-parameter inference, contextual signatures, priorities),narrowing(control-flow narrowing),jsx, andintrinsic_environment.Generic call types survive interning
FunctionParametergainsname;FunctionSignaturegainstype_parameters, both with manualHash/Eqso structural interning still dedups.resolve_functioncomputes its return type after the body binds, so an un-annotated function no longer types asvoid.signature_typethreads type parameters through and extracts parameter names from binding patterns.Six false positives, and the rule that replaced each
(a, b) => …never satisfied(...args: any[]) => any; a rest parameter's array type was compared against the argumentthis.hook(name, …)compared the callee's own type parameter against the caller'snarrowing.rswas complete but unreachable — nothing drove itifand conditional expressions fork a frame per branch, apply guards, and merge. A branch control cannot fall out of contributes nothing, so an earlyreturnleaves the negated guard in force<T>(x: T) => Tvs<U>(x: U) => Ucompared type parameters nominallyRecord<string, unknown>into a target no structural source could satisfyObjectis modelled nominally; the rest keep the permissive error type until modelledAlso fixed:
abstractmembers are no longer treated as overload signatures needing an implementation; class constructors exposeprototype; assignment type-checking is gated on TypeScript sources, since a.mjsfile has no annotation to violate.Constructs the narrowing walk does not model record no facts and fall back to the declared type, so the wiring is conservative by construction.
Verification
Run on this branch at
8082591:cargo check --workspace --all-targetscargo clippy --workspace --all-targets -- -D warningscargo fmt --all -- --checkcargo test --workspacecargo test -p bamts-compiler --libcargo test -p bamts-verification --test corpus_differentialabe934e)ledger verify --gate G0corpus/cases/*.tsunder--error-limit 5000The raised diagnostic limit matters: the default caps rendering at 50, and valita emits 101 diagnostics, so its last hard error was invisible under the default and had to be found past the cap.
Note on history
verification/ts-suite-ledger.jsonis not in this branch and is now git-ignored: a 169 MB generated artifact that exceeds the 100 MB remote object limit, which is why the original branch could not be pushed.bamts-verificationregenerates it on demand.