Columns in error messages

Thank you very much for your work on Fennel. I was wondering if yall would take a patch adding support for printing the column number in the errors (assuming it's possible, of course).

The use case is for editor integration.

Assigned to
7 months ago
4 months ago

~technomancy 7 months ago

That seems reasonable, but it will be difficult to implement correctly, because the parser only stores byte offsets for any given symbol/list and does not have character-level data available. We could hypothetically use the utf8 table on Lua 5.3+, but the column data would have to be inaccurate on 5.1 and 5.2 unfortunately.

~technomancy 6 months ago

Thinking back on this, we already have this problem with the ^^^^^^ characters we use to pinpoint the source of a compiler error. So maybe it's OK to introduce this and make a best-effort where we can and fall back to bytes on 5.1/5.2.

~fsouza 6 months ago

Oh nice, I can give it a shot if you haven't already started. I imagine it'd be similar to the logic for ^^^^^^?

~technomancy 6 months ago

Sure, yeah if you want to work on this that would be fantastic.

I imagine it'd be similar to the logic for ^^^^^^?

Yes, but the logic for that is all nestled away in the fennel.friend module, whereas this would have to be used from the assert-msg function in the fennel.compiler module. So it would need to be either moved or exported.

But maybe first it would be better to make the fennel.friend logic attempt to be UTF8-aware so it doesn't assume that every character is a single byte. Once we have it working for fennel.friend we can reuse the same logic for general compile error assertion messages. Does that make sense?

~technomancy 4 months ago

I've got some code I'm working on that includes byte-based column offsets in the AST nodes.

I can extend it to make the error messages use it, but I figure I'd see if you had done anything on this first.

~fsouza 4 months ago

Oh, sorry I've been quiet because I'm on a long break. I haven't started on anything yet, please go ahead and thanks for checking! :D

~technomancy 4 months ago

I've got the patch for this up here if folks want to take a look https://lists.sr.ht/~technomancy/fennel/patches/33716

~technomancy REPORTED FIXED 4 months ago

This was merged!

Register here or Log in to comment, or comment via email.