Address code review

Compiler/src/inferencestate.jl

@@ -211,7 +211,7 @@ const VarTable = Vector{VarState}
 
 struct StatementState
     vtypes::Union{VarTable,Nothing}
-    bailed::Bool
+    saw_latestworld::Bool
 end
 
 const CACHE_MODE_NULL     = 0x00      # not cached, optimization optional
@@ -265,7 +265,7 @@ mutable struct InferenceState
     ssavalue_uses::Vector{BitSet} # ssavalue sparsity and restart info
     # TODO: Could keep this sparsely by doing structural liveness analysis ahead of time.
     bb_vartables::Vector{Union{Nothing,VarTable}} # nothing if not analyzed yet
-    bb_bailed::Vector{Bool}
+    bb_saw_latestworld::Vector{Bool}
     ssavaluetypes::Vector{Any}
     edges::Vector{Any}
     stmt_info::Vector{CallInfo}
@@ -326,7 +326,7 @@ mutable struct InferenceState
 
         nslots = length(src.slotflags)
         slottypes = Vector{Any}(undef, nslots)
-        bb_bailed = Bool[false for i = 1:length(cfg.blocks)]
+        bb_saw_latestworld = Bool[false for i = 1:length(cfg.blocks)]
         bb_vartables = Union{Nothing,VarTable}[ nothing for i = 1:length(cfg.blocks) ]
         bb_vartable1 = bb_vartables[1] = VarTable(undef, nslots)
         argtypes = result.argtypes
