stability guarantees#61
Merged
Merged
Conversation
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In
.github/workflows/rust-stability.ymlthereportjob only hasneeds: [test, lint]and ignoreslint-tests, so failures inlint-testswon't trigger the failure issue; consider addinglint-teststo theneedsand condition. - The stability section in both
lib.rsandREADME.mdsays "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=1globally 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
No description provided.