tremor-rs / tremor-runtime

Main Tremor Project Rust Codebase
https://www.tremor.rs
Apache License 2.0
842 stars 127 forks source link

Ensure the file and module path where errors in imported files are present are provided in hygienic errors #1629

Open darach opened 2 years ago

darach commented 2 years ago

In v0.12.0-rc2 modular tremor applications will begin to get errors from modules and we currently do not emit the filename and module relative path in hygienic errors.

$ TREMOR_PATH=$TREMOR_PATH:../lib:. tremor server run discord.troy 
tremor version: 0.12.0-rc.2
tremor instance: tremor
rd_kafka version: 0x000002ff, 1.8.2
allocator: snmalloc
Error: 
    1 | use config as c;
    2 | 
    3 | select {
      | ^^^^^^ Found the token `select` but expected one of `<doc-comment>`, `const`, `define`, `fn`, `intrinsic`, `use`
    4 |   "message": {
    5 |     "channel_id": c::welcome_channel,

Error: An error occurred while loading the file `discord.troy`: failed to load troy file: discord.troy
[2022-05-09T12:00:02Z ERROR tremor::server] Error: An error occurred while loading the file `discord.troy`: failed to load troy file: discord.troy
We are SHUTTING DOWN due to errors during initialization!
[2022-05-09T12:00:02Z ERROR tremor::server] We are SHUTTING DOWN due to errors during initialization!

In the example above we are implicitly directed to discord.troy but this is not the file from whence the error originates. Tremor should always indicate the file from which an error originates in a modular application.

Describe the problem you are trying to solve

Describe the solution you'd like

Indicating the location on the module path as a fully qualified file path would allow opening the file in a modern IDE via a quick link for editing and fixing.

Notes

mfelsche commented 1 year ago

This would be solved by giving an ArenaEntry (this is where we keep the loaded sources, so we can reference them in our AST and interpreter (e.g. string literals)) an optional field file_name that can be loaded upon error reporting.

Licenser commented 1 year ago

Not quit, since the entries can represent multiple files from different load paths, but it would at least point to a file then, however w/ the archives, that won't work since it'll be be dependant on the host it's packaged on.

that said in the raft branch we alreadyu store the path info for modules, if that helps? Might be a decent halfway house?

mfelsche commented 1 year ago

We should fix this before we introduce clustering imho.

Licenser commented 1 year ago

We should fix that in a way that works with clustering. Otherwise, the work will become pointless with the release. But please look at the changes to module in the raft branch it makes little sense to do the work from scratch twice in conflicting ways.