HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.03k stars 648 forks source link

[eval] Don't choke on env_parent = None #11679

Open kLabz opened 1 month ago

kLabz commented 1 month ago

Still not sure exactly how things go wrong here. Broke with https://github.com/HaxeFoundation/haxe/commit/6e4fd96cb56b990cdf2b493733c326220489d500

Simn commented 4 weeks ago

As I've said on Slack this isn't the right fix, and the problem suggests that there's something wrong with the exception management.

kLabz commented 5 days ago

So this comes from exceptions in build macros triggered by another build macro. Not sure if this env kind business is correct; do you have a better idea there?

Hopefully the included repro helps (along with the change in evalMain showing where we get an EKEntryPoint triggering the issue)

Note: should check eval debugger behavior here; will try to setup that...

Simn commented 5 days ago

I think this is a decent change, but I worry that it fixes the issue only incidentally... Either way I'm fine with going ahead with this.

kLabz commented 4 days ago

Testing this against eval debugger, with:

Note that for these, Haxe compiler doesn't show the full stack:

[ERROR] src/Bar.hx:1: characters 1-2

 1 | @:build(Macro.build())
   | ^
   | Unexpected &

Eval debugger shows the whole stack, like current test (build macro + build macro + throw): image

I updated kind_name (locally) to something I think is a bit better: image

And also works for expression macro => build macro: image


Haxe compiler not showing the stack for Context.parseInlineString example now seems weird...

kLabz commented 4 days ago

With this, I can get the stack displayed with compiler too; but not sure we actually want that

diff --git a/src/macro/eval/evalExceptions.ml b/src/macro/eval/evalExceptions.ml
index b3954e206..7140610ca 100644
--- a/src/macro/eval/evalExceptions.ml
+++ b/src/macro/eval/evalExceptions.ml
@@ -139,7 +139,15 @@ let catch_exceptions ctx ?(final=(fun() -> ())) f p =
                                                        end else
                                                                Error.raise_typing_error (Printf.sprintf "Unexpected value where haxe.macro.Error was expected: %s" (s_value 0 v).sstring) null_pos
                                                ) (EvalArray.to_list sub)
-                               | _ -> []
+                               | _ ->
+                                       (* Careful: We have to get the message before resetting the context because toString() might access it. *)
+                                       let stack = match eval_stack with
+                                               | [] -> []
+                                               | l when p' = null_pos -> l (* If the exception position is null_pos, we're "probably" in a built-in function. *)
+                                               | _ :: l -> l (* Otherwise, ignore topmost frame position. *)
+                                       in
+                                       let stack = get_exc_error_stack ctx stack in
+                                       List.map (fun p -> (Error.Custom "Called from here", p)) (List.rev stack)
                        in
                        reset_ctx();
                        final();

Edit 2: will see if I can handle this earlier (where we create the haxe.macro.Error) doesn't seem to make much sense in make_runtime_error :thinking:

Edit: surprisingly, this doesn't have a big impact on our misc tests:

Done running 661 tests with 1 failures
SUMMARY:
projects/Issue9389/compile-fail.hxml
 Main.hx:3: characters 5-8 : boop
+Main.hx:3: characters 3-9 : Called from here

Which actually sounds like a nice change to me, adding the position from where that macro was called:

───────┬──────────────────────────────────────────────────────────────────────────
       │ File: projects/Issue9389/Main.hx
───────┼──────────────────────────────────────────────────────────────────────────
   1   │ class Main {
   2   │     static function main() {
   3   │         f(123);
   4   │     }
   5   │ 
   6   │     static macro function f(e) {
   7   │         throw new haxe.macro.Expr.Error("boop", e.pos);
   8   │     }
   9   │ }
───────┴──────────────────────────────────────────────────────────────────────────