Skip to content

stability guarantees#61

Merged
MusicalNinjaDad merged 34 commits into
mainfrom
tidy_tests
Apr 11, 2026
Merged

stability guarantees#61
MusicalNinjaDad merged 34 commits into
mainfrom
tidy_tests

Conversation

@MusicalNinjaDad

Copy link
Copy Markdown
Owner

No description provided.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 7 issues, and left some high level feedback:

  • In .github/workflows/rust-stability.yml the report job only has needs: [test, lint] and ignores lint-tests, so failures in lint-tests won't trigger the failure issue; consider adding lint-tests to the needs and condition.
  • The stability section in both lib.rs and README.md says "released as stable on 20225-03-18", which looks like a typo for the year and might confuse users relying on these dates.
  • The workflow sets RUSTC_BOOTSTRAP=1 globally for all channels, which can mask feature-gating issues on stable/beta; consider scoping or removing it unless you rely on it for a specific reason.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `.github/workflows/rust-stability.yml` the `report` job only has `needs: [test, lint]` and ignores `lint-tests`, so failures in `lint-tests` won't trigger the failure issue; consider adding `lint-tests` to the `needs` and condition.
- The stability section in both `lib.rs` and `README.md` says "released as stable on 20225-03-18", which looks like a typo for the year and might confuse users relying on these dates.
- The workflow sets `RUSTC_BOOTSTRAP=1` globally for all channels, which can mask feature-gating issues on stable/beta; consider scoping or removing it unless you rely on it for a specific reason.

## Individual Comments

### Comment 1
<location path=".github/workflows/rust-stability.yml" line_range="64-66" />
<code_context>
+    - name: clippy
+      run: cargo clippy --tests -- --deny warnings
+
+  report:
+    if: ${{ always() }}
+    needs: [test, lint]
+    runs-on: ubuntu-latest
+    steps:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The reporting job ignores `lint-tests` results, so failures there won’t trigger an issue.

Because `report` only has `needs: [test, lint]`, a failing `lint-tests` job will be ignored by the `needs.*.result` checks and won’t open an issue. To treat test-lint failures consistently, add `lint-tests` to `needs` and include it in the failure condition.

Suggested implementation:

```
  report:
    if: ${{ always() }}
    needs: [test, lint, lint-tests]
    runs-on: ubuntu-latest
    steps:

```

Somewhere in the `report` job there is likely a step that checks `needs.*.result` (e.g., in an `if:` condition for opening an issue). Update that condition to also include `needs.lint-tests.result`, for example:
- From: `${{ needs.test.result == 'failure' || needs.lint.result == 'failure' }}`
- To: `${{ needs.test.result == 'failure' || needs.lint.result == 'failure' || needs.lint-tests.result == 'failure' }}`

If you aggregate results differently (e.g., via env vars or a script), make sure `lint-tests` is treated consistently there as well so that a failing `lint-tests` job causes the report job to open an issue.
</issue_to_address>

### Comment 2
<location path="src/lib.rs" line_range="107-110" />
<code_context>
+//! ### MSRV
+//!
+//! For those of you working with a pinned nightly (etc.) this crate supports every version of
+//! edition 2024 (rust 1.85.1 onwards, released as stable on 20225-03-18). We use
+//! [autocfg](https://crates.io/crates/autocfg/) to seamlessly handle features which have been
+//! stabilised since then.
</code_context>
<issue_to_address>
**issue (typo):** There appears to be a typo in the stable release date string.

The date `20225-03-18` seems to have an extra digit. Please correct it to `2025-03-18` to keep the MSRV documentation accurate.

```suggestion
​//! For those of you working with a pinned nightly (etc.) this crate supports every version of
//! edition 2024 (rust 1.85.1 onwards, released as stable on 2025-03-18). We use
//! [autocfg](https://crates.io/crates/autocfg/) to seamlessly handle features which have been
//! stabilised since then.
```
</issue_to_address>

