Skip to content

Commit 5e999e0

Browse files
shannonzhufacebook-github-bot
shannonzhu
authored and
facebook-github-bot
committed
Don't allow temporary refinement on classes with a getattr override
Summary: There are no guarantees on local checks/assignments if the parent class of an attribute has a `__getattr__` defined. Reviewed By: grievejia Differential Revision: D30402516 fbshipit-source-id: bdbeb316ca4e04a2d9b1c42a8da8ba00ae9ba707
1 parent e870866 commit 5e999e0

File tree

3 files changed

+56
-48
lines changed

3 files changed

+56
-48
lines changed

‎source/analysis/test/integration/attributeTest.ml‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -365,10 +365,7 @@ let test_check_attributes context =
365365
self.baz = 5
366366
return self.baz
367367
|}
368-
[
369-
"Undefined attribute [16]: `Foo` has no attribute `baz`.";
370-
"Incompatible return type [7]: Expected `int` but got `typing.Optional[int]`.";
371-
];
368+
["Undefined attribute [16]: `Foo` has no attribute `baz`."];
372369

373370
(* Ensure synthetic attribute accesses don't mask errors on real ones. *)
374371
assert_strict_type_errors

‎source/analysis/test/integration/refinementTest.ml‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,7 +1088,6 @@ let test_check_temporary_refinement context =
10881088
"Revealed type [-1]: Revealed type for `foo.attribute` is `Optional[int]`.";
10891089
"Revealed type [-1]: Revealed type for `foo.attribute` is `Optional[int]`.";
10901090
];
1091-
(* TODO(T70545381): Don't allow refinement if __getattr__ override exists. *)
10921091
assert_type_errors
10931092
{|
10941093
from typing import Optional
@@ -1106,8 +1105,7 @@ let test_check_temporary_refinement context =
11061105
|}
11071106
[
11081107
"Revealed type [-1]: Revealed type for `foo.attribute` is `Optional[int]`.";
1109-
"Revealed type [-1]: Revealed type for `foo.attribute` is `Optional[int]` (inferred: \
1110-
`typing_extensions.Literal[1]`).";
1108+
"Revealed type [-1]: Revealed type for `foo.attribute` is `Optional[int]`.";
11111109
];
11121110
()
11131111

‎source/analysis/typeCheck.ml‎

Lines changed: 54 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -4227,40 +4227,30 @@ module State (Context : Context) = struct
42274227
Annotation.original target_annotation, true
42284228
| _ -> Type.Top, false
42294229
in
4230-
let is_undefined_attribute parent =
4231-
(* TODO(T64156088): To catch errors against the implicit call to a custom
4232-
definition of `__setattr__`, we should run signature select against the
4233-
value type. *)
4234-
let is_getattr_defined =
4235-
let attribute =
4236-
match Type.resolve_class parent with
4237-
| Some [{ instantiated; class_name; _ }] ->
4238-
GlobalResolution.attribute_from_class_name
4239-
class_name
4240-
~accessed_through_class:false
4241-
~transitive:true
4242-
~resolution:global_resolution
4243-
~name:"__getattr__"
4244-
~instantiated
4245-
| _ -> None
4246-
in
4247-
match attribute with
4248-
| Some attribute when Annotated.Attribute.defined attribute -> (
4249-
match
4250-
Annotated.Attribute.annotation attribute |> Annotation.annotation
4251-
with
4252-
| Type.Parametric
4253-
{
4254-
name = "BoundMethod";
4255-
parameters =
4256-
[Single (Callable { implementation = { annotation; _ }; _ }); _];
4257-
}
4258-
| Type.Callable { implementation = { annotation; _ }; _ } ->
4259-
Type.is_any annotation
4260-
| _ -> false)
4261-
| _ -> false
4230+
let find_getattr parent =
4231+
let attribute =
4232+
match Type.resolve_class parent with
4233+
| Some [{ instantiated; class_name; _ }] ->
4234+
GlobalResolution.attribute_from_class_name
4235+
class_name
4236+
~accessed_through_class:false
4237+
~transitive:true
4238+
~resolution:global_resolution
4239+
~name:"__getattr__"
4240+
~instantiated
4241+
| _ -> None
42624242
in
4263-
not is_getattr_defined
4243+
match attribute with
4244+
| Some attribute when Annotated.Attribute.defined attribute -> (
4245+
match
4246+
Annotated.Attribute.annotation attribute |> Annotation.annotation
4247+
with
4248+
| Type.Parametric
4249+
{ name = "BoundMethod"; parameters = [Single (Callable _); _] }
4250+
| Type.Callable _ ->
4251+
Some attribute
4252+
| _ -> None)
4253+
| _ -> None
42644254
in
42654255
let is_global =
42664256
match name with
@@ -4411,12 +4401,37 @@ module State (Context : Context) = struct
44114401
~resolution:global_resolution
44124402
(Type.single_parameter parent)
44134403
in
4414-
if is_meta_typed_dictionary then
4404+
let is_getattr_returning_any_defined =
4405+
match
4406+
find_getattr parent
4407+
>>| AnnotatedAttribute.annotation
4408+
>>| Annotation.annotation
4409+
with
4410+
| Some
4411+
(Type.Parametric
4412+
{
4413+
name = "BoundMethod";
4414+
parameters =
4415+
[
4416+
Single
4417+
(Callable { implementation = { annotation; _ }; _ });
4418+
_;
4419+
];
4420+
})
4421+
| Some (Type.Callable { implementation = { annotation; _ }; _ })
4422+
->
4423+
Type.is_any annotation
4424+
| _ -> false
4425+
in
4426+
if is_meta_typed_dictionary || is_getattr_returning_any_defined then
44154427
(* Ignore the error from the attribute declaration `Movie.name =
44164428
...`, which would raise an error because `name` was removed as
44174429
an attribute from the TypedDictionary. *)
44184430
errors
4419-
else if is_undefined_attribute parent then
4431+
else
4432+
(* TODO(T64156088): To catch errors against the implicit call to a
4433+
custom definition of `__setattr__`, we should run signature
4434+
select against the value type. *)
44204435
let parent_source_path =
44214436
let ast_environment =
44224437
GlobalResolution.ast_environment global_resolution
@@ -4438,8 +4453,6 @@ module State (Context : Context) = struct
44384453
origin =
44394454
Error.Class { class_type = parent; parent_source_path };
44404455
})
4441-
else
4442-
errors
44434456
| _ -> errors
44444457
in
44454458
let check_nested_explicit_type_alias errors =
@@ -4834,11 +4847,11 @@ module State (Context : Context) = struct
48344847
| Attribute _ as name when is_simple_name name -> (
48354848
match resolved_base, attribute with
48364849
| `Attribute (_, parent), Some (attribute, _)
4837-
when not (Annotated.Attribute.property attribute) ->
4850+
when not
4851+
(Annotated.Attribute.property attribute
4852+
|| Option.is_some (find_getattr parent)) ->
48384853
let is_temporary_refinement =
4839-
is_temporary_refinement
4840-
|| Annotated.Attribute.defined attribute
4841-
|| is_undefined_attribute parent
4854+
is_temporary_refinement || Annotated.Attribute.defined attribute
48424855
in
48434856
Resolution.set_local_with_attributes
48444857
~temporary:is_temporary_refinement

0 commit comments

Comments
 (0)