Show Nice Error when Variable Shadowing Occurs in Inner Functions

[JeffersGlass]

Turns an assert statement that symbol name should be local into a nice error message, when a variable in an inner func shadows an outer func. Currently interp_only, since funcdefs cannot be redshifted (yet).

#231 may be the root of this as well, or another cause - this PR is just about adding the nice error message for now.

Closes #340

[antocuni]

@JeffersGlass I'm -0.5 to merge this as is.

The bug is the same as #231. I tried to apply this patch from #279 and I confirm it fixes the bug:
#279 (comment)

I agree that having the assertion error is not great, but we should't "officialize" the bug by making it a proper exception with a test.

The best course of action IMHO is to complete and merge #279, which would fix both #231 and #340 at once.

If that's not possible and/or takes too long, I'm fine raising a WIP exception instead of the AssertionError, explaining that it's an internal error and it's a bug to be fixed.

@aisipos do you maybe want me and/or @JeffersGlass to take over PR #279 ?

[aisipos]

@antocuni Sorry for the wait on this, I have updated #279, hopefully it's mergeable now.

[JeffersGlass]

So #279 fixes fixes the original example from #340, but not this further example.

def foo() -> None:
    x = 1
    def bar() -> None:
        x = 2
    bar()

def main() -> None:
    foo()

On the 279 branch, this still gives the AssertionError about sym.is_local. I think this would be a good reason to turn that assert into an error, to communicate that shadowing a name is an error in SPy. (At least that's what the docstring for ScopeAnalyzer says )

[antocuni]

So #279 fixes fixes the original example from #340, but not this further example.

def foo() -> None:
    x = 1
    def bar() -> None:
        x = 2
    bar()

def main() -> None:
    foo()

@JeffersGlass good catch (as always :)).

I think that this example is actually a different kind of bug compared to #340.

Let's examine the code around the failing assertion. It says that if a symbol has direct storage, then it must be a local variable, i.e. with depth == 0:

spy/spy/vm/astframe.py

Lines 466 to 467 in 49304de

	elif sym.storage == "direct":
	assert sym.is_local

spy/spy/analyze/symtable.py

Lines 125 to 127 in 49304de

	@property
	def is_local(self) -> bool:
	return self.level == 0

This is and will always be correct, because local variables are the only ones which can be assigned directly. All the other assignments must be of kind cell, i.e. go through an indirection. So far, we support cell only for module-level mutable red variables, but eventually we will need to support them also for the specific case of red closures (which are not supported yet).

So, in the code above the bug is in the ScopeAnalyzer, which should never produce a symbol which is both is_local and direct.

You can see it more clearly from this example:

var x: int = 0
def baz() -> None:
    x = 2

def main() -> None:
    baz()

❯ spy symtable /tmp/test.spy 
Implicit imports:
    builtins

<SymTable 'test' (blue, module)>
    [0] const baz         
    [0] const main        
    [0] var   x          [cell] 
    [1] const int          => <ImportRef builtins.int>

    <SymTable 'test::baz' (red, function)>
        [0] var   @return     
        [1] var   x          [cell]                   <<<<<<<<<<<<<<<<<

    <SymTable 'test::main' (red, function)>
        [0] var   @return     
        [1] const baz         

Here, you can see that inside test::baz the name x is found 1 level up and has storage == 'cell'.

But in your failing example, the ScopeAnalyzer makes a mistake:

def foo() -> None:
    x = 1
    def bar() -> None:
        x = 2
    bar()

def main() -> None:
    foo()

❯ spy symtable /tmp/test.spy 
Implicit imports:

<SymTable 'test' (blue, module)>
    [0] const foo         
    [0] const main        

    <SymTable 'test::foo' (red, function)>
        [0] const bar         
        [0] const x 
        [0] var   @return     

        <SymTable 'test::foo::bar' (red, function)>
            [0] var   @return     
            [1] const x                              <<<<<<<<<<< no "cell" here

    <SymTable 'test::main' (red, function)>
        [0] var   @return     
        [1] const foo         

I think the bug originates here:

spy/spy/analyze/scope.py

Lines 333 to 335 in 49304de

	def declare_Assign(self, assign: ast.Assign) -> None:
	self._declare_target_maybe(assign.target, assign.value)
	self.declare(assign.value)

spy/spy/analyze/scope.py

Lines 350 to 364 in 49304de

	def _declare_target_maybe(self, target: ast.StrConst, value: ast.Expr) -> None:
	# if target name does not exist elsewhere, we treat it as an implicit
	# declaration
	level, scope, sym = self.lookup_ref(target.value)
	if sym is None:
	# First assignment: mark as const unless in a loop
	type_loc = value.loc
	if self.loop_depth > 0:
	varkind: VarKind = "var"
	else:
	varkind = "const"
	self.define_name(target.value, varkind, "auto", target.loc, type_loc)
	else:
	# possible second assignment: promote to var if needed
	self._promote_const_to_var_maybe(target)

