Repository navigation
Widen the synthesized __init__ parameter for fields with a custom pydantic schema - #5432
soaring0616 wants to merge 2 commits into
Conversation
Pyrefly incorrectly reported an error when constructing a model with a custom schema field like `CustomModel(dtype=">i4")`, although the call is valid at runtime. The earlier attempt (meta#3798) widened the stored `converter_param`, which is also used for attribute assignment, and its check also matched metadata inferred as `Any`, so errors like `Model(y="1")` were missed. Add an `init_param_any` flag to `DataclassFieldKeywords`. It is set when an `Annotated` metadata item defines `__get_pydantic_core_schema__`, and only `get_dataclass_init` reads it to widen the synthesized `__init__` parameter. Metadata inferred as `Any` is not treated as a custom schema, so such fields are still checked. The widening applies in both lax and strict mode. Fixes meta#2586
|
Hi @soaring0616! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
rchen152
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left a couple of small suggestions to rename the init_param_any field and exclude classes defined in Pydantic itself from this behavior. Once that's done, I think this will be good to merge!
| let metadata_ty = self.expr_infer(metadata_item, &ignore_errors); | ||
| !metadata_ty.is_any() | ||
| && self.has_static_attr(&metadata_ty, &GET_PYDANTIC_CORE_SCHEMA) |
There was a problem hiding this comment.
Pydantic itself defines some classes that use __get_pydantic_core_schema__, but the only ones that should get __init__ parameter widening are BeforeValidator, PlainValidator, and WrapValidator. Other than those three, classes defined in Pydantic should be excluded. I think something like this should do the trick (caveat: I haven't compiled this, so consider it pseucode):
let cls = match self.expr_infer(metadata_item, &ignore_errors) {
Type::ClassType(cls) => cls.class_object().clone(),
Type::ClassDef(cls) => cls,
_ => return false,
};
self.get_class_member_with_defining_class(&cls, &GET_PYDANTIC_CORE_SCHEMA)
.is_some_and(|member| {
let owner = &member.defining_class;
owner.module_name().first_component() != "pydantic"
|| matches!(
owner.name().as_str(),
"BeforeValidator" | "PlainValidator" | "WrapValidator"
)
})- Rename `init_param_any` to `pydantic_has_custom_schema`, so that the name describes a fact about the field rather than how the flag is used. - Only a `__get_pydantic_core_schema__` hook defined outside Pydantic counts as a custom schema. Hooks defined by Pydantic are skipped because they generally build on the declared type's own validation. `BeforeValidator`, `PlainValidator` and `WrapValidator` are the exceptions, since they receive the raw input. - Add a test showing that `UuidVersion` does not widen the `__init__` parameter.
|
This pull request has been imported. If you are a Meta employee, you can view this in D128513616. (Because this pull request was imported automatically, there will not be any future comments.) |
|
Thanks for the review! I pushed another commit (1f07f21) with some changes:
One question about testing, related to the second comment (Claude suggests me to ask): the Pydantic copy under Also, I accidentally resolved one of the review threads, sorry about that, and I couldn’t find it to reopen it myself. If it doesn’t matter, please feel free to ignore this. But if I overlooked something, could you please reopen it? Thanks! |
Summary
Pyrefly reports a false positive when a model is constructed with a value that the field's custom schema accepts, e.g.
CustomModel(dtype=">i4"). The call is valid at runtime. The error appears in strict mode, and in lax mode for types that are in the lax conversion table (e.g.int, which becomesLaxInt).This PR adds a
pydantic_has_custom_schemaflag toDataclassFieldKeywords. It is set when anAnnotatedmetadata item has a__get_pydantic_core_schema__hook that is defined outside Pydantic (or by Pydantic'sBeforeValidator,PlainValidatororWrapValidator). Onlyget_dataclass_initreads it, to widen the synthesized__init__parameter.The widening applies in both strict and lax mode.
Fixes #2586
What changed
keywords.rs: adds thepydantic_has_custom_schemafield.pydantic.rs: sets the flag when a metadata item's__get_pydantic_core_schema__hook is defined outside Pydantic. Hooks defined by Pydantic are skipped because they generally build on the declared type's own validation (for example,UuidVersionadds a constraint to it), so the accepted input type does not change. The exceptions areBeforeValidator,PlainValidatorandWrapValidator, which receive the raw input. Metadata that is not a class (such asAny, which is whatField(gt=0)is inferred as) never counts.dataclass.rs:get_dataclass_initreads the flag. When it is set, the local variableconverter_parambecomesAny, so only the synthesized__init__parameter is widened.class_field.rs: on theAssignedInBodypath (fields with a default value), onlystrictwas copied from the annotation flags, so the flag was dropped and the__init__parameter kept the declared type. The flag is now carried at two places: when buildingannotation_flags(the filter no longer discards annotations that only set the flag, and the flag is copied), and when merging them into the flags of a right-hand-sideField(...)(f.pydantic_has_custom_schema |= annotation_flags.pydantic_has_custom_schema).Why a separate flag
An earlier attempt (#3798) set the stored
converter_paramtoAny. That field is read in three places:get_dataclass_init,get_dataclass_replace, and attribute assignment (resolve_class_attr_setter). Widening it would also widen attribute assignment, som.dtype = ">i4"would be wrongly accepted when it should be rejected.The new flag is read only by
get_dataclass_init, so only the__init__parameter is widened.test_annotated_custom_schema_widens_init_paramcovers this: it expectsm.dtype = ">i4"to still be an error.Test Plan
test_annotated_custom_schema_widens_init_param: in strict mode,__init__accepts a value for a custom-schema field, whilem.dtype = ">i4"is still rejected. It also checks that a field using Pydantic'sUuidVersionis not widened.test_annotated_custom_schema_with_default: the same for a field with a default value (theAssignedInBodypath).test_annotated_custom_schema_inherited: a subclass that inherits the field from its parent (strict=True) also gets the widened__init__parameter.test_annotated_custom_schema_widens_lax_conversion_table_type: in lax mode, a custom-schema field getsAny, while a normalintfield still getsLaxInt.test_annotated_metadata_without_custom_schema: metadata without__get_pydantic_core_schema__, includingField(gt=0), does not widen the parameter.I ran
cargo test -p pyrefly --lib pydantic::field; all 40 tests pass.The full
cargo testaborts ondeep_phi_chain_term_exprwith a stack overflow. This also happens onmain, so I skipped that test with--skip.Not included
__replace__is not handled here.BeforeValidator,PlainValidatorandWrapValidatorare not covered by a test yet: the Pydantic copy in the test environment does not includefunctional_validators.py.AI disclosure