navicore/patch-seq

Fork 0

no-dead-code-1 #442

Merged

navicore merged 3 commits from no-dead-code-1 into main

2026-04-30 16:28:17 +00:00

navicore commented

2026-04-30 15:30:28 +00:00

Owner

No description provided.

navicore added 2 commits

2026-04-30 15:30:28 +00:00

ready for linux test 8ab06ca5ac

no dead code

CI - Linux / CI - Linux x86_64 (pull_request) Failing after 17s

Details

Claude Code Review / claude-review (pull_request) Failing after 35s

Details

c7bbbd83b0

navicore commented

2026-04-30 15:36:31 +00:00

Author

Owner

Review

Mostly LGTM — the link-time GC approach is clean and the new no_dead_code test pins the invariant well. A few items worth attention:

Doc/code divergence — `BATTERIES_INCLUDED.md` vs runtime `Cargo.toml`

The rewritten docs/BATTERIES_INCLUDED.md (new "Binary Contents" section) makes strong claims:

"There are no seqc flags for capability selection. There are no source annotations."
"The runtime is always built with everything on, and the link removes what the program does not reference."

But the feature-flag mechanism is still alive:

crates/runtime/Cargo.toml:16-41 still defines crypto / http / regex / compression / full features.
crates/runtime/src/lib.rs still gates modules on #[cfg(feature = "crypto")], #[cfg(feature = "http")], etc. (lines 55–72, 172–221, 347–352).

So capability selection is still possible (e.g. cargo build --no-default-features --features crypto); the doc rewrite presents it as if it isn't. Either the docs are running ahead of a planned cleanup that removes those #[cfg]s and feature stanzas, or the docs need to keep at least a brief note that the cargo features still exist as a knob even though they're not the headline mechanism. Worth deciding before this lands so the two stay in sync.

`crates/compiler/src/lib.rs:387-396` — unconditional dead-strip on non-macOS

if cfg!(target_os = "macos") {
    clang.arg("-Wl,-dead_strip");
} else {
    clang.arg("-Wl,--gc-sections");
}

This is fine for the only platforms in CI today (Linux, macOS), but the else branch fires for any non-macOS target, including Windows-MSVC. clang on Windows targeting link.exe does not accept -Wl,--gc-sections and the link will fail. Not a blocker if Windows is explicitly out of scope, but it's now a hard build failure if someone tries it. Consider making the Linux branch explicit (target_os = "linux", or an env override) so the behavior on unsupported targets is at least obvious.

`crates/compiler/tests/no_dead_code.rs` — release-profile assumption is implicit

The test invokes seqc build and inspects the resulting binary, but the no-dead-code result depends on the workspace's release profile (lto = true, codegen-units = 1) being applied to the runtime archive seqc links against. The justfile recipe runs the test with cargo test --release … which keeps everything consistent; running it manually as cargo test -p seq-compiler --test no_dead_code (no --release) will use a debug build of seqc and a debug runtime archive, where --gc-sections will leave a much larger residue and the assertion will fail in confusing ways.

A short comment in the test, or a debug_assertions-gated early return, would save the next person a debugging session.

`crates/compiler/src/error_flag_lint/analyzer.rs:200-239` — clippy refactor is semantics-preserving

I traced this carefully because converting arm => { if guard { … } } into arm if guard => { … } changes match fall-through semantics in the general case: when the guard fails, the new form continues to subsequent arms, while the old form silently no-ops. Here it's safe — no other named arm matches "over" or "2dup", so a guard miss falls through to the catch-all _ => { /* no-op */ }, which is the same effect as the original empty if. Worth a one-line comment if you expect future contributors to add new arms above the catch-all, since adding e.g. an _ if … => arm earlier in the match could quietly start handling these names with the unintended branch.

Minor

