Shadowing in closures in let blocks influences bindings in neighboring functions

[Seelengrab]

MWE:

julia> let; 
           function a()
               b()
           end
           function b()
               b = 1
               return b
           end
           @show a()
           @show a()
       end
a() = 1
ERROR: MethodError: objects of type Int64 are not callable
The object of type `Int64` exists, but no method is defined for this combination of argument types when trying to treat it as a callable object.
Maybe you forgot to use an operator such as *, ^, %, / etc. ?
Stacktrace:
 [1] (::var"#a#a##6")()
   @ Main ./REPL[9]:3
 [2] top-level scope
   @ REPL[9]:10
 [3] macro expansion
   @ show.jl:1229 [inlined]

As far as I can tell, what's going on is that a() and b() end up sharing the closed-over b in b(); either that, or the assignment in b() ends up overwriting the binding to b in the let block, after which the call to b() in a() fails due to the new object not being a function.

I'd expect this to behave the same as without the let block, that is, just print a() = 1 twice. This was found by a user of Supposition.jl, downstream issue is Seelengrab/Supposition.jl#54. To be clear, the shadowing in b() is fine, it's just surprising/unexpected/undesirable to have that shadowing influence what happens in a() (which shouldn't care about the shadowing in b, and just call the function).

[nsajko]

I don't think this is a bug. Simpler example:

julia> let
           function b() 
               b = 1 
               return b
           end 
           b() 
           b
       end
1

The function expression, when not at module scope, defines a reassignable name.

Perhaps a candidate for strict mode? Xref #54903.

[jariji]

Related: #51223, #51183

[Seelengrab]

The function expression, when not at module scope, defines a reassignable name.

I'm not sure I understand what you're trying to say, this is not the usual terminology. All non-const bindings in julia are reassignable, the question is just which scope you end up assigning into. Do you mean to say that in a local scope, bindings inherited from a parent scope can be assigned to? If so, ok, but that's exactly the point I'm trying to make - I'm arguing that this shouldn't happen for function definitions (or the internally generated closure name) and that those names should completely shadow the definition of the parent scope, rather than reassign it. Surely reassigning a function from within itself and having that show up in yet another function is an unintended consequence of the current scoping rules? Note that due to the way closures are lowered, this even ends up with Core.Boxes everywhere! None of that happens in global scope, where it's just a GlobalRef in a that's completely shadowed (and thus unaffected) within b().

I also want to point out that as a package author, there is pretty much nothing I can do to guard against this. The original MWE (involving @testset) did not make it clear that the expression suddenly moved into a local scope. How do you propose I fix this on my end?

---

Nit: Regardless of this being a bug or not, I picked the compiler: lowering label to communicate that this related to variable scoping, which is decided during lowering. I don't understand why you removed the label?

[nsajko]

I'm arguing that this shouldn't happen

I don't know what was the intention, but agree that this is a footgun. Fixing it seems potentially breaking though. As in, the PR to fix it would have to get a Pkgeval run.

I don't understand why you removed the label?

Because this seems like a semantics/language design issue, not just an implementation bug.

[Seelengrab]

I guess that's true 🤔 The underlying issue on my end would also be solved if @testset didn't introduce a local scope, but I'm not sure how feasible that is. Maybe this just shows the age of the Test stdlib..

[adienes]

I think this is more or less the same issue as #14948, although a slightly different manifestation ofc.

[Seelengrab]

FWIW I consider the fact that @testset introduces a local scope a nice feature allowing me to have testset-local variables.

From the POV of dev-UX, this is an antipattern, since it subtly changes the meaning of code inside. IMO it'd be better to have individual testsets more isolated; introducing a new global scope would be fine. That's also what TestItems.jl does (I don't use it, but lots of people seem to enjoy the features it has).

I think this is more or less the same issue as #14948, although a slightly different manifestation ofc.

Thank you, that indeed seems related! In particular the discourse thread at the end is interesting, because this issue is an example where moving code around definitely changes what the code means.

[nsajko]

introducing a new global scope would be fine

Ah, sorry, that option didn't come to mind. A module per testset sounds nice.

[Seelengrab]

Unfortunately it's also breaking for the Test stdlib, because then you don't get the implicit namespace inheritance you get from the current design. Accessing variables from an outer scope would require explicit namespace traversal upwards, which is not required today. Hence the issue being about lowering this edge case :)

[andrewjradcliffe]

Amusingly, one can use this to inspect variables inside other functions as well:

julia> let
           function a()
               c = 2
               b()
           end
           function b()
               b = 1
               return b
           end
           function c()
               5
           end
           @show a()
           @show c
       end
a() = 1
c = 2
2

[mbauman]

There's nothing really special about either let or closures — this is local scope using the same name and reassigning to it. I'm not sure what a "fix" would really even look like.

Maybe local-scope named functions could use a local const for their names (if/when we have it: #5148)?

[Seelengrab]

I think that's a good idea, yeah. I really hope noone relies on this rebinding behavior for local functions in particular...

[jariji]

Imho the problem here is the same old problem that definition and reassignment use the same = syntax. In #51223 I propose an alternative := reassignment syntax.

[andrewjradcliffe]

The surprise is that function declarations which appear inside let blocks, function bodies, macro bodies and comprehensions do not confer hard scope semantics. On the other hand, definition and assignment can be disambiguated using the local keyword, e.g.

let
    function a()
        b()
    end
    function b()
        local b = 1
        return b
    end
    @show a()
    @show a()
end

However, being pragmatic, it's a worthwhile compromise in order to support classics such as mimicry of objects using lambdas:

function make_account(initial_amount)
    x = initial_amount
    function deposit!(arg)
        x = x + arg;
        nothing
    end
    function withdraw!(arg)
        if x < arg
            error("insufficient balance")
        else
            x = x - arg;
        end
        nothing
    end
    function f(op::Symbol, arg)
        if op === :add
            deposit!(arg)
        elseif op === :sub
            withdraw!(arg)
        elseif op === :ret
            x
        else
            error("unknown operation")
        end
    end
    f
end
withdraw!(account, amount) = account(:sub, amount)
deposit!(account, amount) = account(:add, amount)
get_balance(account) = account(:ret, nothing)

In general, many of what appear to be quirks on the surface hark back to Scheme. There, define and set! mostly make this a non-issue. In Julia, local is enough to resolve symbol shadowing where necessary. Though tedious on the surface, judicious use of local makes Julia code easier on the human eye. However, I write a lot of Rust code, so I expect font-locked keywords to precede every variable declaration; YMMV.

[c42f]

While this issue as a whole is certainly a case of "lexical scope is working as intended", I don't love the outcome in this case and I feel it would be great to make it an error.

Maybe local-scope named functions could use a local const for their names (if/when we have it: #5148)?

@mbauman I like this solution a lot.

For outer functions, the function keyword always introduces a const binding at module scope. I feel it would be extremely sensible if we did it for closures too and used that to remove this footgun.

[Seelengrab]

Sorry, maybe I'm misunderstanding something - what expression exactly should error? Note that this was uncovered through a macro expansion inside of an @testset, which introduces the let block under the hood. This syntax currently works, so changing that would IMO be breaking.