Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor codegen error reporting #1031

Open
volsa opened this issue Nov 24, 2023 · 4 comments
Open

Refactor codegen error reporting #1031

volsa opened this issue Nov 24, 2023 · 4 comments
Labels
refactor internal change, cleanup, code-style-improvement

Comments

@volsa
Copy link
Member

volsa commented Nov 24, 2023

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 volsa added the refactor internal change, cleanup, code-style-improvement label Nov 24, 2023
@ghaith
Copy link
Collaborator

ghaith commented Dec 27, 2023

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.

@volsa
Copy link
Member Author

volsa commented Dec 27, 2023

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

@ghaith
Copy link
Collaborator

ghaith commented Dec 27, 2023

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

@volsa volsa closed this as completed Feb 9, 2024
@mhasel
Copy link
Member

mhasel commented Feb 22, 2024

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. Add validation for array size declaration with unresolved reference #1105, Add validation for invalid call parameter count #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 mhasel reopened this Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor internal change, cleanup, code-style-improvement
Projects
None yet
Development

No branches or pull requests

3 participants