@@ -374,7 +374,7 @@ mutable struct InferenceState
 
         this = new(
             mi, WorldWithRange(world, valid_worlds), mod, sptypes, slottypes, src, cfg, spec_info,
-            currbb, currpc, ip, handler_info, ssavalue_uses, bb_vartables, bb_bailed, ssavaluetypes, edges, stmt_info,
+            currbb, currpc, ip, handler_info, ssavalue_uses, bb_vartables, bb_saw_latestworld, ssavaluetypes, edges, stmt_info,
             tasks, pclimitations, limitations, cycle_backedges, callstack, parentid, frameid, cycleid,
             result, unreachable, bestguess, exc_bestguess, ipo_effects,
             restrict_abstract_call_sites, cache_mode, insert_coverage,

Compiler/src/ssair/irinterp.jl

@@ -50,7 +50,7 @@ function abstract_eval_phi_stmt(interp::AbstractInterpreter, phi::PhiNode, ::Int
 end
 
 function abstract_call(interp::AbstractInterpreter, arginfo::ArgInfo, sstate::StatementState, irsv::IRInterpretationState)
-    si = StmtInfo(true, sstate.bailed) # TODO better job here?
+    si = StmtInfo(true, sstate.saw_latestworld) # TODO better job here?
     call = abstract_call(interp, arginfo, si, irsv)::Future
     Future{Any}(call, interp, irsv) do call, interp, irsv
         irsv.ir.stmts[irsv.curridx][:info] = call.info

Compiler/src/tfuncs.jl

@@ -1426,7 +1426,7 @@ end
         # as well as compute the info for the method matches
         op = unwrapva(argtypes[op_argi])
         v = unwrapva(argtypes[v_argi])
-        callinfo = abstract_call(interp, ArgInfo(nothing, Any[op, TF, v]), StmtInfo(true, si.bailed), sv, #=max_methods=#1)
+        callinfo = abstract_call(interp, ArgInfo(nothing, Any[op, TF, v]), StmtInfo(true, si.saw_latestworld), sv, #=max_methods=#1)
         TF = Core.Box(TF)
         RT = Core.Box(RT)
         return Future{CallMeta}(callinfo, interp, sv) do callinfo, interp, sv

Compiler/src/types.jl

@@ -41,7 +41,7 @@ struct StmtInfo
     need thus not be computed.
     """
     used::Bool
-    bailed::Bool
+    saw_latestworld::Bool
 end
 
 struct SpecInfo

base/essentials.jl

@@ -467,20 +467,18 @@ Evaluate an expression with values interpolated into it using `eval`.
 If two arguments are provided, the first is the module to evaluate in.
 """
 macro eval(ex)
-    g = ccall(:jl_gensym, Ref{Symbol}, ())
-    return Expr(:let, Expr(:(=), g,
+    return Expr(:let, Expr(:(=), :eval_local_result,
             Expr(:escape, Expr(:call, GlobalRef(Core, :eval), __module__, Expr(:quote, ex)))),
         Expr(:block,
             Expr(:var"latestworld-if-toplevel"),
-            g))
+            :eval_local_result))
 end
 macro eval(mod, ex)
-    g = ccall(:jl_gensym, Ref{Symbol}, ())
-    return Expr(:let, Expr(:(=), g,
+    return Expr(:let, Expr(:(=), :eval_local_result,
             Expr(:escape, Expr(:call, GlobalRef(Core, :eval), mod, Expr(:quote, ex)))),
         Expr(:block,
             Expr(:var"latestworld-if-toplevel"),
-            g))
+            :eval_local_result))
 end
 
 # use `@eval` here to directly form `:new` expressions avoid implicit `convert`s

src/codegen.cpp

@@ -6724,8 +6724,6 @@ static std::pair<Function*, Function*> get_oc_function(jl_codectx_t &ctx, jl_met
 static void emit_latestworld(jl_codectx_t &ctx)
 {
     auto world_age_field = get_tls_world_age_field(ctx);
-    // we're at toplevel; insert an atomic barrier between every instruction
-    // TODO: inference is invalid if this has any effect (which it often does)
     LoadInst *world = ctx.builder.CreateAlignedLoad(ctx.types().T_size,
         prepare_global_in(jl_Module, jlgetworld_global), ctx.types().alignof_ptr,
         /*isVolatile*/false);
@@ -9586,6 +9584,8 @@ static jl_llvm_functions_t
             }
 
             mallocVisitStmt(sync_bytes, have_dbg_update);
+            // N.B.: For toplevel thunks, we expect world age restore to be handled
+            // by the interpreter which invokes us.
             if (ctx.is_opaque_closure)
                 ctx.builder.CreateStore(last_age, world_age_field);
             assert(type_is_ghost(retty) || returninfo.cc == jl_returninfo_t::SRet ||

src/interpreter.c

@@ -886,7 +886,10 @@ jl_value_t *NOINLINE jl_interpret_toplevel_thunk(jl_module_t *m, jl_code_info_t
     s->mi = NULL;
     s->ci = NULL;
     JL_GC_ENABLEFRAME(s);
+    jl_task_t *ct = jl_current_task;
+    size_t last_age = ct->world_age;
     jl_value_t *r = eval_body(stmts, s, 0, 1);
+    ct->world_age = last_age;
     JL_GC_POP();
     return r;
 }

src/toplevel.c

@@ -225,8 +225,9 @@ static jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex
 
     for (int i = 0; i < jl_array_nrows(exprs); i++) {
         // process toplevel form
-        ct->world_age = jl_atomic_load_relaxed(&jl_world_counter);
+        ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
         form = jl_expand_stmt_with_loc(jl_array_ptr_ref(exprs, i), newm, filename, lineno);
+        ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
         (void)jl_toplevel_eval_flex(newm, form, 1, 1, &filename, &lineno);
     }
     ct->world_age = last_age;
@@ -863,7 +864,6 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_val
 
     if (head == jl_module_sym) {
         jl_value_t *val = jl_eval_module_expr(m, ex);
-        ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
         JL_GC_POP();
         return val;
     }
@@ -919,9 +919,6 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_val
             jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno,
                 "syntax: malformed \"using\" statement");
         }
-        if (!expanded) {
-            ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
-        }
         JL_GC_POP();
         return jl_nothing;
     }
@@ -970,12 +967,6 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_val
             jl_eval_errorf(m, *toplevel_filename, *toplevel_lineno,
                 "syntax: malformed \"import\" statement");
         }
-        if (!expanded) {
-            // To avoid having to roundtrip every `using` expression through
-            // lowering, just to add the world-age increment effect, do it
-            // manually here.
-            ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
-        }
         JL_GC_POP();
         return jl_nothing;
     }
@@ -1010,9 +1001,15 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval_flex(jl_module_t *JL_NONNULL m, jl_val
         jl_value_t *res = jl_nothing;
         int i;
         for (i = 0; i < jl_array_nrows(ex->args); i++) {
+            root = jl_array_ptr_ref(ex->args, i);
+            if (jl_needs_lowering(root)) {
+                ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
+                root = jl_expand_with_loc_warn(root, m, *toplevel_filename, *toplevel_lineno);
+            }
             ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
-            res = jl_toplevel_eval_flex(m, jl_array_ptr_ref(ex->args, i), fast, 0, toplevel_filename, toplevel_lineno);
+            res = jl_toplevel_eval_flex(m, root, fast, 1, toplevel_filename, toplevel_lineno);
         }
+        ct->world_age = last_age;
         JL_GC_POP();
         return res;
     }
@@ -1125,8 +1122,8 @@ JL_DLLEXPORT jl_value_t *jl_toplevel_eval_in(jl_module_t *m, jl_value_t *ex)
     jl_lineno = 1;
     jl_filename = "none";
     size_t last_age = ct->world_age;
-    ct->world_age = jl_atomic_load_relaxed(&jl_world_counter);
     JL_TRY {
+        ct->world_age = jl_atomic_load_relaxed(&jl_world_counter);
         v = jl_toplevel_eval(m, ex);
     }
     JL_CATCH {
@@ -1184,14 +1181,14 @@ static jl_value_t *jl_parse_eval_all(jl_module_t *module, jl_value_t *text,
     int last_lineno = jl_lineno;
     const char *last_filename = jl_filename;
     size_t last_age = ct->world_age;
-    ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
     int lineno = 0;
     jl_lineno = 0;
     const char *filename_str = jl_string_data(filename);
     jl_filename = filename_str;
     int err = 0;
 
     JL_TRY {
+        ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
         for (size_t i = 0; i < jl_expr_nargs(ast); i++) {
             expression = jl_exprarg(ast, i);
             if (jl_is_linenode(expression)) {
@@ -1203,6 +1200,7 @@ static jl_value_t *jl_parse_eval_all(jl_module_t *module, jl_value_t *text,
             ct->world_age = jl_atomic_load_relaxed(&jl_world_counter);
             expression = jl_expand_with_loc_warn(expression, module,
                                                  jl_string_data(filename), lineno);
+            ct->world_age = jl_atomic_load_relaxed(&jl_world_counter);
             result = jl_toplevel_eval_flex(module, expression, 1, 1, &filename_str, &lineno);
         }
     }

stdlib/REPL/src/REPL.jl

@@ -343,9 +343,9 @@ __repl_entry_eval_expanded_with_loc(mod::Module, @nospecialize(ast), toplevel_fi
 
 function toplevel_eval_with_hooks(mod::Module, @nospecialize(ast), toplevel_file=Ref{Ptr{UInt8}}(Base.unsafe_convert(Ptr{UInt8}, :REPL)), toplevel_line=Ref{Cint}(1))
     if !isexpr(ast, :toplevel)
-        ast = __repl_entry_lower_with_loc(mod, ast, toplevel_file, toplevel_line)
+        ast = invokelatest(__repl_entry_lower_with_loc, mod, ast, toplevel_file, toplevel_line)
         check_for_missing_packages_and_run_hooks(ast)
-        return __repl_entry_eval_expanded_with_loc(mod, ast, toplevel_file, toplevel_line)
+        return invokelatest(__repl_entry_eval_expanded_with_loc, mod, ast, toplevel_file, toplevel_line)
     end
     local value=nothing
     for i = 1:length(ast.args)

test/ccall.jl

@@ -70,10 +70,8 @@ function gen_ccall_echo(x, T, U, ret=nothing)
         func_ex = :($ret($func_ex))
     end
     @gensym func_name
-    quote
-        @noinline $(esc(func_name))(x) = $func_ex
-        $(esc(func_name))($(esc(x)))
-    end
+    @eval @noinline $func_name(x) = $func_ex
+    :($func_name($(esc(x))))
 end
 
 macro ccall_echo_func(x, T, U)

test/core.jl

@@ -8339,6 +8339,16 @@ let foo = eval(Expr(:toplevel, :(module BarModuleInc; struct FooModuleInc; end;
     @test foo == BarModuleInc.FooModuleInc()
 end
 
-eval(:(module BarModuleInc; module BazModuleInc; struct FooModuleInc; end; end; const foo = BazModuleInc.FooModuleInc(); end))
-@Core.latestworld
-@test BarModuleInc.foo == BarModuleInc.BazModuleInc.FooModuleInc()
+let
+    eval(:(module BarModuleInc2; module BazModuleInc; struct FooModuleInc; end; end; const foo = BazModuleInc.FooModuleInc(); end))
+    @Core.latestworld
+    @test BarModuleInc2.foo == BarModuleInc2.BazModuleInc.FooModuleInc()
+end
+
+# `toplevel` has implicit world age increment between expansion and evaluation
+macro define_call(sym)
+    Core.eval(__module__, :($sym() = 1))
+    :($sym())
+end
+@test eval(Expr(:toplevel, :(@define_call(f_macro_defined1)))) == 1
+@test @define_call(f_macro_defined2) == 1

test/error.jl

@@ -93,8 +93,7 @@ end
 @testset "MethodError for methods without line numbers" begin
     try
         eval(Expr(:function, :(f44319()), 0))
-        @Core.latestworld
-        f44319()
+        @invokelatest f44319()
     catch e
         s = sprint(showerror, e)
         @test s == """MethodError: no method matching f44319(::Int$(Sys.WORD_SIZE))