Repository navigation
fix Implement static checking for model_validate in pydantic #1123 - #3390
asukaminato0721 wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR adds a synthesized model_validate classmethod to Pydantic dataclass-style models in pyrefly's static checker, and adds call-time constraint checking that walks dict literals (and same-scope literal initializers) passed to Model.model_validate(...) to flag values that violate field constraints such as Field(ge=...). It also extends emit_pydantic_argument_constraint to recurse through union types so each member is checked.
Changes:
- Synthesize a
model_validateclassmethod on every Pydantic model whose own class doesn't already define one, with a typed-dict-shapedobjparameter that recursively expands nested models, lists, and dicts. - Add
check_pydantic_model_validate_constraintsinvoked from the generic call path; it inspects dict literals (and traces a name's same-scope initializer) and the inferred typed-dict shape forge/le/etc. violations on nested fields. - Add four new pydantic tests covering nested-field range checking,
from_attributes, custommodel_validatepreservation, and asuper().model_validate(...)flow.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pyrefly/lib/alt/class/pydantic.rs | Implements synthesized model_validate builder, dict-shape type construction, and the recursive expr/type constraint checkers; adds union recursion to emit_pydantic_argument_constraint. |
| pyrefly/lib/alt/class/dataclass.rs | Wires the synthesized model_validate into Pydantic dataclass synthesized fields, suppressed only when the current class itself defines model_validate. |
| pyrefly/lib/alt/call.rs | Invokes the new constraint checker on every call expression. |
| pyrefly/lib/test/pydantic/field.rs | Adds tests for nested constraint detection, from_attributes, custom-method preservation, and super().model_validate(...). |
Comments suppressed due to low confidence (5)
pyrefly/lib/alt/class/pydantic.rs:995
- When
expectedis aType::Union, this iterates each member and recursively checks the sameactualagainst every member, emitting a constraint violation for any union member whose nested-model shape disagrees. For a value typed asint | strthis is harmless because non-pydantic members are no-ops, but forItemA | ItemB(both pydantic models) the same actual will be validated against both models and may produce duplicate or false-positive errors when the value is only intended to satisfy one branch of the union. Consider only emitting an error if all members fail (i.e. a "best-match" or "any-passes" semantics) for nested model unions.
Type::Union(union) => {
for expected in union.members.iter() {
self.check_pydantic_field_expr_constraints(
expected, actual, range, errors, seen,
);
}
}
pyrefly/lib/alt/class/pydantic.rs:1084
- Same union false-positive concern as in
check_pydantic_field_expr_constraints: when the expected type is a union of multiple pydantic models, the actual value is validated against every member and any failing branch will report a constraint error, even if another branch in the union accepts the value.
match (expected, actual) {
(Type::Union(expected), _) => {
for expected in expected.members.iter() {
self.check_pydantic_field_type_constraints(
expected, actual, range, errors, seen,
);
}
}
pyrefly/lib/alt/class/pydantic.rs:832
- When the user writes
bad = {"items": [..., {"quantity": -3}]}followed byInventory.model_validate(bad), this code recurses into the initializer expression ofbadand emits a constraint violation at the literal's range — but the assignmentbad = {...}is, by itself, not a pydantic call. The error then surfaces on the assignment line even when the user could plausibly usebadfor unrelated purposes, and the diagnostic gives no indication that the violation comes from a downstreammodel_validatecall. Consider either reporting the error at themodel_validatecall'srange(with a note pointing back to the literal) or qualifying the message so it isn't confusing on the assignment line.
if let Some(initializer) = self.pydantic_name_initializer(obj) {
self.check_pydantic_model_validate_expr_constraints(
&cls,
dataclass,
initializer,
range,
errors,
&mut seen,
);
seen.clear();
}
pyrefly/lib/alt/class/pydantic.rs:288
- Recursion into nested container types only handles
listanddict; nested pydantic models insidetuple,set,frozenset,Sequence,Mapping, etc. will not have their dict-shape input form added toobj_ty. This makes the accepted argument type formodel_validateinconsistent with what Pydantic actually accepts at runtime (it coerces dict shapes through any iterable container, not justlist/dict).
} else if cls.class_object() == self.stdlib.list_object()
&& let [elem] = cls.targs().as_slice()
{
self.heap.mk_class_type(
self.stdlib
.list(self.pydantic_model_validate_type(elem.clone(), seen)),
)
} else if cls.class_object() == self.stdlib.dict_object()
&& let [key, value] = cls.targs().as_slice()
{
self.heap.mk_class_type(self.stdlib.dict(
key.clone(),
self.pydantic_model_validate_type(value.clone(), seen),
))
} else {
Type::ClassType(cls)
}
}
pyrefly/lib/alt/class/pydantic.rs:881
pydantic_name_initializeronly resolvesBinding::Expr/Binding::NameAssign/Binding::AnnotatedTypechains and ignores re-assignments. If the user writesbad = {"quantity": 5}; bad = {"quantity": -3}; Inventory.model_validate(bad), only one of these initializers will be inspected (whichever the binding key resolves to), and the analysis may either miss real violations or report them on a stale initializer that doesn't actually flow into the call. Consider documenting this best-effort behavior in a comment, or restricting it to bindings that are provably the unique reaching definition.
fn pydantic_name_initializer<'b>(&'b self, actual: &Expr) -> Option<&'b Expr> {
let Expr::Name(name) = actual else {
return None;
};
let key = Key::BoundName(ShortIdentifier::expr_name(name));
let idx = self.bindings().key_to_idx_hashed_opt(Hashed::new(&key))?;
let mut idx = idx;
let mut gas = Gas::new(100);
while !gas.stop() {
match self.bindings().get(idx) {
Binding::Forward(forward)
| Binding::PromoteForward(forward)
| Binding::ForwardToFirstUse(forward) => idx = *forward,
binding => return Self::pydantic_initializer_from_binding(binding),
}
}
None
}
fn pydantic_initializer_from_binding(binding: &Binding) -> Option<&Expr> {
match binding {
Binding::Expr(_, expr) => Some(expr),
Binding::NameAssign(assign) => Some(&assign.expr),
Binding::AnnotatedType(_, inner) => Self::pydantic_initializer_from_binding(inner),
_ => None,
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if metadata.is_pydantic_model() | ||
| && !self | ||
| .get_class_fields(cls) | ||
| .is_some_and(|fields| fields.contains(&Name::new_static("model_validate"))) | ||
| { | ||
| let root_model_type = self | ||
| .get_pydantic_root_model_type_via_mro(cls, &metadata) | ||
| .map(|(ty, _)| ty); | ||
| fields.insert( | ||
| Name::new_static("model_validate"), |
| self.check_pydantic_model_validate_constraints( | ||
| ty, | ||
| &x.arguments.args, | ||
| &x.arguments.keywords, | ||
| x.arguments.range, | ||
| errors, | ||
| ); |
| ClassSynthesizedField::new(self.heap.mk_function(Function { | ||
| signature: Callable::list( | ||
| ParamList::new(params), | ||
| self.heap.mk_self_type(self.as_class_type_unchecked(cls)), | ||
| ), | ||
| metadata, | ||
| })) |
This comment has been minimized.
This comment has been minimized.
|
This pull request has been imported. If you are a Meta employee, you can view this in D114357524. (Because this pull request was imported automatically, there will not be any future comments.) |
Merging this PR will not alter performance
Comparing Footnotes
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9eae227 to
6ad8481
Compare
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
stroxler
left a comment
There was a problem hiding this comment.
Cool! We've got some issues that need to be addressed before we can merge this though:
- Exponential synthesis cost: a 61-line nested-model fixture takes 27.3s versus 3.3s on the parent, without calling
model_validate. - Union false positives: input valid for one model alternative is rejected for violating another alternative’s constraints.
- Custom methods aren’t preserved: inherited overrides get shadowed; own overrides still receive inappropriate checks against their raw input.
- API regression: the synthesized signature rejects Pydantic 2.12’s supported
extra=argument. - Incorrect alias selection: alias precedence and call-time
by_alias/by_namecontrols aren’t respected. - Dictionary-spread false positives: an overwritten literal remains subject to constraint checking.
More detailed AI review report:
1. P1 — Eager recursive input-shape synthesis grows exponentially
Location: pydantic.rs, lines 261–280, recursive expansion, lines 330–355.
Every model eagerly constructs a recursively expanded input type. Removing a model from seen after each visit means shared descendants are reconstructed for each field: a chain of models with two fields pointing to the preceding model produces exponential work.
from pydantic import BaseModel
class M0(BaseModel):
x: int
class M1(BaseModel):
left: M0
right: M0
class M2(BaseModel):
left: M1
right: M1
# Repeat through M14. No model_validate call is needed.Measured total checker wall times on the same Linux node and fixture generator:
- Depth 6, 29 project lines: parent 3.258s; PR 3.352s.
- Depth 10, 45 project lines: parent 3.282s; PR 4.755s.
- Depth 14, 61 project lines: parent 3.296s; PR 27.289s.
This fixture has no range constraints. The extra work also does not provide strict argument validation: object and dict[str, Any] already accept the input, and union normalization promotes anonymous TypedDict branches to ordinary dictionaries, discarding their field structure.
Fix: remove the eager structural input expansion and retain the existing permissive signature. If structural typing is needed separately, use specialization-aware memoization and explicit complexity bounds. Constraint checking should be lazy and should exit immediately when no relevant constraint is reachable.
2. P1 — Union alternatives are treated as simultaneous requirements
Location: pydantic.rs, lines 1208–1214; the type walker repeats this at lines 1297–1303.
class Positive(BaseModel):
x: int = Field(ge=0)
class Negative(BaseModel):
x: int = Field(le=-1)
class Either(BaseModel):
item: Positive | Negative
Either.model_validate({'item': {'x': -1}})Pydantic accepts the input as Negative. The PR reports a violation of Positive.x's ge=0 constraint. Both walkers descend into every expected union member and emit each member's failures independently.
Fix: respect alternative semantics. Emit a definitive constraint error only when all viable alternatives fail, or check the discriminator-selected branch. Conservatively skipping unsupported ambiguous unions is preferable to false positives.
3. P1 — Custom model_validate methods are not actually preserved
Two independently reproduced failures need fixing:
- dataclass.rs, lines 224–236: the synthesis guard checks only members defined on the current class, so it shadows an inherited custom method.
- pydantic.rs, lines 1061–1072: call-time checking recognizes any method named
model_validateon a Pydantic class, including an own custom override that ignores or transforms its input.
Inherited-method repro:
class Parent(BaseModel):
x: int
@classmethod
def model_validate(cls, obj: object = None, *, strict=None,
from_attributes=None, context=None,
by_alias=None, by_name=None) -> Self:
return cls(x=1)
class Child(Parent):
pass
Child.model_validate()The PR reports missing obj; both the parent checker and Pydantic accept it. Separately, an own override returning cls(x=1) on a model with x = Field(ge=0) still causes Model.model_validate({'x': -1}) to be rejected, although the override never validates that input.
Fix: inspect the visible non-synthesized member through the MRO, following the existing model_construct approach, and restrict built-in constraint checking to the actual Pydantic implementation or explicitly tagged synthesized functions—not a name match.
4. P1 — The synthesized signature removes Pydantic 2.12's extra= argument
Location: pydantic.rs, lines 285–318.
class Model(BaseModel):
x: int
Model.model_validate({'x': 1}, extra='forbid')With Pydantic 2.12.0 installed, runtime execution succeeds and the parent checker reports no errors. The PR reports Unexpected keyword argument extra. Replacing the library signature with a hard-coded parameter list loses supported version-specific parameters and will drift again as the API evolves.
Fix: preserve the installed library's method signature rather than reconstructing it. If synthesis is necessary, derive the signature from the real method and change only the intended component.
5. P2 — Alias resolution chooses the wrong input value
Location: pydantic.rs, lines 1123–1133, duplicated at lines 1253–1263.
class Aliased(BaseModel):
model_config = ConfigDict(validate_by_name=True, validate_by_alias=True)
x: int = Field(ge=0, alias='y')
Aliased.model_validate({'x': -1, 'y': 1})Pydantic gives the alias priority and accepts y=1; the PR checks x=-1. The walkers also ignore the call's by_alias and by_name controls. On an alias-only model, passing {'x': 1, 'y': -1} with by_name=True, by_alias=False is accepted at runtime but rejected by the PR.
Fix: centralize effective input-key selection, apply call-time controls over model configuration, and preserve Pydantic's alias precedence. Do not emit a definite error when dynamic options make the selected value uncertain.
6. P2 — Unknown dictionary spreads leave overridden literals marked as certain
Location: pydantic.rs, lines 1160–1167.
class Model(BaseModel):
x: int = Field(ge=0)
patch: dict[str, int] = {'x': 1}
Model.model_validate({'x': -1, **patch})Pydantic accepts the final x=1; the PR reports a violation at the overwritten -1. The literal-field map drops opaque unpackings instead of accounting for their ability to overwrite earlier keys.
Fix: preserve dictionary update ordering and key provenance. After an unresolved spread, earlier potentially overwritten keys are no longer certain and must not independently produce a definitive constraint violation.
Recommended implementation direction
Keep the existing library method and signature. Add a small, tagged/implementation-aware constraint-checking path that uses shared field/alias metadata, avoids re-inferring arguments when no constraints apply, and handles alternatives and uncertain input provenance conservatively. The current three recursive walkers duplicate logic while disagreeing about containers, aliases, and generic specialization. Avoid expanding this patch into complete Pydantic schema interpretation.
stroxler
left a comment
There was a problem hiding this comment.
whoops meant to request changes
Summary
Fixes #1123
Added synthesized model_validate support for Pydantic models including nested model/list dict input shapes.
Wired the synthesized method into Pydantic dataclass fields.
Added call-time constraint checking for model_validate, including inline literals and same-scope literal initializers.
Test Plan
add test