### Comment 3
<location path="tests/test_usage.rs" line_range="167-176" />
<code_context>
+mod lifetime_conversion {
</code_context>
<issue_to_address>
**suggestion (testing):** The new lifetime tests only use values with identical lifetimes, so they don’t actually exercise the intended lifetime relationships in the where-clauses.

In `lifetime_conversion` and `lifetime_duration`, all `&i32` values in the assertions are created in a single scope per test, so constraints like `'pass: 't`, `'fail: 'e`, `'t: 'o`, `'e: 'f`, and `'input: 't`/`'input: 'e` are only exercised in the trivial “all lifetimes equal” case.

Please add tests that:
- Create `okval` and `errval` in different nested scopes so each lifetime bound is exercised in both directions (e.g., `okval` outlives `errval` and vice versa), and
- Use helper functions (separate stack frames) to ensure the derives don’t accidentally over‑ or under‑constrain lifetimes.

That way the tests actually validate the lifetime relationships enforced by the macros, not just the behavior when all references share the same lifetime.

Suggested implementation:

```rust
mod lifetime_conversion {

    use super::*;

    // Helper functions to exercise lifetime relationships across stack frames.
    // These are written so you can swap the return type to your actual
    // conversion/derive types while preserving the lifetime structure.
    fn convert_ok_err<'t, 'e>(ok: &'t i32, err: &'e i32) -> (&'t i32, &'e i32) {
        (ok, err)
    }

    fn convert_err_ok<'t, 'e>(ok: &'t i32, err: &'e i32) -> (&'e i32, &'t i32) {
        (err, ok)
    }

    #[test]
    fn ok_outlives_err_nested_scope() {
        // 'outer: lifetime of okval
        let ok_storage = 1;
        let okval = &ok_storage;

        // We bind the result in the outer scope so any returned references
        // must be valid for 'outer.
        let (out_ok, out_err): (&i32, &i32);

        {
            // 'inner: lifetime of errval, strictly shorter than 'outer
            let err_storage = 2;
            let errval = &err_storage;

            // This call must be valid with 't: 'e or 'input: 't-like constraints,
            // depending on how you model the relationship in your conversion types.
            let (ok_res, err_res) = convert_ok_err(okval, errval);

            assert_eq!(*ok_res, 1);
            assert_eq!(*err_res, 2);

            out_ok = ok_res;
            out_err = err_res;
        }

        // out_ok must still be valid here (okval outlives errval).
        assert_eq!(*out_ok, 1);

        // out_err is only sound here if the conversion did NOT over‑extend
        // the lifetime of errval beyond its scope. When you wire this up to
        // your actual conversion types, the compiler should reject any
        // over‑constrained derives.
        assert_eq!(*out_err, 2);
    }

    #[test]
    fn err_outlives_ok_nested_scope() {
        // 'outer: lifetime of errval
        let err_storage = 10;
        let errval = &err_storage;

        let (out_ok, out_err): (&i32, &i32);

        {
            // 'inner: lifetime of okval, strictly shorter than 'outer
            let ok_storage = 20;
            let okval = &ok_storage;

            // This exercises the opposite direction of the lifetime relationship.
            let (err_res, ok_res) = convert_err_ok(okval, errval);

            assert_eq!(*ok_res, 20);
            assert_eq!(*err_res, 10);

            out_ok = ok_res;
            out_err = err_res;
        }

        // out_err must still be valid here (errval outlives okval).
        assert_eq!(*out_err, 10);

        // As above, when integrated with your conversion types, any
        // under‑constrained derive that fails to enforce 'fail: 'e‑like
        // relationships should be caught by the compiler.
        assert_eq!(*out_ok, 20);
    }

```

To fully match your review comment and exercise the actual macros / derives:

1. Replace the `convert_ok_err` and `convert_err_ok` return types `(&'t i32, &'e i32)` / `(&'e i32, &'t i32)` with the real conversion/result types used in `lifetime_conversion` (likely something involving `Try` / `Try_ConvertResult` or custom enums with derives).
2. Inside both helper functions, construct and return those real conversion types, preserving the `'t` / `'e` lifetimes in their type parameters as intended by the macros.
3. In the assertions, pattern‑match or use methods on your conversion types instead of directly handling tuples, while ensuring that the references you extract are stored in `out_ok` / `out_err` at the outer scope so the compiler checks that no lifetime is over‑ or under‑constrained.
4. Mirror the same strategy in the `lifetime_duration` module: add analogous tests that create `okval` and `errval` in nested scopes and call helper functions in a separate stack frame, wired to whatever duration-related types/macros are being tested there.
</issue_to_address>

### Comment 4
<location path="README.md" line_range="87" />
<code_context>
+> 🔬 **Additional Experimental Features**
+>
+> - [`#![feature(if_let_guard)]`](https://github.com/rust-lang/rust/issues/51114) (stable since 1.95.0)
+> - [`#![feature(let_chains)]`](https://github.com/rust-lang/rust/issues/139951) (stable since 1.88.0)
+>
+> This list includes any unstable features used by direct & transitive dependencies (currently, none).
</code_context>
<issue_to_address>
**issue (typo):** The GitHub issue number for `let_chains` appears to be incorrect.

The tracking issue for `let_chains` is `rust-lang/rust#53667`, so the current link to `.../issues/139951` appears incorrect and should be updated to the correct issue URL to avoid misleading readers.

```suggestion
> - [`#![feature(let_chains)]`](https://github.com/rust-lang/rust/issues/53667) (stable since 1.88.0)
```
</issue_to_address>

### Comment 5
<location path="README.md" line_range="97-98" />
<code_context>
+ We run automated tests **every month** to ensure no fundamental changes affect this crate and
+ test every PR against the current nightly, as well as the current equivalent beta & stable.
+ If you find an issue before we do, please
+ [raise an issue on github](https://github.com/MusicalNinjaDad/try_v2/issues).
+
+### MSRV
</code_context>
<issue_to_address>
**nitpick (typo):** Capitalize "GitHub" in the link text.

Change `github` to `GitHub` in the link text.

```suggestion
 If you find an issue before we do, please
 [raise an issue on GitHub](https://github.com/MusicalNinjaDad/try_v2/issues).
```
</issue_to_address>

### Comment 6
<location path="README.md" line_range="103" />
<code_context>
+
+### MSRV
+
+ For those of you working with a pinned nightly (etc.) this crate supports every version of
+ edition 2024 (rust 1.85.1 onwards, released as stable on 20225-03-18). We use
+ [autocfg](https://crates.io/crates/autocfg/) to seamlessly handle features which have been
+ stabilised since then.
</code_context>
<issue_to_address>
**suggestion (typo):** Capitalize "Rust" when referring to the language version.

In `edition 2024 (rust 1.85.1 onwards, ...)`, please capitalize `Rust` as it’s a proper noun.

```suggestion
 edition 2024 (Rust 1.85.1 onwards, released as stable on 20225-03-18). We use
```
</issue_to_address>

### Comment 7
<location path="CHANGELOG.md" line_range="7" />
<code_context>
+
+### Documentation & Testing
+
+- Document & continously validate stability guarantees & experimental feature usage
+
+### Bugfixes
</code_context>
<issue_to_address>
**issue (typo):** Fix the spelling of "continuously".

```suggestion
- Document & continuously validate stability guarantees & experimental feature usage
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread .github/workflows/rust-stability.yml
Comment thread src/lib.rs
Comment thread tests/test_usage.rs
Comment thread README.md Outdated
Comment thread README.md
Comment thread README.md Outdated
Comment thread CHANGELOG.md Outdated
@MusicalNinjaDad MusicalNinjaDad merged commit 0045e1d into main Apr 11, 2026
18 checks passed
@MusicalNinjaDad MusicalNinjaDad deleted the tidy_tests branch April 11, 2026 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant