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

Define and use an abstract ErasedReportedError type instead of () #130

Closed
9 of 11 tasks
fmease opened this issue Jan 30, 2022 · 2 comments · Fixed by #170
Closed
9 of 11 tasks

Define and use an abstract ErasedReportedError type instead of () #130

fmease opened this issue Jan 30, 2022 · 2 comments · Fixed by #170

Comments

@fmease
Copy link
Owner

fmease commented Jan 30, 2022

Heavily inspired by rustc.

  • The goal is to prevent the case where a pass fails (currently with Err(())) but no error is reported (via Diagnostic::error().report) which leads to a failed assertion and a reported internal compiler error in main.
  • Defined in crate::diagnostic for now as ErrorReported
    • Ideally, no “plain” constructor but returned by Reporter::report/Diagnostic::report.
      However, this won't work with crate::resolver::scope::RegistrationError::DuplicateDefinition which doesn't immediately result in a reported diagnostic by design. Which means we probably need to expose some constructor named sth. like ErrorReported::promise_error_will_be_reported() ErrorReported::error_will_be_reported_unchecked unless we find a way too make this type-safe
  • Change crate::error's Result definition to default E to ErrorReported instead of ()
  • Remove the #![allow(clippy::result_unit_err)] in the crate roots as it will no longer be necessary
  • Require Error variants of possibly erroneous (→ PossiblyErroneous) data to hold/be contstructed with an ErrorReported changing PossiblyErroneous::error to take an _: ErrorReported as an argument. ErrorReported has to impl Clone because AST nodes are Clone but that is not an issue. Discarding is also not a problem right now (since a positive error count leads to a failing exit status even without an ErrorReported (maybe we want to change it making it more strict))
  • Store ErrorReported in Health::Tainted to be able to make Health and Outcome combinators correct (i.e. remove their calls to ErrorReported::error_will_be_reported_unchecked)
  • Add documentation to ErrorReported (in [WIP] Fix exposure related bugs #137)
  • Bugs in the current implementation
    • Diagnostic::{warning,debug}().report(reporter) also yields an ErrorReported even though only a warning or debug message, respectively, was issued. ErrorReported should only be obtainable by ::{error, bug}()
    • SilentReporter yields a ErrorReported. Instead, it should return () or similar. We can achieve this (I think) by making the Reporter enum a trait and adding an assoc type Output
fmease added a commit that referenced this issue Feb 14, 2022
Works towards making it harder to accidentally abort compilation due to an error
*without emitting a diagnostic* (#130).
Instead of blindly creating `()`, `ErrorReported::error_will_be_reported_unchecked()`
needs to be used in non-trivial cases forcing the programmer to ponder if it's
correct what they are doing.
@fmease
Copy link
Owner Author

fmease commented Feb 14, 2022

Halfway implemented via 6cb2bfe

@fmease fmease changed the title Define and use an abstract ErrorReported type instead of () Define and use an abstract ErasedReportedError type instead of () Apr 10, 2022
@fmease
Copy link
Owner Author

fmease commented Apr 10, 2022

Renamed from ErrorReported to ErasedReportedError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant