Refactor codegen error reporting
Is your refactor request related to a problem? Please describe.
Currently we have some lines making use of map_err in our codegen code. While this is not an issue per-se, it can remove the initial diagnostic with a somewhat less detailed diagnostic (and thus error message). For example take the following code
TYPE STRUCT1 : STRUCT
x : INT := 'wrong type';
END_STRUCT END_TYPE
Compiling it, will return "Some initial values were not generated", however initially a cannot_generate_string_literal diagnostic is returned, which is remapped to a cannot_generate_initializer diagnostic removing the previous diagnostic (and any other codegen error is later remapped to a cannot_generate_initializer error anyways because of the pipeline here).
Describe the solution you'd like
The given code should probably return the underlying root issue, namely a "Cannot generate String-Literal for type INT" error message (cannot_generate_string_literal). So either remapping diagnostics should maintain a reference
- to any previous diagnostic (sort of like a stack-trace) or
- only to the root issue.
Additional context
These remapping are only a problem because we map them to e.g. a SyntaxError which has no fields for references. One solution could be introducing an initial: Option<Diagnostic> field for all variants of the Diagnostics enum and on any remapping that field is "carried over".
cc @ghaith : IIRC you're already working on something similar to this issue?
I was playing around with codegen yesterday to try and move to the newer inkwell version and realized this would be a requirement. With the current inkwell version all builder methods are now results, and they fail if the builder is not setup correctly, which apparently was just being ignored in our usecase. As part of this issue I would recommend we add anyhow for the result types to add better error reporting and either use the thiserror crate or remove it since it's currently not being used.
I'm in favor of removing / not using thiserror, the Diagnostic enum should be sufficient I think?
I'm in favor of removing / not using
thiserror, theDiagnosticenum should be sufficient I think?
Not with my refactor of the diagnostics =) , but I don't think thiserror will be needed in anycase
Reopening this since I feel like we should wait for #1104 and futhermore ensure that
- every validation that can be done before entering codegen is actually done before codegen (e.g. #1105, #1027)
- errors that cannot be caught before the codegen are not reported on a one-per-compilation basis (this is probably not possible for inkwell errors)