Skip to content

Add hacking md#73

Merged
MusicalNinjaDad merged 30 commits into
mainfrom
hacking_md
Apr 15, 2026
Merged

Add hacking md#73
MusicalNinjaDad merged 30 commits into
mainfrom
hacking_md

Conversation

@MusicalNinjaDad
Copy link
Copy Markdown
Owner

As suggested on Zulip (#project-goals/2026-workshop > Stabilize the Try trait @ 💬)

This will probably end up as a try-book or spread across various docs & books

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

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

  • In wip_ShortCircuitT.rs, the Exit<T, E> enum’s OtherError(T, E) variant appears to violate the newly documented invariant that only the first variant may store the output type T; if this is meant to be a valid example, consider changing it (e.g. to OtherError(E)) or otherwise clarifying that it’s intentionally a compile-fail/WIP case.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `wip_ShortCircuitT.rs`, the `Exit<T, E>` enum’s `OtherError(T, E)` variant appears to violate the newly documented invariant that only the first variant may store the output type `T`; if this is meant to be a valid example, consider changing it (e.g. to `OtherError(E)`) or otherwise clarifying that it’s intentionally a compile-fail/WIP case.

## Individual Comments

### Comment 1
<location path="tests/compilation/examples/wip_ShortCircuitT.rs" line_range="1-7" />
<code_context>
+
+#[derive(Debug, Try, Try_ConvertResult)]
+#[must_use]
+enum Exit<T, E> {
+    Ok(T),
+    TestsFailed,
+    OtherError(T, E),
+}
+
+fn main() {}
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify and enforce the expected compile outcome for this WIP compilation test

Since the new rule forbids any other variant from storing the `Output` type, this enum is intentionally violating that (`OtherError(T, E)` stores `T`). Please make the test’s expectation explicit by either wiring it into the compile-test harness with a specific outcome (e.g., compile-fail with a particular diagnostic) or adding a short comment noting whether this is expected to compile or not. This will keep regressions visible if the derive behavior changes for this pattern.

```suggestion
#![feature(never_type)]
#![feature(try_trait_v2)]
#![feature(try_trait_v2_residual)]

use try_v2::{Try, Try_ConvertResult};

/// WIP compilation test for the `ShortCircuitT` rule.
///
/// This enum intentionally *violates* the rule that no variant other than the
/// "success" variant may store the `Output` type `T`: `OtherError(T, E)` holds
/// a `T`.
///
/// Expected behavior:
/// - As long as the derive macros accept this pattern, this file is just a
///   smoke-test example and is expected to **compile successfully**.
/// - If/when `Try`/`Try_ConvertResult` are tightened to reject such enums,
///   this file should be moved into the compile-fail suite and annotated with
///   the expected diagnostic (e.g. `//~ ERROR only the success variant may
///   contain the Output type T`), so regressions remain visible.
#[derive(Debug, Try, Try_ConvertResult)]
```
</issue_to_address>

### Comment 2
<location path="docs/hacking.md" line_range="10" />
<code_context>
+I like the trait for two reasons:
+
+1. I don't like it when std can do stuff I can't, `Try` opens up the power and versatility of `?` to my own code.
+1. I don't like writing extra code when it feels like I'm just working _around_ language constraints. `Try` lets me add `impl`s directly to a custom `Result` type or flatten nested contructs inside `Option`s & `Result`s
+
+## My related crates
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "contructs" to "constructs".

This appears to be a misspelling of "constructs" in the sentence about flattening nested types.

```suggestion
1. I don't like writing extra code when it feels like I'm just working _around_ language constraints. `Try` lets me add `impl`s directly to a custom `Result` type or flatten nested constructs inside `Option`s & `Result`s
```
</issue_to_address>

### Comment 3
<location path="docs/hacking.md" line_range="44" />
<code_context>
+
+### 2 more experimental features
+
+As wierd as it may be from the naming `try_blocks`, `try_blocks_heterogeneous` are more separate from a usage point of view.
+
+## Simple case
</code_context>
<issue_to_address>
**issue (typo):** Correct the spelling of "wierd" to "weird".

```suggestion
As weird as it may be from the naming `try_blocks`, `try_blocks_heterogeneous` are more separate from a usage point of view.
```
</issue_to_address>

### Comment 4
<location path="docs/hacking.md" line_range="349" />
<code_context>
+The stdlib implementations are almost identical. I took a lazy approach and have leveraged `std::option::IntoIter` to allow:
+
+```rust
+let tests: Vec<TestResult<i32, &'static str>> = vec![Ok(1), TestsFailed, Ok(2), OtherError("something wierd"), Ok(3), Ok(4)];
+
+let first_results: TestResult<Vec<i32>, &'static str> = tests.into_iter().collect();
</code_context>
<issue_to_address>
**issue (typo):** Fix the spelling of "wierd" in the string literal.

If this isn’t intentionally misspelled, update the string to use "something weird" instead.

```suggestion
let tests: Vec<TestResult<i32, &'static str>> = vec![Ok(1), TestsFailed, Ok(2), OtherError("something weird"), Ok(3), Ok(4)];
```
</issue_to_address>

### Comment 5
<location path="docs/hacking.md" line_range="368" />
<code_context>
+
+### Macro `Try_Methods` (WIP): derives `unwrap()`
+
+`Option` & `Result` have a large set of sematically overlapping ergonomic methods for:
+
+- Querying the variant
</code_context>
<issue_to_address>
**issue (typo):** Correct "sematically" to "semantically".

This is in the docs line describing overlapping ergonomic methods; use the adverb form here.

```suggestion
`Option` & `Result` have a large set of semantically overlapping ergonomic methods for:
```
</issue_to_address>

### Comment 6
<location path="docs/hacking.md" line_range="225" />
<code_context>
+- must have _at least one_ generic type
+- the _first_ generic type must be the `Output` type (produced when not short-circuiting)
+- the output variant (does not short-circuit) must be the _first_ variant and store the output type as the _only unnamed_ field
+- no other variant can store the Output type (see #72 add a nice error message)
+
+While technically, the generic ordering requirement could be relaxed with slightly more complex logic, it is [deliberately tight](https://en.wikipedia.org/wiki/Poka-yoke) - to avoid accidental, and hard to spot, mistakes caused by switching generics.
</code_context>
<issue_to_address>
**suggestion (typo):** Rephrase the parenthetical "see #72" for grammatical clarity.

The parenthetical reads a bit awkwardly. Consider something like "(see #72 for adding a nice error message)" or similar for smoother grammar while preserving the meaning.

```suggestion
- no other variant can store the Output type (see #72 for adding a nice error message)
```
</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 tests/compilation/examples/wip_ShortCircuitT.rs
Comment thread docs/hacking.md Outdated
Comment thread docs/hacking.md Outdated
Comment thread docs/hacking.md Outdated
Comment thread docs/hacking.md Outdated
Comment thread docs/hacking.md Outdated
@MusicalNinjaDad MusicalNinjaDad enabled auto-merge (squash) April 15, 2026 10:23
@MusicalNinjaDad MusicalNinjaDad merged commit a41bab5 into main Apr 15, 2026
18 checks passed
@MusicalNinjaDad MusicalNinjaDad deleted the hacking_md branch April 15, 2026 10:24
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