janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.43k stars 221 forks source link

janet_dobytes doesn't report line, col info for compile errors #1285

Closed sogaiu closed 11 months ago

sogaiu commented 11 months ago

In the context of embedding janet in TIC-80, there was a report that location information was not available in compilation error messages.

Investigation suggested that this was due to the use of janet_dobytes. Some of the relevant lines are:

                ret = janet_wrap_string(cres.error);
                if (cres.macrofiber) {
                    janet_eprintf("compile error in %s: ", sourcePath);
                    janet_stacktrace_ext(cres.macrofiber, ret, "");
                } else {
                    janet_eprintf("compile error in %s: %s\n", sourcePath,
                                  (const char *)cres.error);
                }

Note that no location information is present before the strings beginning with "compile error".

I tried patching this to look like:

                ret = janet_wrap_string(cres.error);
                int32_t line = -1, column = -1;
                // try to get error loc info from compiler, then parser if needed
                if ((cres.error_mapping.line > 0) &&
                    (cres.error_mapping.column > 0)) {
                    line = cres.error_mapping.line;
                    column = cres.error_mapping.column;
                } else if ((parser.line > 0) && (parser.column > 0)) {
                    line = parser.line;
                    column = parser.column;
                } else {
                    janet_eprintf("Sorry, no error location info found.");
                }
                if (cres.macrofiber) {
                    janet_eprintf("%d:%d: compile error in %s: ",
                                  line, column, sourcePath);
                    janet_stacktrace_ext(cres.macrofiber, ret, "");
                } else {
                    janet_eprintf("%d:%d: compile error in %s: %s\n",
                                  line, column, sourcePath,
                                  (const char *)cres.error);
                }

Basically, the changes are to check whether there is sensible-looking info available via the compilation result (cres) and if not, use location information from parsing. I don't know if the latter can fail (may be it can't?), but if it does, emit some message explaining that no location info was located.

It has worked in limited testing.

A question that came up is whether something like this [1] would be useful or sensible in janet's janet_dobytes.

Any interest?


[1] Further down in janet_dobytes, location info (including sourcePath) is provided for parse errors, so if it makes sense to modify janet_dobytes, perhaps that would be worth adding too.

sogaiu commented 11 months ago

Re:

[1] Further down in janet_dobytes, location info (including sourcePath) is provided for parse errors, so if it makes sense to modify janet_dobytes, perhaps that would be worth adding too.

That's not quite right.

Better to have said "make the messages for compilation errors have a format that is more in line with others", i.e. like:

<sourcePath>:<line>:<column>: <message>

The sourcePath info is in the current message, it's just currently not at the beginning:

compile error in <sourcePath>: