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..

[nsajko]

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

[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 :)