Shake your foundation: make, don't panic & derive..#142
Open
Shake your foundation: make, don't panic & derive..#142
Conversation
Add a 'make help' target with awk-based extraction of target descriptions. Also adds standalone 'fmt', 'clippy', 'stdlib-smir', 'build-info', and nightly administration targets. Uses the final Makefile structure: targets for golden file management, UI testing, and nightly lifecycle are included but depend on scripts added in later commits.
Some types (e.g., dyn Trait in certain positions) cause rustc's layout computation to panic rather than returning an error. Wrap layout calls in catch_unwind so the type visitor can continue; panicked types are recorded and reported in a summary rather than crashing the whole run.
Instead of hardcoding the library path, resolve it from the active nightly toolchain at runtime. This avoids breakage when the toolchain directory name changes.
Drop the [metadata] section from rust-toolchain.toml; the UI test scripts now derive the rustc commit hash directly from the nightly date via the Rust manifest. Also fixes clippy uninlined_format_args warnings.
cds-amal
force-pushed
the
stack/pr1-foundation
branch
from
d14cc18 to
21a3ddd
Compare
The receipt-driven integration-test target requires receipts (PR 2) and per-nightly golden directories (PR 3). Revert to master's flat-file version so CI passes on the foundation PR.
There was a problem hiding this comment.
Pull request overview
This PR reduces hardcoded nightly metadata and makes tooling derive the active Rust nightly/commit dynamically, while also hardening the Stable MIR type collection against rustc-internal layout panics.
Changes:
- Removed
rust-toolchain.tomlmetadata forrustc-commitand derived the rustc commit fromrustc -vVin UI tooling and CI. - Added panic-catching around
ty.layout()to avoid crashing when rustc panics during layout computation, and surfaced a warning summary. - Expanded/organized Makefile targets (help, UI test utilities, nightly management, stdlib smir artifact generation) and improved portability in some scripts.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/ui/ensure_rustc_commit.sh |
Derives rustc commit from rustc -vV and fetches missing commits before checkout/worktree creation. |
src/printer/ty_visitor.rs |
Catches rustc panics during layout computation and records them for later reporting. |
src/printer/collect.rs |
Emits a warning summary when layout panics were encountered during collection. |
src/printer/schema.rs |
Modernizes an assert message using captured formatting. |
src/printer/items.rs |
Simplifies debug formatting with captured formatting syntax. |
src/bin/cargo_stable_mir_json.rs |
Derives toolchain library path via rustc --print sysroot instead of hardcoded nightly paths. |
rust-toolchain.toml |
Removes the [metadata] rustc-commit field. |
Makefile |
Adds help/quality/test/nightly/stdlib targets and improves structure/portability. |
.github/workflows/test.yml |
Reads nightly channel from rust-toolchain.toml and derives rustc commit dynamically for UI tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
TIL (thanks Copilot!), the previous approach temporarily replaced the
process-wide panic hook with a no-op to suppress backtraces from caught
layout panics. Turns out that's a thread-safety footgun: rustc's own
worker threads could panic while the no-op hook is installed, silently
swallowing unrelated diagnostics. The hook swap was also racy with
anything else that calls set_hook concurrently.
catch_unwind is what actually keeps the process alive; the hook swap was
purely cosmetic (suppressing stderr noise). Dropped it entirely and
accepted the default backtrace output for caught panics. The end-of-run
LayoutPanic summary still reports everything it did before. My research
surfaced a way to suppress the stderr noise that I decided was too much
for a little noise, but I'm including here for completeness.
Set teh hook once at startup (not per-call) to a hook that checks a
thread-local flag:
```rust
thread_local! {
static SUPPRESS_PANIC_OUTPUT: Cell<bool> = const { Cell::new(false) };
}
// Called once, e.g. in driver setup:
std::panic::set_hook(Box::new(|info| {
SUPPRESS_PANIC_OUTPUT.with(|flag| {
if !flag.get() {
eprintln!("{info}");
}
});
}));
// Then in try_layout_shape, just toggle the flag:
SUPPRESS_PANIC_OUTPUT.with(|f| f.set(true));
let result = catch_unwind(AssertUnwindSafe(|| ty.layout()...));
SUPPRESS_PANIC_OUTPUT.with(|f| f.set(false));
This is thread-safe (thread-local, not global), no race conditions, no
risk of swallowing other threads' panics, and our collected LayoutPanic
report at teh end works exactly as before.
If this script is sourced from a working directory outside the repo tree, rustup won't find rust-toolchain.toml and may select whatever default toolchain happens to be active, giving us the wrong commit hash. We now derive the repo root from BASH_SOURCE (two levels up from the script's own directory) and cd there before invoking rustc, so the toolchain selection stays correct regardless of the caller's CWD.
TOOLCHAIN_NAME defaults to empty in the Makefile, which meant make clean would always run rustup toolchain uninstall "" and fail. Now we only attempt the uninstall when the variable is actually set.
The workflow was downloading yq from GitHub releases without any integrity check (a supply-chain risk, however small, on CI runners). We now download the upstream checksums file alongside the binary and verify the SHA256 before installing. While we're at it, the identical 8-line install block was copy-pasted across all three jobs. Extracted into .github/scripts/install-yq.sh so there's exactly one place to update the version or change the verification logic.
The checksums file from mikefarah/yq uses a custom multi-hash-per-line format that isn't compatible with sha256sum -c (which expects GNU coreutils format). Switched to checksums-bsd, which uses the standard BSD-style "SHA256 (file) = hash" layout; a small sed converts that to GNU format for verification.
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
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.
This PR introduces independent improvements that lays the groundwork for robust multi-nightly
support. Each one stands alone, but together they establish the tooling and
resilience the rest of the stack builds on.
feat(make): self-documenting help target and modernized Makefile.
make helpnow extracts target descriptions via an awk one-liner, instead of complicated
make shellery. Also adds standalone
fmt,clippy,stdlib-smir, andbuild-infotargets. The Makefile establishes its final structure here (including stubs
for golden file management and UI testing that become functional in later
PRs); we write the API we desire and make it real in subsequent PRs.
fix(ty_visitor): catch
rustcpanics during layout computation. Some types(notably
dynTraitin certain positions) cause rustc's layout engine to panicrather than returning Err. Turns out
catch_unwindis the only viable optionhere; rustc doesn't surface these as recoverable errors. We wrap
ty.layout(),record panicked types, and report them in a summary instead of taking the
whole run down with them.
fix(cargo_stable_mir_json): derive toolchain lib path dynamically. The helper
binary hardcoded the library path, which broke every time the nightly
directory name changed (i.e., on every nightly bump; not ideal). Now resolves
it at runtime from the active toolchain.
refactor: derive rustc commit from toolchain, drop [metadata] section.
rust-toolchain.toml carried a [metadata] rustc-commit = ... field that had to
be updated by hand on every bump. The UI test scripts now derive the commit
hash from the Rust release manifest instead, which means one fewer thing to
forget, yay! Also inlines format args across the codebase to satisfy
clippy::uninlined_format_args.Test plan
cargo run -- -Zno-codegen tests/integration/programs/enum.rsproducesoutput (exercises the
ty_visitorpanic catch on types with tricky layouts)