lowering: track location of macros better for stackwalk

Noticed since `scrub_backtrace` is looking for a `macro expansion` when printing an exception thrown in `@test`, but that was missing for simple expressions, particularly those at the top-level or spliced in. Fix #44757
JuliaLang · Apr 15, 2022 · b28c996 · b28c996
1 parent 95bec91
commit b28c996
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 47 deletions.
diff --git a/src/ast.c b/src/ast.c
@@ -52,6 +52,9 @@ JL_DLLEXPORT jl_sym_t *jl_new_sym;
 JL_DLLEXPORT jl_sym_t *jl_using_sym;
 JL_DLLEXPORT jl_sym_t *jl_splatnew_sym;
 JL_DLLEXPORT jl_sym_t *jl_block_sym;
+JL_DLLEXPORT jl_sym_t *jl_if_sym;
+JL_DLLEXPORT jl_sym_t *jl_for_sym;
+JL_DLLEXPORT jl_sym_t *jl_while_sym;
 JL_DLLEXPORT jl_sym_t *jl_new_opaque_closure_sym;
 JL_DLLEXPORT jl_sym_t *jl_opaque_closure_method_sym;
 JL_DLLEXPORT jl_sym_t *jl_const_sym;
@@ -364,6 +367,9 @@ void jl_init_common_symbols(void)
     jl_popaliasscope_sym = jl_symbol("popaliasscope");
     jl_thismodule_sym = jl_symbol("thismodule");
     jl_block_sym = jl_symbol("block");
+    jl_if_sym = jl_symbol("if");
+    jl_for_sym = jl_symbol("for");
+    jl_while_sym = jl_symbol("while");
     jl_atom_sym = jl_symbol("atom");
     jl_statement_sym = jl_symbol("statement");
     jl_all_sym = jl_symbol("all");
@@ -992,7 +998,7 @@ int jl_has_meta(jl_array_t *body, jl_sym_t *sym) JL_NOTSAFEPOINT
     return 0;
 }
 
-static jl_value_t *jl_invoke_julia_macro(jl_array_t *args, jl_module_t *inmodule, jl_module_t **ctx, size_t world, int throw_load_error)
+static jl_expr_t *jl_invoke_julia_macro(jl_array_t *args, jl_module_t *inmodule, jl_module_t **ctx, size_t world, int throw_load_error)
 {
     jl_task_t *ct = jl_current_task;
     JL_TIMING(MACRO_INVOCATION);
@@ -1004,10 +1010,9 @@ static jl_value_t *jl_invoke_julia_macro(jl_array_t *args, jl_module_t *inmodule
     margs[0] = jl_array_ptr_ref(args, 0);
     // __source__ argument
     jl_value_t *lno = jl_array_ptr_ref(args, 1);
+    if (!jl_typeis(lno, jl_linenumbernode_type))
+        lno = jl_new_struct(jl_linenumbernode_type, jl_box_long(0), jl_nothing);
     margs[1] = lno;
-    if (!jl_typeis(lno, jl_linenumbernode_type)) {
-        margs[1] = jl_new_struct(jl_linenumbernode_type, jl_box_long(0), jl_nothing);
-    }
     margs[2] = (jl_value_t*)inmodule;
     for (i = 3; i < nargs; i++)
         margs[i] = jl_array_ptr_ref(args, i - 1);
@@ -1033,7 +1038,6 @@ static jl_value_t *jl_invoke_julia_macro(jl_array_t *args, jl_module_t *inmodule
             jl_rethrow();
         }
         else {
-            jl_value_t *lno = margs[1];
             jl_value_t *file = jl_fieldref(lno, 1);
             if (jl_is_symbol(file))
                 margs[0] = jl_cstr_to_string(jl_symbol_name((jl_sym_t*)file));
@@ -1044,9 +1048,13 @@ static jl_value_t *jl_invoke_julia_macro(jl_array_t *args, jl_module_t *inmodule
                                            jl_current_exception()));
         }
     }
+    margs[0] = result;
+    jl_expr_t *wrap = jl_exprn(jl_block_sym, 2);
+    jl_exprargset(wrap, 0, lno);
+    jl_exprargset(wrap, 1, result);
     ct->world_age = last_age;
     JL_GC_POP();
