and_ and or_: reject non-callable arguments at the call site#1585
Closed
HrachShah wants to merge 9 commits into
Closed
and_ and or_: reject non-callable arguments at the call site#1585HrachShah wants to merge 9 commits into
HrachShah wants to merge 9 commits into
Conversation
os.environb yields bytes on POSIX and the documented use case
for to_bool is reading values out of environment variables. The
truthy and falsy lookup tuples only contain str and int, so a
bytes('true') input raised 'Cannot convert value to bool: b\'true\''
even though its ASCII-decoded value is the canonical truthy
literal.
Decode bytes and bytearray as ASCII before the lowercase + lookup
step, matching the str path. Non-ASCII bytes raise ValueError
('Cannot convert value to bool: ...') the same way an unknown
string does, since the decoded text would still not match any
known literal. Unknown byte values raise the same ValueError.
When a class is rebuilt from attrs.fields() via attrs.make_class, the 'these' mapping contains Attribute instances rather than _CountingAttr objects. from_counting_attr was only reading the private _default, _validator, and _converter attributes, which Attribute does not expose, so the rebuild crashed with "'Attribute' object has no attribute '_default'". Detect an Attribute argument and read the public default/validator/ converter attributes directly. Add a regression test and a changelog entry.
for more information, see https://pre-commit.ci
The and_/or_ combinators previously accepted any object as a validator and deferred the failure to the call site of the resulting validator, where a try/except Exception silently caught the 'X object is not callable' TypeError and treated it as a successful validation. A typo like and_(instance_of(int), 'not_callable') thus always passed regardless of the value being validated. This adds an isinstance/_AndValidator/_OrValidator-agnostic callable check at the construction site and raises NotCallableError with a clear, type-aware message that names the offending argument. The same diagnostic covers the deep_iterable and deep_mapping list/ tuple branches, which previously relied on a downstream is_callable attribute validator producing a less specific message; the test_noncallable_validators cases for both TestDeepIterable and TestDeepMapping now accept either diagnostic.
The number-comparison validators previously let a non-comparable
value (e.g. None, a string, a complex number) propagate the
underlying TypeError from operator.< / .> / etc. as-is, e.g.
"TypeError: '>=' not supported between instances of 'NoneType' and
'int'". That message names Python internals, not the attribute being
validated, so a usage like `attr.ib(validator=ge(5))` against `None`
raised something the caller had no easy way to connect back to the
validator or its bound.
Wrap the comparison in a try/except TypeError that re-raises the
same `f"'{attr.name}' must be {compare_op} {bound}: {value}"` message
the failure case already used, so uncomparable values now surface as
a `ValueError` consistent with the other failure paths and explicitly
name the attribute.
Adds a parametrized `test_uncomparable_raises_value_error` in
TestLtLeGeGt covering None, str, complex, and a class that always
returns NotImplemented from __lt__ / __gt__, and a changelog entry.
for more information, see https://pre-commit.ci
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
2 participants
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.
Summary
and_andor_now raiseNotCallableErrorat the call site when given a non-callable argument, instead of silently deferring the failure to the wrapped validator's__call__site.Motivation
The pre-fix code path was:
valscould therefore contain any object — string, int, list,None, etc. — and theor_()/and_()call would succeed. The actual failure only surfaced when the wrapped validator was invoked against a real value, where the innertry/except Exceptioneither swallowed a'str' object is not callable(foror_, treating the bad argument as "always-valid") or crashed deep inside attribute-set code (forand_).A typo like
or_(instance_of(int), "not_callable")therefore silently passed on every value, which is the worst possible failure mode for a validator.Changes
src/attr/_make.py—and_now checks each arg is callable, raisingNotCallableErrorimmediately otherwise.src/attr/validators.py— same fix foror_.tests/test_validators.py— addedtest_rejects_non_callableregression covering strings,None, ints, and lists. UpdatedTestDeepIterable.test_noncallable_validatorsto recognise that theand_wrapper now intercepts list-wrapped bad values with its own diagnostic (the bare-value case still surfaces through_DeepIterable'sis_callable()validator).changelog.d/1579.change.md— news fragment.Test plan
The pre-existing
tests/test_converters.py::TestPipe::test_wrapped_annotationfailure is unrelated to this change.