From 7f7302f7916086a95da783462f1a9b8f3008e34b Mon Sep 17 00:00:00 2001 From: jun0 Date: Fri, 25 Sep 2026 05:48:04 +0900 Subject: [PATCH] =?UTF-8?q?[rustjava-2026-09-19-adoption-audit-prune]=20ch?= =?UTF-8?q?ore:=2009-19=20=EC=98=88=EC=99=B8=20=ED=81=B4=EB=9E=98=EC=8A=A4?= =?UTF-8?q?=20=EC=82=AC=EC=8A=AC=20=EA=B5=B0=EC=82=B4=20=EC=A0=9C=EA=B1=B0?= =?UTF-8?q?=20=E2=80=94=20=EC=B6=9C=EB=A0=A5=EC=88=9C=EC=84=9C=20=EA=B2=80?= =?UTF-8?q?=EC=82=AC=EA=B8=B0=20=EC=82=AD=EC=A0=9C=20=C2=B7=20=EB=B8=94?= =?UTF-8?q?=EB=9D=BC=EC=9D=B8=EB=93=9C=20=EC=8A=A4=ED=8C=9F=20=EB=B3=B4?= =?UTF-8?q?=EA=B3=A0=20=EC=B6=95=EC=86=8C=20=C2=B7=20=EB=82=A1=EC=9D=80=20?= =?UTF-8?q?=EC=A3=BC=EC=84=9D=20=EC=A0=95=EC=A0=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - scripts/check-script-output-order.py + CI 잡 script_output_order + DoD 줄 삭제(같은 커밋 — dod_parity) - check-named-exception-classes-are-loadable.py: 블라인드 스팟 보고 제거, 스캔을 NAMED.finditer 한 번으로 - rust.yml·스크립트 머리의 «unwrap → panic» 서술 정정(#76 이후 거짓) - jvm.rs 회차 이력 주석 → 불변식만 - test_exception_fallback_recursion: asked == 2 → asked < 20(가드 깊이 대신 «상한 전 정지») Co-Authored-By: Claude Opus 5.5 --- .github/workflows/rust.yml | 15 +- CLAUDE.md | 1 - .../2026-09-25-adoption-audit-prune.md | 19 ++ jvm/src/jvm.rs | 48 ++--- .../test_exception_fallback_recursion.rs | 7 +- ...ck-named-exception-classes-are-loadable.py | 145 ++------------- scripts/check-script-output-order.py | 167 ------------------ 7 files changed, 53 insertions(+), 349 deletions(-) create mode 100644 docs/worklog/2026-09-25-adoption-audit-prune.md delete mode 100644 scripts/check-script-output-order.py diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index d7701949..0e1f8940 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -81,18 +81,9 @@ jobs: - uses: actions/checkout@v7 - run: python3 scripts/check-dod-ci-parity.py - # a checker that iterates a set prints its findings in a different order on different runs, so two - # rounds cannot diff their output — and every other axis stays green while it does. This reads the - # checkers' AST rather than re-running them. One runner, not the matrix. - script_output_order: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v7 - - run: python3 scripts/check-script-output-order.py - - # Jvm::exception unwraps new_class(), so naming a class the loader cannot resolve panics instead - # of throwing. This compares the names against the registered protos (see the script's docstring - # for what it cannot see). One runner, not the matrix. + # Jvm::exception returns whatever new_class() failed with, so naming a class the loader cannot + # resolve raises NoClassDefFoundError instead of the intended exception. This compares the literal + # names against the registered protos. One runner, not the matrix. named_exception_classes: runs-on: ubuntu-latest steps: diff --git a/CLAUDE.md b/CLAUDE.md index 2effba1a..d83ee6a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,7 +33,6 @@ python3 scripts/check-dod-ci-parity.py python3 scripts/check-named-exception-classes-are-loadable.py python3 scripts/check-merge-dropped-symbols.py - python3 scripts/check-script-output-order.py ``` ★★**이 블록은 이제 «기계가 지킨다» — `scripts/check-dod-ci-parity.py`(CI job `dod_parity`)가 이 코드블록과 `rust.yml` 을 «각각 파싱해» 대칭차를 낸다.** 어긋나면 그 자리에서 red 다. diff --git a/docs/worklog/2026-09-25-adoption-audit-prune.md b/docs/worklog/2026-09-25-adoption-audit-prune.md new file mode 100644 index 00000000..6faf24ac --- /dev/null +++ b/docs/worklog/2026-09-25-adoption-audit-prune.md @@ -0,0 +1,19 @@ +## [2026-09-25] 09-19 예외 클래스 사슬 군살 제거 — 검사기 1개 삭제 · 보고 1개 축소 · 낡은 주석 정정 (rustjava-2026-09-19-adoption-audit-prune) + +- **무엇을**: `scripts/check-script-output-order.py` 와 그 CI 잡 `script_output_order`, DoD 줄을 지웠다. + `check-named-exception-classes-are-loadable.py` 에서 블라인드 스팟 보고(비리터럴 호출처 계수)를 빼고 + 스캔을 `NAMED.finditer` 한 번으로 줄였다. `rust.yml`·스크립트 머리의 «unwrap → panic» 서술(#76 이후 거짓)과 + `jvm.rs` 의 회차 이력 주석을 불변식만 남기고 줄였다. `test_exception_fallback_recursion.rs` 의 `asked == 2` 를 + `asked < 20` 으로 풀었다. +- **왜**: 사슬 8 PR 에서 검사기가 잡은 것 0(rust.yml 최근 100회 전부 success) · 출력순서 검사기는 현 위반 0에 + 후속 제안도 기각됐고, 블라인드 스팟 보고가 가리키는 1곳은 테스트다. 주석의 회차 이력은 git/worklog 가 이미 가진다. +- **사용자 영향**: 없음 — 제품 동작 변경 0. 검사기 출력은 블라인드 스팟 3~4줄이 빠지고 판정 줄(43 / 852 / 268)은 같다. + +### 판정 메모 +- `asked == 1`(두 곳)은 **남겼다**: 카운터는 숨긴 이름에 대한 질의만 센다(`test-utils` `HidesOneClass`) — + «첫 질의에서 멈추고 다시 묻지 않는다»가 불변식 자체다. `asked == 2` 는 반복 가드가 «몇 번째에» 걸리는지를 + 고정하므로 «상한(20) 전에 멈췄다»로 완화했다. +- 후속 제안 접기(계약 6): 이 사슬(`2026-09-19-*`·`2026-09-20-*`)의 제안은 **전건 이미 처분**돼 있다 + (12건 = 채택 10 · 09-21 기각 2) ⇒ 접을 카드 0, `.json` 을 쓰지 않았다. +- 회차 기록은 이 파일에만 적었다 — `REPORT.md`·`STATE.md` 를 동결하는 PR #97 이 열려 있어, 거기 적으면 + 어느 쪽이 먼저 착지하든 충돌 또는 해시 잠금 red 가 된다. diff --git a/jvm/src/jvm.rs b/jvm/src/jvm.rs index 83800461..207ecf90 100644 --- a/jvm/src/jvm.rs +++ b/jvm/src/jvm.rs @@ -93,15 +93,8 @@ impl Jvm { "java/lang/Class", ]; for class_name in bootstrap_classes.iter() { - // Fails like the closure walk below, and for the same reason -- nothing can be *raised* - // yet, this is what loads the classes an exception is made of -- so the error is - // `Unraisable` rather than a Java exception. It has to say which name it was, which the - // `unwrap` this once was did not: a host with a gap in its class set read - // `called Option::unwrap() on a None value` and had to bisect this list to find out - // which of six. Measured by last round's sweep: 5 of the 12 named refusals were this - // line, and two of those names (`java/lang/Object`, `java/io/Serializable`) are also in - // the error path's closure, so the same gap got a good message or a useless one - // depending only on which loop reached it first. + // Nothing can be *raised* yet -- these are the classes an exception is made of -- so a gap + // is `Unraisable`, and it names the class so the host does not have to bisect this list. let Some(class_definition) = jvm.inner.bootstrap_class_loader.load_class(&jvm, class_name).await? else { return Err(JavaError::Unraisable(format!( "the class set has no {class_name}, which is one of the {} classes loaded before \ @@ -130,35 +123,14 @@ impl Jvm { // Everything the error path needs before anything at all can be raised. // - // `Jvm::exception` builds a message with `java/lang/String` and then an instance of - // `java/lang/NoClassDefFoundError`, so a class set missing either cannot report even its own - // gap -- reporting the absent String needs a String, and reporting the absent reporter needs - // the reporter. Two earlier rounds each closed one of those names and each found the next by - // reading the code and guessing. Then the whole question was measured at once, by hiding every - // name construction asks the loader for, one at a time (`jvm/tests/test_error_path_class_sweep.rs`, - // 37 non-array names): five *more* recurse with no floor -- `java/lang/Throwable`, - // `java/lang/Error`, `java/lang/LinkageError`, `java/lang/CharSequence` and - // `java/lang/Comparable` -- and they are exactly the supertype and interface closure of those - // two. Of course they are: resolving a class resolves its supertypes, and a gap found *there* - // is reported with the very classes still being resolved. The closure is 9 names and the - // account of it is complete: 2 were already checked, 5 recursed, and the remaining 2 - // (`java/lang/Object`, `java/io/Serializable`) are in `bootstrap_classes` above, so they fail - // before this runs -- naming themselves there, as of the round that closed that gap. - // - // So the closure is what is checked, not a list someone has to remember to extend. Asked of - // the loader directly and never through `resolve_class`, because the reporting path cannot - // report *this* failure: building the report is the thing that is missing. A bare - // `resolve_class` here would hand the question to `Jvm::exception`, which is the cycle itself - // -- measured, that is still `stack overflow, aborting`, only during construction instead of - // later. - // - // Start-up cost, counted rather than timed (the previous round measured wall clock here and - // discarded it as below this host's noise): the walk asks the loader 9 times where the two - // asserts it replaces asked twice, so construction goes from 44 loader questions to 51. - // - // Here rather than in `bootstrap_classes` above, and before the properties loop below: that - // loop is the first thing in construction that needs a String, and resolution runs class - // initialisation, which needs the thread attached above. + // `Jvm::exception` builds a `java/lang/String` message and a `java/lang/NoClassDefFoundError` + // instance, and resolving those resolves their supertype and interface closure. A gap anywhere + // in that closure cannot be reported -- the report needs the missing class -- so it would + // recurse until the stack overflows. The closure is walked here instead of a fixed list, and + // asked of the loader directly: `resolve_class` would hand the failure to `Jvm::exception`, + // which is the cycle itself. It runs after the thread is attached (initialisation needs it) and + // before the properties loop below, the first thing that needs a String. + // `jvm/tests/test_error_path_class_sweep.rs` hides each name construction asks for. let mut pending = Vec::from(["java/lang/String".to_owned(), "java/lang/NoClassDefFoundError".to_owned()]); let mut asked = HashSet::new(); while let Some(class_name) = pending.pop() { diff --git a/jvm/tests/test_exception_fallback_recursion.rs b/jvm/tests/test_exception_fallback_recursion.rs index da048f81..34a0b344 100644 --- a/jvm/tests/test_exception_fallback_recursion.rs +++ b/jvm/tests/test_exception_fallback_recursion.rs @@ -63,8 +63,9 @@ async fn a_class_set_missing_a_bootstrap_class_is_an_error_naming_it() { // that calls `fillInStackTrace` again. Before `Jvm::exception` refused to build an exception it was // already building on the same thread, this cap (the sweep's 20) overflowed the default 2 MiB test // stack -- `stack overflow, aborting`, rc 134 -- and the run said nothing about the first failure. Now -// the repeat returns `Unraisable` naming the exception the thread started with, and the loader is -// asked twice: once for the first failure, once by the construction that repeated it. +// the repeat returns `Unraisable` naming the exception the thread started with. The invariant is +// that this happens before the cap: how many questions the guard needs first is its business +// (today 2 -- the first failure and the repeat), and pinning that number would pin the guard's depth. #[tokio::test] async fn a_failure_while_raising_is_reported_instead_of_recursing() { let (message, asked) = unraisable_hiding("[Ljava/lang/String;", 20).await; @@ -72,5 +73,5 @@ async fn a_failure_while_raising_is_reported_instead_of_recursing() { message.contains("into raising java/lang/NoClassDefFoundError ([Ljava/lang/String;), which is the first failure"), "{message}" ); - assert_eq!(asked, 2); + assert!(asked < 20, "asked {asked} times: the guard did not stop it before the cap"); } diff --git a/scripts/check-named-exception-classes-are-loadable.py b/scripts/check-named-exception-classes-are-loadable.py index 40f3f187..a47994c5 100755 --- a/scripts/check-named-exception-classes-are-loadable.py +++ b/scripts/check-named-exception-classes-are-loadable.py @@ -1,27 +1,11 @@ #!/usr/bin/env python3 """Every exception class the Rust code names by string must be one the runtime can load. -What this answers: *will an error path throw, or panic*. `Jvm::exception` (jvm/src/jvm.rs) ends in - - let instance = self.new_class(r#type, "(Ljava/lang/String;)V", (message_str,)).await.unwrap(); - -Since 2026-09-19 (`rustjava-jvm-exception-throws-instead-of-unwrap`) that `.unwrap()` is gone: a name -the loader cannot resolve now returns the NoClassDefFoundError the loader raised, rather than aborting -the process. The check did not lose its job, it changed: an unresolvable name no longer kills the -runtime, it silently raises *the wrong exception* -- the caller asked for IOException and gets -NoClassDefFoundError, so the `catch` that was supposed to handle it does not match. That is quieter -than a crash and therefore worth locking, and it is invisible until something walks that path. It is also self-referential: jvm.rs:842 reports a missing class by -calling `exception("java/lang/NoClassDefFoundError", ...)`, so the error path's own class has to be -loadable or the report itself panics. - -Measured after gate 2 corrected three defects in the first draft: 43 distinct `java/`-prefixed -names across 846 `exception(...)` call sites, and 268 distinct class names registered in the loader. -Nothing is missing -- the baseline is 0, which is what makes the lock cheap. The first draft read -41 / 812 / 263 and every one of those was an undercount: it scanned line by line (losing 34 calls -rustfmt had broken across a newline), matched only `as_proto()` (losing three `list_proto()` -registrations), and keyed types by bare name (letting one of a colliding pair answer for the -other). The one that was missing before, `java/lang/BootstrapMethodError`, -is the reason this exists: removing its registration is the round-trip test (see below). +A name the loader cannot resolve does not crash: `Jvm::exception` (jvm/src/jvm.rs) returns whatever +`new_class()` failed with, so the caller asked for IOException and gets NoClassDefFoundError, and the +`catch` that was supposed to handle it does not match. That is quiet and invisible until something +walks that path, so it is locked here. The baseline is 0 missing names, which keeps the lock cheap. +`java/lang/BootstrapMethodError` was the one missing before this existed. HOW THE TWO SETS ARE BUILT named every string literal in the first argument of an `exception(` call, in any *.rs in the @@ -33,35 +17,14 @@ three are needed. WHAT THIS DOES NOT SEE -- it is a floor, not a proof: - * A name built at run time (`format!`, a `const`, a variable, a match arm returning &str) is not a - literal at the call site, so it is invisible here. Only the literal spelling is checked. - THIS ONE IS NOW COUNTED AND PRINTED, AND DELIBERATELY NOT FAILED ON -- decided 2026-09-19, - `2026-09-18-nonliteral-exception-call-sites-p0`, adopting the proposal of the same name. Do not - "finish the job" by turning that count into a non-zero exit; the proposal asked for exactly that - and both of its premises had expired by the time it was picked up: - - "now that the baseline is 0" -- it is 1. `jvm/tests/test_exception_construction.rs` has to - pass an unloadable name through a variable, because a literal there makes *this* check red. - A gate at zero would have been red on main the day it was written, against a site that is - correct. The proposal predicted this exact cost in its own `why` field. - - "instead of crashing the whole runtime" -- since `rustjava-jvm-exception-throws-instead-of- - unwrap` landed, an unloadable name does not crash. It raises NoClassDefFoundError instead of - the intended exception: a `catch` that does not match, which is a wrong behaviour and not a - dead process. - So the harm is real but smaller, and the gate's own cost is now paid up front rather than - hypothetically. Printing the number keeps the floor measured between rounds -- which is what the - proposal was actually worried about -- without turning a correct test into a build failure. - What would change the answer: a run-time-assembled name appearing in *product* code (every site - today is a test), or the count growing without anyone noticing it grew. + * A name built at run time (`format!`, a `const`, a variable) is not a literal at the call site, + so it is invisible here. Only the literal spelling is checked. * Only `exception(` is scanned. A class named through `new_class(` or `find_class(` directly is - not covered; those paths return Result to their caller rather than unwrapping, which is why the - panic axis is this one. + not covered; those paths return Result to their caller. * A registered proto whose class fails to *initialise* at run time still loads here. This checks resolvability of the name, not the health of the class. * Names are compared as written. A typo that happens to match another real class passes. -DONE SEPARATELY, as its own axis: `Jvm::exception` reporting instead of unwrapping -(`jvm/tests/test_exception_construction.rs`). This check is what keeps that report from being needed. - Exit: 0 every named class is loadable 1 at least one named class has no registered proto 2 could not measure (missing file, or a registered entry whose name cannot be resolved) @@ -93,18 +56,6 @@ # `pub fn as_proto() -> RuntimeClassProto { … }` inside such a block. PROTO_FN = re.compile(r"pub fn ([a-z_]+)\(\)\s*->\s*RuntimeClassProto\s*\{(.*?)\n \}", re.S) NAME_FIELD = re.compile(r'name:\s*"([^"]+)"') -# Every `exception(` site, literal or not, so the blind spot can be counted rather than assumed. -# The prefix group matters: `exception(` is a substring of eight helper functions in the test trees -# (`assert_exception(`, `suppress_io_exception(`, …, 41 sites) whose first parameter is `jvm`, not a -# class name. Counting those as run-time-assembled names answers 33 where the answer is 1. -# Anchored on the literal so the regex engine can use a fast substring search: the earlier form -# `[A-Za-z0-9_]*exception\(` made it try every word-character position and doubled the check's -# runtime. Whether the site is a *bare* `exception(` is decided by looking at the preceding -# character instead, which is the same question and costs nothing. -ANY_SITE = re.compile(r'exception\(\s*') -IDENT_CHARS = frozenset("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_") -FIRST_ARG_LITERAL = re.compile(r'"(?:[^"\\]|\\.)*"') -IS_DEFINITION = re.compile(r"\bfn\s+$") # The independent witness for "did the parse come up short". `REGISTERED` is the pattern under @@ -138,71 +89,24 @@ def rust_files(): yield entry -_SCAN_CACHE = None - - -def scan_call_sites(): - """Both axes in one walk: {literal name: [site, ...]} and [non-literal site, ...]. +def named_classes(): + """{class name: [file:line, ...]} for every literal exception(...) name. - One pass because the two used to be two, and reading every *.rs twice tripled the check - (measured 3.3-4.3 s -> 10.0-13.3 s). Cached because `main()` asks for both. + Matched against the whole file rather than line by line, because rustfmt breaks a long call + after `exception(` and the pattern's `\\s*` has to cross that newline. No prefix filter: a call + written `raise_exception("java/lang/X", …)` names a class too, and the helpers whose first + argument is `jvm` cannot match `NAMED` anyway. """ - global _SCAN_CACHE - if _SCAN_CACHE is not None: - return _SCAN_CACHE - named, blind = {}, [] + found = {} for path in rust_files(): text = read(path) rel = path.relative_to(ROOT) - for match in ANY_SITE.finditer(text): - if IS_DEFINITION.search(text[max(0, match.start() - 12) : match.start()]): - continue - # `count` rather than an index of newline offsets: there are ~850 matches in the whole - # tree, and building a per-character index for every file cost more than it saved - # (measured: it was the regression, not the scan). + for match in NAMED.finditer(text): line = text.count("\n", 0, match.start()) + 1 - literal = NAMED.match(text, match.start()) - if literal: - # ★ No prefix filter on this branch, and that is the point. This is the gate's input - # set, and `NAMED` already requires `("java…` right after the paren -- which the - # helper functions cannot satisfy, because their first argument is `jvm`. Filtering - # here buys nothing (measured: 43 / 846 / 268 either way) and costs coverage: a call - # written `raise_exception("java/lang/X", …)` would be dropped from the gate *and* - # from the blind-spot report, so a class the runtime cannot load would read as - # `✓ all loadable`. Measured on the form that filtered here: rc 0 against an injected - # `raise_exception("java/lang/TotallyUnloadableProbe", …)` that the previous version - # caught with rc 1. - named.setdefault(literal.group(1), []).append(f"{rel}:{line}") - elif match.start() and text[match.start() - 1] in IDENT_CHARS: - continue # assert_exception( and friends: a different function, first argument `jvm` - elif not FIRST_ARG_LITERAL.match(text[match.end() :]): - blind.append(f"{rel}:{line}") - _SCAN_CACHE = (named, blind) - return _SCAN_CACHE - - -def named_classes(): - """{class name: [file:line, ...]} for every literal exception(...) name. - - Matched against the whole file rather than line by line, because rustfmt breaks a long call - after `exception(` and the pattern's `\\s*` has to cross that newline. Line numbers are - recovered from the match offset so the report still points at a place. - """ - found, _ = scan_call_sites() + found.setdefault(match.group(1), []).append(f"{rel}:{line}") return found -def runtime_assembled_sites(): - """`Jvm::exception` call sites whose class name is *not* a literal -- this check's blind spot. - - Reported, never failed on. The decision not to gate it is recorded in the docstring above; the - number is printed so that "how big is the blind spot" stops being something a round has to go - and measure before it can answer. - """ - _, sites = scan_call_sites() - return sites - - def loadable_classes(): """Names the bootstrap loader can return, i.e. the registered protos. @@ -282,26 +186,11 @@ def loadable_classes(): return names -def report_blind_spot(): - """One line, always, whether the check passes or fails. Never changes the exit code.""" - sites = runtime_assembled_sites() - if not sites: - print("Blind spot: 0 call sites build the class name at run time -- everything below is checked.") - return - print(f"Blind spot: {len(sites)} call site(s) build the class name at run time, so this check") - print("does not see them. Not an error -- an unloadable name there raises NoClassDefFoundError") - print("rather than the intended exception, which is a wrong catch, not a crash:") - for site in sites: - print(f" ? {site}") - - def main(): named = named_classes() loadable = loadable_classes() missing = sorted(name for name in named if name not in loadable) - report_blind_spot() - if missing: print(f"{len(missing)} named exception class(es) the runtime cannot load:") for name in missing: diff --git a/scripts/check-script-output-order.py b/scripts/check-script-output-order.py deleted file mode 100644 index 856f4573..00000000 --- a/scripts/check-script-output-order.py +++ /dev/null @@ -1,167 +0,0 @@ -#!/usr/bin/env python3 -"""Refuse a set read in a position whose order reaches a checker's output. - -Why: on 2026-09-19 `check-merge-dropped-symbols.py` iterated a set of paths, so two runs of the -same command printed the same six findings in two different orders — Python's per-process string -hash seed decides. A before/after diff read as a regression until the unchanged version was shown -to disagree with itself. The fix was one word, `sorted(...)`, and nothing locks it: measured that -round, removing it again leaves `cargo fmt`, `cargo clippy`, `cargo test` and all four python -checkers green. The guard was zero, which is why this file exists. - -What it asserts, in one line a reader can check: **in `scripts/*.py`, no set is read in one of the -three positions whose order reaches output — the iterable of a `for` or a comprehension, the first -argument of `str.join`, or a `*`-unpacking — unless it is inside `sorted(...)`.** - -The last two were added after a review wrote `", ".join(myset)` and `print(*myset)` and watched both -pass. That is the 2026-09-19 incident exactly, minus the loop: a set of paths printed in hash order. -Three positions is not "every position", and the sentence above says so rather than promising a -coverage this does not have — the list below is the rest. - -Why static rather than re-running under two `PYTHONHASHSEED`s: an unordered set is only *visibly* -unordered when the hash order happens to differ from the sorted one, so a two-run comparison is a -coin flip per run — with the two paths of the real incident it agrees with itself about half the -time. This axis has no such gap: it does not need the bug to be observable to see it, it costs no -re-run, and it fires on a *new* set appearing anywhere in these files, not only on the one line the -2026-09-19 round fixed. - -Blind spots — listed rather than implied, and none of them is a claim of safety: - * **Order laundered through a container is not followed.** `d = dict.fromkeys(myset)` and then - `for k in d` keeps the set's order and passes; so do `y = list(myset)` then `for x in y`, and - `bag["k"] = myset` then `for x in bag["k"]`. An earlier version of this file said "a set feeding - a dict is caught at the set" — that was **false**, shown by a review that ran it, and a wrong - blind-spot entry is worse than a missing one because a reader takes it as a guarantee. Measured - on this tree: `dict.fromkeys` appears **0 times**, so this is a hole in the promise and not a - live miss. Closing it properly means following order taint through containers, which is a - different program from this one; doing `fromkeys` alone would buy the *look* of that program for - two lines, which is the failure this paragraph is about. - * **Set-ness is inferred syntactically** — a `set()`/`{…}`/set comprehension, a set *operator*, or - a name or local function carrying one. So a set that arrives some other way is missed: a - tuple-unpacked call return, an import, a parameter. One such name exists today, - `ci_runs, ci_tcs = parse_ci(...)` in `check-dod-ci-parity.py` — read only through `sorted()` or a - set operator, so nothing is wrong there now, but an unsorted read of it would pass. Method- - spelled set operations (`a.difference(b)`) are not read either, only the operators (`a - b`). - * **Names have no scope.** `found = set()` in one function and `found = [...]` in another make the - *list* read go red. No such collision exists today, but `found` is one of the names from the - original incident, so the false red is reachable — and a false red invites a wrong `sorted()`, - which is a worse outcome than a miss. - * **Draining is not reading.** `while s: s.pop()` takes a set in hash order and passes (0 today). - * Every `scripts/*.py`, not just the CI checkers: the surveys get diffed across rounds too, and - they cost nothing to include — all of them pass today. - -Exit: 0 nothing found, 1 at least one unordered read, 2 the files it checks are not there. -""" - -import ast -import sys -from pathlib import Path - -ROOT = Path(__file__).resolve().parents[1] -SET_OPS = (ast.BitOr, ast.BitAnd, ast.Sub, ast.BitXor) - - -def produces_set(node, names, funcs): - """Is this expression a set, as far as syntax can tell.""" - if isinstance(node, (ast.Set, ast.SetComp)): - return True - if isinstance(node, ast.Call) and isinstance(node.func, ast.Name): - return node.func.id in ("set", "frozenset") or node.func.id in funcs - if isinstance(node, ast.BinOp) and isinstance(node.op, SET_OPS): - return produces_set(node.left, names, funcs) or produces_set(node.right, names, funcs) - if isinstance(node, ast.Name): - return node.id in names - return False - - -def set_values(tree): - """(names, functions) that carry a set. Iterated to a fixpoint because one feeds the other: - `found = set()` makes `symbols()` a set-returning function, which makes `theirs_symbols` a set. - """ - names, funcs = set(), set() - growing = True - while growing: - before = len(names) + len(funcs) - for node in ast.walk(tree): - if isinstance(node, ast.Assign): - pairs = [] - if isinstance(node.value, ast.Tuple) and isinstance(node.targets[0], ast.Tuple): - pairs = list(zip(node.targets[0].elts, node.value.elts)) # a, b = set(), [] - else: - pairs = [(t, node.value) for t in node.targets] - for target, value in pairs: - if isinstance(target, ast.Name) and produces_set(value, names, funcs): - names.add(target.id) - elif isinstance(node, ast.AugAssign) and isinstance(node.target, ast.Name): - if produces_set(node.value, names, funcs): - names.add(node.target.id) - elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): - for inner in ast.walk(node): - if isinstance(inner, ast.Return) and inner.value is not None and produces_set(inner.value, names, funcs): - funcs.add(node.name) - growing = len(names) + len(funcs) > before - return names, funcs - - - -def order_reaching_output(tree): - """Every expression whose element order can end up in a printed line. - - Three shapes, and the docstring's promise is exactly these three: what a `for` or comprehension - walks, what `str.join` is handed, and what a `*` spreads. A `Starred` in a target - (`a, *rest = ...`) is a Store and is not one of them. - """ - for node in ast.walk(tree): - if isinstance(node, (ast.For, ast.AsyncFor)): - yield node.iter - for generator in getattr(node, "generators", []): - yield generator.iter - if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "join" and node.args: - yield node.args[0] - if isinstance(node, ast.Starred) and isinstance(node.ctx, ast.Load): - yield node.value - - -def unordered_reads(tree, names, funcs): - """[(line, source)] for every set read in one of those positions without a sorted() over it.""" - found = [] - for iterable in order_reaching_output(tree): - pending = [iterable] - while pending: - node = pending.pop() - if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "sorted": - continue # anything under a sorted() is ordered, however it was built - if produces_set(node, names, funcs): - found.append((iterable.lineno, ast.unparse(iterable))) - break - pending.extend(ast.iter_child_nodes(node)) - return sorted(found) - - -def main(): - scripts = sorted((ROOT / "scripts").glob("*.py")) - if not scripts: - # A move must not turn this into a green run over nothing — the sibling lineage's - # whole subject is checks that pass by looking at zero things. - print("cannot measure: no scripts/*.py to read", file=sys.stderr) - return 2 - - total = 0 - for path in scripts: - tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) - names, funcs = set_values(tree) - bad = unordered_reads(tree, names, funcs) - for line, source in bad: - print(f"✗ {path.relative_to(ROOT)}:{line}: reads a set — two runs can print this in two orders") - print(f" {source}") - print(" wrap it in sorted(), as check-merge-dropped-symbols.py does") - total += len(bad) - if not bad: - print(f" ✓ {path.relative_to(ROOT)}") - - # The count names the three positions it looked at rather than claiming "could reach output": - # those are not the same set, and the docstring's blind spots are the difference. - print(f"{len(scripts)} script(s): {total} set(s) read unordered in a for/comprehension, str.join or *unpacking") - return 1 if total else 0 - - -if __name__ == "__main__": - sys.exit(main())