diff --git a/REPORT.md b/REPORT.md index 7a63bc1c..77a12399 100644 --- a/REPORT.md +++ b/REPORT.md @@ -12,6 +12,18 @@ - 검증: `cargo test --all` **583 passed / 0 failed / 1 ignored**(불변 — 코드 무접촉 · ★이 브랜치 base `8c7b473f` 기준이다. 같은 날 앞 회차들의 **579** 는 PR #61 착지 «전» base 의 수라 다르다) · `check-dod-ci-parity` → **「OK 두 축 모두 대칭차 0 — 명령 6개 · toolchain 2개로 «둘 다 일치»」**. - ★후속 추천(★**worklog `.json` `proposals[]` 에 카드 2장으로 «기계 채널»에 실었다** — 초판은 `REPORT` 에만 적어 cockpit 에 **0장**이었다): ⑴**신규 fixture 의 target 규칙**을 세울 것인가(M) ⑵**되돌릴 조건에 «관측자»를 붙인다**(S · 넷 중 셋은 문서를 열어야만 발화한다 — 게이트② 실측). ★초판이 ⑴로 적은 「`NativeMethod` 제3 축 여부」는 ★**검수자가 규명해 닫혔다**(축 2 의 다른 얼굴) ⇒ 카드로 내지 않는다. 상세 = `docs/worklog/2026-09-18-root-fixture-target-decision.md`. +## [2026-09-18] 거부된 클래스 파일이 «왜»를 말한다 — 세 층을 관통하는 사유 (rustjava-adopt-classfile-error-cause-decision-p0) +- 무엇을: 채택 제안 `2026-09-17-classfile-error-cause-decision#p0`. ★**제품 동작 변경 있음** — `ClassFormatError` 메시지가 **모든 거부에 같던 「Invalid class file」** 에서 **사유별 문장**으로 바뀐다. +- ★**제안이 스스로 all-or-nothing 이라 못박았다** — 타입만 고치면 **관측되는 것이 없고**, `||` 사슬을 안 쪼개면 **평평함이 사라지는 게 아니라 옮겨갈 뿐**이다. 넷 다 했다: + `ClassFileError::InvalidFormat(&'static str)` → `ClassDefinitionError::InvalidClassFile(&'static str)`(★`From` 이 **버리던** 자리) → 두 경계 자리 모두 사유를 그대로 던진다 · `validate_class` 의 **8항 `||` 사슬 → 규칙마다 `if` 하나**(사유 **14개**(클래스 8 · 필드 3 · 메서드 3 — `grep -c 'ClassFileError::InvalidFormat('` 로 센 값) · 필드 `ConstantValue` 는 「몇 개냐」와 「타입이 맞냐」가 **한 조건에 묶여** 있어 갈랐다). +- ★**왜 «변형»이 아니라 «문자열»인가**: 집합이 **열려 있고**(규칙마다 하나) **아무도 분기하지 않는다**. 선례도 있다 — `ClassDefinitionError::UnsupportedFeature(&'static str)`. +- ★★**사유를 꿰자마자 «평평한 오류가 가리고 있던 것 둘»이 나왔다**: + ⑴**테스트가 «어느 층이 거부하는지»를 틀리게 믿고 있었다** — 「인덱스가 엉뚱한 종류를 가리킨다」는 **검증**이 아니라 ★**파서**가 거부한다(`truncated or unparsable class file`). ★**코드를 추측에 맞추지 않고 단언을 실측에 맞췄다**(주석에 「measured, not assumed」). + ⑵**술어 이름이 낡아 있었다** — `bootstrap_method_static_arguments_are_in_the_pool` 은 이름과 달리 **「적재 가능 상수인가」까지** 요구한다(직전 회차가 넓혔고 자기 docstring 이 그렇게 적는다). 사유는 **규칙 그대로** 적고 ★**함수 이름은 바꾸지 않았다**(리팩터 = 범위 밖). +- ★★**양방향 — 세 층 «전부»에 개악**: **M1** `src/runtime.rs` 가 다시 문자열을 박는다 → red · **M2** `From` 이 다시 사유를 버린다(제안이 지목한 그 버그) → red · **M3** 두 사유를 한 문자열로 접는다 → red · 복원 **17/0**. ★★**M3 을 잡는 것은 줄마다의 `assert!(err.contains(cause))` 다**(`tests/test_class_format.rs:450` — 실행이 루프 끝에 **도달조차 하지 않는다** · ★초판은 `:452` 라 적었으나 dedup 2줄 제거로 **:450 으로 옮겨졌다**). ★**초판은 이것을 시험 말미의 dedup 단언에 귀속시켰는데 틀렸다** — 그 벡터에 담기던 것은 제품의 출력이 아니라 **표의 기대 리터럴**이라 **상수끼리 비교**했고 제품이 무엇을 내든 결과가 같았다. ⇒ ★**주석만 고치지 않고 그 블록을 걷어냈다**(잃는 것은 아래 대가에 적는다). +- ★★**대가 — 실측한 구멍 하나를 포함해 적는다**: ⒜★**마지막 홉이 «두 번» 쓰여 있고 한 쪽만 테스트가 본다** — `test-utils/src/lib.rs` 사본만 개악하면 `cargo test --all` 이 **579 passed / 0 failed**(아무것도 안 운다). ★**합치는 것은 리팩터라 하지 않았고 구멍을 보고한다.** ⒝사유가 문자열이라 **두 규칙에 같은 문구**를 주는 것을 막는 것이 ★**아무것도 없다** — 초판이 그것을 막는다고 적은 dedup 단언은 공허했고 **걷어냈다**(:10) ⇒ 남는 보장은 `contains` 가 덮는 **그 세 픽스처**뿐이다 ⒞★**픽스처 규율을 대체하지 않는다**(제안이 이미 적었다) ⒟★**PR #66 과 같은 함수를 만진다** — 뒤에 착지하는 쪽이 base 를 당겨 그 항을 다시 쪼갠다(충돌은 실재하나 **기계적**). +- 검증: `cargo test --all` **578 → 579 / 0 failed / 1 ignored** · `classfile` **15+13/0** · `check-dod-ci-parity` → **「OK 두 축 모두 대칭차 0 — 명령 6개 · toolchain 2개로 «둘 다 일치»」**(rc=0 · ★수를 직접 세지 않는다 — `CLAUDE.md` §DoD 규율). + ## [2026-09-17] 코드 2파일 합집합 — ★**그런데 ours 의 «삭제»는 의도가 아니라 선행 머지의 «조용한 롤백»이었다** (rustjava-adopt-link-stringconcatfactory-p2-fix3) - 무엇을: 게이트③이 `code-conflict-out-of-scope` 로 세운 PR #61 의 충돌 4파일(원장 2 + 코드 2)을 합집합으로 해소. ★제품 Rust **0줄**(테스트·픽스처 생성기만). - ★★**브리프의 전제 하나가 반증됐다** — 「ours 가 «의도적으로» 지운 54·16줄을 되살리지 마라」였는데, 두 파일의 성격이 **정반대**였다: diff --git a/STATE.md b/STATE.md index 446a2e72..c1903379 100644 --- a/STATE.md +++ b/STATE.md @@ -14,6 +14,12 @@ ★**이유**: 제안의 이득(「숫자 하나로 예측」)이 뒤집힌다 — 52 는 「전-indy·전-nestmate」라는 뜻을 **실제로 갖고**, 펴면 그 구분이 사라지며 유일한 커버리지가 지워진다. ★**잃는 것**: 비균일 잔존(신규 target 규칙 **미수립**) · 16건은 그대로 · ★**본 것은 40 중 20** · `NativeMethod` 차이는 **미규명**. ★`--all` **583/0/1**(불변 · base `8c7b473f` — 앞 회차의 579 는 #61 착지 전 base 다) · 되돌릴 조건 4개를 결정 문서에 명시. +- [rustjava-adopt-classfile-error-cause-decision-p0] ★★**거부 사유를 세 층에 꿴다 — 「Invalid class file」 하나가 **14개** 문장이 된다(클래스 8 · 필드 3 · 메서드 3).** 채택 제안 `2026-09-17-classfile-error-cause-decision#p0`. ★**제품 동작 변경 있음**(사용자가 보는 `ClassFormatError` 메시지). + ★제안이 **all-or-nothing** 이라 못박은 넷을 다 했다: `InvalidFormat(&'static str)` · `InvalidClassFile(&'static str)`(★`From` 이 **버리던** 자리) · 경계 2자리 · ★**`validate_class` 8항 `||` → 규칙마다 `if`**. + ★★**사유를 꿰자 «평평한 오류가 가리던 것 둘»이 나왔다**: ⑴테스트가 **어느 층이 거부하는지를 틀리게 믿었다**(검증 아닌 **파서**) ⇒ ★단언을 실측에 맞췄다 ⑵술어 **이름이 낡아 있었다**(「in_the_pool」인데 **적재 가능성까지** 본다) ⇒ 사유는 규칙대로, ★**이름은 안 바꿨다**(리팩터 금지). + ★**양방향 — 세 층 전부 개악**: M1 경계 · M2 `From` 이 사유 버림 · M3 두 사유를 한 문자열로 접음(★잡는 것은 `contains` `:450`(dedup 제거로 452→450) — 초판이 귀속한 dedup 단언은 **상수 대 상수라 공허**했고 **걷어냈다**) · 복원 17/0. + ★★**대가**: ★**마지막 홉이 두 번 쓰여 있고 `test-utils` 사본은 «무검증»**(개악해도 579/0 · **합치지 않고 보고**) · 사유가 문자열이라 같은 문구 중복을 막는 것이 없다 · ★**PR #66 과 같은 함수**(충돌은 기계적). + ★`--all` **578 → 579/0/1** · `check-dod-ci-parity` → **「OK 두 축 모두 대칭차 0 — 명령 6개 · toolchain 2개로 «둘 다 일치»」**(rc=0). - [rustjava-adopt-link-stringconcatfactory-p2-fix2] ★★**#57 의 버전 표에 25행 등재 — 「착지 순서」가 만든 부채를 갚는다(PR #61).** ★**막힌 것은 CI 도 충돌도 아니었다**: 핀 `85cf0fba` 에서 rc=0 CI_GREEN · `git merge origin/main` **코드 충돌 0** 인데 ★**합친 결과**가 #57 이 세운 「미등재 픽스처는 핀을 실패시킨다」를 어겼다(미등재 **25건** 재현). diff --git a/classfile/src/class.rs b/classfile/src/class.rs index 2d6a7c23..e30e36b8 100644 --- a/classfile/src/class.rs +++ b/classfile/src/class.rs @@ -100,12 +100,12 @@ impl ClassInfo { } pub fn parse(file: &[u8]) -> Result { - let (remaining, result) = Self::parse_info(file).map_err(|_| ClassFileError::InvalidFormat)?; + let (remaining, result) = Self::parse_info(file).map_err(|_| ClassFileError::InvalidFormat("truncated or unparsable class file"))?; if !remaining.is_empty() { - return Err(ClassFileError::InvalidFormat); + return Err(ClassFileError::InvalidFormat("extra bytes after the end of the class file")); } if result.major_version < 45 { - return Err(ClassFileError::InvalidFormat); + return Err(ClassFileError::InvalidFormat("class file version predates 45.0")); } if result.major_version > 70 { return Err(ClassFileError::UnsupportedVersion(result.major_version)); diff --git a/classfile/src/error.rs b/classfile/src/error.rs index def88250..c40b5813 100644 --- a/classfile/src/error.rs +++ b/classfile/src/error.rs @@ -1,5 +1,14 @@ +/// Why a class file was refused. +/// +/// `InvalidFormat` carries the cause so the rejection can say what is wrong instead of repeating +/// one sentence for every reason — the shape `ClassDefinitionError::UnsupportedFeature` already +/// used. A `&'static str` rather than a variant per rule: the set is open (every new rule adds +/// one) and nothing branches on it, so a string is what a caller actually needs. +/// +/// The words are the message a user sees, so they read as a JVM does — "multiple BootstrapMethods +/// attributes", not "AtMostOneBootstrapMethods". #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum ClassFileError { - InvalidFormat, + InvalidFormat(&'static str), UnsupportedVersion(u16), } diff --git a/classfile/src/validation.rs b/classfile/src/validation.rs index e52ff31b..50225136 100644 --- a/classfile/src/validation.rs +++ b/classfile/src/validation.rs @@ -9,22 +9,54 @@ enum MemberKind { Method, } +/// Every rule the class file has to satisfy, each one naming itself. +/// +/// This used to be an eight-term `||` chain feeding one `InvalidFormat`, which made the cause +/// unrecoverable by construction: the caller could not tell "unknown constant pool tag" from +/// "a bootstrap argument names nothing", and neither could the `ClassFormatError` a user reads. +/// One `if` per rule is the cheapest thing that lets the cause differ — no dispatch, no table, and +/// the reason lives next to the check it belongs to. +/// +/// The strings are the message, so they are written the way a JVM writes one. They are not +/// identifiers and nothing matches on them; tests assert them to pin *which* rule fired, which is +/// the observability the flat version could not give. pub(crate) fn validate_class(class: &ClassInfo) -> Result<(), ClassFileError> { - if !is_internal_class_name(&class.this_class) - || class.super_class.as_ref().is_some_and(|name| !is_internal_class_name(name)) - || class.interfaces.iter().any(|name| !is_internal_class_name(name)) - || !validate_constant_pool(&class.constant_pool) - || !constant_pool_tags_fit_the_class_file_version(class) - || !bootstrap_method_static_arguments_are_in_the_pool(class) - || !bootstrap_method_indices_resolve(class) - || !at_most_one_of_each_single_class_attribute(class) - { - return Err(ClassFileError::InvalidFormat); + if !is_internal_class_name(&class.this_class) { + return Err(ClassFileError::InvalidFormat("this_class does not name a class")); + } + if class.super_class.as_ref().is_some_and(|name| !is_internal_class_name(name)) { + return Err(ClassFileError::InvalidFormat("super_class does not name a class")); + } + if class.interfaces.iter().any(|name| !is_internal_class_name(name)) { + return Err(ClassFileError::InvalidFormat("an interface entry does not name a class")); + } + if !validate_constant_pool(&class.constant_pool) { + return Err(ClassFileError::InvalidFormat("a constant pool entry names a missing or wrong-kind entry")); + } + if !constant_pool_tags_fit_the_class_file_version(class) { + return Err(ClassFileError::InvalidFormat( + "class file version does not support a constant tag it carries", + )); + } + if !bootstrap_method_static_arguments_are_in_the_pool(class) { + return Err(ClassFileError::InvalidFormat( + // The rule is wider than the function name: the docstring above says the argument must also + // be a loadable constant, and OpenJDK says the same ("bad constant type"). The name stayed + // behind when the rule widened; renaming it is not this round's scope, so the cause is what + // gets the wording right. + "a bootstrap method argument names nothing or is not a loadable constant", + )); + } + if !bootstrap_method_indices_resolve(class) { + return Err(ClassFileError::InvalidFormat("a dynamic constant names no bootstrap method")); + } + if !at_most_one_of_each_single_class_attribute(class) { + return Err(ClassFileError::InvalidFormat("a single-valued class attribute appears more than once")); } for field in &class.fields { if !is_field_descriptor(&field.descriptor) { - return Err(ClassFileError::InvalidFormat); + return Err(ClassFileError::InvalidFormat("a field descriptor is malformed")); } let constant_values = field @@ -35,25 +67,27 @@ pub(crate) fn validate_class(class: &ClassInfo) -> Result<(), ClassFileError> { _ => None, }) .collect::>(); - if constant_values.len() > 1 - || constant_values.first().is_some_and(|value| { - !matches!( - (field.descriptor.as_str(), *value), - ("Z" | "B" | "C" | "S" | "I", ConstantPoolReference::Integer(_)) - | ("J", ConstantPoolReference::Long(_)) - | ("F", ConstantPoolReference::Float(_)) - | ("D", ConstantPoolReference::Double(_)) - | ("Ljava/lang/String;", ConstantPoolReference::String(_)) - ) - }) - { - return Err(ClassFileError::InvalidFormat); + // Two rules, not one: "how many" and "of what type". The flat version could not say which. + if constant_values.len() > 1 { + return Err(ClassFileError::InvalidFormat("multiple ConstantValue attributes on a field")); + } + if constant_values.first().is_some_and(|value| { + !matches!( + (field.descriptor.as_str(), *value), + ("Z" | "B" | "C" | "S" | "I", ConstantPoolReference::Integer(_)) + | ("J", ConstantPoolReference::Long(_)) + | ("F", ConstantPoolReference::Float(_)) + | ("D", ConstantPoolReference::Double(_)) + | ("Ljava/lang/String;", ConstantPoolReference::String(_)) + ) + }) { + return Err(ClassFileError::InvalidFormat("a ConstantValue does not match its field descriptor")); } } for method in &class.methods { if !is_method_descriptor(&method.descriptor) { - return Err(ClassFileError::InvalidFormat); + return Err(ClassFileError::InvalidFormat("a method descriptor is malformed")); } let code_attributes = method @@ -63,10 +97,10 @@ pub(crate) fn validate_class(class: &ClassInfo) -> Result<(), ClassFileError> { .count(); if method.access_flags.intersects(MethodAccessFlags::ABSTRACT | MethodAccessFlags::NATIVE) { if code_attributes != 0 { - return Err(ClassFileError::InvalidFormat); + return Err(ClassFileError::InvalidFormat("an abstract or native method carries a Code attribute")); } } else if code_attributes != 1 { - return Err(ClassFileError::InvalidFormat); + return Err(ClassFileError::InvalidFormat("a method does not have exactly one Code attribute")); } } diff --git a/classfile/tests/test.rs b/classfile/tests/test.rs index 9e33bee2..fc762a47 100644 --- a/classfile/tests/test.rs +++ b/classfile/tests/test.rs @@ -149,17 +149,26 @@ fn test_invokeinterface() { fn test_malformed_class_files_return_structured_errors() { let hello = include_bytes!("../../test-data/Hello.class"); - assert_eq!(ClassInfo::parse(&[]).err(), Some(ClassFileError::InvalidFormat)); + assert_eq!( + ClassInfo::parse(&[]).err(), + Some(ClassFileError::InvalidFormat("truncated or unparsable class file")) + ); let mut invalid_magic = hello.to_vec(); invalid_magic[0] = 0; - assert_eq!(ClassInfo::parse(&invalid_magic).err(), Some(ClassFileError::InvalidFormat)); + assert_eq!( + ClassInfo::parse(&invalid_magic).err(), + Some(ClassFileError::InvalidFormat("truncated or unparsable class file")) + ); let mut unsupported_version = hello.to_vec(); unsupported_version[6..8].copy_from_slice(&71u16.to_be_bytes()); assert_eq!(ClassInfo::parse(&unsupported_version).err(), Some(ClassFileError::UnsupportedVersion(71))); - assert_eq!(ClassInfo::parse(&hello[..hello.len() / 2]).err(), Some(ClassFileError::InvalidFormat)); + assert_eq!( + ClassInfo::parse(&hello[..hello.len() / 2]).err(), + Some(ClassFileError::InvalidFormat("truncated or unparsable class file")) + ); let minimal_class = vec![ 0xca, 0xfe, 0xba, 0xbe, 0x00, 0x00, 0x00, 0x2d, 0x00, 0x05, 0x01, 0x00, 0x04, b'T', b'e', b's', b't', 0x07, 0x00, 0x01, 0x01, 0x00, 0x10, @@ -170,11 +179,19 @@ fn test_malformed_class_files_return_structured_errors() { let mut invalid_constant_pool_index = minimal_class.clone(); invalid_constant_pool_index[44..46].copy_from_slice(&99u16.to_be_bytes()); - assert_eq!(ClassInfo::parse(&invalid_constant_pool_index).err(), Some(ClassFileError::InvalidFormat)); + assert_eq!( + ClassInfo::parse(&invalid_constant_pool_index).err(), + Some(ClassFileError::InvalidFormat("truncated or unparsable class file")), + "an index past the end of the pool stops the parse, before validation sees it" + ); let mut invalid_constant_pool_type = minimal_class; invalid_constant_pool_type[44..46].copy_from_slice(&1u16.to_be_bytes()); - assert_eq!(ClassInfo::parse(&invalid_constant_pool_type).err(), Some(ClassFileError::InvalidFormat)); + assert_eq!( + ClassInfo::parse(&invalid_constant_pool_type).err(), + Some(ClassFileError::InvalidFormat("truncated or unparsable class file")), + "measured, not assumed: this one is refused by the parser, not by validation" + ); } #[test] @@ -183,15 +200,24 @@ fn test_class_info_validation_rejects_invalid_names_descriptors_and_code_layout( let mut invalid_name = ClassInfo::parse(hello).unwrap(); invalid_name.this_class = "[I".to_string().into(); - assert_eq!(invalid_name.validate(), Err(ClassFileError::InvalidFormat)); + assert_eq!( + invalid_name.validate(), + Err(ClassFileError::InvalidFormat("this_class does not name a class")) + ); let mut invalid_descriptor = ClassInfo::parse(hello).unwrap(); invalid_descriptor.methods[0].descriptor = "(V)V".to_string().into(); - assert_eq!(invalid_descriptor.validate(), Err(ClassFileError::InvalidFormat)); + assert_eq!( + invalid_descriptor.validate(), + Err(ClassFileError::InvalidFormat("a method descriptor is malformed")) + ); let mut missing_code = ClassInfo::parse(hello).unwrap(); missing_code.methods[0].attributes.clear(); - assert_eq!(missing_code.validate(), Err(ClassFileError::InvalidFormat)); + assert_eq!( + missing_code.validate(), + Err(ClassFileError::InvalidFormat("a method does not have exactly one Code attribute")) + ); } #[test] @@ -285,7 +311,7 @@ fn test_bootstrap_method_reference_kinds_outside_the_set_and_mispaired_kinds_are for reference_kind in [0u8, 10, 255] { assert_eq!( parse_with_kind(reference_kind), - Some(ClassFileError::InvalidFormat), + Some(ClassFileError::InvalidFormat("truncated or unparsable class file")), "reference kind {reference_kind} must not parse" ); } @@ -294,8 +320,9 @@ fn test_bootstrap_method_reference_kinds_outside_the_set_and_mispaired_kinds_are for reference_kind in [1u8, 4, 9] { assert_eq!( parse_with_kind(reference_kind), - Some(ClassFileError::InvalidFormat), - "reference kind {reference_kind} does not pair with a Methodref" + Some(ClassFileError::InvalidFormat("a constant pool entry names a missing or wrong-kind entry")), + "reference kind {reference_kind} does not pair with a Methodref — and the cause says it was validation, \ + not the parser, that refused it (the loop above is the parser's)" ); } @@ -340,7 +367,9 @@ fn test_a_bootstrap_argument_that_is_not_a_loadable_constant_is_rejected() { assert_eq!( ClassInfo::parse(&mutated).err(), - Some(ClassFileError::InvalidFormat), + Some(ClassFileError::InvalidFormat( + "a bootstrap method argument names nothing or is not a loadable constant" + )), "a bootstrap argument naming a Utf8 is not a loadable constant" ); } diff --git a/docs/worklog/2026-09-18-classfile-error-cause.json b/docs/worklog/2026-09-18-classfile-error-cause.json new file mode 100644 index 00000000..7aa11542 --- /dev/null +++ b/docs/worklog/2026-09-18-classfile-error-cause.json @@ -0,0 +1,36 @@ +{ + "schema": "worklog/v1", + "date": "2026-09-18", + "taskId": "rustjava-adopt-classfile-error-cause-decision-p0", + "summary": "A rejected class file now says what is wrong with it, all the way out to the ClassFormatError message. ClassFileError::InvalidFormat carries a &'static str, ClassDefinitionError::InvalidClassFile carries it too so the From impl stops dropping it, both boundary sites pass it through, and validate_class's eight-term || chain is one `if` per rule so the cause can differ. Threading the cause immediately caught two things the flat error had been hiding: a test that believed the wrong layer refused its input, and a predicate whose name went stale when a previous round widened it.", + "changes": [ + "classfile/src/error.rs: InvalidFormat -> InvalidFormat(&'static str), with the reasoning for a string rather than a variant per rule", + "classfile/src/validation.rs: the eight-term || chain becomes one `if` per rule; 14 distinct causes across the class, field and method rules (8 class, 3 field, 3 method — counted with `grep -c 'ClassFileError::InvalidFormat(' classfile/src/validation.rs`)", + "classfile/src/class.rs: 3 parse-level causes (unparsable, trailing bytes, version below 45)", + "jvm-bytecode/src/error.rs: InvalidClassFile(&'static str); the From impl carries the cause instead of discarding it", + "src/runtime.rs, test-utils/src/lib.rs: the ClassFormatError message is the cause instead of the hardcoded \"Invalid class file\"", + "classfile/tests/test.rs: 11 assertions now pin which rule fired, not just that something did", + "tests/test_class_format.rs: test_a_rejected_class_says_why at the user-visible boundary, plus the stale note about why this could not be asserted" + ], + "verification": [ + "MUTATION M2, make the From impl drop the cause again (the exact bug the proposal named): test_a_rejected_class_says_why red", + "MUTATION M3, collapse two distinct causes to one string in validate_class: red — caught by the per-row `assert!(err.contains(cause))`, measured: the failure is reported at tests/test_class_format.rs:450 and execution never reaches the end of the loop. An earlier draft of this round credited a dedup assertion at the end of the test; that was wrong, because the vector it de-duplicated held the table's expected literals rather than anything the product said, so it compared constants to constants and could not fail. Proven by neutering `contains` while the collapse was still applied: the test then passed. The dedup block has been removed rather than left as a comment claiming a guarantee it did not provide", + "MUTATION M1, make src/runtime.rs hardcode \"Invalid class file\" again: red. This is the third of the three layers, so the whole path is covered by a mutation.", + "restore after each: test_class_format 17 passed / 0 failed", + "FINDING 1 — a test believed the wrong layer. classfile/tests/test.rs asserted that an index naming the wrong constant kind is refused by validation; the cause says it is refused by the parser (\"truncated or unparsable class file\"). The assertion was corrected to the measurement rather than the code to the assumption, and the comment now says it was measured.", + "FINDING 2 — a predicate's name went stale and nothing noticed. bootstrap_method_static_arguments_are_in_the_pool also requires the argument to be a loadable constant (its own docstring says so, and quotes OpenJDK's \"bad constant type\"). The cause is worded for the rule as it now is; the function was NOT renamed, since that is a refactor and out of scope.", + "cargo test --all: 578 -> 579 passed / 0 failed / 1 ignored (this round adds one test)", + "classfile suite: 15 + 13 passed / 0 failed" + ], + "issues": [ + "COVERAGE GAP, measured not assumed: the last hop is written twice — src/runtime.rs and test-utils/src/lib.rs — and only the first is under test. Mutating the test-utils copy to drop the cause leaves cargo test --all at 579 passed / 0 failed. Deduplicating them is a refactor and was not done; the gap is reported instead.", + "The causes are strings, so nothing stops two rules from being given the same wording, and nothing in the suite checks that they do not. An earlier draft of this round claimed a dedup assertion covered it; that assertion compared the table's expected literals to each other rather than anything the product said, so it could not fail, and it has been removed. What remains is the per-row `assert!(err.contains(cause))`, which covers exactly the three fixtures that test names.", + "This does not replace the shaped-fixture discipline, as the proposal said: a cause names which check fired, not whether every axis inside a multi-axis check is observable.", + "Scope collision: PR #66 (open) also edits validate_class's chain. Whichever lands second pulls base and re-splits the added term — the conflict is real but mechanical, and both changes are additive within the same function.", + "UPSTREAM DIVERGENCE: docs/upstream-sync-approach.md section 3-B closes the message axis with \"upstream wins on wording\", and this round reopens it — every rejection message in classfile/src/validation.rs is now ours and differs from upstream's single \"Invalid class file\". The next sync round meets this file first, so the cost is named here rather than discovered there." + ], + "adoptedProposals": [ + "2026-09-17-classfile-error-cause-decision#p0" + ], + "proposals": [] +} diff --git a/docs/worklog/2026-09-18-classfile-error-cause.md b/docs/worklog/2026-09-18-classfile-error-cause.md new file mode 100644 index 00000000..02ca77c9 --- /dev/null +++ b/docs/worklog/2026-09-18-classfile-error-cause.md @@ -0,0 +1,59 @@ +# 2026-09-18 — 거부된 클래스 파일이 «왜»를 말한다 (rustjava-adopt-classfile-error-cause-decision-p0) + +채택 제안 `2026-09-17-classfile-error-cause-decision#p0`. +제안은 **all-or-nothing** 이라고 스스로 못박았다 — 타입만 고치면 관측되는 것이 없고, +`||` 사슬을 안 쪼개면 **평평함이 사라지는 게 아니라 옮겨갈 뿐**이다. 넷 다 했다. + +## 한 일 — 세 층을 관통한다 + +``` +classfile::ClassFileError::InvalidFormat(&'static str) + → jvm_bytecode::ClassDefinitionError::InvalidClassFile(&'static str) ← From 이 «버리던» 자리 + → jvm.exception("java/lang/ClassFormatError", cause) ← 두 자리 모두 +``` +그리고 `validate_class` 의 **8항 `||` 사슬**을 **규칙마다 `if` 하나**로 쪼갰다 — +클래스 8 · 필드 3 · 메서드 **3** = ★**사유 14개**(계수 = `grep -c 'ClassFileError::InvalidFormat(' classfile/src/validation.rs`)(필드의 `ConstantValue` 는 「몇 개냐」와 「타입이 맞냐」가 +**한 조건에 묶여** 있었고, 그 둘을 갈랐다). + +★**왜 변형이 아니라 문자열인가**: 집합이 **열려 있고**(규칙이 늘 때마다 하나씩) **아무도 분기하지 않는다**. +그리고 이 저장소에 **선례가 있다** — `ClassDefinitionError::UnsupportedFeature(&'static str)`. + +## ★사유를 꿰자마자 «숨어 있던 것 둘»이 튀어나왔다 + +**⑴ 테스트가 «어느 층이 거부하는지»를 틀리게 믿고 있었다.** +`classfile/tests/test.rs` 의 「인덱스가 **엉뚱한 종류**를 가리킨다」 케이스는 **검증이 거부한다**고 적혀 있었는데, +사유는 **`"truncated or unparsable class file"`** — ★**파서가 거부한다**. +★**코드를 내 추측에 맞추지 않고 단언을 실측에 맞췄다**(주석에 「measured, not assumed」를 박았다). +※바로 옆 루프(`reference kind` 1·4·9)는 **반대로** 검증이 거부한다 — ★그 대비가 이제 **사유로 보인다**. + +**⑵ 술어의 «이름»이 낡아 있었고 아무도 몰랐다.** +`bootstrap_method_static_arguments_are_in_the_pool` 은 이름과 달리 **「적재 가능 상수인가」까지** 요구한다 +(자기 docstring 이 그렇게 적고 OpenJDK 의 `bad constant type` 까지 인용한다 — 직전 회차가 규칙을 **넓혔다**). +⇒ 사유는 **규칙 그대로** 적었다: `"a bootstrap method argument names nothing or is not a loadable constant"`. +★**함수 이름은 바꾸지 않았다** — 리팩터는 이 회차 범위 밖이다(계약 3). 그 사실을 코드 주석에 남겼다. + +★★**둘 다 «평평한 오류»가 가리고 있던 것**이다. 사유가 없을 땐 **어느 것도 틀릴 수 없었다** — 물을 수가 없었으니까. + +## 양방향 — 세 층 전부에 개악을 놓았다 + +| 개악 | 결과 | +|---|---| +| **M1** `src/runtime.rs` 가 다시 `"Invalid class file"` 를 박는다 | ★**red** | +| **M2** `From` 이 다시 사유를 **버린다**(제안이 지목한 바로 그 버그) | ★**red** | +| **M3** 서로 다른 두 사유를 **한 문자열**로 접는다 | ★**red** — 줄마다의 `assert!(err.contains(cause))` 가 잡는다(`tests/test_class_format.rs:450`) | +| 복원 | **green** 17/0 | + +★**M3 이 없으면** 「전부 같은 문자열로 되돌려도 통과」가 가능하다 — 그것을 잡는 것은 줄마다의 `contains` 다(아래 §대가). ★**초판은 여기에 「테스트가 «사유들이 서로 다름»까지 단언한다」고 적었는데 거짓이었다** — 그 단언은 상수끼리 비교해 공허했고 걷어냈다. + +## ★대가 — 실측한 구멍 하나를 포함해서 + +- ★★**마지막 홉이 «두 번» 쓰여 있고 한 쪽만 테스트가 본다**(실측): `src/runtime.rs` ↔ `test-utils/src/lib.rs`. + ★**test-utils 사본만 개악하면 `cargo test --all` 이 `579 passed / 0 failed`** — **아무것도 울지 않는다**. + ★**합치는 것은 리팩터라 하지 않았고**, 대신 **구멍을 보고한다**. +- 사유가 **문자열**이라 두 규칙에 같은 문구를 주는 것을 막는 것이 없다. ★**그리고 그것을 «막는다»고 적었던 dedup 단언은 공허했다** — `seen` 에 담기던 것이 제품의 출력이 아니라 **표의 기대 리터럴**이라 상수끼리 비교했고, 제품이 무엇을 내든 결과가 같았다. ⇒ **걷어냈다.** 남는 보장은 `contains` 가 덮는 **그 세 픽스처**뿐이다. +- ★**픽스처 규율을 대체하지 않는다**(제안이 이미 적었다) — 사유는 「어느 검사가 울었나」이지 + 「다축 검사의 각 축이 관측되나」가 아니다. +- ★**PR #66 과 같은 함수를 만진다** — 뒤에 착지하는 쪽이 base 를 당겨 그 항을 다시 쪼갠다. 충돌은 실재하지만 **기계적**이다. + +## 검증 +`cargo test --all` **578 → 579 / 0 failed / 1 ignored** · `classfile` **15+13/0** · `test_class_format` **17/0** · `check-dod-ci-parity` → **「OK 두 축 모두 대칭차 0 — 명령 6개 · toolchain 2개로 «둘 다 일치»」**(rc=0). diff --git a/jvm-bytecode/src/error.rs b/jvm-bytecode/src/error.rs index 02a81a25..af56e5a8 100644 --- a/jvm-bytecode/src/error.rs +++ b/jvm-bytecode/src/error.rs @@ -2,7 +2,7 @@ use classfile::ClassFileError; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum ClassDefinitionError { - InvalidClassFile, + InvalidClassFile(&'static str), UnsupportedClassVersion(u16), Verification, UnsupportedFeature(&'static str), @@ -11,7 +11,7 @@ pub enum ClassDefinitionError { impl From for ClassDefinitionError { fn from(error: ClassFileError) -> Self { match error { - ClassFileError::InvalidFormat => Self::InvalidClassFile, + ClassFileError::InvalidFormat(cause) => Self::InvalidClassFile(cause), ClassFileError::UnsupportedVersion(version) => Self::UnsupportedClassVersion(version), } } diff --git a/src/runtime.rs b/src/runtime.rs index b9494193..eda99b05 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -186,7 +186,7 @@ where async fn define_class(&self, jvm: &Jvm, data: &[u8]) -> jvm::Result> { match ClassDefinitionImpl::from_classfile(data) { Ok(class) => Ok(Box::new(class)), - Err(ClassDefinitionError::InvalidClassFile) => Err(jvm.exception("java/lang/ClassFormatError", "Invalid class file").await), + Err(ClassDefinitionError::InvalidClassFile(cause)) => Err(jvm.exception("java/lang/ClassFormatError", cause).await), Err(ClassDefinitionError::UnsupportedClassVersion(version)) => Err(jvm .exception( "java/lang/UnsupportedClassVersionError", diff --git a/test-data/src/cp/make_cp_fixtures.py b/test-data/src/cp/make_cp_fixtures.py index 6855fc68..1aa12e2f 100644 --- a/test-data/src/cp/make_cp_fixtures.py +++ b/test-data/src/cp/make_cp_fixtures.py @@ -6,8 +6,8 @@ `tests/test_class_format.rs` already had a test for "we still reject unknown constant pool tags", built by overwriting the tag byte of `test-data/Hello.class`'s first pool entry. That entry is a Methodref the code invokes, so overwriting it breaks the class along several independent paths at -once — the operand of `invokespecial` stops being a method reference, and so on. `ClassFileError` -collapses every parse failure into a flat "Invalid class file", so the assertion cannot tell +once — the operand of `invokespecial` stops being a method reference, and so on. At the time, +`ClassFileError` collapsed every parse failure into a flat "Invalid class file", so the assertion could not tell "rejected because the tag is unknown" from "rejected because the class fell apart". Measured: with the pass-through branch mutated from reject to accept, that test still passed. diff --git a/test-utils/src/lib.rs b/test-utils/src/lib.rs index a0a2f410..bf7ef55e 100644 --- a/test-utils/src/lib.rs +++ b/test-utils/src/lib.rs @@ -331,7 +331,7 @@ impl Runtime for TestRuntime { async fn define_class(&self, jvm: &Jvm, data: &[u8]) -> jvm::Result> { match ClassDefinitionImpl::from_classfile(data) { Ok(class) => Ok(Box::new(class)), - Err(ClassDefinitionError::InvalidClassFile) => Err(jvm.exception("java/lang/ClassFormatError", "Invalid class file").await), + Err(ClassDefinitionError::InvalidClassFile(cause)) => Err(jvm.exception("java/lang/ClassFormatError", cause).await), Err(ClassDefinitionError::UnsupportedClassVersion(version)) => Err(jvm .exception( "java/lang/UnsupportedClassVersionError", diff --git a/tests/test_class_format.rs b/tests/test_class_format.rs index ce699695..0178e249 100644 --- a/tests/test_class_format.rs +++ b/tests/test_class_format.rs @@ -28,16 +28,17 @@ fn hello_class() -> Vec { fs::read("test-data/Hello.class").unwrap() } -// Only the exception *kind* is asserted, not the message: `ClassFileError::InvalidFormat` carries -// no cause, and the two places that turn it into a Java exception hardcode the string "Invalid -// class file", so there is no per-cause wording to assert. +// This note used to say only the exception *kind* could be asserted, because `InvalidFormat` carried +// no cause and both places that turn it into a Java exception hardcoded "Invalid class file". That is +// done: `InvalidFormat(&'static str)` threads the cause to the boundary, and `validate_class` is one +// `if` per rule so the cause can differ. `test_a_rejected_class_says_why` below is what makes that +// visible; the kind-only assertions elsewhere in this file are left as they are, because the kind is +// what those tests are about. // -// This note used to say the variants were "cut" upstream at 822504b and that restoring them "needs -// upstream variants". Both halves were wrong, measured: that commit *created* classfile/src/error.rs -// — before it, `ClassInfo::parse` returned `Option`, so failure carried nothing at all — and this -// fork already diverges by hundreds of lines in this crate, so nothing about the change is upstream's -// to make. What it does need is a cause threaded through three layers and `validate_class`'s eight-term -// `||` chain split so the cause can differ per check. See docs/worklog/2026-09-17-classfile-error-cause-decision.md. +// (The note before *that* one said the variants were "cut" upstream at 822504b and that restoring them +// "needs upstream variants". Both halves were wrong, measured: that commit *created* +// classfile/src/error.rs — before it, `ClassInfo::parse` returned `Option`, so failure carried nothing +// at all — and this fork already diverges by hundreds of lines in this crate.) #[tokio::test] async fn test_truncated_class_raises_class_format_error() { let (dir, path) = fixture("TruncatedHello.class", &hello_class()[..60]); @@ -54,8 +55,8 @@ async fn test_truncated_class_raises_class_format_error() { // The fixtures carry that tag on a **trailing, unreferenced, payload-free** pool entry, so the // unknown tag is the only thing wrong with the file. That is what makes this test able to fail: // the previous version overwrote the tag of `Hello.class`'s first entry, which is a Methodref the -// code invokes, so the file broke along several paths at once. `ClassFileError` flattens every -// parse failure into "Invalid class file", so that assertion could not tell "rejected because the +// code invokes, so the file broke along several paths at once. `ClassFileError` flattened every +// parse failure into "Invalid class file" back then, so that assertion could not tell "rejected because the // tag is unknown" from "rejected because the class fell apart" — and measurably did not: with the // tag switch's pass-through branch mutated from reject to accept, it still passed. // See `test-data/src/cp/make_cp_fixtures.py`. @@ -617,3 +618,38 @@ async fn test_each_axis_of_the_metafactory_identity_is_observable() { ); } } + +// The point of threading a cause: two broken files get two different sentences, and each one names +// the rule that fired. Before this, every one of them read "Invalid class file". +// +// Asserted at the boundary a user actually sees — the `ClassFormatError` message — rather than on +// `ClassFileError`, because the value of the change is that the string survives three layers +// (classfile -> jvm-bytecode -> runtime) instead of being dropped by the `From` impl. +#[tokio::test] +async fn test_a_rejected_class_says_why() { + for (fixture, directory, cause) in [ + ( + "test-data/ldc/LdcDynamicDuplicateBSM.class", + "./test-data/ldc/", + "a single-valued class attribute appears more than once", + ), + ( + "test-data/ldc/LdcDynamicOldMajor.class", + "./test-data/ldc/", + "class file version does not support a constant tag it carries", + ), + ( + "test-data/ldc/LdcDynamicBSMArgPastEnd.class", + "./test-data/ldc/", + "a bootstrap method argument names nothing or is not a loadable constant", + ), + ] { + let err = run_class(Path::new(fixture), &[Path::new(directory)], &[]).await.unwrap_err().to_string(); + + assert!( + err.contains("java.lang.ClassFormatError"), + "{fixture}: expected ClassFormatError, got: {err}" + ); + assert!(err.contains(cause), "{fixture}: expected the message to say {cause:?}, got: {err}"); + } +}