Skip to content

Commit de7ce2a

Browse files
shannonzhufacebook-github-bot
authored andcommitted
Error on re-annotating global identifiers
Summary: This diff adds illegal annotation target errors when trying to re-annotate an already annotated global. On master, we already error for a re-annotation of a local: ``` def foo() -> None: x: int = 1 x: str # error is on this line x = "" return ``` Illegal annotation target [35]: Target `x` cannot be annotated after it is first declared. But we don't do this for re-annotation of a global: ``` constant: int = 1 def foo() -> None: global constant constant: str # no error ``` And instead relied on the fact that we were special casing out any assignments that touch globals from changing the local annotation map. In this case, we should be throwing the same error on re-annotation of an already declared global, which would be a lot more user friendly than silently letting the re-annotation pass without actually changing the type, then only erroring if we try to assign a value later. Reviewed By: grievejia Differential Revision: D30404490 fbshipit-source-id: 21cb9beb60bb651cb5fa36c6e96a964bfdfa80ed
1 parent 5e999e0 commit de7ce2a

File tree

2 files changed

+13
-5
lines changed

2 files changed

+13
-5
lines changed

source/analysis/test/integration/annotationTest.ml

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -743,7 +743,10 @@ let test_check_immutable_annotations context =
743743
constant: str
744744
constant = "hi"
745745
|}
746-
[];
746+
[
747+
"Illegal annotation target [35]: Target `constant` cannot be annotated after it is first \
748+
declared.";
749+
];
747750
assert_type_errors
748751
{|
749752
import typing
@@ -815,7 +818,11 @@ let test_check_immutable_annotations context =
815818
constant: str
816819
return constant
817820
|}
818-
["Incompatible return type [7]: Expected `str` but got `int`."];
821+
[
822+
"Illegal annotation target [35]: Target `constant` cannot be annotated after it is first \
823+
declared.";
824+
"Incompatible return type [7]: Expected `str` but got `int`.";
825+
];
819826
assert_type_errors
820827
{|
821828
def foo(x: int) -> None:

source/analysis/typeCheck.ml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4477,10 +4477,10 @@ module State (Context : Context) = struct
44774477
let check_previously_annotated errors =
44784478
match name with
44794479
| Name.Identifier identifier ->
4480-
let is_locally_defined =
4480+
let is_defined =
44814481
Option.is_some
44824482
(Resolution.get_local
4483-
~global_fallback:false
4483+
~global_fallback:true
44844484
~reference:(Reference.create identifier)
44854485
resolution)
44864486
in
@@ -4492,7 +4492,8 @@ module State (Context : Context) = struct
44924492
in
44934493
if
44944494
explicit
4495-
&& is_locally_defined
4495+
&& (not (Define.is_toplevel define))
4496+
&& is_defined
44964497
&& not is_reannotation_with_same_type
44974498
then
44984499
emit_error

0 commit comments

Comments
 (0)