Skip to content

Commit 714cd37

Browse files
shahhamdihassanmeta-codesync[bot]
authored andcommitted
Treat converter=None on a dataclass field as no converter
Summary: attrs treats `converter=None` exactly like leaving the converter out, and its stubs declare `converter: None` as the no-converter form. Pyrefly treated it as a real converter: the synthesized `__init__` parameter became `Unknown`, so `A("asd")` was accepted for an `int` field. Pyright, basedpyright and ty treat it as no converter. Following Rebecca's direction, `None` is handled where the field's converter is stored: a `converter=` argument whose type is `None` is never stored, so everything that reads the stored converter treats the field as having none. That fixes the `__init__` parameter and also turns on the checks that only run without a converter: the `x.default` return type against the field type, an assignment to the attribute, and the `x.converter` decorator now applies. This covers every `dataclass_transform` field specifier, not only attrs, and because it checks the argument's type, a variable set to `None` counts too. The attrs check of `default=` and `factory=` against the field type gets the same rule separately. It runs for every call to an attrs field specifier, including in classes pyrefly doesn't treat as attrs classes, where nothing is stored, so it reads the call's own `converter=` argument. A test from the previous diff fails if it reads the stored converter instead. A converter whose type is a union with `None` still makes the `__init__` parameter partly `Unknown`; its `bug` test is unchanged. Not mypy_primer: on 17 vendored libraries (the 8 standard ones plus 9 that use attrs), the base and patched builds reported the same diagnostics. None of them pass `converter=None`, so this only shows that real converters are unaffected. Fixes #5283 Reviewed By: rchen152 Differential Revision: D130289218 fbshipit-source-id: a25dca184a213c156172b34f9d3425f150232650
1 parent cf67d00 commit 714cd37

4 files changed

Lines changed: 16 additions & 15 deletions

File tree

‎pyrefly/lib/alt/class/dataclass.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -902,8 +902,10 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
902902
let converter_param = self
903903
.attrs_converters_combinator_param(args, annotated_field_ty, errors)
904904
.or_else(|| {
905+
// attrs treats `converter=None` as no converter.
905906
map.0
906907
.get(&DataclassFieldKeywords::CONVERTER)
908+
.filter(|converter| !converter.is_none())
907909
.map(|converter| self.get_converter_param(converter, annotated_field_ty))
908910
});
909911
// Note that we intentionally don't try to fill in `default`, since we can't distinguish

