rm: remove specialized string opcode - #5
Merged
Conversation
Deploying lk-lang with
|
| Latest commit: |
a752812
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://301fb021.lk-d8q.pages.dev |
| Branch Preview URL: | https://rm-string-starts-with-opcode.lk-d8q.pages.dev |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details🔇 Additional comments (1)
📝 WalkthroughWalkthrough此PR完整移除了LK VM中的 ChangesStringStartsWith Opcode全栈移除
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/vm/compiler/tests/call_intrinsics.rs (1)
311-329: ⚡ Quick win保留对外层 direct call 仍被内联的断言。
这个新用例现在只验最终结果，覆盖不到“移除
StringStartsWith后，外层直接函数调用没有退化成Call/CallDirect”这个编译层契约。这里继续检查编译后的入口指令形状，会更容易抓到优化回退。🤖 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 `@core/src/vm/compiler/tests/call_intrinsics.rs` around lines 311 - 329, The test compiler_runs_direct_function_with_string_method only asserts the final result but must also assert that the outer direct call remains inlined (i.e., the compiled entry uses CallDirect and not Call) after the StringStartsWith intrinsic is removed; update the test to compile the program (or obtain its compiled entry) and add an assertion that the entry instruction/opcode for the price call is CallDirect (or the equivalent direct-call opcode used by the compiler), referencing the test function name compiler_runs_direct_function_with_string_method and the StringStartsWith intrinsic so the check fails if the compiler falls back to Call/CallDirect degradation.core/src/vm/artifact.rs (1)
16-16: ⚡ Quick win补一条旧版本 artifact 的拒绝测试。
这次版本号升级是在防止旧指令流按新的 opcode 表被静默误解码；当前只有 round-trip 正例，没有覆盖
version=2必须失败的回归场景。可直接补一条很小的回归测试
#[test] fn module_artifact_round_trips_compiled_module() { let source = "fn f(x) { return x + 1; }\nreturn f(3);\n"; let tokens = crate::token::Tokenizer::tokenize(source).expect("tokenize"); let program = crate::stmt::StmtParser::new(&tokens).parse_program().expect("parse"); let module = Compiler::compile_module(&program).expect("compile"); let imports = vec![ImportStmt::Items { items: vec![crate::stmt::import::ImportItem { name: "abs".to_string(), alias: None, }], source: ImportSource::Module("math".to_string()), }]; let artifact = ModuleArtifact::new(imports.clone(), &module).expect("artifact"); let json = artifact.to_json_string().expect("json"); let decoded = ModuleArtifact::from_json_str(&json).expect("decode"); assert_eq!(decoded.imports, imports); let decoded_module = decoded.into_module().expect("module"); assert_eq!(decoded_module.entry, module.entry); assert_eq!(decoded_module.globals, module.globals); assert_eq!(decoded_module.functions.len(), module.functions.len()); assert_eq!(decoded_module.functions[0].code, module.functions[0].code); } + +#[test] +fn module_artifact_rejects_previous_version() { + let module = Module::single(Function::default()); + let json = ModuleArtifact::new(Vec::new(), &module) + .expect("artifact") + .to_json_string() + .expect("json") + .replace(r#""version": 3"#, r#""version": 2"#); + + assert!(ModuleArtifact::from_json_str(&json).is_err()); +}🤖 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 `@core/src/vm/artifact.rs` at line 16, Add a regression test that ensures old artifact version 2 is rejected: create a minimal serialized artifact blob whose header version field is 2 (instead of MODULE_ARTIFACT_VERSION) and assert that the artifact decoder (e.g., ModuleArtifact::deserialize or the crate's public artifact deserialization function) returns an Err or panics as appropriate; place the test alongside existing round-trip tests so it fails if older versions are silently accepted. Make sure the test references MODULE_ARTIFACT_VERSION to show intent and to construct a contrasting invalid version value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/src/vm/artifact.rs`:
- Line 16: Add a regression test that ensures old artifact version 2 is
rejected: create a minimal serialized artifact blob whose header version field
is 2 (instead of MODULE_ARTIFACT_VERSION) and assert that the artifact decoder
(e.g., ModuleArtifact::deserialize or the crate's public artifact
deserialization function) returns an Err or panics as appropriate; place the
test alongside existing round-trip tests so it fails if older versions are
silently accepted. Make sure the test references MODULE_ARTIFACT_VERSION to show
intent and to construct a contrasting invalid version value.
In `@core/src/vm/compiler/tests/call_intrinsics.rs`:
- Around line 311-329: The test compiler_runs_direct_function_with_string_method
only asserts the final result but must also assert that the outer direct call
remains inlined (i.e., the compiled entry uses CallDirect and not Call) after
the StringStartsWith intrinsic is removed; update the test to compile the
program (or obtain its compiled entry) and add an assertion that the entry
instruction/opcode for the price call is CallDirect (or the equivalent
direct-call opcode used by the compiler), referencing the test function name
compiler_runs_direct_function_with_string_method and the StringStartsWith
intrinsic so the check fails if the compiler falls back to Call/CallDirect
degradation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 036cb4e4-ee48-4684-b037-be61c8c63fc2
📒 Files selected for processing (22)
OPCODE.mdSTATUS.mdcore/src/llvm/callee_eval.rscore/src/llvm/scalar/block_helpers.rscore/src/llvm/scalar/blocks.rscore/src/llvm/scalar/blocks/string_methods.rscore/src/llvm/scalar/facts.rscore/src/llvm/scalar/inline.rscore/src/llvm/straightline_main.rscore/src/llvm/straightline_value.rscore/src/llvm/tests/strings.rscore/src/vm/artifact.rscore/src/vm/compiler/call.rscore/src/vm/compiler/tests/call_intrinsics.rscore/src/vm/exec.rscore/src/vm/exec/dispatch.rscore/src/vm/exec/exec_tests/container.rscore/src/vm/exec/value_ops.rscore/src/vm/ir.rshandoff.mdplan-progress.mdplan.md
💤 Files with no reviewable changes (16)
- handoff.md
- plan.md
- core/src/llvm/straightline_value.rs
- core/src/llvm/scalar/blocks/string_methods.rs
- OPCODE.md
- plan-progress.md
- STATUS.md
- core/src/vm/exec.rs
- core/src/llvm/scalar/blocks.rs
- core/src/vm/exec/value_ops.rs
- core/src/vm/exec/exec_tests/container.rs
- core/src/vm/exec/dispatch.rs
- core/src/llvm/scalar/block_helpers.rs
- core/src/vm/compiler/call.rs
- core/src/llvm/scalar/facts.rs
- core/src/llvm/tests/strings.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
core/src/vm/ir.rs (1)
227-232: LGTM!Also applies to: 341-346
lollipopkit
added a commit
that referenced
this pull request
Jul 7, 2026
…trait prescan 契约锁 review PR #17 发现的三项修复: 1. **chan.rs 持锁 raise → 永久死锁**(最重):raise 走 longjmp 跳过 Rust drop,存活的 MutexGuard 永不解锁。两处: - channel() 的 "Channel not found" raise 时 registry 全局锁 guard 还在 match 临时值里活着(**当前可达**:try{recv(999)}catch 后 一切 channel 操作死锁)→ 先 cloned() 出 Option 再 raise; - select send 臂在持 ChanInner state 锁时调用可 raise 的 own() (当前 tag 集下 latent)→ armed send 载荷进循环前预深拷, kind 预验证,循环内 fallback 改 unreachable。 blocking send/recv 本就遵守 drop-before-raise 纪律——正是这纪律 证明这两处是疏漏。纪律成文进 CLAUDE.md lkrt 段。 2. **trait prescan 形状契约锁**(J1 计划欠账):examples 差分对 unsupported 静默 skip,compiler 改 impl 注册序列发射形状会静默 丢 trait 覆盖。新 differential_trait_dispatch_contract 走 run_differential(硬性要求 lower 成功)——形状漂移即红。 3. **CLAUDE.md**:lkrt 边界规则补 dev-dep lk-core(仅 order- conformance 测试)例外条款 + 锁跨 raise 硬规则。 新差分 ×3:trait 契约、chan 未知 id catch 后可用、select 闭 send catch 后可用(2/3 是死锁回归测试,修前会挂死)。 留档不做(review findings #3/#5/#6):硬编码名单收拢进单表、 fixpoint 快照 clone 优化、select spin-poll 换 Condvar 多路等待。 门禁:50/51 · 差分 12/12 · workspace 0 · fuzz 150 · -D warnings all-features 0 · clippy/fmt 0 · bench 见下
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit