Janet compilation errors don't show line numbers

[fuxoft]

When I tried to run my Janet code, I got the following error in TIC-80 console:

compile error in main: unexpected type in destruction, got 0

This was a compilation error in my code but there was no line number printed so it was not trivial for me to find the source of the error.

For runtime errors, the line and column numbers are printed correctly.

[sogaiu]

Would you mind sharing some minimally reproducing code?

[fuxoft]

Certainly. This is the file err.janet:

# author:  game developer, email, etc.
# desc:    short description
# site:    website link
# license: MIT License (change this to your license of choice)
# version: 0.1
# script: janet
# strict:  true

# Unlike other languages, the tic80 API
# is provided as a module.
(import tic80)
blah

Trying to run the above code on TIC80 produces the error compile error in main: unknown symbol blah but does not say on which line the error occured.

In standalone Janet, the same file produces the error err.janet:13:1: compile error: unknown symbol blah, so I know the error is on line 13, column 1.

[fuxoft]

By the way, what does strict: true mean in Janet files? Is there a documentation for this?

[sogaiu]

Thanks for the code. I was able to reproduce and did some investigation.

The short summary is that it may take some work to get line and column information for this situation.

A longer explanation follows for any parties interested in undertaking the work :)

I'm not the author of any of the Janet integration code, but IIUC, the lines here:

    if (janet_dostring(core->currentVM, code, "main", &result)) {
        reportError(core, result);

look to be part of the code path responsible for the output you reported.

janet_dostring is defined here and it calls janet_dobytes which is defined here.

Within janet_dobytes are these lines:

                    janet_eprintf("compile error in %s: %s\n", sourcePath,
                                  (const char *)cres.error);

which appear to be what constructs the error string beginning "compile error in". (I didn't see any obvious usable line and column information near these lines.)

I think in standalone janet, code execution is arranged for in a different way and the location information is available for use in constructing the message. My guess is that the relevant code for constructing the error message is here. That code appears to be passed line and column information which might be made available around here. Note that all of that code is in pure Janet and not C [1].

(As a remark in favor of the above guess, comparing the error messages closely, one can see that the TIC-80 integrated code starts with "compile error in", whereas the standalone janet error has the string "compile error:" following the location information.)

@AlecTroemel What do you think of the explanation above?

---

By the way, what does strict: true mean in Janet files? Is there a documentation for this?

I don't know. May be someone else does though.

---

[1] janet_dobytes basically wraps janet_compile using the janet_parser_* functions for parsing.

boot.janet's cli-main uses run-context which in turn uses compile, which wraps janet_compile_lint.

Both janet_compile_lint and janet_compile (itself a wrapper of the former) return a JanetCompileResult which has an error_mapping field (this is of type JanetSourceMapping and has error location IIUC). Perhaps error information IS accessible in janet_dobytes via janet_compile's return value and consequently, may be upstream would consider accepting (or making) changes so that this location information is used in the compilation error messages...

[Skeptim]

By the way, what does strict: true mean in Janet files? Is there a documentation for this?

There is no documentation for this in the wiki (the github search bar has a "Wikis" filter). I found #1653 about strict: true for fennel and the only mention of it in the code except demo carts is in fennel.c so I think it has been added by mistake to the janet demo cart. I suppose the janet demo cart was created by modifying the fennel one.

[Skeptim]

I added a link and TODO about strict tag on the wiki here and there, but I am not competent enough to write something correct about this sorry ^^

[AlecTroemel]

for the strict: true, Yes I believe thats an incorrect copy/paste from the fennel default cart that I made while making the janet default cart. I believe "strict" has to do with global variable ordering (referring to a global before its defined), seems to have been introduced in fennel 1.0 https://fennel-lang.org/changelog

[AlecTroemel]

Compilation error line numbers... brings back some bad memories 🤣. I definitely remember pounding my head against this.

Reading through your notes @sogaiu they seem right on the money.

If I remember correctly I believe the issue was that Janet only has line number (stack trace) information within the context of a Janet Fiber. Running cart code is executed within the main default fiber by invoking the "TIC" function within an existing environment, but the compilation step had no fiber associated with it for some reason? At least thats how I interpret this if statement @sogaiu referenced in their post

Following the usage of that bad-compile function in boot.janet I found this dofile function . Perhaps that's something we could use/copy? My thought is to include our own version of a paired-down boot.janet

[sogaiu]

@AlecTroemel Thanks for taking a look and sharing your thoughts.

This line (essentially what you linked to):

                if (cres.macrofiber) {

mentions macrofiber. Based on a brief search, my current impression is that that's only used and set in macroexpand1 here. I think it only gets set when there is a problem during macroexpansion.

So my current guess is that these lines:

                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);
                }

are just about distinguishing between errors that happened during macroexpansion and those that did not, respectively.

I don't know if it would work, but it seems worth trying to see if appropriate line and column number information is available via the cres (JanetCompileResult) that is obtained here, as that happens before the if / else mentioned above and cres is used there anyway.

May be we can try modifying janet_dobytes as an experiment to see if that works in the context of TIC-80. If it turns out to work and is generally useful, may be it would be a nice modification to Janet itself.

[sogaiu]

I did a quick experiment and unfortunately got -1s for both line and column info.

Perhaps that's replicating:

brings back some bad memories

---

This loop seems to be driven by a janet_parser_* function, each time through, a form is obtained. Perhaps it would be feasible to get the location of that form somehow...

Looks like the janet_parser_* functions (often?) take a JanetParser. That seems to have line and column info of some sort in it...

[sogaiu]

Ok, with the following diff, I get:

diff --git a/src/core/run.c b/src/core/run.c
index 25748811..966ee027 100644
--- a/src/core/run.c
+++ b/src/core/run.c
@@ -58,10 +58,16 @@ int janet_dobytes(JanetTable *env, const uint8_t *bytes, int32_t len, const char
             } else {
                 ret = janet_wrap_string(cres.error);
                 if (cres.macrofiber) {
-                    janet_eprintf("compile error in %s: ", sourcePath);
+                    janet_eprintf("%d:%d: compile error in %s: ",
+                                  parser.line,
+                                  parser.column,
+                                  sourcePath);
                     janet_stacktrace_ext(cres.macrofiber, ret, "");
                 } else {
-                    janet_eprintf("compile error in %s: %s\n", sourcePath,
+                    janet_eprintf("%d:%d: compile error in %s: %s\n",
+                                  parser.line,
+                                  parser.column,
+                                  sourcePath,
                                   (const char *)cres.error);
                 }
                 errflags |= 0x02;

I don't know how well that would work in general, but when I counted lines and columns in my editor with the cart code provided above, it seemed like it might be appropriate:

 1: # author:  game developer, email, etc.
 2: # desc:    short description
 3: # site:    website link
 4: # license: MIT License (change this to your license of choice)
 5: # version: 0.1
 6: # script: janet
 7: # strict:  true
 8:
 9: # Unlike other languages, the tic80 API
10: # is provided as a module.
11: (import tic80)
12: blah
------------------------------------------------------------------
    123456789

Perhaps line 12, column 4 refers to the end of the form being processed?

---

Another idea with the diff is to check for positive values within cres.error_mapping.(line|column) (like is done here) first, and fall back to the values from parser if either of the values is -1. No idea if the values in error_mapping can be positive in this context though.

[sogaiu]

@AlecTroemel Not related to this issue but while looking over code for this issue I noticed withsyms here.

When I search the repository, I only see withsyms in that one location. May be that should be with-syms?

Submitted #2287 -- if it's inappropriate / wrong, please mention :)

[sogaiu]

@AlecTroemel For the moment, may be it's better to try something along the lines of your idea:

Following the usage of that bad-compile function in boot.janet I found this dofile function . Perhaps that's something we could use/copy?

IIUC, dofile is basically a wrapper of run-context and I think that latter (sort-of) has a chunks argument which can be used to feed in the source code. Indeed, dofile takes the file-ish info and makes it available via a function:

  (defn chunks [buf _] (:read f 4096 buf))

which it then uses in a call to run-context.

But presumably we have the source in a string or buffer so perhaps we can make that available in a similar fashion.

My thought is to include our own version of a paired-down boot.janet

Since run-context is a built-in function in janet, may be we can get by without having to do much -- a brief scan of dofile suggested to me that it doesn't use any private pieces from boot.janet, so if there are parts from it we want to reuse, may be there isn't much (anything?) we have to copy from boot.janet?

---

Alternatively, I wonder if making a custom janet_dobytes and replacing this line with a call to the customized function might be easier.

[sogaiu]

I've made an attempt at a custom janet_dobytes function -- tic80_janet_dobytes_ext. It's just a combination of janet_dostring, janet_dobytes, and the diff from above that makes use of the location information from the parser.

Here is the relevant commit. It also changes evalJanet to use the new function in an attempt to get better reporting there too, though don't know if that is helpful.

@AlecTroemel If you have some time at some point, may be you could give this branch a try?

Not sure what cases are covered by this, but it seems to produce some meaningful info for the reproduction case.

---

This approach doesn't check whether the return value from janet_compile has usable location information. We could try to check whether the return value comes with associated line and column info that is positive first, and only if not use the parser's location information.

[sogaiu]

I pushed some changes to implement something like:

We could try to check whether the return value comes with associated line and column info that is positive first, and only if not use the parser's location information.

to the aforementioned branch.

Here is the commit with the most relevant changes.

I tried putting some intentional errors in cart code in a few different places and at least the reported line numbers seemed sensible.

[AlecTroemel]

@sogaiu just pulled down and built your branch. From playing around it does indeed add the line numbers (and column) numbers which is amazing! thank you for hacking on this.

Reading through your changes, they seem really reasonable to me. It makes me wonder in what situation you'd not want janet_dobytes to print line numbers. Maybe it doesnt make sense in a REPL context, where you're evaluation but there's no source lines? However as a counter example the python3 repl reports lines on syntax errors

>>> for a in [1,2,3]:
...   asldkfnal
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
NameError: name 'asldkfnal' is not defined

Part of me feels like you're original diff could be merged into janet upstream.

[AlecTroemel]

However in either case, @sogaiu you're branch seems like it solves this issue!

I'll keep testing that branch on a couple larger tic80 carts.

[sogaiu]

@AlecTroemel Thanks a lot for testing -- especially since you have relevant carts and are familiar with the code (^^:

Part of me feels like you're original diff could be merged into janet upstream.

Sounds oddly familiar :)

May be we can try modifying janet_dobytes as an experiment to see if that works in the context of TIC-80. If it turns out to work and is generally useful, may be it would be a nice modification to Janet itself.

Though I wasn't sure about potential edge cases, like you were hinting at:

Maybe it doesnt make sense in a REPL context, where you're evaluation but there's no source lines?

However for this particular case, I thought I'd seen some kind of arrangement to get location info along with something like "<repl>" for the name of source. Not so clear about it ATM.

At the built-in repl I get:

$ janet
Janet 1.30.0-51c0cf97 linux/x64/gcc - '(doc)' for help
repl:1:> blah
repl:1:1: compile error: unknown symbol blah

May be the repl:1:1: compile error: part comes from this part of bad-compile in boot.janet.

Perhaps we can bug bakpakin about potentially changing janet_dobytes and see what he thinks...it might be that boot.janet depends on current behavior so possibly it's not so straight-forward or desirable.

[sogaiu]

it might be that boot.janet depends on current behavior

I tried patching run.c with a modified janet_dobytes and haven't managed to turn up any problematic behavior yet (though testing has been limited).

IIUC, the janet executable runs cli-main from boot.janet like this:

    /* Run startup script */
    Janet mainfun;
    janet_resolve(env, janet_csymbol("cli-main"), &mainfun);
    Janet mainargs[1] = { janet_wrap_array(args) };
    JanetFiber *fiber = janet_fiber(janet_unwrap_function(mainfun), 64, 1, mainargs);
    fiber->env = env;

    /* Run the fiber in an event loop */
    status = janet_loop_fiber(fiber);

and AFAICT janet_loop_fiber and functions it calls don't use janet_dobytes.

So may be the kinds of changes we have in mind won't affect the main janet executable at least.

[sogaiu]

Ok, I asked in janet-lang/janet#1285 whether there is any interest in modifying janet_dobytes.

[sogaiu]

There's now a PR to janet.

[sogaiu]

The PR appears to have been merged.

@AlecTroemel I tried the changes with TIC-80 (less change now) and it seemed to work here. May be you could see how it works for you?

Specifically what I tried was:

• Pulled the latest changes in vendor/janet
• Rebuilt janet.c using make
• Built TIC-80 as usual from build using cmake .. followed by make -j 4

At the time of this writing, there isn't yet a release of Janet that includes the change to janet_dobytes.

[sogaiu]

It looks like there was a new release of Janet. This one appears to contain the changes to janet_dobytes.

@AlecTroemel Perhaps once you're happy that things are working it would make sense to update the version of Janet used by TIC-80?

[AlecTroemel]

@sogaiu I rebuilt tic80 with the version of the janet that included your changes over the weekend.. then totally forgot to respond here 🤦

Everything has been working just as intended for me! I didn't realize how many times I had been missing the compile error line number until I tried using an older build.

I agree upgrading Janet here is the thing to do.. reading through the release notes for both 1.30 and 1.31 (tic80 is on 1.29), it looks like there are a lot of other great changes as well!

[sogaiu]

@AlecTroemel Thanks for the status update!

Everything has been working just as intended for me!

Glad to hear it -- hopefully they will work well for @fuxoft and others as well once Janet is upgraded :)

Speaking of which, would you like to do a PR for that?

[AlecTroemel]

@sogaiu ya I totally can, I'll create it now so I dont forget

[sogaiu]

Thanks!

I'll try it out now.

[sogaiu]

Looks like Windows builds are not working again -- have commented in the PR.

[sogaiu]

@fuxoft nesbox merged @AlecTroemel's PR #2306.

I tried building it and it seems to work for me.

I think if one scrolls down here, there may be links where a build result is available.

May be you can get an appropriate version somehow (whether by building or via a build artifact / result) and let us know if it works for you.

[sogaiu]

@fuxoft FYI, I think the latest release contains the changes from #2306.

Perhaps this issue has been addressed?

[nesbox]

I think we can close this.