Probably the correct logic is something like this (but it's almost 1am here so I might be wrong 😅):

    def _declare_target_maybe(self, target: ast.StrConst, value: ast.Expr) -> None:
        # if target name does not exist elsewhere, we treat it as an implicit
        # declaration
        level, scope, sym = self.lookup_ref(target.value)
        if sym is None:
            # First assignment: mark as const unless in a loop
            type_loc = value.loc
            if self.loop_depth > 0:
                varkind: VarKind = "var"
            else:
                varkind = "const"
            self.define_name(target.value, varkind, "auto", target.loc, type_loc)

        elif level == 0:
            # possible second assignment: promote to var if needed
            self._promote_const_to_var_maybe(target)

        else:
            # we should raise "shadowing error" here, possibly by calling define_name
            XXX

[JeffersGlass]

Thank you for the full explanation! I think that all makes sense to me, but in looking at implementing it, it raises a further question about test_assignexpr_globals:

spy/spy/tests/test_scope.py

Lines 213 to 229 in cfeae13

	def test_assignexpr_globals(self):
	scopes = self.analyze("""
	x = 1
	var y: i32 = 1
	def main() -> None:
	z0 = (x := 1) # scope captures const x; runtime should reject assigning to it
	z1 = (y := 1)
	""")
	scope = scopes.by_funcdef(self.mod.get_funcdef("main"))
	assert scope._symbols == {
	"x": MatchSymbol("x", "const", "global-const", level=1),
	"y": MatchSymbol("y", "var", "explicit", level=1, storage="cell"),
	"z0": MatchSymbol("z0", "const", "auto"),
	"z1": MatchSymbol("z1", "const", "auto"),
	"@return": MatchSymbol("@return", "var", "auto"),
	}

As the comment suggests, the module level x ends up as global-const... should this now also raise a shadowing error? With an implementation like you outlined above, it does now raise an error, but I wanted to check that that was correct before adjusting another test.

[antocuni]

As the comment suggests, the module level x ends up as global-const... should this now also raise a shadowing error?

yes, I think it should raise shadowing error eagerly in that case

[JeffersGlass]

I think I've gotten a bit turned around in my head about how the scoping is supposed to work... backing up a step and writing a test, does this test capture the desired properties in the scopes of the two functions?

def test_inner_function_storage(self):
    src = """
    def foo() -> None:
        x = 1
        def bar() -> None:
            x = 2
        bar()
    """
    scopes = self.analyze(src)
    def get_scope(name: str) -> SymTable:
        for funcdef, scope in scopes.inner_scopes.items():
            if funcdef.name == name:
                return scope
        raise KeyError
    
    x_sym_foo = get_scope("foo").lookup_maybe("x")
    assert x_sym_foo is not None
    assert x_sym_foo.varkind == "var"
    assert x_sym_foo.storage == "cell"

    assert not get_scope("bar").has_definition("x") #x should be referenced but not defined in inner scope
    
    x_sym_bar = get_scope("bar").lookup_maybe("x")
    assert x_sym_bar is not None
    assert x_sym_bar.varkind == "var"
    assert x_sym_bar.storage == "cell"

[antocuni]

I think I've gotten a bit turned around in my head about how the scoping is supposed to work... backing up a step and writing a test, does this test capture the desired properties in the scopes of the two functions?
[cut]

that's a good question, to which I don't know the definitive answer :).
I think part of the doubts come from the fact that simple assignment is also an implicit vardef declaration, and because of that we need to decide whether it's "implicitly var" or "implicitly const".

Let's try to rewrite our example to be more explicit:

# case 1: I expect a ScopeError because x=2 tries to mutate a const
def foo() -> None:
    const x = 1
    def bar() -> None:
        x = 2
    bar()

# case 2: I expect it works because x is var
def foo() -> None:
    var x = 1
    def bar() -> None:
        x = 2
    bar()

I think that your test should pass if we decide that x = 1 creates a var.

The current rules w.r.t var/const are:

• at global level, it's "implicitly const" unless declared var
• at function level, it's "implicitly const" if it's assigned only once / not assigned in any loop
• else, it's "implicitly var"

I never took into consideration the case "it's assigned only once in the outer scope but also inside a closure". Note that so far we didn't need to think about it because we simply don't support red closures. For blue closures, the answer is easy since the outer variables are always const.

I think that the best way is to declare that if you want to mutate a closed-over variable, you MUST declare it as var explicitly. This is very consistent with what happens already for module-level names.

@JeffersGlass does this answer your question?