feat: add fail and require_not_none - #306
Open
msto wants to merge 1 commit into
Open
Conversation
`require` cannot narrow types. By the time it is called, `x is not None` has already been evaluated to a `bool`, and no type checker tracks that bool's provenance back to `x`. Anyone mechanically rewriting `assert x is not None` as `require(x is not None)` silently loses narrowing, and the natural next step is a `cast()` — trading an assertion that is stripped under `-O` for one that is never checked at all. Add two functions that do narrow: - `fail(message)` returns `NoReturn`, so a type checker treats any branch that calls it as terminating. Used in a guard it narrows the checked expression, and it composes with every narrowing form `assert` supports — isinstance, truthiness, tagged unions, tuple length, TypedDict keys, TypeIs predicates, and the rest — because the narrowing is performed by the `if` statement and `fail` only replaces the `raise`. - `require_not_none(value)` narrows via its return type, so it can be used in expression position (e.g. a comprehension) where a statement-level guard does not fit. A guard plus `fail` can express the same check, but only by repeating the subexpression, which is both verbose and a correctness hazard when the expression is not pure. `is not None` is the only narrowing form worth a dedicated unwrapper: it is the most common form by a wide margin, and it is the only one where the check yields a value to bind rather than a predicate about a value. Forms such as tagged unions and tuple length cannot be expressed as unwrappers at all. `require` is reimplemented over `fail` so the message handling lives in one place, and its docstring now notes the lack of narrowing. The narrowing behavior is pinned with `typing.assert_type` in the tests, which mypy enforces via `poe check-typing` since `files = ["./"]` covers tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded and exported 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Note
Authorship: the content below was drafted by an AI coding agent and filed via gh under @msto's account. It reflects the agent's analysis, not a statement authored by @msto
Summary
requirecannot narrow types, and never will be able to. By the time it is called,x is not Nonehas already collapsed to abool, and no type checker tracks that bool's provenance back tox. I verified this is unanimous across mypy 1.0.1, mypy 2.3, ty, pyrefly, and basedpyright — including that aLiteral[False] -> NoReturnoverload does not rescue it, sincex is not Nonetypes as plainbool.That makes
requirea quiet footgun for its main use case: anyone mechanically rewritingassert x is not Noneintorequire(x is not None)loses narrowing, and the natural next step is acast()— trading an assertion stripped under-Ofor one that is never checked at all.This adds the two functions that do narrow, and documents the gap on
requireitself.fail(message) -> NoReturn— a type checker treats any branch calling it as terminating, so a guard narrows the checked expression. It composes with every narrowing formassertsupports, because theifdoes the narrowing andfailonly replaces theraise. I tested 15 forms across all four checkers;failmatchedasserton every one, with zero disagreements:is not None·isinstance· truthiness ·type() is· enum identity · literal equality · tagged union ·callable()·issubclass· literal membership · tuple length · TypedDict key ·hasattr· userTypeIs·is Truerequire_not_none(value) -> ValueType— narrows via the return type, so it works in expression position where a guard does not fit:A guard plus
failexpresses the same check, but only by repeating the subexpression — verbose, and a correctness hazard when the expression is not pure.Why only this one unwrapper
is not Noneis the largest assert category in fgpyo itself (26 of 78, 33%), andisinstance/issubclass/callable/hasattrasserts number zero. It is also the only form where the check yields a value to bind rather than a predicate about a value — tagged unions, tuple length, and TypedDict keys cannot be unwrappers at all. So norequire_isinstancehere; it would be YAGNI with a messy typing story (tuple-of-classes overloads, and generic erasure means it could never verifylist[int]).No behavior change to
require. It is reimplemented overfailso message handling lives in one place; its signature, semantics, and lazy-message evaluation are unchanged.Test plan
Narrowing is pinned with
typing.assert_type, which mypy enforces viapoe check-typing(files = ["./"]covers tests). I mutation-checked those three assertions by flipping the expected types — mypy caught all three, so they have teeth rather than passing vacuously. Also adds error-path and lazy-message coverage forfailandrequire_not_none, a falsey-value test ("",0,[],Falseare returned, onlyNoneraises), and the previously-untested contract thatrequiredoes not evaluate a callable message when satisfied.poe check-allis green: lock, format, lint, mypy, 855 tests, 20 doctests.Follow-up
all_not_noneinfgpyo/util/types.pyis aTypeGuard, which narrows only the positive branch;TypeIsnarrows both and is strictly better. fgpyo also calls it internally asassert all_not_none(...), which has the same-Oproblem. Happy to send that as a separate PR on top.🤖 Generated with Claude Code