Apply ErrorStdOut to run-time errors

[geek]

[Moderator's note: Topic moved from Bug Reports.]

In v1.1 builds of AHK, the /ErrorStdOut command line flag (and presumably #ErrorStdOut) have caused both load-time and run-time errors to be emitted to the stderr stream. In v2.0, the ErrorStdOut behavior only catches load-time errors, with run-time errors being displayed on-screen. This occurs both with the traditional error dialog from beta 1-7, and the new dialog introduced in beta 8.

This causes issues with the remote AHK execution sandbox API that I provide for the AHK Discord. Specifically, when a run-time error occurs I no longer receive the error in a usable form back to the caller. Instead, the script hangs with a dialog that nobody can see, until the timeout kicks in and the process is forcibly killed. This has caused a lot of confusion with the Discord, when scripts just time out instead of giving the call trace.

I could probably work around this with the new /include flag, injecting code to register a custom OnError handler, but I'd prefer not to because I think the original behavior was useful more generally, and because it could mildly interfere with the testing/demonstration of users' own OnError routines.

Was this change intentional? If it was, could an additional flag be added to also emit run-time errors to stderr?

Thanks!

[lexikos]

In v1.1 builds of AHK, the /ErrorStdOut command line flag (and presumably #ErrorStdOut) have caused both load-time and run-time errors to be emitted to the stderr stream.

No it doesn't, nor does the documentation indicate that it should.

Prior to v2.0-a110 (a577668), the DBGp command stderr -c 2 caused error dialogs to be suppressed. This was implemented by the merge with AutoHotkeyU (AutoHotkey_L revision 42), but never documented. The stderr command is supposed to redirect the stderr stream, but AutoHotkey repurposes it for OutputDebug. Overloading it to redirect error dialogs is even more of a departure from the spec, although the spec is primarily for PHP, where I think the default error behaviour is to write to a log (and if that fails, write to stderr).

It would be better for the debugger to have more explicit control over individual aspects of error handling, but this will require custom extensions to the protocol. As far as I can tell, the spec basically only covers stderr redirection and exception breakpoints (which just pause execution when a breakpoint is thrown, and don't even provide a formal way to retrieve the thrown exception).

I could probably work around this with the new /include flag, injecting code to register a custom OnError handler

This is very close to the intended purpose of /include and OnError. The chance of "interference" might be reduced by providing a way to specify the priority of a handler, so that any handler you register could remain at the end of the list, even if the script doesn't specify -1 as the second parameter. However, it is already possible to override the behaviour by hooking the OnError function (OnError.DefineProp("Call", ...)).

could an additional flag be added to also emit run-time errors to stderr?

Of course, but what would it look like?

[geek]

This is rather embarrassing I seem to have mistaken the mechanism of some of the functionality of my own service.

Since the initial builds of the remote sandbox, I've included run-time import tampering that modifies the imports table to override the MessageBoxW call and make it run custom machine code that dumps the text contents to StdOut. That's the magic change that has been allowing run-time errors to be printed rather than being shown on-screen, not the /ErrorStdOut flag as I had been under the impression of.

When I added AHKv2 support to my framework, I replaced the machine code blobs with a simple function definition that shadows the MsgBox() builtin. The builtin, for obvious reasons, does not get called by the interpreter to display errors, so the previous error redirection behavior was lost. That said, with the new error box introduced in beta 8 I would expect that even if I ported my machine code hooks for v2 I would still not see the behavior I was looking for.

For now I'll use /include to set up an explicit OnError() hook. I had enjoyed the convenience of getting the fully formatted error dialog information with the context of surrounding lines for free, but a simpler trace with the key info should be sufficient for my purposes. I don't think adding a higher mechanism to lodge an OnError handler permanently at the end of the list is strictly necessary, as I may be one of two or three people to ever use it.

Long-term, I am definitely still interested in an explicit flag to print run-time errors to stderr instead of to screen. I think such a flag would be helpful for people who build on ExecScript() style behavior, like I'm doing with my service. As for what that could look like, one familiar interface could be to copy the behavior of Run's verbs, allowing flags like /ErrorStdOut=*Runtime or /ErrorStdOut="*Runtime UTF-8". If an entirely new flag were added, I would keep it simple and make it /RuntimeErrorStdOut or /ErrorStdOutRuntime (so that it appears next to ErrorStdOut/#ErrorStdOut in a alphabetic list).

Thank you for working through this with me

[lexikos]

I don't understand. If you're using machine code blobs within the script, you already have control of the script's contents and are either already using /include or have no need of it.

I see no need for the asterisk, if we go that way. "Runtime" is not a valid encoding name. FileAppend and FileRead already combine options and encoding names in one parameter, with just a space delimiting them.

[geek]

I don't understand. If you're using machine code blobs within the script, you already have control of the script's contents and are either already using /include or have no need of it.

I am using /include and it does work, for the most part.

With my MessageBoxW hooks in v1, I can get error traces like this to return to the user:

Code: Select all

Error:  Unhandled exception.

	Line#
	114: {
	116: SetKeyDelay,-1
	118: stdout := FileOpen("*", "w")
	119: lib := CloudAHK_keybd_event_MCL()
	121: NumPut(stdOut.__Handle, lib.hStdOut, "UPtr")  
	122: CloudAHK_OverwriteImport("USER32.DLL", "keybd_event", lib.keybd_event)  
	123: }
--->	001: Throw,new Exception()
	002: Exit
	003: {
	005: For k,v in p
	006: out .= "	" (IsObject(v) ? Print_Dump(v) : v)  
	007: stdout.Write(SubStr(out, 2) "
")  
	008: stdout.__Handle  
	009: }

The current thread will exit.

With the OnError hook in v2, (using the catch from the first first example on https://lexikos.github.io/v2/docs/objects/Error.htm) the most detail I seem to have access to is this:

Code: Select all

Error: Error.

File:    *
Line:    3
What:    
Stack:
> Auto-execute

It's not clear to me how I can make OnError return anything that looks like the v1 output / v2's own error dialog. The file is * for stdin, so the OnError routine can't just look to the original file to pull the information. I could probably build it to emit some kind of machine-parseable format that the invoker can detect and pull context lines from the string it originally passed to AHK, but that seems overly complicated and prone to breaking for something that the interpreter is already capable of producing.

The context lines are important for this service as the code boxes that get executed do not themselves have line numbers rendered, a limitation of the Discord platform I can't change. Having the context lines printed in the error message make it much easier for the user to identify what caused the error.

I see no need for the asterisk, if we go that way. "Runtime" is not a valid encoding name. FileAppend and FileRead already combine options and encoding names in one parameter, with just a space delimiting them.

This sounds fine, I trust your judgement

[lexikos]

Stdout error messages do not include the context lines.

A more flexible solution would be to give the script access to the missing information, such as by providing a function that retrieves a range of lines. This could be used with the paths and line numbers retrieved from the Error's Stack property, not just the top level File and Line properties. I think this needs further consideration to ensure that any related functionality will be consistent.

Object-oriented interfaces to debugging and introspection features could potentially be used by both the script itself and external processes, via COM.

[lexikos]

For the short term, you could try the linecontext branch that I just uploaded. It adds a function with usage as follows:

Code: Select all

MsgBox _ScriptGetLines(A_LineFile, A_LineNumber, -2) ; Two lines above and two below
MsgBox _ScriptGetLines(A_LineFile, A_LineNumber, 2) ; Two lines starting at A_LineNumber

It counts and outputs "executable lines", not directly corresponding to textual lines.

I have moved and renamed the topic.

[geek]

With some help from Cloaker to get it compiled, and some added #Warn statements to make warnings not halt the script with a pop-up, I was able to get _ScriptGetLines to get my /include shims to register an error handler that prints good outputs. I can do whatever ASCII art shenanigans I want with the added flexibility.

For the input MsgBox Uninitialized I was able to get a response

Code: Select all

* (2) : ==> Warning: This variable appears to never be assigned a value.
     Specifically: global Uninitialized
UnsetError: This variable has not been assigned a value.

File: *
Line: 2
What: 

042: Return -1
043: }
>>> 002: MsgBox(Uninitialized)
003: Exit

Thread will continue

One thing that Cloaker had hoped for with this functionality would be that instead of returning a newline-terminated string, the function would return an array of line objects with fields like "number", "text", and/or "file". As it is, (and I admit this is a weakness of the v1 generated output too) the returned lines have already processed special characters like newlines so that MsgBox "a`nb`nc`nd" show as

Code: Select all

001: MsgBox("a
b
c
d")
002: ...

I decided that this could be addressed for my purposes with some regular expressions (below) but Cloaker is right that having an array return value would be easier to work with.

Code: Select all

    RegExReplace(
      RegExReplace(
        RegExReplace(
          RTrim(_ScriptGetLines(err.File, err.Line, -2), "`n"),
          "\n(?!\d{3}:)",
          "``n"
        ),
        "`am)^.{30}\K.+",
        "..."
      ),
      "^([^\n]+\n){2}\K",
      ">>> "
    )

I'm also not sure that the positive number parameter is something that will be used much. When I first glanced at your example my expectation was that negative numbers would give that many lines extra before, positive that many lines extra. I personally don't think I'll ever not want the "lines before".

So, do you think you could make it that _ScriptGetLines (or whatever this function ends up being named) takes just positive numbers which act like the previous negative numbers, and returns something like this?

Code: Select all

[
	{file: "/included file's name/path", number: 42, text: "whatever"},
	{file: "/included file's name/path", number: 43, text: "whatever"},
	{file: "*", number: 1, text: "whatever"},
	{file: "*", number: 1, text: "whatever"},
	{file: "*", number: 1, text: "whatever"}
]

[lexikos]

I have pushed some commits which implement your suggestions, if I've interpreted them correctly.

As it is, (and I admit this is a weakness of the v1 generated output too) the returned lines have already processed special characters like newlines so that MsgBox "a`nb`nc`nd" show as [multiple lines]

The original escape sequences are not kept in memory. Even the line text isn't kept in memory; it is reconstructed, although this uses the post-processed text of each arg, which is kept.

I think there is a reason to change it to resolve escape sequences later, but I don't recall what it was. They could be left as is in the arg text and be resolved only within specific contexts (like quoted strings and hotstrings). The line text still wouldn't match the original if it used a continuation mechanism.

[geek]

No need to worry about the escape sequences, I didn't mean to suggest that I'm looking for the original line content or anything like that. I'm just massaging it so that one "line" as the interpreter understands it results in one row of output in my exception trace, as Discord (the primary platform for accessing the API) isn't a great place to render a bunch of lines of output unexpectedly when I can make a meaningful output in fewer. With the object changes, it should be much easier to ensure it behaves with a little bit of my own code (plus I can set up line length truncation, which further helps with the Discord experience). I see no need to handle that at the interpreter level.

I can't take a look at the new commits just yet, but when I do I'll be sure to tell you how it worked out for me!

Thank you for being so responsive with this!

[geek]

I got 2.0-beta.13+3+a7efc866 running in the sandbox, and updated my OnError handler to work with the object response. I'm getting beautiful outputs now

Using /include code like (a little messy because I wrote this on my phone):

Code: Select all

#Warn All, StdOut
OnError CloudAHK_Error, -1

MsgBox(Text) {
  FileAppend(Text "`n", "*")
}

CloudAHK_Error(err, mode) {
  out := ""
  for i, line in _ScriptGetLines(err.File, err.Line, 2)
  {
    if (line.File != err.File)
      continue

    out .= Format("{}{:03}: {}`n",
      line.Number == err.Line ? "> " : "  ",
      line.Number,
      StrReplace(
        StrReplace(
          RegExReplace(line.Text, "s)^.{30}\K.+", "..."),
          "`n"
          "``n"
        ),
        "`r",
        "``r"
      )
    )
  }

  MsgBox Format("
    (LTrim
    {}: {}

    {}Line: {}
    What: {}

    {}
    {}
    )",
    type(err),
    err.Message,
    err.File == "*" ? "" : "File: " err.File,
    err.Line,
    err.What,
    out,
    (mode == "ExitApp" ? "Script" : "Thread") " will " (mode == "Return" ? "continue" : "exit")
  )

  return -1
}

I was able to turn this input:

Code: Select all

; test
MsgBox "a"
MsgBox b
MsgBox "c"
return
MsgBox "d"

into this output:

Code: Select all

* (3) : ==> Warning: This variable appears to never be assigned a value.
     Specifically: global b
* (6) : ==> Warning: This line will never execute, due to Return preceding it.
a
UnsetError: This variable has not been assigned a value.

Line: 3
What: 

  002: MsgBox("a")
> 003: MsgBox(b)
  004: MsgBox("c")
  005: Return

Thread will continue
c

It is admittedly a little jumbled with the mixed stderr/stdout but that's my fault for making the sandbox merge the streams to start with. Everything you've done is working great

[geek]

Do you have plans to eventually merge this functionality into the mainline v2, or should I look into managing this addition as a fork?

[lexikos]

I want to avoid adding new functions or syntax that I'll end up wanting to change down the track, but I also want to avoid having multiple potential-feature branches that need to be maintained in parallel or merged repeatedly. Once the alpha branch is restarted, some experimental features might be present in the binary but disabled by default (enabled with a directive or similar), and some might be disabled via preprocessor directives. This would likely be the latter. I also have ideas for enabling external modules, but that won't happen soon and isn't suitable for this type of function.

I imagine that the _ScriptGetLines function may become part of a larger module or object-based API, which would mean a change of syntax.

It seems like something that might also be useful for a library to parse scripts and extract information. That is another item on my list, that when done properly, will be dependent on a lot of other work being done. For a bunch of reasons, I started experimenting with hacking up a short term solution yesterday and worked out that it's easier to create a library that can both parse and execute scripts than to strip out the execution part.

The long-term (post v2.1) plan is to create an API usable by the script itself, for introspection and loading other scripts as modules for execution or inspection. An object-based API is easily exposed via COM, making it accessible to other AutoHotkey versions or external applications. The program hosting a script would be able to control the environment in which the script executes, beyond just adding code to the file. HotKeyIt's AutoHotkey.dll can do this to an extent (maybe just by adding code?), but the interface and capabilities aren't ideal, each script has its own OS thread (which is both a benefit and a curse), and it doesn't seem to be actively maintained at the moment.

For the short-ish term, I intend to implement the basic parts of toralf's request (for both v1 and v2). I will probably make the library a separate build configuration within the alpha branch, after v2.0 is released. It will not be "officially supported", but building up to date binaries at least won't require merging branches. _ScriptGetLines may be enabled by default in the dll build, disabled in the other builds.