julia icon indicating copy to clipboard operation
julia copied to clipboard

variable `include`d from file not defined in `Main` in 1.12

Open pjaap opened this issue 6 months ago • 27 comments

Hey there, I have a failing test with Julia 1.12 (beta3). Here is a MWE:

A file test.jl constains only the line a = 42.

julia> function foo()
           include("test.jl")
           return a
       end
foo (generic function with 1 method)

With julia 1.11.5:

julia> using Test; @test foo() == 42
Test Passed

With Julia 1.12 (beta3):

julia> using Test; @test foo() == 42
Error During Test at REPL[2]:1
  Test threw exception
  Expression: foo() == 42
  UndefVarError: `a` not defined in `Main`
  Suggestion: check for spelling errors or missing imports.
  Stacktrace:
   [1] foo()
     @ Main ./REPL[1]:3
   [2] top-level scope
     @ REPL[2]:1
   [3] macro expansion
     @ ~/.julia/juliaup/julia-1.12.0-beta3+0.x64.linux.gnu/share/julia/stdlib/v1.12/Test/src/Test.jl:676 [inlined]
ERROR: There was an error during testing

According to the docs, include should evaluate the contents of test.jl in the current module, which is Main. So I believe this is a regression in Julia 1.12

pjaap avatar May 23 '25 13:05 pjaap

Reproducer that doesn't depend on the test system:

julia> function foo()
           include("/tmp/j.jl")  # contents: `a = 7`
           return a
       end
foo (generic function with 1 method)
 
julia> foo()
ERROR: UndefVarError: `a` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
 [1] foo()
   @ Main ./REPL[1]:3
 [2] top-level scope
   @ REPL[2]:1

nsajko avatar May 23 '25 14:05 nsajko

Thanks, I changed the title

pjaap avatar May 23 '25 14:05 pjaap

Duplicate of #58286? That was slightly different, but the symptoms look similar.

giordano avatar May 23 '25 14:05 giordano

A less minimal MWE that gives some insight:

julia> function foo()
           include("test.jl")
           return a
       end
foo (generic function with 1 method)

julia> a # removing this doesn't change anything
ERROR: UndefVarError: `a` not defined in `Main`
Suggestion: check for spelling errors or missing imports.

julia> foo() # the first call fails
ERROR: UndefVarError: `a` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
 [1] foo()
   @ Main ./REPL[1]:3
 [2] top-level scope
   @ REPL[3]:1

julia> a # removing this doesn't change anything
42

julia> foo() # the second call succeeds
42

So it looks like a world age issue (or something of that flavor -- honestly all that stuff is above me) of some sort, where in v1.12 the global is not available until after the function has returned (and then is defined in future calls). It looks like there might be relevant discussion in the above-referenced issue, but that discussion is a little technical for me.

I'll remark that using include within a function is a footgun and is something that we might prefer to forbid (or wish we had, in an alternate timeline). I think people tend to misunderstand the implications of evaluation in global scope.

mikmoore avatar May 23 '25 18:05 mikmoore

Our UndefVarError error messages are not world-aware, so they can print somewhat nonsensical results when they didn't happen in the latest world

julia> function foo()
           @eval global a = 1
           return a
       end
foo (generic function with 1 method)

julia> foo()
ERROR: UndefVarError: `a` not defined in `Main`
Suggestion: check for spelling errors or missing imports.

vtjnash avatar May 23 '25 20:05 vtjnash

I think the main question is whether return a is supposed to declare the weak existence of global a (resolves-or-creates a during method definition). Of course we couldn't have done that before bindings were world-aware, but I think we could do that now without too much issue (other than conflicting with printing the backdated const warning)?

julia> GlobalRef(Main, :a).binding
Binding Main.a
   38660:∞ - undefined binding - guard entry

vtjnash avatar May 23 '25 20:05 vtjnash

I think the main question is whether return a is supposed to declare the weak existence of global a (resolves-or-creates a during method definition).

Last time this came up, I think we reached the conclusion that only explicit global a does that, but I don't recall if there was a good reason for that.

Keno avatar May 27 '25 02:05 Keno

It does lead to this hilarious situation:

julia> function f()
         @eval const foo = 3
         return foo
       end
f (generic function with 1 method)

julia> f()
WARNING: Detected access to binding `Main.foo` in a world prior to its definition world.
  Julia 1.12 has introduced more strict world age semantics for global bindings.
  !!! This code may malfunction under Revise.
  !!! This code will error in future versions of Julia.
Hint: Add an appropriate `invokelatest` around the access to this binding.
To make this warning an error, and hence obtain a stack trace, use `julia --depwarn=error`.
3

julia> function f()
         @eval global bar = 3
         return bar
       end
f (generic function with 1 method)

julia> f()
ERROR: UndefVarError: `bar` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
 [1] f()
   @ Main ./REPL[5]:3
 [2] top-level scope
   @ REPL[6]:1

vtjnash avatar May 29 '25 19:05 vtjnash

@JeffBezanson and I have concluded we should fix this by replacing the UndefVarError with a call to invokelatest, just like the error message said, and then delete the warning since the compiler would have already done exactly what the warning said to do, removing the need for the user to be aware of it or do it themselves. it took a little bit to convince ourselves that there wouldn't be unfortunate side-effects from that, but it seems actually much simpler and only loses one very useless optimization (that of guaranteeing we infer that accessing a variable that is never defined will be guaranteed to throw an exception)

vtjnash avatar May 29 '25 20:05 vtjnash

I don't think I like that at all. I think it makes the mental model of how this feature works way too complicated. In particular, it means that e.g. tasks started before the constant definition will always show the latest one, but if you swap the order of the definitions, you get the world fixing behavior. I think this would be very confusing to users. The UndefVarError with the world age explanation is much better. Also, I don't think the optimization is useless. It prevents type inference from going bad in dynamically unreachable paths. Those now always get inferred ::Any, which poisons a lot of inference results. Somebody was complaining about that just the other day. The backdated const thing is a temporary hack - we should not elevate it to language semantics.

My preferred resolution to this issue is to have lowering insert explicit global exprs when it resolved a symbol to global scope and then use the ordinary soft global declaration.

Keno avatar Jun 01 '25 07:06 Keno

If we do an implicit invokelatest here, I don't see why we wouldn't also do an implicit invokelatest on MethodError. To be clear, I'm not advocating for that, but I think it's easier to see there why it's not a good idea.

Keno avatar Jun 01 '25 07:06 Keno

Turning all method invocations into invokelatest is costly and unexpected. Turning all globals into un-inferred invokelatest is simply business as normal for many years and prevents breaking existing code

vtjnash avatar Jun 01 '25 15:06 vtjnash

Except it's not, because the longstanding behavior was to have all global accesses have invokelatest behavior. Giving only some accesses invokelatest behavior depending on ehat happened to be defined is not a good idea.

Keno avatar Jun 01 '25 15:06 Keno

Talking with Jeff in-person, there was a compromise proposal to have access to undefined bindings still load the binding's value. I still don't like that variant, but it's significantly better than an implicit invokelatest.

Keno avatar Jun 02 '25 21:06 Keno

IIUC, you mean that proposal would apply only to later declared globals, not later using or const globals? (perhaps essentially just making all bindings soft globals until either declared or get resolved as an import)

vtjnash avatar Jun 02 '25 21:06 vtjnash

Correct. An additional variation could cause const to implicitly assign to the global on declaration, which could replace the backdated const logic.

Keno avatar Jun 02 '25 21:06 Keno

This was discussed very extensively by triage today. The overall sense was that this is a tricky issue, but it sounded like we were leaning towards keeping the current behavior, but further improving the error message if possible. That said, there was a desire for a full writeup of the remaining options, which this comment aims to do.

In general, I think I was able to convince people that the invokelatest option is not workable. I think the example that convinced @JeffBezanson is the following:

module A
    export x
    x = 1
end
module B
    export x
    x = 2
end
t = @async while true
    wait()
    @eval using .A
    x
end
yield(t) # resolves `x` to A
using .B
yield(t) # `UndefVarError` (ambiguous)

In particular, the re-resolution of implicit bindings in the middle of a loop feels weird in the absence of an explicit license (via invokelatest or @latestworld) to do that.

@vtjnash made the argument that an implicit invokelatest is closest to the pre-1.12 behavior. And while I think that's technically true, it think it also misses the fact that we have significantly expanded how bindings can change in 1.12. In particular, in 1.11 bindings were "saturating" in a sense. Once you've resolved them, (or assigned a const to them), they don't change again. The 1.12 behavior is more flexible and the value of a binding can differ significantly as you advance worlds.

This then leaves two variants of my compromise proposal above that have some subtle differences:

Proposal A: Lowering introduces "super weak global" binding kind when resolving symbols to global

This is an extension of the observation that:

function foo()
    @eval global a = 1
    global a
    return a
end

works the way the OP in this issue expected. So why not (as was my original preference) implicitly introduce global when lowering resolves a symbol to global scope. The issue with this is that even though the so-introduced global is weak, it is still stronger than implicit resolution, so the following common pattern:

bar() = return x
using .MyPackage # exports x
bar()

Would break, because x was implicitly declared as a weak global. So that's not workable. But could we instead have an even weaker version of the declaration that (tentatively called "super weak") that is weaker than implicit resolution?

There are a lot of appealing things about it. It would address the current issue and is based in lowering (which also decides about all the other declarations). As a particular implication, a future strict mode setting could disable the introduction of these super weak declarations and mandate explicit weak or strong declarations where desired.

For completeness, there are some more design parameters for this:

  • If we also have explicit declarations (const x, import Foo, import Foo: x) be a shadow assignment to the global, this would also subsume the current backdated const logic.
  • setglobal! on super weak globals is disallowed (but a later change to a stronger kind of global and subsequent assignment can be observed in the old world - as in the OP example)

That said, one downside that @JeffBezanson noted was that this would cause x and Main.x to semantically diverge with respect to the binding declaration. In particular, the former would declare the global (and make the OP work), while the latter would not and exhibit the current 1.12 behavior.

An additional downside is that it would potentially cause additional binding invalidation traffic.

Proposal B: All undef bindings are implicitly super weak

This is a tweak of Proposal A that matches my second proposal above (to have access to undef bindings implicitly load the global value). The way to think about it is that all bindings (whether accessed or not) implicitly have the "super weak global" behavior. This would once again make x and Main.x behave semantically equivalent. The primary downsides are:

  • It completely disables the above mentioned optimization to cut off inference on access to undef vars.
  • If we ever want a strict mode for this, it would no longer be lexical, but rather a runtime flag on the Module's binding table. Not the end of the world, but the kind of thing that makes on pause and think whether we're on the wrong track.

My personal thoughts

During triage, I thought, I'd convinced myself that it would be good to do one of these. My primary motivation is that I think it's weird that the OP "works" the first time, but not the second time. However, thinking on it a bit more afterwards, I actually think it's good to get the error. It is true that the example works the second time. However, for other binding kinds (const, imports, etc), the binding value read would be one generation behind. Anything that makes the first definition work different to make things magically work will just cause confusion as to why it doesn't work on redefinition.

So overall, I remain moderately in favor of just keeping the current behavior. I still think Proposal A would be acceptable, but would significantly complicate the semantics, which I think can end up just more confusing.

Keno avatar Jun 05 '25 22:06 Keno

I also prefer keeping the current behavior as it is by far the simplest behavior to explain and it was kind of dodgy to do this in the first place. The main issue are the cases where someone does this kind of thing:

function foo()
    @eval import Bar: bar
    return bar
end

I don't recall the exact example and everything was posted in Zoom chat, so it's gone 🤦‍♂

StefanKarpinski avatar Jun 05 '25 22:06 StefanKarpinski

Returning to the original example and how to disambiguate it:

function foo()
    include("file.jl")
    return x
end

My understanding is that there are a few possible meanings that are intended:

  • x already has some meaning before the include—this is fine and requires no disambiguation
  • x is a global in the current module, in which case one should write return global x
  • x is imported from some package that is loaded during the include; what should one write then? Do we recommend explicitly qualifying it and doing return Foo.x?

