Skip to content

Commit

Permalink
Trade one refinement bug for another.
Browse files Browse the repository at this point in the history
Summary:
This diff attempts to fix a bug where temporary refinements could shadow
nontemporary refinements so that a temporary refinement of a sub-attribute
could invalidate a temporary refinement of an attribute.

Unfortunately the fix - to prefer temporary refinements over non-temporary
ones - causes another bug (see the regression indicated in unit tests) where
on assignment to a mutable attribute we fail to wipe refinement of a final
sub-attribute.

I would argue that this new bug is not actually new - it's a pre-existing
issue because the final sub-attribute is not in fact a nontemporary refinement:
any attribute nested *under* a mutable attribute is temporary. These refinements
ought to be wiped not only on assignment but on any function call.

------------

Unfortunately, fixing the new bug would probably be a major time investment (I'd
guess a half a day to figure out if there's a hard but small fix... all fixes to
this code are hard because it's very confusing and the data structures don't
model the problem very well... and a week or so if no easy fix exists because
we might have to rewrite most of the refinement data structure).

I do not believe it is worth the time investment right now to try to get a full
fix given that we are working on a re-architecture and it's unlikely anything we learn
from fiddling with the existing data structures would map well to the
key/binding architecture we are planning to use in the future.

I propose we either accept the bug this diff fixes, or accept this diff with the
understanding that it makes a different bug more visible. I slightly favor accepting
this code since the failure to mark sub-attributes as temporary is a pre-existing
problem that just became a little more visible, as opposed to a wholly new regression.

Reviewed By: yangdanny97

Differential Revision: D64289922

fbshipit-source-id: ef09b38bc1f0d1f47894ed6d61eabd5731310686
  • Loading branch information
stroxler authored and facebook-github-bot committed Oct 14, 2024
1 parent f9e9b69 commit a55ae02
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 43 deletions.
32 changes: 7 additions & 25 deletions source/analysis/test/integration/typeRefinementTest.ml
Original file line number Diff line number Diff line change
Expand Up @@ -652,27 +652,6 @@ let test_check_local_refinement =
"Revealed type [-1]: Revealed type for `x` is `int`.";
"Revealed type [-1]: Revealed type for `x` is `int`.";
];
(* TODO(T204207195) These two test cases illustrate a mysterious bug where an equality check
on a sub-attribute can undo a non-temporary attribute refinement. *)
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
from typing import Final, Optional, Callable
class A:
ax: int = ...
class B:
bx: Final[Optional[A]] = ...
def foo(b: Optional[B], pred: Callable[[], bool]) -> None:
if (b and b.bx and pred()):
reveal_type(b)
reveal_type(b.bx)
reveal_type(b.bx.ax)
|}
[
"Revealed type [-1]: Revealed type for `b` is `Optional[B]` (inferred: `B`).";
"Revealed type [-1]: Revealed type for `b.bx` is `Optional[A]` (inferred: `A`, final).";
"Revealed type [-1]: Revealed type for `b.bx.ax` is `int`.";
];
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
{|
Expand All @@ -688,10 +667,9 @@ let test_check_local_refinement =
reveal_type(b.bx.ax)
|}
[
"Revealed type [-1]: Revealed type for `b` is `B`.";
"Revealed type [-1]: Revealed type for `b.bx` is `Optional[A]` (final).";
"Revealed type [-1]: Revealed type for `b` is `Optional[B]` (inferred: `B`).";
"Revealed type [-1]: Revealed type for `b.bx` is `Optional[A]` (inferred: `A`, final).";
"Revealed type [-1]: Revealed type for `b.bx.ax` is `typing_extensions.Literal[42]`.";
"Undefined attribute [16]: `Optional` has no attribute `ax`.";
];
]

Expand Down Expand Up @@ -1925,6 +1903,9 @@ let test_check_temporary_refinement =
];
labeled_test_case __FUNCTION__ __LINE__
@@ assert_type_errors
(* Note: this test broke when we fixed a bug where non-temporary refinements that were
nullary could mask temporary refinements: now, wehen a temporary refinement gets
updated we sometimes fail to wipe out nontemporary refinements. *)
{|
import typing
class A:
Expand All @@ -1945,7 +1926,8 @@ let test_check_temporary_refinement =
[
"Revealed type [-1]: Revealed type for `a.a.x` is `int`.";
"Revealed type [-1]: Revealed type for `a.a` is `typing.Optional[A]` (inferred: `B`).";
"Revealed type [-1]: Revealed type for `a.a.x` is `object` (final).";
(* Should be `object`, we failed to invalidate the refinement on `a.a = B()`. *)
"Revealed type [-1]: Revealed type for `a.a.x` is `int`.";
];
]

Expand Down
41 changes: 23 additions & 18 deletions source/analysis/typeInfo.ml
Original file line number Diff line number Diff line change
Expand Up @@ -396,21 +396,6 @@ module Store = struct

let has_nontemporary_type_info ~name { type_info; _ } = ReferenceMap.mem type_info name

let get_unit ?(include_temporary = true) ~name { type_info; temporary_type_info } =
let temporary =
if include_temporary then
ReferenceMap.find temporary_type_info name
else
None
in
let found =
match temporary with
| Some _ -> temporary
| None -> ReferenceMap.find type_info name
in
Option.value ~default:LocalOrGlobal.empty found


(** Map an operation over what's at a given name. If there's nothing already existing, use
`empty`.
Expand Down Expand Up @@ -440,10 +425,30 @@ module Store = struct
}


let get_base ~name store = get_unit ~name store |> LocalOrGlobal.base
let select_nontemporary_or_temporary (nontemporary, temporary) =
match nontemporary, temporary with
| None, info -> info
| info, None -> info
| Some info, Some _ ->
(* Nontemporary refinements can get mirrored into temporary refinements. Prefer the
nontemporary if we find both. *)
Some info


let get_type_info ~name ~attribute_path store =
get_unit ~name store |> LocalOrGlobal.get_type_info ~attribute_path
let get_base ~name { type_info; temporary_type_info } =
let get_type_info_from type_info_map =
Reference.Map.Tree.find type_info_map name >>= LocalOrGlobal.base
in
(get_type_info_from type_info, get_type_info_from temporary_type_info)
|> select_nontemporary_or_temporary


let get_type_info ~name ~attribute_path { type_info; temporary_type_info } =
let get_type_info_from type_info_map =
Reference.Map.Tree.find type_info_map name >>= LocalOrGlobal.get_type_info ~attribute_path
in
(get_type_info_from type_info, get_type_info_from temporary_type_info)
|> select_nontemporary_or_temporary


let set_base ?(temporary = false) ~name ~base store =
Expand Down

0 comments on commit a55ae02

Please sign in to comment.