Repository navigation
Make no-panics CI check test-aware - #1160
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
scope: ci
There was a problem hiding this comment.
Pull request overview
Adds a dedicated Python-based CI check to detect newly introduced panic-style Rust constructs (unwrap, expect, assert!) in changed production code, replacing the previous grep-based heuristic in the code style workflow.
Changes:
- Introduces
scripts/check_no_panics.pyto scan only added Rust lines and attempt to exclude test-only contexts via lightweight lexical tracking. - Updates
.github/workflows/code_style.ymlto run the new script instead of inline shell/grep logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/check_no_panics.py | Implements added-line Rust diff scanning + test-context detection + self-tests. |
| .github/workflows/code_style.yml | Switches the “no panics” CI job to call the new script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| PANIC_PATTERN = re.compile(r"\.(?:unwrap|expect)\(|(?<!_)assert(?:_eq|_ne)?!") | ||
| TEST_ATTR_PATTERN = re.compile(r"^\s*#\s*\[\s*(?:test|cfg\s*\(\s*test\s*\))\s*\]") |
There was a problem hiding this comment.
Fixed in a2ef532 and still present on the current PR head. TEST_ATTR_PATTERN now recognizes #[tokio::test(...)], #[rstest], #[test_case(...)], and composite #[cfg(...test...)] attributes, and the self-tests cover those top-level cases.
| def test_named_tests_module_without_cfg_is_ignored(self) -> None: | ||
| lines = [ | ||
| "mod tests {\n", | ||
| " fn helper() {\n", | ||
| " assert!(true);\n", |
There was a problem hiding this comment.
Fixed in a2ef532 and still present on the current PR head. I renamed the test to test_named_tests_module_marks_context so the name matches the behavior it verifies.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Replace bash no-panics check with lexer-based Python script
Solid improvement. The Python script properly lexes Rust to distinguish test code from production code, handling comments, strings, raw strings, char literals, and brace-depth tracking for #[test]/#[cfg(test)]/mod tests blocks. Self-tests built into the script are a nice touch.
Positives:
- Proper lexer state machine handles edge cases the grep pipeline missed (inline
#[test]functions, nestedmod testsblocks, string literals containing.unwrap()) - Only scans
src/andcrates/paths, correctly excludingtests/integration tests --self-testflag lets CI verify the checker itself
Minor (non-blocking)
TEST_ATTR_PATTERNdoesn't match proc-macro test attributes like#[tokio::test],#[rstest], or#[test_case]. These are almost always insidemod tests {}blocks in this project (which ARE caught), but a top-level#[tokio::test]insrc/orcrates/would be a false positive. Could expand to#[(?:test|tokio::test|rstest|cfg\s*\(\s*test\s*\))]in a follow-up.- Python 3.10+ required for
int | Nonesyntax. Fine on current CI runners but worth a comment in the script header. - Char literal lexer will misparse lifetime annotations (
'a) as char literals, but this only causes false negatives (missing violations in code "inside" the phantom char literal), which is the safe direction.
LGTM.
…-aware # Conflicts: # .github/workflows/code_style.yml
There was a problem hiding this comment.
Pull request overview
Adds a dedicated Python-based CI check to prevent introducing panic-style Rust calls (unwrap, expect, assert!) into changed production code, replacing the previous ad-hoc shell/awk/grep logic in the code style workflow.
Changes:
- Introduce
scripts/check_no_panics.pyto analyze PR diffs and flag panic-style calls on newly added Rust lines while excluding test contexts. - Update the
no-panicsGitHub Actions job to run the new script instead of inline shell parsing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
scripts/check_no_panics.py |
Implements diff-based Rust scanning with test-context detection and a // safety: suppression mechanism. |
.github/workflows/code_style.yml |
Replaces the previous shell-based “no panics” check with the new Python script invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # This can misclassify lifetimes like `'a` as char literals. That only | ||
| # risks false negatives by masking later code on the same line. |
| return 0 | ||
|
|
||
| print("::error::Found panic-style calls outside test-only Rust code.") | ||
| print("Production code must use proper error handling instead of panicking.") |
There was a problem hiding this comment.
Fixed in 81058eb. The checker now restores the failure hint about suppressing false positives with an inline // safety: <reason> comment.
| - name: Check for .unwrap(), .expect(), assert!() in production code | ||
| run: | | ||
| BASE="${{ github.event.pull_request.base.sha }}" | ||
| # Get the full diff for .rs files (production only, exclude tests/ directory) | ||
| DIFF=$(git diff "$BASE"...HEAD -- 'src/**/*.rs' 'crates/**/*.rs' || true) | ||
|
|
||
| if [ -z "$DIFF" ]; then | ||
| echo "No production Rust changes detected." | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Extract added lines, skipping those inside test modules. | ||
| # Track whether we're inside a test module by watching hunk headers | ||
| # (lines starting with @@) whose context contains "mod tests" or "#[cfg(test)]". | ||
| ADDED=$(echo "$DIFF" | awk ' | ||
| /^@@/ { | ||
| # Hunk context (after the second @@) tells us the function/module scope | ||
| in_test = (tolower($0) ~ /mod tests/ || $0 ~ /#\[cfg\(test\)\]/ || $0 ~ /#\[test\]/) | ||
| } | ||
| /^\+[^+]/ && !in_test { print } | ||
| ' || true) | ||
|
|
||
| if [ -z "$ADDED" ]; then | ||
| echo "No production Rust changes detected (test-only changes excluded)." | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Match panic-inducing patterns, excluding safety suppressions | ||
| VIOLATIONS=$(echo "$ADDED" \ | ||
| | grep -E '\.(unwrap|expect)\(|[^_]assert(_eq|_ne)?!' \ | ||
| | grep -Ev 'debug_assert|// safety:' \ | ||
| || true) | ||
|
|
||
| if [ -n "$VIOLATIONS" ]; then | ||
| echo "::error::Found .unwrap(), .expect(), or assert!() in production code." | ||
| echo "Production code must use proper error handling instead of panicking." | ||
| echo "Suppress false positives with an inline '// safety: <reason>' comment." | ||
| echo "" | ||
| echo "$VIOLATIONS" | head -20 | ||
| echo "" | ||
| COUNT=$(echo "$VIOLATIONS" | wc -l | tr -d ' ') | ||
| echo "Total: $COUNT violation(s)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "OK: No panic-inducing calls in changed production code." | ||
| python3 scripts/check_no_panics.py --base "$BASE" --head HEAD |
There was a problem hiding this comment.
Fixed in 81058eb. The no-panics job now pins Python with actions/setup-python@v5 and python-version: "3.12", matching the other repo workflows that run Python.
|
Already got approval -> merging to help with CI |
* Make no-panics check test-aware * Handle proc-macro test attrs in no-panics check * Pin Python for no-panics CI job
* Make no-panics check test-aware * Handle proc-macro test attrs in no-panics check * Pin Python for no-panics CI job
Summary
Replace the grep-based
No panics in production codecheck with a small Rust-aware Python script.Why
The existing job scans added lines only and produces false positives for
assert!/expect()inside#[cfg(test)]modules insrc/**/*.rs.What changed
scripts/check_no_panics.pygit diff#[cfg(test)],#[test], andmod testsscopes with brace-aware parsing// safety:suppressions for explicit local invariants.github/workflows/code_style.ymlValidation
python3 scripts/check_no_panics.py --self-testpython3 scripts/check_no_panics.py --base origin/staging --head HEADpython3 -m py_compile scripts/check_no_panics.py