Add Outcome.Try with usage analyzers FCE019–FCE022 - #265
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e4730a698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Outcome.Try's value overloads called Outcome<T>.Success(operation()) inside the guarded try, so when the operation returned null, Success's own null-guard threw ArgumentNullException inside the catch region. Under a broad TException (Exception, or ArgumentNullException) onError then mapped that library guard as an anticipated failure, hiding a contract violation. Move Success(result) after the catch in both the synchronous and asynchronous value overloads so a null result surfaces as the ArgumentNullException it is. The operation call and EnsureTask stay inside the try, so a synchronous throw and a null task are still caught as before; the void overloads use the argument-less Success property and are unaffected. Cover both overloads with a test and document the contract in their <exception> summaries. Addresses a Codex review finding on PR #265.
|
@codex Please perform an in-depth review of the code introduced or modified by this pull request, which implements a new feature. The objective is to determine whether the feature is correctly designed, correctly implemented, sufficiently tested, and consistently integrated into the existing project. Review scopeStart by reading:
Do not review the diff in isolation. Verify that the changes are consistent with the abstractions, behaviors, contracts, and conventions already established in the project. Review areas1. Requirements and expected behaviorVerify that the implementation actually satisfies the described requirements. Look specifically for:
Do not treat a behavior as incorrect merely because a different design would also have been possible. Report an issue only when there is a demonstrable contradiction, omission, defect, or concrete risk. 2. Design and architectureEvaluate:
Also verify that the feature does not introduce a second competing way to achieve an existing operation without an explicit and justified reason. 3. Public API and developer experienceFor every public API, fluent API, extension method, overload, or generic abstraction added or modified, review:
Mentally validate several realistic usage scenarios, including edge cases and invalid usages. 4. Code correctnessLook for concrete defects, including:
Do not report micro-optimizations without a realistic impact. 5. TestsAssess whether the tests genuinely demonstrate the intended behavior of the feature. Verify coverage of:
Identify tests that may pass without actually proving the announced behavior, as well as tests that are excessively coupled to implementation details. Do not mechanically request a test for every line or method. Recommend tests only when they protect an important behavior or an identified risk. 6. DocumentationVerify that:
Do not criticize writing style unless it creates a real technical ambiguity or communicates incorrect behavior. 7. Regressions and integrationAnalyze the impact on existing code, including:
Do not perform a general audit of the entire repository. Inspect existing code only when it is necessary to evaluate the changes introduced by this pull request. Expected outputStart with a concise summary containing:
Then report only genuinely actionable findings, classified using these severity levels:
For each finding, include:
Group findings that share the same root cause instead of reporting multiple variations of the same issue. After the findings, add a separate section for open questions. Do not present a question as a defect unless the answer establishes that an actual issue exists. Finish with:
Review rules
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4442dc7006
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Outcome.Try's value overloads called Outcome<T>.Success(operation()) inside the guarded try, so when the operation returned null, Success's own null-guard threw ArgumentNullException inside the catch region. Under a broad TException (Exception, or ArgumentNullException) onError then mapped that library guard as an anticipated failure, hiding a contract violation. Move Success(result) after the catch in both the synchronous and asynchronous value overloads so a null result surfaces as the ArgumentNullException it is. The operation call and EnsureTask stay inside the try, so a synchronous throw and a null task are still caught as before; the void overloads use the argument-less Success property and are unaffected. Cover both overloads with a test and document the contract in their <exception> summaries. Addresses a Codex review finding on PR #265.
6668405 to
c788911
Compare
Add Outcome.Try, the single sanctioned way to run a throwing operation and capture its throw as a failed Outcome through a mandatory error mapper — the inverse of GetResultOrThrow / ThrowIfFailure / ToException. Six overloads on netstandard2.0: value and side-effecting forms, each synchronous, asynchronous with a CancellationToken, and token-less asynchronous. Each catches a single caller-named exception type and lets OperationCanceledException propagate. The token-less Func<Task> / Func<Task<T>> overloads mirror the BCL's Task.Run set so an async lambda binds to an awaited Task-returning delegate instead of the synchronous Action as async void. A null result and a null returned task are contract violations that surface rather than being mapped, even under a broad TException. Document the entry/exit of the outcome flow in the Outcome guide (EN + FR). Refs: #267
Ship four usage analyzers in the package that guard Outcome.Try's narrow intended use: - FCE019 TryCatchesTooBroadly (Warning, on): catches exactly System.Exception. - FCE020 TryCatchesRichProtocolException (Warning, opt-in): catches an HTTP / socket / DB protocol exception; advisory prompt for a dedicated adapter. - FCE021 PreferNonThrowingAlternativeToTry (Warning, on): wraps a call that has a framework-available non-throwing TryXxx / TryCreate counterpart (advisory, framework-aware). - FCE022 TryCatchesCancellation (Warning, on): binds the caught type to OperationCanceledException or a subtype, a provably dead catch. Cover them with tests, including snippets compiled against the .NET Framework 4.7.2 reference set to prove FCE021's framework-awareness, and bilingual rule pages plus the analyzer index.
ADR-0028 records the decision to provide Outcome.Try as the guarded bridge into the outcome flow (mandatory mapper, single caught type, cancellation propagates, analyzer-guarded). ADR-0029 records completing the async surface with token-less Func<Task> / Func<Task<T>> overloads, refining ADR-0028's "rather than broadening the API" clause for the type-visible async-void case. Both accepted; numbered above main's ADR-0027 (Dependabot). English canonical with French translations, indexed.
c788911 to
4971be0
Compare
2 participants
Summary
Add
Outcome.Try, the single sanctioned bridge from throwing code into the outcome flow: it runs an operation, catches one caller-named exception type, and maps it to an error through a mandatory mapper, while always letting cancellation propagate. Four usage analyzers (FCE019–FCE022) guard its intended use at build time, and the design is recorded in ADR-0028 (guarded bridge) and ADR-0029 (async-surface completion).Type of change
Changes
Outcome.Trywith six overloads onnetstandard2.0: value and side-effecting forms, each synchronous, asynchronous with aCancellationToken, and token-less asynchronous. Each catches a single caller-named exception type, requires an explicit error mapper, and letsOperationCanceledExceptionpropagate.Func<Task>/Func<Task<T>>overloads (mirroring the BCL'sTask.Runset) ensure anasynclambda binds to an awaitedTask-returning delegate instead of the synchronousActionasasync void— closing a silent-crash footgun structurally rather than by detection.nulloperation result and anullreturned task are treated as contract violations that surface (rather than being mapped) even under a broadTException; the guards sit outside the exception-mapping region.Trycatches exactlySystem.Exception.Trycatches an HTTP / socket / DB protocol exception; advisory prompt to consider a dedicated result-inspecting adapter.Trywraps a call with a framework-available non-throwingTryXxx/TryCreatecounterpart (advisory, framework-aware).Trybinds the caught type toOperationCanceledException(or a subtype), a provably dead catch.Testing
dotnet build FirstClassErrors.slndotnet test FirstClassErrors.slnFirstClassErrors.Analyzers.UnitTests)Full solution build: 0 warnings, 0 errors.
dotnet test FirstClassErrors.sln: all projects passing (core 447, analyzers 132, and the rest), including the async-lambda and fire-and-forget regressions, the null-task/null-result contract, and tests compiling snippets against the .NET Framework 4.7.2 reference set for FCE021's framework-awareness. Re-verified after rebasing onto the latestmain.Documentation
doc/updatedFrench user-facing docs are kept in lockstep with the English canonical (Outcome guide, the four analyzer rule pages, the analyzer index) and both ADRs have French translations.
Architecture decisions
Notes:
mainmerged its own ADR-0027 ("Repair Dependabot pull requests within a risk boundary"). Numbers only — no decision or content changed.Related issues
Refs #267 — the async-void footgun; resolved structurally in this PR by the token-less overloads. The issue is left open, repurposed to track an optional defence-in-depth analyzer for the residual non-accidental cases (an explicitly
Action-typed variable, anasync voidmethod group passed by name).