crates/runtime/src/report.rs:271 — sort_by_key(|b| Reverse(b.1)) is equivalent to the old sort_by(|a,b| b.1.cmp(&a.1)) and a bit more idiomatic. No issue.
docs/design/done/BINARY_FOOTPRINT.md is helpful but contains anecdotal byte counts (e.g. "~250K backtrace symbolizer") that will drift as the toolchain changes. Not worth blocking on, but worth a sentence noting the date the measurements were taken (the headline table already does this for 2026-04-29; the per-category numbers don't).

Nothing in the diff that I'd block on. The doc-vs-code divergence on feature flags is the one I'd resolve before merge.

## Review Mostly LGTM — the link-time GC approach is clean and the new `no_dead_code` test pins the invariant well. A few items worth attention: ### Doc/code divergence — `BATTERIES_INCLUDED.md` vs runtime `Cargo.toml` The rewritten `docs/BATTERIES_INCLUDED.md` (new "Binary Contents" section) makes strong claims: > "There are no `seqc` flags for capability selection. There are no source annotations." > "The runtime is always built with everything on, and the link removes what the program does not reference." But the feature-flag mechanism is still alive: - `crates/runtime/Cargo.toml:16-41` still defines `crypto` / `http` / `regex` / `compression` / `full` features. - `crates/runtime/src/lib.rs` still gates modules on `#[cfg(feature = "crypto")]`, `#[cfg(feature = "http")]`, etc. (lines 55–72, 172–221, 347–352). So *capability selection* is still possible (e.g. `cargo build --no-default-features --features crypto`); the doc rewrite presents it as if it isn't. Either the docs are running ahead of a planned cleanup that removes those `#[cfg]`s and feature stanzas, or the docs need to keep at least a brief note that the cargo features still exist as a knob even though they're not the headline mechanism. Worth deciding before this lands so the two stay in sync. ### `crates/compiler/src/lib.rs:387-396` — unconditional dead-strip on non-macOS ```rust if cfg!(target_os = "macos") { clang.arg("-Wl,-dead_strip"); } else { clang.arg("-Wl,--gc-sections"); } ``` This is fine for the only platforms in CI today (Linux, macOS), but the `else` branch fires for *any* non-macOS target, including Windows-MSVC. clang on Windows targeting `link.exe` does not accept `-Wl,--gc-sections` and the link will fail. Not a blocker if Windows is explicitly out of scope, but it's now a hard build failure if someone tries it. Consider making the Linux branch explicit (`target_os = "linux"`, or an env override) so the behavior on unsupported targets is at least obvious. ### `crates/compiler/tests/no_dead_code.rs` — release-profile assumption is implicit The test invokes `seqc build` and inspects the resulting binary, but the no-dead-code result depends on the workspace's release profile (`lto = true`, `codegen-units = 1`) being applied to the runtime archive `seqc` links against. The justfile recipe runs the test with `cargo test --release …` which keeps everything consistent; running it manually as `cargo test -p seq-compiler --test no_dead_code` (no `--release`) will use a debug build of `seqc` and a debug runtime archive, where `--gc-sections` will leave a much larger residue and the assertion will fail in confusing ways. A short comment in the test, or a `debug_assertions`-gated early return, would save the next person a debugging session. ### `crates/compiler/src/error_flag_lint/analyzer.rs:200-239` — clippy refactor is semantics-preserving I traced this carefully because converting `arm => { if guard { … } }` into `arm if guard => { … }` changes match fall-through semantics in the general case: when the guard fails, the new form continues to subsequent arms, while the old form silently no-ops. Here it's safe — no other named arm matches `"over"` or `"2dup"`, so a guard miss falls through to the catch-all `_ => { /* no-op */ }`, which is the same effect as the original empty `if`. Worth a one-line comment if you expect future contributors to add new arms above the catch-all, since adding e.g. an `_ if … =>` arm earlier in the match could quietly start handling these names with the unintended branch. ### Minor - `crates/runtime/src/report.rs:271` — `sort_by_key(|b| Reverse(b.1))` is equivalent to the old `sort_by(|a,b| b.1.cmp(&a.1))` and a bit more idiomatic. No issue. - `docs/design/done/BINARY_FOOTPRINT.md` is helpful but contains anecdotal byte counts (e.g. "~250K backtrace symbolizer") that will drift as the toolchain changes. Not worth blocking on, but worth a sentence noting the date the measurements were taken (the headline table already does this for `2026-04-29`; the per-category numbers don't). Nothing in the diff that I'd block on. The doc-vs-code divergence on feature flags is the one I'd resolve before merge.

navicore added 1 commit

2026-04-30 16:24:58 +00:00

Green. All five review items addressed:

CI - Linux / CI - Linux x86_64 (pull_request) Failing after 17s

Details

Claude Code Review / claude-review (pull_request) Failing after 24s

Details

cddf6dd702

1. Test profile gate — crates/compiler/tests/no_dead_code.rs now
  #[cfg_attr(any(target_os = "macos", debug_assertions), ignore)]. just
  test skips it in debug; just check-binary-contents runs it in release.
  Linux CI no longer at risk of hitting the debug-runtime symbol residue.
  2. Linker branch tightened — crates/compiler/src/lib.rs now branches
  target_os = "macos" → -dead_strip, target_os = "linux" → --gc-sections,
  anything else → no flag (with a comment explaining why Windows-MSVC etc.
   fall through silently).
  3. Broken doc path — docs/design/done/NO_DEAD_CODE.md:185 now points at
  docs/design/done/BINARY_FOOTPRINT.md.
  4. BATTERIES_INCLUDED softened — kept the user-visible claim, added a
  brief note that the cargo features in crates/runtime/Cargo.toml still
  exist as a build-system internal but never reach seqc.
  5. analyzer.rs guard-arm comment — added a comment above the "over" if …
   arm explaining the catch-all fall-through assumption and what to watch
  for when adding new arms.

  just ci green on macOS. Linux should now stay green too since the test
  only runs in release where it was empirically validated.

navicore commented

2026-04-30 16:27:12 +00:00

Author

Owner

Re-review of `cddf6dd`

All five items resolved. Quick verification:

Test profile gate — #[cfg_attr(any(target_os = "macos", debug_assertions), ignore)] plus the new comment in crates/compiler/tests/no_dead_code.rs:45-55 is exactly the right shape: cargo test (debug) on Linux now silently skips with a clear reason rather than failing confusingly, and just check-binary-contents still enforces it under release.
Linker branch tightened — crates/compiler/src/lib.rs:392-396 now explicit on target_os = "linux" with the comment naming Windows-MSVC as the reason for the silent no-op fallback. Good. (Side effect: hypothetical FreeBSD/NetBSD users lose dead-stripping too even though GNU ld would accept --gc-sections — fine for now, easy to extend later if anyone asks.)
NO_DEAD_CODE.md path fix — docs/design/done/NO_DEAD_CODE.md:185 now points at the correct location.
BATTERIES_INCLUDED — The added blockquote at docs/BATTERIES_INCLUDED.md:82-87 resolves the divergence: it preserves the user-visible claim ("no seqc flags for capability selection") while accurately stating the cargo features still exist for direct runtime consumers. Doc and code are aligned.
analyzer.rs guard comment — crates/compiler/src/error_flag_lint/analyzer.rs:203-208 correctly warns about the fall-through assumption. Placement above "over" covers "2dup" too since they're the only guarded arms in the match.

Nothing further from me. LGTM.

## Re-review of `cddf6dd` All five items resolved. Quick verification: 1. **Test profile gate** — `#[cfg_attr(any(target_os = "macos", debug_assertions), ignore)]` plus the new comment in `crates/compiler/tests/no_dead_code.rs:45-55` is exactly the right shape: `cargo test` (debug) on Linux now silently skips with a clear reason rather than failing confusingly, and `just check-binary-contents` still enforces it under release. 2. **Linker branch tightened** — `crates/compiler/src/lib.rs:392-396` now explicit on `target_os = "linux"` with the comment naming Windows-MSVC as the reason for the silent no-op fallback. Good. (Side effect: hypothetical FreeBSD/NetBSD users lose dead-stripping too even though GNU ld would accept `--gc-sections` — fine for now, easy to extend later if anyone asks.) 3. **NO_DEAD_CODE.md path fix** — `docs/design/done/NO_DEAD_CODE.md:185` now points at the correct location. 4. **BATTERIES_INCLUDED** — The added blockquote at `docs/BATTERIES_INCLUDED.md:82-87` resolves the divergence: it preserves the user-visible claim ("no `seqc` flags for capability selection") while accurately stating the cargo features still exist for direct runtime consumers. Doc and code are aligned. 5. **analyzer.rs guard comment** — `crates/compiler/src/error_flag_lint/analyzer.rs:203-208` correctly warns about the fall-through assumption. Placement above `"over"` covers `"2dup"` too since they're the only guarded arms in the match. Nothing further from me. LGTM.

navicore merged commit a595d2ef60 into main

2026-04-30 16:28:17 +00:00

navicore referenced this pull request from a commit

2026-04-30 16:28:19 +00:00

Merge pull request 'no-dead-code-1' (#442) from no-dead-code-1 into main