‎pyrefly/lib/alt/solve.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4664,7 +4664,11 @@ impl<'ctx, 'answer, Ans: LookupAnswer> AnswersSolver<'ctx, 'answer, Ans> {
46644664
{
46654665
let got = self.expr_infer(expr, errors);
46664666
if let Some(annot_ty) = &annot_ty
4667-
&& call.arguments.find_keyword("converter").is_none()
4667+
// attrs treats `converter=None` as no converter.
4668+
&& call.arguments.find_keyword("converter").is_none_or(|converter| {
4669+
self.expr_infer(&converter.value, &self.error_swallower())
4670+
.is_none()
4671+
})
46684672
&& self
46694673
.bindings()
46704674
.get_class_fields(spec.class_def_index)

‎pyrefly/lib/test/attrs/fields.rs‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,6 @@ class C:
545545

546546
// attrs treats `converter=None` as no converter: https://github.com/facebook/pyrefly/issues/5283
547547
attrs_testcase!(
548-
bug = "A `None`-typed converter makes the `__init__` param `Unknown`",
549548
test_attrs_field_converter_none_init_param,
550549
r#"
551550
from typing import reveal_type
@@ -565,13 +564,12 @@ class C:
565564
d: int = field()
566565
e: int = field(converter=to_int)
567566
568-
reveal_type(C.__init__) # E: revealed type: (self: C, a: Unknown, b: Unknown, c: Unknown, d: int, e: str) -> None
567+
reveal_type(C.__init__) # E: revealed type: (self: C, a: int, b: int, c: int, d: int, e: str) -> None
569568
"#,
570569
);
571570

572571
// attrs treats `converter=None` as no converter: https://github.com/facebook/pyrefly/issues/5283
573572
attrs_testcase!(
574-
bug = "A `None`-typed converter skips the check of the default against the field type",
575573
test_attrs_field_converter_none_default_type_check,
576574
r#"
577575
import attr
@@ -583,11 +581,11 @@ def make_str() -> str: ...
583581
584582
@define
585583
class C:
586-
a: int = field(default="nope", converter=None)
587-
b: int = attr.ib(default="nope", converter=None)
588-
c: int = field(default="nope", converter=NO_CONVERTER)
584+
a: int = field(default="nope", converter=None) # E: `Literal['nope']` is not assignable to `int`
585+
b: int = attr.ib(default="nope", converter=None) # E: `Literal['nope']` is not assignable to `int`
586+
c: int = field(default="nope", converter=NO_CONVERTER) # E: `Literal['nope']` is not assignable to `int`
589587
d: int = field(default="nope") # E: `Literal['nope']` is not assignable to `int`
590-
f: int = field(factory=make_str, converter=None)
588+
f: int = field(factory=make_str, converter=None) # E: `str` is not assignable to `int`
591589
"#,
592590
);
593591

@@ -612,7 +610,6 @@ reveal_type(C.__init__) # E: revealed type: (self: C, x: str | Unknown) -> None
612610

613611
// attrs treats `converter=None` as no converter: https://github.com/facebook/pyrefly/issues/5283
614612
attrs_testcase!(
615-
bug = "`converter=None` skips the `@x.default` return check and the `@x.converter` decorator",
616613
test_attrs_field_converter_none_decorators,
617614
r#"
618615
from typing import reveal_type
@@ -621,7 +618,7 @@ from attrs import define, field
621618
@define
622619
class C:
623620
a: int = field(converter=None)
624-
b: int = field(converter=None)
621+
b: int = field(converter=None) # E: Return type `str` of the `@b.default` method is not assignable to field `b` of type `int`
625622
626623
@a.converter
627624
def _a(self, attribute, value: str) -> int:
@@ -631,13 +628,12 @@ class C:
631628
def _b(self) -> str:
632629
return "nope"
633630
634-
reveal_type(C.__init__) # E: revealed type: (self: C, a: Unknown, b: Unknown = ...) -> None
631+
reveal_type(C.__init__) # E: revealed type: (self: C, a: str, b: int = ...) -> None
635632
"#,
636633
);
637634

638635
// attrs treats `converter=None` as no converter: https://github.com/facebook/pyrefly/issues/5283
639636
attrs_testcase!(
640-
bug = "`converter=None` skips the check of an assignment to the attribute",
641637
test_attrs_field_converter_none_attribute_assignment,
642638
r#"
643639
from attrs import define, field
@@ -646,7 +642,7 @@ from attrs import define, field
646642
class C:
647643
a: int = field(converter=None)
648644
649-
C(1).a = "nope"
645+
C(1).a = "nope" # E: `Literal['nope']` is not assignable to attribute `a` with type `int`
650646
"#,
651647
);
652648

‎pyrefly/lib/test/dataclass_transform.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,6 @@ Data(x=NotConvertibleToInt()) # E: `NotConvertibleToInt` is not assignable to p
673673

674674
// `converter=None` means no converter, as in attrs: https://github.com/facebook/pyrefly/issues/5283
675675
testcase!(
676-
bug = "`converter=None` makes the `__init__` param `Unknown`",
677676
test_converter_none_init_param,
678677
r#"
679678
from typing import dataclass_transform, Any, reveal_type
@@ -686,7 +685,7 @@ class C:
686685
x: str = my_field(converter=None)
687686
y: str = my_field()
688687
z: str = my_field(converter=to_str)
689-
reveal_type(C.__init__) # E: revealed type: (self: C, x: Unknown, y: str, z: int) -> None
688+
reveal_type(C.__init__) # E: revealed type: (self: C, x: str, y: str, z: int) -> None
690689
"#,
691690
);
692691

0 commit comments

Comments
 (0)