Raise error on variable use before write (#666)

Summary:
Closes #15, #248

This change adds additional checks when building bindings, so that
variables that are used before initialised are correctly flagged as
errors.

Currently, we error on uninitialised variables by checking their flow
style. If the flow style is uninitialised or partially uninitialised, an
error is generated. When no flow style information is obtainable (which
is the case for use before write), a default "other" flow style value is
returned.

In this diff, we perform additional checks before returning a default
"other" value. By looking up the scopes for static information for
variables, we can correctly detect if a variable is declared in an upper
scope or in the current scope, and return "uninitialised" flow style
when the variable is used before write.

When doing so, we need to consider the special case for type usages,
so that name resolutions in types do not fail.

Help needed:
There's a failing test that I need some help: `test::flow::test_assert_not_in_flow`

```
testcase!(
    test_assert_not_in_flow,
    r#"
from typing import assert_type, Literal
if 0.1:
    vari = "test"
    raise SystemExit
assert_type(vari, Literal["test"])   <-- Shall we raise an error here?
"#,
);
```

Pull Request resolved: #666

Reviewed By: yangdanny97

Differential Revision: D78496802

Pulled By: stroxler

fbshipit-source-id: 5c13e661f90b6b2c701e500f4aec9745b8a4a1c2

Author: fangyi-zhou

conformance/third_party/conformance.exp

@@ -861,6 +861,16 @@
       "stop_column": 79,
       "stop_line": 18
     },
+    {
+      "code": -2,
+      "column": 51,
+      "concise_description": "`GoodAlias4` is uninitialized",
+      "description": "`GoodAlias4` is uninitialized",
+      "line": 19,
+      "name": "unbound-name",
+      "stop_column": 61,
+      "stop_line": 19
+    },
     {
       "code": -2,
       "column": 79,
@@ -871,6 +881,16 @@
       "stop_column": 83,
       "stop_line": 19
     },
+    {
+      "code": -2,
+      "column": 40,
+      "concise_description": "`GoodAlias5` is uninitialized",
+      "description": "`GoodAlias5` is uninitialized",
+      "line": 22,
+      "name": "unbound-name",
+      "stop_column": 50,
+      "stop_line": 22
+    },
     {
       "code": -2,
       "column": 17,
@@ -981,6 +1001,26 @@
       "stop_column": 65,
       "stop_line": 45
     },
+    {
+      "code": -2,
+      "column": 40,
+      "concise_description": "`BadAlias4` is uninitialized",
+      "description": "`BadAlias4` is uninitialized",
+      "line": 46,
+      "name": "unbound-name",
+      "stop_column": 49,
+      "stop_line": 46
+    },
+    {
+      "code": -2,
+      "column": 44,
+      "concise_description": "`BadAlias5` is uninitialized",
+      "description": "`BadAlias5` is uninitialized",
+      "line": 47,
+      "name": "unbound-name",
+      "stop_column": 53,
+      "stop_line": 47
+    },
     {
       "code": -2,
       "column": 72,
@@ -990,6 +1030,16 @@
       "name": "bad-argument-type",
       "stop_column": 76,
       "stop_line": 47
+    },
+    {
+      "code": -2,
+      "column": 40,
+      "concise_description": "`BadAlias7` is uninitialized",
+      "description": "`BadAlias7` is uninitialized",
+      "line": 48,
+      "name": "unbound-name",
+      "stop_column": 49,
+      "stop_line": 48
     }
   ],
   "aliases_variance.py": [],
@@ -6002,6 +6052,26 @@
       "stop_column": 27,
       "stop_line": 18
     },