-    return result;
+    return wrap;
 }
 
 static jl_value_t *jl_expand_macros(jl_value_t *expr, jl_module_t *inmodule, struct macroctx_stack *macroctx, int onelevel, size_t world, int throw_load_error)
@@ -1082,18 +1090,32 @@ static jl_value_t *jl_expand_macros(jl_value_t *expr, jl_module_t *inmodule, str
         struct macroctx_stack newctx;
         newctx.m = macroctx ? macroctx->m : inmodule;
         newctx.parent = macroctx;
-        jl_value_t *result = jl_invoke_julia_macro(e->args, inmodule, &newctx.m, world, throw_load_error);
-        jl_value_t *wrap = NULL;
-        JL_GC_PUSH3(&result, &wrap, &newctx.m);
-        // copy and wrap the result in `(hygienic-scope ,result ,newctx)
+        jl_expr_t *lno = jl_invoke_julia_macro(e->args, inmodule, &newctx.m, world, throw_load_error);
+        jl_value_t *result = jl_exprarg(lno, 1);
+        int hygienic = 0;
+        // copy and wrap the result in `(hygienic-scope ,result ,newctx) if applicable
         if (jl_is_expr(result) && ((jl_expr_t*)result)->head == jl_escape_sym)
             result = jl_exprarg(result, 0);
         else
-            wrap = (jl_value_t*)jl_exprn(jl_hygienicscope_sym, 2);
+            hygienic = 1;
+        JL_GC_PUSH3(&result, &lno, &newctx.m);
         result = jl_copy_ast(result);
         if (!onelevel)
-            result = jl_expand_macros(result, inmodule, wrap ? &newctx : macroctx, onelevel, world, throw_load_error);
-        if (wrap) {
+            result = jl_expand_macros(result, inmodule, hygienic ? &newctx : macroctx, onelevel, world, throw_load_error);
+        if (throw_load_error && jl_is_expr(result) &&
+                (((jl_expr_t*)result)->head == jl_block_sym ||
+                ((jl_expr_t*)result)->head == jl_if_sym ||
+                ((jl_expr_t*)result)->head == jl_for_sym ||
+                ((jl_expr_t*)result)->head == jl_while_sym ||
+                ((jl_expr_t*)result)->head == jl_call_sym ||
+                ((jl_expr_t*)result)->head == jl_foreigncall_sym)) {
+            // record the original line number of the macro, if we think it will be safe,
+            // ensuring we preserve the original line in the stacktrace in these cases
+            jl_exprargset(lno, 1, result);
+            result = (jl_value_t*)lno;
+        }
+        if (hygienic) {
+            jl_value_t *wrap = (jl_value_t*)jl_exprn(jl_hygienicscope_sym, 2);
             jl_exprargset(wrap, 0, result);
             jl_exprargset(wrap, 1, newctx.m);
             result = wrap;
@@ -1138,6 +1160,7 @@ JL_DLLEXPORT jl_value_t *jl_macroexpand(jl_value_t *expr, jl_module_t *inmodule)
     JL_GC_PUSH1(&expr);
     expr = jl_copy_ast(expr);
     expr = jl_expand_macros(expr, inmodule, NULL, 0, jl_atomic_load_acquire(&jl_world_counter), 0);
+    // we do not call jl_tag_macrocall_source here, since we assume the user of macrocall expects to handle it themselves
     expr = jl_call_scm_on_ast("jl-expand-macroscope", expr, inmodule);
     JL_GC_POP();
     return expr;
@@ -1149,6 +1172,7 @@ JL_DLLEXPORT jl_value_t *jl_macroexpand1(jl_value_t *expr, jl_module_t *inmodule
     JL_GC_PUSH1(&expr);
     expr = jl_copy_ast(expr);
     expr = jl_expand_macros(expr, inmodule, NULL, 1, jl_atomic_load_acquire(&jl_world_counter), 0);
+    // we do not call jl_tag_macrocall_source here, since we assume the user of macrocall expects to handle it themselves
     expr = jl_call_scm_on_ast("jl-expand-macroscope", expr, inmodule);
     JL_GC_POP();
     return expr;

diff --git a/stdlib/Test/src/Test.jl b/stdlib/Test/src/Test.jl
@@ -27,7 +27,7 @@ export TestLogger, LogRecord
 using Random
 using Random: AbstractRNG, default_rng
 using InteractiveUtils: gen_call_with_extracted_types
-using Base: typesplit
+using Base: typesplit, remove_linenums!
 using Serialization: Serialization
 
 const DISPLAY_FAILED = (
@@ -452,19 +452,20 @@ macro test(ex, kws...)
 
     # Build the test expression
     test_expr!("@test", ex, kws...)
-    orig_ex = Expr(:inert, ex)
 
     result = get_test_result(ex, __source__)
 
-    return quote
+    ex = Expr(:inert, ex)
+    result = quote
         if $(length(skip) > 0 && esc(skip[1]))
-            record(get_testset(), Broken(:skipped, $orig_ex))
+            record(get_testset(), Broken(:skipped, $ex))
         else
             let _do = $(length(broken) > 0 && esc(broken[1])) ? do_broken_test : do_test
-                _do($result, $orig_ex)
+                _do($result, $ex)
             end
         end
     end
+    return result
 end
 
 """
@@ -492,10 +493,10 @@ Test Broken
 """
 macro test_broken(ex, kws...)
     test_expr!("@test_broken", ex, kws...)
-    orig_ex = Expr(:inert, ex)
     result = get_test_result(ex, __source__)
     # code to call do_test with execution result and original expr
-    :(do_broken_test($result, $orig_ex))
+    ex = Expr(:inert, ex)
+    return :(do_broken_test($result, $ex))
 end
 
 """
@@ -522,9 +523,9 @@ Test Broken
 """
 macro test_skip(ex, kws...)
     test_expr!("@test_skip", ex, kws...)
-    orig_ex = Expr(:inert, ex)
-    testres = :(Broken(:skipped, $orig_ex))
-    :(record(get_testset(), $testres))
+    ex = Expr(:inert, ex)
+    testres = :(Broken(:skipped, $ex))
+    return :(record(get_testset(), $testres))
 end
 
 # An internal function, called by the code generated by the @test
@@ -612,7 +613,8 @@ function get_test_result(ex, source)
             $negate,
         ))
     else
-        testret = :(Returned($(esc(orig_ex)), nothing, $(QuoteNode(source))))
+        ex = Expr(:block, source, esc(orig_ex))
+        testret = :(Returned($ex, nothing, $(QuoteNode(source))))
     end
     result = quote
         try
@@ -622,7 +624,6 @@ function get_test_result(ex, source)
             Threw(_e, Base.current_exceptions(), $(QuoteNode(source)))
         end
     end
-    Base.remove_linenums!(result)
     result
 end
 
@@ -703,18 +704,18 @@ In the final example, instead of matching a single string it could alternatively
 """
 macro test_throws(extype, ex)
     orig_ex = Expr(:inert, ex)
+    ex = Expr(:block, __source__, esc(ex))
     result = quote
         try
-            Returned($(esc(ex)), nothing, $(QuoteNode(__source__)))
+            Returned($ex, nothing, $(QuoteNode(__source__)))
         catch _e
             if $(esc(extype)) != InterruptException && _e isa InterruptException
                 rethrow()
             end
             Threw(_e, nothing, $(QuoteNode(__source__)))
         end
     end
-    Base.remove_linenums!(result)
-    :(do_test_throws($result, $orig_ex, $(esc(extype))))
+    return :(do_test_throws($result, $orig_ex, $(esc(extype))))
 end
 
 const MACROEXPAND_LIKE = Symbol.(("@macroexpand", "@macroexpand1", "macroexpand"))
@@ -1624,10 +1625,9 @@ function _inferred(ex, mod, allow = :(Union{}))
         ex = Expr(:call, GlobalRef(Test, :_materialize_broadcasted),
             farg, ex.args[2:end]...)
     end
-    Base.remove_linenums!(let ex = ex;
+    result = let ex = ex
         quote
-            let
-                allow = $(esc(allow))
+            let allow = $(esc(allow))
                 allow isa Type || throw(ArgumentError("@inferred requires a type as second argument"))
                 $(if any(a->(Meta.isexpr(a, :kw) || Meta.isexpr(a, :parameters)), ex.args)
                     # Has keywords
@@ -1651,7 +1651,8 @@ function _inferred(ex, mod, allow = :(Union{}))
                 result
             end
         end
-    end)
+    end
+    return remove_linenums!(result)
 end
 
 function is_in_mods(m::Module, recursive::Bool, mods)

diff --git a/test/compiler/inference.jl b/test/compiler/inference.jl
@@ -3431,7 +3431,7 @@ end
             end
         end
     end
-    @test occursin("thunk from $(@__MODULE__) starting at $(@__FILE__):$((@__LINE__) - 5)", string(timingmod.children))
+    @test occursin("thunk from $(@__MODULE__) starting at $(@__FILE__):$((@__LINE__) - 6)", string(timingmod.children))
     # END LINE NUMBER SENSITIVITY
 
     # Recursive function

diff --git a/test/deprecation_exec.jl b/test/deprecation_exec.jl
@@ -8,8 +8,6 @@
 using Test
 using Logging
 
-using Base: remove_linenums!
-
 module DeprecationTests # to test @deprecate
     f() = true
 

diff --git a/test/goto.jl b/test/goto.jl
@@ -87,7 +87,7 @@ end
 @test goto_test5_3()
 
 
-@test Expr(:error, "goto from a try/finally block is not permitted") ==
+@test Expr(:error, "goto from a try/finally block is not permitted around $(@__FILE__):$(3 + @__LINE__)") ==
     Meta.lower(@__MODULE__, quote
         function goto_test6()
             try

diff --git a/test/syntax.jl b/test/syntax.jl
@@ -3,6 +3,7 @@
 # tests for parser and syntax lowering
 
 using Random
+using Base: remove_linenums!
 
 import Base.Meta.ParseError
 
@@ -38,13 +39,8 @@ end
 
 # issue #9704
 let a = :a
-    @test :(try
-            catch $a
-            end) == :(try
-                      catch a
-                      end)
-    @test :(module $a end) == :(module a
-                                end)
+    @test :(try catch $a end) == :(try catch a end)
+    @test :(module $a end) == :(module a end)
 end
 
 # string literals
@@ -710,7 +706,7 @@ m1_exprs = get_expr_list(Meta.lower(@__MODULE__, quote @m1 end))
 let low3 = Meta.lower(@__MODULE__, quote @m3 end)
     m3_exprs = get_expr_list(low3)
     ci = low3.args[1]::Core.CodeInfo
-    @test ci.codelocs == [3, 1]
+    @test ci.codelocs == [5, 2]
     @test is_return_ssavalue(m3_exprs[end])
 end
 
@@ -2599,10 +2595,10 @@ import .TestImportAs.Mod2 as M2
 end
 
 @testset "issue #37393" begin
-    @test :(for outer i = 1:3; end) == Expr(:for, Expr(:(=), Expr(:outer, :i), :(1:3)), :(;;))
+    @test remove_linenums!(:(for outer i = 1:3; end)) == Expr(:for, Expr(:(=), Expr(:outer, :i), :(1:3)), :(;;))
     i = :i
-    @test :(for outer $i = 1:3; end) == Expr(:for, Expr(:(=), Expr(:outer, :i), :(1:3)), :(;;))
-    @test :(for outer = 1:3; end) == Expr(:for, Expr(:(=), :outer, :(1:3)), :(;;))
+    @test remove_linenums!(:(for outer $i = 1:3; end)) == Expr(:for, Expr(:(=), Expr(:outer, :i), :(1:3)), :(;;))
+    @test remove_linenums!(:(for outer = 1:3; end)) == Expr(:for, Expr(:(=), :outer, :(1:3)), :(;;))
     # TIL that this is possible
     for outer $ i = 1:3
         @test 1 $ 2 in 1:3