|
| 1 | +# 2026-09-17 — Capture order, and a class file that killed the process |
| 2 | + |
| 3 | +`taskId: rustjava-adopt-link-stringconcatfactory-p2-fix` · |
| 4 | +gate-2 request-changes follow-up for PR #61 (pin `87ef6a70`) |
| 5 | + |
| 6 | +Two findings, both the reviewer's, both correct. |
| 7 | + |
| 8 | +## F1 — the capture *order* was unobservable |
| 9 | + |
| 10 | +The reviewer reversed one line in `LambdaBody::call` (`&self.captures` → `.iter().rev()`) and the |
| 11 | +whole suite stayed green at **576 / 0**. Not a refusal — a **wrong answer**, silently. |
| 12 | + |
| 13 | +The cause is in the fixtures, not the assertion style: every one of them captures **at most one |
| 14 | +value**. `LambdaKinds` captures `base` in one lambda and `bound` in another; `Lambda` and |
| 15 | +`LambdaCapturingThis` capture one apiece. With one capture there is no order to get wrong, so the |
| 16 | +line that orders them is not covered by anything. The round's own mutation M10 ("captures not |
| 17 | +stored") tested *presence*, which is a different axis and reads deceptively like the same one. |
| 18 | + |
| 19 | +Two lambdas now capture two values each, and they were chosen to fail differently: |
| 20 | + |
| 21 | +| lambda | captures | prints | why this one | |
| 22 | +|---|---|---|---| |
| 23 | +| `pair` | `(String, int)` | `a:7` | a swap shows up in the **text** | |
| 24 | +| `weighted` | `(int, int)` | `120` | a swap shows up **only in the value** — no type check could catch it | |
| 25 | + |
| 26 | +RM4 now dies on both: `a:7 → 7:a` and `120 → 2001`. |
| 27 | + |
| 28 | +This is not an exotic shape. `(a, b) -> a + b` is what javac emits for the most ordinary lambda |
| 29 | +there is, and until this round it would have silently returned the arguments backwards. |
| 30 | + |
| 31 | +## F2 — a valid class file aborted the host process |
| 32 | + |
| 33 | +``` |
| 34 | +thread 'main' panicked at jvm/src/type.rs:74:13: Invalid type |
| 35 | +``` |
| 36 | + |
| 37 | +A call site whose descriptor is `I` — a *field* descriptor — reaches `lower()`, which read it with |
| 38 | +the panicking `JavaType::parse` / `as_method`. A panic is not a guest exception: the process dies. |
| 39 | +`verifier.rs` already writes that exact sentence about `ldc` ("reaching it would abort the host, |
| 40 | +not the guest"), so the repo's own doctrine names this a defect. |
| 41 | + |
| 42 | +### Where to fix it, and why not where it was suggested |
| 43 | + |
| 44 | +The review proposed two lines in `lambda.rs` (`try_parse` + `let … else`). That removes the abort, |
| 45 | +but it answers **`UnsupportedOperationException`** — and that answer is wrong: |
| 46 | + |
| 47 | +``` |
| 48 | +$ java -cp . MetafactoryFieldDescriptorCallSite # OpenJDK 26.0.1 |
| 49 | +java.lang.ClassFormatError: Method "run" in class MetafactoryFieldDescriptorCallSite |
| 50 | + has illegal signature "I" |
| 51 | +``` |
| 52 | + |
| 53 | +The file is not *unsupported*. It is **malformed**, and this repository has spent several rounds on |
| 54 | +keeping those two words apart. So the fix belongs where JVMS puts the rule. |
| 55 | + |
| 56 | +JVMS 4.4.6 says a `NameAndType` descriptor is "a valid field descriptor or method descriptor" — |
| 57 | +which it must be, because `Fieldref` and `Methodref` share that entry kind. *Which* one is decided |
| 58 | +by the entry that refers to it, and 4.4.10 states it: `CONSTANT_InvokeDynamic` names a method, |
| 59 | +`CONSTANT_Dynamic` names a field type. The existing `NameAndType` arm is therefore **right as it |
| 60 | +stands** and was left alone; the missing check was on the referring arm, which the codebase already |
| 61 | +does for `Methodref` via `validate_member_reference(..., MemberKind::Method)`. |
| 62 | + |
| 63 | +**Cost measured before tightening**, as the ticket required: 175 committed class files, 44 |
| 64 | +invokedynamic/dynamic references, **exactly one** newly rejected — the fixture written for this |
| 65 | +finding. Nothing else in the tree moves. |
| 66 | + |
| 67 | +`lower()` still uses `try_parse`. That is a second layer, and I could not construct an input that |
| 68 | +passes the tightened validation and still fails it — so **it is not independently observable**, and |
| 69 | +I am not claiming a mutation kills it. It stays because the two layers fail differently: the outer |
| 70 | +one produces a guest exception, and the missing inner one produces a dead process. |
| 71 | + |
| 72 | +### A side effect worth naming |
| 73 | + |
| 74 | +The same panic exists on `origin/main` through the string-concat path |
| 75 | +(`Interpreter::extract_invoke_params`), which the review scoped out as a separate ticket. Putting |
| 76 | +the rule at the usage site closes that entrance too — not by widening this round, but because a |
| 77 | +rule in the right place covers everything that reads through it. |
| 78 | + |
| 79 | +## Mutations |
| 80 | + |
| 81 | +| | mutation | result | |
| 82 | +|---|---|---| |
| 83 | +| **RM4** | capture read order reversed (the reviewer's) | **red** — `7:a` / `2001` | |
| 84 | +| **F2-M** | the `InvokeDynamic` descriptor rule reverted | **red** — the file is no longer `ClassFormatError` | |
| 85 | + |
| 86 | +`cargo test --all`: **578 passed / 0 failed / 1 ignored**. Regenerating the indy fixtures leaves |
| 87 | +every pre-existing one byte-identical — including after merging this branch's generator with the |
| 88 | +sibling one that landed as #59, which is the check that the merge kept both. |
| 89 | + |
| 90 | +## One process note |
| 91 | + |
| 92 | +`git checkout --` to undo a mutation also reverted an **uncommitted** fix sitting in the same file, |
| 93 | +and the next command reported the fix as missing. Commit before mutating; the mutation matrix in |
| 94 | +this round was re-run on a committed tree. |
0 commit comments