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

Refactoring the typechecking error handling #85

Merged
merged 25 commits into from
Dec 3, 2018

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Nov 28, 2018

I have a strong urge to refactor the error accumulation; this
recover_with and return_with isn’t really pretty, adds quite useless
noise to the code, and prevents easy use of existing combinators like
List.iter.

So I propose this:

  • We collect error messages and warnings in a mutable reference (a list
    or rope) that we just pass down as argument (inside env). This is
    used to collect errors (and warnings), but not for control flow.
    (This simulates the writer monad, without the syntactic overhead).
  • The exception no longer carries an argument. This is raised when we
    cannot continue due to an error.
    (This simulates the Maybe monad for computations-that-may-fail.)
  • There is a recover_with function that catches the Error, and continues
    with a useful default.
  • At the end, we report all errors and warnings in the mutable ref. And
    if there are any errors, we abort compilation.
    See recover_and_report

Now any use of error does not abort futher processing, only
fatal_error does.

The code in pipeline.ml is maybe not the prettiest yet, but lets first
see if this approach actually works inside typing.ml.

I have a strong urge to refactor the error accumulation; this
`recover_with` and `return_with` isn’t really pretty, adds quite useless
noise to the code, and prevents easy use of existing combinators like
`List.iter`.

So I propose this:
* We collect error messages and warnings in a mutable reference (a list
  or rope) that we just pass down as argument (inside `env`). This is
  used to collect errors (and warnings), but not for control flow.
  (This simulates the writer monad, without the syntactic overhead).
* The exception no longer carries an argument. This is raised when we
  cannot continue due to an error.
  (This simulates the Maybe monad for computations-that-may-fail.)
* There is a `recover_with` function that catches the Error, and continues
  with a useful default.
* At the end, we report all errors and warnings in the mutable ref. And
  if there are any errors, we abort compilation.
  See `recover_and_report`

Now any use of `error` does not abort futher processing, only
`fatal_error` does.

The code in `pipeline.ml` is maybe not the prettiest yet, but lets first
see if this approach actually works inside `typing.ml`.
and more closely match what we had before
the function gather_field_exp is run thrice; not a good place for a
non-fatal error.

This could be refined to make sure the check happens only once, either
by passing down a flag to exactly one invocation of `gather_field_exp`,
or moving the check to a separate, explicitly used function alltogether.
this means that now also warnings will be reported nicely! Will
hopefully make @paulyoung even happier.
as these are checked elsewhere. Improves the error message.
@nomeata nomeata changed the title [wip] Refactoring the typechecking error handling Refactoring the typechecking error handling Nov 28, 2018
src/typing.mli Outdated
@@ -8,6 +8,10 @@ type ret_env = typ option

type scope = val_env * typ_env * con_env

type error = Error of (Source.region * string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove the parens around the product unless you want the extra heap indirection this implies in Caml

Copy link
Contributor

@crusso crusso Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this scratches your itch go for it, but it's a shame that some of the type checker functions now need to take an env when before they did not. I would rather abstract all the definitions on a single state parameter and to it via a single, outer common argument but I guess that's just style.

Copy link
Collaborator Author

@nomeata nomeata Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather abstract all the definitions on a single state parameter and to it via a single, outer common argument but I guess that's just style.

Sorry, I can’t parse it. I wanted to avoid increasing the number of arguments to most functions, so I put it in env. Do you suggest to have a static_env (read-only, only passed down) and dynamic_env (the current one) (with possibly different names, of course)?

Happy to refactor it accordingly; but maybe based on whatever Andreas’ preference is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the parens around the product unless you want the extra heap indirection this implies in Caml

@crusso do you know of any resources on this?

Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still skeptical overall, but if you want to proceed I'm fine. However, there is a risk here: if you are not very careful, continuing will more often than not either (1) cause many noisy follow-up errors, thereby actually worsening the user experience, or even (2) break invariants of the type checker. I see you already made a lot of errors fatal. I tried to audit all remaining places of non-fatal errors, and a number of them fail on one of these points, so need to be changed. I hope I haven't missed more, can't harm to check again -- as one rule of thumb, any error in an inference-like context should probably be fatal.

I also have some issues with the interface, see comments.

src/pipeline.ml Outdated Show resolved Hide resolved
src/pipeline.ml Outdated Show resolved Hide resolved
src/typing.ml Outdated Show resolved Hide resolved
src/typing.mli Outdated Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Outdated Show resolved Hide resolved
@nomeata
Copy link
Collaborator Author

nomeata commented Nov 29, 2018

Ok, all nits should be resolved (assuming I did not overly eagerly mark any as resolved). But in any case I will wait for #88 to be approved and merged first.

this pulls in #88 and addresses the nit that `typing.mli` should not
mention the `errs` field.
Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good, just a few suggestions and nits.

src/pipeline.ml Outdated Show resolved Hide resolved
src/pipeline.ml Outdated Show resolved Hide resolved
src/severity.ml Show resolved Hide resolved
src/typing.ml Outdated Show resolved Hide resolved
src/typing.ml Outdated Show resolved Hide resolved
src/typing.ml Show resolved Hide resolved
src/typing.ml Outdated Show resolved Hide resolved
src/typing.ml Outdated Show resolved Hide resolved
hopefully we can resolve #92 soon, stack traces in the test output are a
PITA (I should probably add a post-proccessing filter that normalizes
the output a bit).
the definiton of messages_result will eventually go into a separate
module
Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nomeata nomeata merged commit b0d6332 into master Dec 3, 2018
@nomeata nomeata deleted the typecheck-error-refactor branch December 3, 2018 13:21
dfinity-bot added a commit that referenced this pull request Oct 28, 2020
## Changelog for candid:
Branch: 
Commits: [dfinity/candid@a1dcbad4...3e3ad95a](dfinity/candid@a1dcbad...3e3ad95)

* [`119703ba`](dfinity/candid@119703b) [Spec] Relax LEB128 decoding ([dfinity/candid⁠#79](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/79))
* [`10f08432`](dfinity/candid@10f0843) Update prim.test.did
* [`b2524816`](dfinity/candid@b252481) parser for test suite ([dfinity/candid⁠#78](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/78))
* [`71bf6e76`](dfinity/candid@71bf6e7) release 0.5.2 ([dfinity/candid⁠#80](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/80))
* [`b9f387e3`](dfinity/candid@b9f387e) test suite for JS ([dfinity/candid⁠#81](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/81))
* [`9e5dc775`](dfinity/candid@9e5dc77) Release ([dfinity/candid⁠#82](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/82))
* [`1df9d2d7`](dfinity/candid@1df9d2d) more candid test data ([dfinity/candid⁠#83](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/83))
* [`9e4156d9`](dfinity/candid@9e4156d) fix newtype ([dfinity/candid⁠#85](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/85))
* [`6880a430`](dfinity/candid@6880a43) display for types ([dfinity/candid⁠#86](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/86))
* [`04b1b068`](dfinity/candid@04b1b06) release ([dfinity/candid⁠#87](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/87))
* [`117c6436`](dfinity/candid@117c643) Refactor Lexer ([dfinity/candid⁠#89](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/89))
* [`0a5789f9`](dfinity/candid@0a5789f) fix value pretty printer ([dfinity/candid⁠#92](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/92))
* [`9f35a5aa`](dfinity/candid@9f35a5a) refactor error ([dfinity/candid⁠#94](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/94))
* [`2e742927`](dfinity/candid@2e74292) Parse annvals in textual format ([dfinity/candid⁠#93](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/93))
* [`0a144c79`](dfinity/candid@0a144c7) use principal from ic-types ([dfinity/candid⁠#84](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/84))
* [`01412b14`](dfinity/candid@01412b1) release ([dfinity/candid⁠#95](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/95))
* [`f540df54`](dfinity/candid@f540df5) release ([dfinity/candid⁠#98](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/98))
* [`798675d8`](dfinity/candid@798675d) Add generic functions to encode/decode around a tuple ([dfinity/candid⁠#99](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/99))
* [`0d26e568`](dfinity/candid@0d26e56) release ([dfinity/candid⁠#100](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/100))
* [`191b6f1f`](dfinity/candid@191b6f1) Reset record_nesting_depth after each value ([dfinity/candid⁠#101](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/101))
* [`8e7be65d`](dfinity/candid@8e7be65) fix record ([dfinity/candid⁠#103](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/103))
* [`159533b2`](dfinity/candid@159533b) Update construct.test.did
* [`a6ea0991`](dfinity/candid@a6ea099) add service initialization parameters ([dfinity/candid⁠#88](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/88))
* [`3a1f56fa`](dfinity/candid@3a1f56f) refactor: sort dependencies and add traits for error types ([dfinity/candid⁠#105](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/105))
* [`89df78ee`](dfinity/candid@89df78e) support service constructor ([dfinity/candid⁠#106](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/106))
* [`738d5ed4`](dfinity/candid@738d5ed) fix for actor class codegen ([dfinity/candid⁠#107](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/107))
* [`97ba7a0f`](dfinity/candid@97ba7a0) export init args in js ([dfinity/candid⁠#108](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/108))
* [`d4e00adc`](dfinity/candid@d4e00ad) fix js init export ([dfinity/candid⁠#109](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/109))
* [`c1662abe`](dfinity/candid@c1662ab) [spec] Reverse subtyping ([dfinity/candid⁠#110](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/110))
* [`713595be`](dfinity/candid@713595b) The “reverse variant extension rule” is redundand ([dfinity/candid⁠#113](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/113))
* [`79d49a01`](dfinity/candid@79d49a0) Spec: Opt decoding also from non-opt values ([dfinity/candid⁠#114](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/114))
* [`2cfc0ecf`](dfinity/candid@2cfc0ec) improve pretty printing for values ([dfinity/candid⁠#116](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/116))
* [`8fafe345`](dfinity/candid@8fafe34) Un-rename Soundness document ([dfinity/candid⁠#115](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/115))
* [`8e6fc502`](dfinity/candid@8e6fc50) Bump spec version ([dfinity/candid⁠#112](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/112))
* [`7cedebcb`](dfinity/candid@7cedebc) fix clippy ([dfinity/candid⁠#117](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/117))
* [`a732a639`](dfinity/candid@a732a63) Candid UI Canister ([dfinity/candid⁠#111](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/111))
* [`d97b271c`](dfinity/candid@d97b271) disable pretty printing for large vectors ([dfinity/candid⁠#118](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/118))
* [`aceb7f92`](dfinity/candid@aceb7f9) derive candid type for functions ([dfinity/candid⁠#119](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/119))
* [`c3dc0ad7`](dfinity/candid@c3dc0ad) rename derived code for CDK ([dfinity/candid⁠#120](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/120))
* [`d1f8de7d`](dfinity/candid@d1f8de7) release ([dfinity/candid⁠#121](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/121))
* [`3e3ad95a`](dfinity/candid@3e3ad95) remove multi-line string in test suites ([dfinity/candid⁠#125](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity/candid/issues/125))
dfinity-bot added a commit that referenced this pull request May 31, 2022
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@eef711d1...91832636](dfinity/ic-hs@eef711d...9183263)

* [`4920ec29`](dfinity/ic-hs@4920ec2) System API for ECDSA signing ([dfinity/ic-hs⁠#79](https://togithub.com/dfinity/ic-hs/issues/79))
* [`47b7f0b6`](dfinity/ic-hs@47b7f0b) Mirror changes of the spec related to the canister HTTP calls ([dfinity/ic-hs⁠#85](https://togithub.com/dfinity/ic-hs/issues/85))
* [`91832636`](dfinity/ic-hs@9183263) upgrade nixpkgs to release-22.05 ([dfinity/ic-hs⁠#76](https://togithub.com/dfinity/ic-hs/issues/76))
mergify bot pushed a commit that referenced this pull request May 31, 2022
## Changelog for ic-hs:
Branch: master
Commits: [dfinity/ic-hs@eef711d1...91832636](dfinity/ic-hs@eef711d...9183263)

* [`4920ec29`](dfinity/ic-hs@4920ec2) System API for ECDSA signing ([dfinity/ic-hs⁠#79](https://togithub.com/dfinity/ic-hs/issues/79))
* [`47b7f0b6`](dfinity/ic-hs@47b7f0b) Mirror changes of the spec related to the canister HTTP calls ([dfinity/ic-hs⁠#85](https://togithub.com/dfinity/ic-hs/issues/85))
* [`91832636`](dfinity/ic-hs@9183263) upgrade nixpkgs to release-22.05 ([dfinity/ic-hs⁠#76](https://togithub.com/dfinity/ic-hs/issues/76))
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 this pull request may close these issues.

4 participants