StefanKarpinski avatar Jun 05 '25 22:06 StefanKarpinski

  • x is imported from some package that is loaded during the include; what should one write then? Do we recommend explicitly qualifying it and doing return Foo.x?

You can always write @world(x, ∞) to get the invokelatest behavior explicitly. If that's what you want (what you, really, really want, even though it's a bad idea), I think that's just what you'd have to do.

Keno avatar Jun 05 '25 22:06 Keno

Ok, but what's the right way to do it? "Don't do that"?

StefanKarpinski avatar Jun 05 '25 22:06 StefanKarpinski

Ok, but what's the right way to do it? "Don't do that"?

Yes. What are you gonna do with it the value anyway? It's often some struct defined in the package for which all the methods only exist in the newest world anyway. The general pattern is something like:

_do_something_with_the_package() = ...
function do_something_with_the_package()
    @isdefined(ThePackage) && return _do_something_with_the_package()
    @eval using ThePackage
    invokelatest(_do_something_with_the_package)
end

Keno avatar Jun 05 '25 22:06 Keno

Thank you all for the intense discussions on that problem. I didn't know that the underlying issue is so involved.

I am no expert on the inner guts of Julia, but the proposed workarounds scare me a bit. Maybe I should elaborate on the origin of the problem.

We write user-orientated simulation code and read parameters from files, as in https://github.com/WIAS-PDELib/ChargeTransport.jl/blob/35c1c0dca78da724dd9a8a60205f5f87ea6be6a9/examples/PSC_2D_unstructuredGrid.jl#L40 where the parameter files just contain declared variables: https://github.com/WIAS-PDELib/ChargeTransport.jl/blob/master/parameter_files/Params_PSC_PCBM_MAPI_Pedot.jl This code base is used by a bunch of collaborators. And it is broken on current Julia 1.12 as reported in the first comment.

For my understanding, our usage of include is covered by the documentation of include.

edit: typo

pjaap avatar Jun 06 '25 11:06 pjaap

is it true that include("file.jl") does the same thing as (eval ∘ Meta.parse ∘ String ∘ read)("file.jl") ? I think current behavior at least here looks like yes, right?

adienes avatar Jun 06 '25 12:06 adienes

For my understanding, our usage of include is covered by the documentation of include.

Not really. Your code is written in a style that is somewhat common in other systems but doesn't work the same in Julia. In particular, in other systems, include would evaluate into the local scope, which is explicitly what the documentation says our include does not do. The easy workaround for you is to put explicit global declarations into that function for all the variables that you expect the script to override. There is a bit of a broader issue though, which is that using global state here (perhaps accidentally on the assumption that include would evaluate into local scope) is a bit of a code smell, and the issues you're seeing are a result of that. Global state here has other issues, including data races if multiple jobs are run in parallel, sub-optimal performance (although for configuration it probably doesn't matter). I would recommend that longer term, you consider using a proper configuration struct. That'd have the additional benefit of allowing you to e.g. throw errors on unknown parameters, etc. Note that this is only a recommendation. You can choose get the same behavior as in other systems, but you'd need an explicit @eval splice of file contents into the rest of the script.

Keno avatar Jun 06 '25 18:06 Keno

is it true that include("file.jl") does the same thing as (eval ∘ Meta.parse ∘ String ∘ read)("file.jl") ?

Semantically equivalent up to debug info and source provenance tracking, yes.

Keno avatar Jun 06 '25 18:06 Keno

For my understanding, our usage of include is covered by the documentation of include.

Not really. [...]

Thanks for the clarification. I'll forward the struct idea to my group.

pjaap avatar Jun 10 '25 06:06 pjaap

Per above discuss, no further action.

Keno avatar Jun 19 '25 04:06 Keno

This seems to keep coming up quite a bit, and we're still in early prerelease. We have an easy fix for it proposed to prevent breakage, so maybe we should just do that?

vtjnash avatar Aug 29 '25 17:08 vtjnash

@keno, we're discussing this again. Can you distill what the semantic issue you have with emitting an invoke latest for unknown globals is? Is it just a performance issue or is it a behavioral difference?

StefanKarpinski avatar Sep 11 '25 17:09 StefanKarpinski