+    {
+      "code": -2,
+      "column": 7,
+      "concise_description": "`T` is uninitialized",
+      "description": "`T` is uninitialized",
+      "line": 35,
+      "name": "unbound-name",
+      "stop_column": 8,
+      "stop_line": 35
+    },
+    {
+      "code": -2,
+      "column": 17,
+      "concise_description": "`T` is uninitialized",
+      "description": "`T` is uninitialized",
+      "line": 44,
+      "name": "unbound-name",
+      "stop_column": 18,
+      "stop_line": 44
+    },
     {
       "code": -2,
       "column": 17,

conformance/third_party/conformance.result

@@ -41,8 +41,6 @@
   ],
   "aliases_typealiastype.py": [
     "Line 44: Expected 1 errors",
-    "Line 46: Expected 1 errors",
-    "Line 48: Expected 1 errors",
     "Line 52: Expected 1 errors",
     "Line 53: Expected 1 errors",
     "Line 54: Expected 1 errors",
@@ -58,7 +56,8 @@
     "Line 64: Expected 1 errors",
     "Line 17: Unexpected errors ['Argument `tuple[type[TypeVar[T]]]` is not assignable to parameter `type_params` with type `tuple[ParamSpec | TypeVar | TypeVarTuple, ...]` in function `typing.TypeAliasType.__new__`']",
     "Line 18: Unexpected errors ['Argument `tuple[type[TypeVar[S]], type[TypeVar[T]]]` is not assignable to parameter `type_params` with type `tuple[ParamSpec | TypeVar | TypeVarTuple, ...]` in function `typing.TypeAliasType.__new__`']",
-    "Line 19: Unexpected errors ['Argument `tuple[type[TypeVar[T]]]` is not assignable to parameter `type_params` with type `tuple[ParamSpec | TypeVar | TypeVarTuple, ...]` in function `typing.TypeAliasType.__new__`']",
+    "Line 19: Unexpected errors ['`GoodAlias4` is uninitialized', 'Argument `tuple[type[TypeVar[T]]]` is not assignable to parameter `type_params` with type `tuple[ParamSpec | TypeVar | TypeVarTuple, ...]` in function `typing.TypeAliasType.__new__`']",
+    "Line 22: Unexpected errors ['`GoodAlias5` is uninitialized']",
     "Line 23: Unexpected errors ['Argument `tuple[type[TypeVar[S]], type[TypeVar[TStr]], type[ParamSpec[P]], type[TypeVarTuple[Ts]]]` is not assignable to parameter `type_params` with type `tuple[ParamSpec | TypeVar | TypeVarTuple, ...]` in function `typing.TypeAliasType.__new__`']",
     "Line 35: Unexpected errors ['Expected a type form, got instance of `GenericAlias`']",
     "Line 36: Unexpected errors ['Expected a type form, got instance of `GenericAlias`']",
@@ -263,7 +262,6 @@
   "generics_syntax_declarations.py": [],
   "generics_syntax_infer_variance.py": [],
   "generics_syntax_scoping.py": [
-    "Line 35: Expected 1 errors",
     "Line 92: Expected 1 errors",
     "Line 95: Expected 1 errors",
     "Line 98: Expected 1 errors",

conformance/third_party/results.json

@@ -3,7 +3,7 @@
   "pass": 70,
   "fail": 66,
   "pass_rate": 0.51,
-  "differences": 293,
+  "differences": 291,
   "passing": [
     "aliases_explicit.py",
     "aliases_newtype.py",
@@ -80,7 +80,7 @@
     "aliases_implicit.py": 6,
     "aliases_recursive.py": 17,
     "aliases_type_statement.py": 8,
-    "aliases_typealiastype.py": 25,
+    "aliases_typealiastype.py": 24,
     "aliases_variance.py": 4,
     "annotations_forward_refs.py": 7,
     "annotations_generators.py": 3,
@@ -109,7 +109,7 @@
     "generics_self_basic.py": 3,
     "generics_self_protocols.py": 1,
     "generics_self_usage.py": 12,
-    "generics_syntax_scoping.py": 7,
+    "generics_syntax_scoping.py": 6,
     "generics_typevartuple_basic.py": 1,
     "generics_typevartuple_concat.py": 1,
     "generics_typevartuple_specialization.py": 4,

pyrefly/lib/binding/expr.rs

@@ -217,6 +217,7 @@ impl<'a> BindingsBuilder<'a> {
         &mut self,
         name: &Identifier,
         value: Result<Binding, LookupError>,
+        used_in_static_type: bool,
     ) -> Idx<Key> {
         let key = Key::BoundName(ShortIdentifier::new(name));
         if name.is_empty() {
@@ -236,7 +237,7 @@ impl<'a> BindingsBuilder<'a> {
                     // Don't check flow for global/nonlocal lookups
                     if let Some(error_message) = self
                         .scopes
-                        .get_flow_style(&name.id)
+                        .get_flow_style(&name.id, used_in_static_type)
                         .uninitialized_error_message(name)
                     {
                         self.error(
@@ -636,7 +637,11 @@ impl<'a> BindingsBuilder<'a> {
                 let binding = self
                     .lookup_name_usage(Hashed::new(&name.id), usage)
                     .map(Binding::Forward);
-                self.ensure_name(&name, binding);
+                self.ensure_name(
+                    &name,
+                    binding,
+                    matches!(usage, Usage::StaticTypeInformation),
+                );
             }
             Expr::Yield(x) => {
                 self.record_yield(x.clone());
@@ -673,7 +678,7 @@ impl<'a> BindingsBuilder<'a> {
                         .lookup_name(Hashed::new(&name.id), LookupKind::Regular)
                         .map(Binding::Forward),
                 };
-                self.ensure_name(&name, binding);
+                self.ensure_name(&name, binding, true);
             }
             Expr::Subscript(ExprSubscript { value, .. })
                 if self.as_special_export(value) == Some(SpecialExport::Literal) =>

pyrefly/lib/binding/scope.rs

@@ -806,10 +806,47 @@ impl Scopes {
         None
     }
 
-    pub fn get_flow_style(&self, name: &Name) -> &FlowStyle {
+    fn get_static_info(&self, name: &Name, should_skip_current_scope: bool) -> Option<&StaticInfo> {
+        let name = Hashed::new(name);
+        let mut iter = self.iter_rev();
+        if should_skip_current_scope {
+            iter.next();
+        }
+        for scope in iter {
+            if let Some(info) = scope.stat.0.get_hashed(name) {
+                return Some(info);
+            }
+        }
+        None
+    }
+
+    /// Get the flow style for `name`, depending on whether `name` is used in a
+    /// static type.
+    ///
+    /// If we can find a flow info for `name`, return its style. Otherwise, we
+    /// check the static type information to see if we have a uninitialized
+    /// binding, in which case, `FlowStyle::Uninitialized` is returned.
+    /// Otherwise we return `FlowStyle::Other` to indicate no information
+    /// available.
+    pub fn get_flow_style(&self, name: &Name, used_in_static_type: bool) -> &FlowStyle {
         match self.get_flow_info(name) {
             Some(flow) => &flow.style,
-            None => &FlowStyle::Other,
+            None => {
+                // If the name is used for static type information, we can look
+                // at the current scope.
+                // Otherwise, we should skip the current scope, because it may
+                // permit a name to be used before it is defined.
+                if self.get_static_info(name, !used_in_static_type).is_some() {
+                    // If we have a static binding, then we are in a scope where
+                    // the name is defined, so we can return Other.
+                    &FlowStyle::Other
+                } else {
+                    // If we don't have a static binding, then we are in a scope
+                    // where the name is not defined, so we return
+                    // Uninitialized.
+                    &FlowStyle::Uninitialized
+                }
+            }
         }
     }
 

pyrefly/lib/binding/stmt.rs

@@ -153,12 +153,16 @@ impl<'a> BindingsBuilder<'a> {
         })
     }
 
-    pub fn ensure_mutable_name(&mut self, x: &ExprName) -> Idx<Key> {
+    pub fn ensure_mutable_name(&mut self, x: &ExprName, usage: &mut Usage) -> Idx<Key> {
         let name = Ast::expr_name_identifier(x.clone());
         let binding = self
             .lookup_name(Hashed::new(&name.id), LookupKind::Mutable)
             .map(Binding::Forward);
-        self.ensure_name(&name, binding)
+        self.ensure_name(
+            &name,
+            binding,
+            matches!(usage, Usage::StaticTypeInformation),
+        )
     }
 
     fn define_nonlocal_name(&mut self, name: &Identifier) {
@@ -271,7 +275,7 @@ impl<'a> BindingsBuilder<'a> {
                 for target in &mut x.targets {
                     let mut delete_link = self.declare_current_idx(Key::UsageLink(target.range()));
                     if let Expr::Name(name) = target {
-                        let idx = self.ensure_mutable_name(name);
+                        let idx = self.ensure_mutable_name(name, delete_link.usage());
                         self.scopes.upsert_flow_info(
                             Hashed::new(&name.id),
                             idx,
@@ -527,7 +531,7 @@ impl<'a> BindingsBuilder<'a> {
                             .declare_current_idx(Key::Definition(ShortIdentifier::expr_name(name)));
                         // Ensure the target name, which must already be in scope (it is part of the implicit dunder method call
                         // used in augmented assignment).
-                        self.ensure_mutable_name(name);
+                        self.ensure_mutable_name(name, assigned.usage());
                         self.ensure_expr(&mut x.value, assigned.usage());
                         // TODO(stroxler): Should we really be using `bind_key` here? This will update the
                         // flow info to define the name, even if it was not previously defined.

pyrefly/lib/test/assign.rs

@@ -625,10 +625,17 @@ x3: B
 );
 
 testcase!(
-    bug = "False negative",
+    test_use_before_write,
+    r#"
+y  # E: `y` is uninitialized
+y = 42
+    "#,
+);
+
+testcase!(
     test_read_before_write,
     r#"
-x = y  # this should be an error
+x = y  # E: `y` is uninitialized
 y = 42
     "#,
 );
@@ -757,7 +764,7 @@ def f(x):
 
 def f(x):
     x = 3
-    x = "None" 
+    x = "None"
     "#,
 );
 

pyrefly/lib/test/flow.rs

@@ -1177,7 +1177,7 @@ def foo() -> list[int]:
     results: list[int] = [1, 2, 3]
     for i, x in enumerate(results):
         results[i] = x * 10
-    return results  
+    return results
 "#,
 );
 
@@ -1201,7 +1201,7 @@ bar: str = "bar"
 
 def func():
     foo: str | None = None
-    
+
     for x in []:
         for y in []:
             pass
@@ -1226,7 +1226,7 @@ from typing import assert_type, Literal
 if 0.1:
     vari = "test"
     raise SystemExit
-assert_type(vari, Literal["test"])
+assert_type(vari, Literal["test"]) # E: `vari` is uninitialized
 "#,
 );
 

pyrefly/lib/test/scope.rs

@@ -75,7 +75,7 @@ def f():
     x += "a"  # E: `x` is not mutable from the current scope
 def g():
     global x
-    x += "a"  
+    x += "a"
 def h0():
     global x
     def h1():
@@ -173,12 +173,12 @@ def f():
     global c1
     global c2
     # Should be permitted, the resulting operation is in-place
-    c0 += C()  
+    c0 += C()
     # Should be permitted, the resulting operation returns a new C which is okay
-    c1 -= C()  
+    c1 -= C()
     # Should *not* be permitted, this changes the type of the global in a way
     # that is incompatible with static analysis of the global scope
-    c2 *= C()  
+    c2 *= C()
 f()
 # This shows what would go wrong if we allow the aug assign on `c2`
 assert_type(c2, C)
@@ -234,7 +234,7 @@ def outer():
         x += "a"  # E: `x` is not mutable from the current scope
     def g():
         nonlocal x
-        x += "a"  
+        x += "a"
     def h0():
         nonlocal x
         def h1():
@@ -258,7 +258,7 @@ def outer():
     # A minor variation on f(), relevant to specific implementation bugs in our scope analysis
     def g():
         nonlocal x
-        del x  
+        del x
     f()
     f()  # This will crash at runtime!
 "#,
@@ -437,3 +437,13 @@ def f(arg: int) -> None:
     z  # E: Could not find name `z`
 "#,
 );
+
+testcase!(
+    test_forward_reference_ok,
+    r#"
+def foo():
+    x = y
+
+y = 42
+"#,
+);