Improve evalstr to show syntax warnings (#1059)

src/GAP.jl

@@ -66,7 +66,7 @@ const last_error = Ref{String}("")
 
 const disable_error_handler = Ref{Bool}(false)
 
-function error_handler()
+function copy_gap_error_to_julia()
     global disable_error_handler
     if disable_error_handler[]
         return
@@ -75,6 +75,12 @@ function error_handler()
     ccall((:SET_LEN_STRING, libgap), Cvoid, (GapObj, Cuint), Globals._JULIAINTERFACE_ERROR_BUFFER::GapObj, 0)
 end
 
+function get_and_clear_last_error()
+    err = last_error[]
+    last_error[] = ""
+    return err
+end
+
 function ThrowObserver(depth::Cint)
     global disable_error_handler
     if disable_error_handler[]
@@ -88,12 +94,10 @@ function ThrowObserver(depth::Cint)
     # at the top of GAP's exception handler chain, turn the GAP exception
     # into a Julia exception
     if depth <= 0
-        error("Error thrown by GAP: $(last_error[])")
+        error("Error thrown by GAP: $(get_and_clear_last_error())")
     end
 end
 
-const error_handlerwrap = error_handler
-
 # path to JuliaInterface.so
 const real_JuliaInterface_path = Ref{String}()
 JuliaInterface_path() = real_JuliaInterface_path[]
@@ -104,7 +108,7 @@ function initialize(argv::Vector{String})
     end
 
     handle_signals = isdefined(Main, :__GAP_ARGS__)  # a bit of a hack...
-    error_handler_func = handle_signals ? C_NULL : @cfunction(error_handlerwrap, Cvoid, ())
+    error_handler_func = handle_signals ? C_NULL : @cfunction(copy_gap_error_to_julia, Cvoid, ())
 
     # Tell GAP to read a file during startup (after its `lib/system.g`),
     # such that `JuliaInterface` is added to the autoloaded GAP packages,

src/ccalls.jl

@@ -138,15 +138,18 @@ whereas `x` in the latter example lives in the Julia session.
 """
 function evalstr(cmd::String)
     res = evalstr_ex(cmd * ";")
+
+    # If there is an error string on the GAP side, copy it into `last_error`.
+    # We do this even if there is no error indicated via `res`, to be able to
+    # handle syntax warnings
+    copy_gap_error_to_julia()
+
+    msg = get_and_clear_last_error()
     if any(x::GapObj->x[1] === false, res)
-      # error
-      global last_error
-      # HACK HACK HACK: if there is an error string on the GAP side, call
-      # error_handler to copy it into `last_error`
-      if !Wrappers.IsEmpty(Globals._JULIAINTERFACE_ERROR_BUFFER::GapObj)
-        error_handler()
-      end
-      error("Error thrown by GAP: $(last_error[])")
+      error("Error thrown by GAP: $msg")
+    elseif !isempty(msg)
+      # Syntax warnings may be printed here
+      print(msg)
     end
     res = res[end]::GapObj
     if Wrappers.ISB_LIST(res, 2)

test/Project.toml

@@ -1,13 +1,15 @@
 [deps]
 Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595"
 Documenter = "e30172f5-a6a5-5a46-863b-614d45cd2de4"
+IOCapture = "b5f81e59-6552-4d32-b1f0-c071b021bf89"
 Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
 Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
 
 [compat]
 Aqua = "0.8.2"
 Documenter = "^0.27.0"
+IOCapture = "0.2.5"
 Pkg = "1.6"
 Random = "1.6"
 Test = "1.6"

test/basics.jl

@@ -125,6 +125,21 @@ end
     l = GAP.evalstr("[1,~,3]")
     @test l[2] === l
     @test GAP.gap_to_julia(GAP.Globals.StringViewObj(l)) == "[ 1, ~, 3 ]"
+
+    # from issue #1058:
+    c = IOCapture.capture() do
+           GAP.evalstr("function() res:= 1; return res; end")
+       end
+    #
+    expected = """
+               Syntax warning: Unbound global variable in stream:1
+               function() res:= 1; return res; end;
+                          ^^^
+               Syntax warning: Unbound global variable in stream:1
+               function() res:= 1; return res; end;
+                                          ^^^
+               """
+    @test c.output == expected
 end
 
 @testset "randseed!" begin

test/runtests.jl

@@ -1,6 +1,7 @@
 using Test
 using Documenter
 using GAP
+using IOCapture
 
 include("Aqua.jl")