Repository navigation
Fix dead isinstance check in _decorator_infos_for_class() - #13858
Merged
Viicos merged 1 commit intoSep 25, 2026
Merged
Conversation
The else branch (reached for computed fields, after every other DecoratorInfo subtype has been ruled out) called isinstance(var_value, ComputedFieldInfo) as a bare statement: the boolean result was computed and immediately discarded, so it had no effect. It also checked the wrong variable — var_value is the PydanticDescriptorProxy, not its .decorator_info, so the check would always have been False even if its result had been used. Changed it to assert isinstance(info, ComputedFieldInfo), matching the same invariant-check style already used a few lines above at PydanticDescriptorProxy.__post_init__ (line 186). This makes the implicit assumption in this branch an explicit, enforced one: if a future DecoratorInfo subtype is ever added without a matching elif here, this now fails fast with a clear AssertionError at class creation time instead of silently mis-classifying it as a computed field and surfacing a confusing AttributeError later in update_from_config. Full test suite passes (6110 passed, 1 pre-existing unrelated failure in test_generic_model_pickle_different_module reproduced identically on main without this change).
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Viicos
changed the title
_decorator_infos_for_class()
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.
What
_decorator_infos_for_class's finalelsebranch (reached for computed fields, after every otherDecoratorInfosubtype has been ruled out by the precedingelifchain) contained:as a bare statement. The boolean result is computed and immediately discarded, so this line has no effect at all — not even for static type narrowing, since a bare expression statement doesn't narrow anything for mypy/pyright. It also checks the wrong variable:
var_valueis thePydanticDescriptorProxyitself, not its.decorator_info— so even if the result were used, this specific check would always evaluate toFalse.Fix
This checks the right variable (
info, the actualDecoratorInfo) and turns the no-op into a real invariant check, matching the same style already used a few lines above atPydanticDescriptorProxy.__post_init__(line 186:isinstance(self.decorator_info, ComputedFieldInfo)).Right now this branch really can only be reached with a
ComputedFieldInfo, so nothing changes for any current input. The value is defensive: if aDecoratorInfosubtype is ever added without updating thiselifchain to match, this now fails fast with a clearAssertionErrorat class-creation time, instead of silently filing it underres.computed_fieldsand surfacing a confusingAttributeErrormuch later, deep insideupdate_from_config.Small and mechanical enough that I went straight to a PR per the template's "unless your change is trivial" note — happy to open an issue first instead if you'd rather discuss it there.
Testing
Full suite: 6110 passed, 1 pre-existing failure (
test_generic_model_pickle_different_module, a subprocess/cloudpickle environment issue) reproduced identically onmainwith this change reverted, so unrelated to this diff.