Pyrefly should not allow forward reference variable names in function scope

[grievejia]

Describe the Bug

Minmal repro:

def test() -> None:
    x = y
    y = 1
    reveal_type(x)

Expected: Error on first assignment. Revealed Any
Actual: No error on first assignment. Revealed Literal[1]

Sandbox Link

https://pyrefly.org/sandbox/?code=CYUwZgBALiDOUAoCUEC0A+CA5A9gOxAC4AoCMiADwgF4IBPU8umiARkbIGIIAnEANxABDADYB9KHQAOIBBSRA

(Only applicable for extension issues) IDE Information

No response

[fangyi-zhou]

This seems have a similar root cause to #15.

I've tried to tackle this issue but it turns out to be more complicated that I thought, I'll nonetheless write down what I tried here, in hope that someone will validate my attempt and potentially give additional pointers.

When a successful name lookup is being validated in BindingBuilder::ensure_name:

pyrefly/pyrefly/lib/binding/expr.rs

Lines 230 to 242 in 9c3cf0b

	Ok(value) => {
	if !self.module_info.path().is_interface() {
	// Don't check flow for global/nonlocal lookups
	if let Some(error_message) = self
	.scopes
	.get_flow_style(&name.id)
	.uninitialized_error_message(name)
	{
	self.error(name.range, ErrorKind::UnboundName, None, error_message);
	}
	}
	self.insert_binding(key, value)
	}

The flow style is checked for potential uninitialisation, this is done by inspecting the flow style. How the flow style is obtained is via Scope::get_flow_style:

pyrefly/pyrefly/lib/binding/scope.rs

Lines 784 to 789 in 9c3cf0b

	pub fn get_flow_style(&self, name: &Name) -> &FlowStyle {
	match self.get_flow_info(name) {
	Some(flow) => &flow.style,
	None => &FlowStyle::Other,
	}
	}

It's important to note, if the flow info lookup is unsuccessful (i.e. in this case when we look at y at the scope of test), the flow style will be defaulting to Other.

I've attempted to return the flow style as an Option, and then raise an error for unbound name whenever None is raised. This however becomes an overkill. As it would falsely raise an error for valid forward defined globals:

def test():
  x = y

y = 1

It appears that the correct scope valiation would require the outer scopes to be resolved first, and then we can validate whether we're using any forward declared names if they are present at a later time in the same scope.

[samwgoldman]

@fangyi-zhou thanks for your effort! I think modeling "uninitialized" in the flow state is a reasonable approach to solve this issue and address the linked #15 as well.

I've attempted to return the flow style as an Option, and then raise an error for unbound name whenever None is raised. This however becomes an overkill. As it would falsely raise an error for valid forward defined globals:

This is a good insight and clear example. Although the code for test precedes the assignment to y, the body of test is likely to be executed after y is assigned.

I think it's reasonable to optimistically assume that bindings are initialized when accessed from a closure.

[fangyi-zhou]

The issue is that currently the checks for uninitialised variables happens during the binding builder. As far as I understand, the scope information for the entire file is not yet available, since we're in the middle of traversing the statements. This means we cannot reliably check whether the uninitialised variable is declared later within the same scope or later in an higher scope (e.g. a global).

[fangyi-zhou]

This issue can be closed, since #666 is merged.