rusty icon indicating copy to clipboard operation
rusty copied to clipboard

Refactor codegen error reporting

Open volsa opened this issue 2 years ago • 4 comments

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?

volsa avatar Nov 24 '23 12:11 volsa

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.

ghaith avatar Dec 27 '23 05:12 ghaith

I'm in favor of removing / not using thiserror, the Diagnostic enum should be sufficient I think?

volsa avatar Dec 27 '23 08:12 volsa

I'm in favor of removing / not using thiserror, the Diagnostic enum should be sufficient I think?

Not with my refactor of the diagnostics =) , but I don't think thiserror will be needed in anycase

ghaith avatar Dec 27 '23 18:12 ghaith

Reopening this since I feel like we should wait for #1104 and futhermore ensure that

  1. every validation that can be done before entering codegen is actually done before codegen (e.g. #1105, #1027)
  2. 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)

mhasel avatar Feb 22 '24 12:02 mhasel