cleanups: Eliminate errstr and (nearly) eliminate Unexpected
#778
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Improves the typing of the errors (and the error messages themselves) by eliminating another instance of `errstr`. Saves another instance of `errstr` by removing the unused `unary` method. There are now 5 left in the entire library :). Strictly speaking, we ought to do a deprecation cycle on these. But because we are changing the `expression` module so thoroughly, and we don't expect any users are directly using it, it doesn't seem worth the difficult (impossible?) task of preserving the old API for the sake of deprecation messages.
This is a big diff but it's almost entirely mechanical. Replaces the old expression::terminal method with two new `verify_terminal_parent` and `verify_terminal` methods, which do the appropriate checks and return a strongly-typed error. Does not directly eliminate any instances of errstr or Unexpected (though the next commit will), but it does eliminate a large class of them: now when you try to parse an expression with a bad number of children, e.g. and_v with 3 children, you will get an error message that says this rather than an opaque "Unexected(<<and_v>>)" or whatever you get now. It does reduce the semantic::PolicyError type to a single variant, which has no information and is only used to indicate that the entailment calculation failed. This can be replaced by an Option (and will be, in the next commit). It also eliminates some uses of concrete::PolicyError, but the variants are still used by the absurd Policy::is_valid method, so we have to keep them for now. Also, you may notice that this commit and others have a ton of calls to .map_err. I apologize for this. But when we change error types so that parsing returns a string-parsing-specific error rather than the giant Error enum, these should mostly go away.
All semantic errors were actually parsing errors (except for the entailment failure mode, which was better represented as an option). Now that we are strongly-typing parsing errors we do not need this enum.
This eliminates another instance of `errstr` and provides well-typed errors when parsing locktimes and threshold values. It copies the AbsoluteLockTime and RelativeLockTime error variants from the main Error enum into ParseError. The old copies are now used only in script decoding, and will be removed in a later PR when we give script decoding its own error.
This gets rid of several more instances of errstr.
The private function `with_huffman_tree` can only fail if it is given an empty input. In both places we call it, we unwrap the result because we know this is impossible. Instead, just change the function to panic internally instead of returning an error we're just going to unwrap. Since the error was constructed with errstr, this gets rid of a call to errstr.
When we are parsing keys using from_str, use the appropriate variant of ParseError. Also there was one place where we were taking an Error, converting it to a string, then wrapping in Error::Unexpected. I suspect this was a rebase mistake or something like that. Just get rid of the conversion. There are now 2 instances of Error::Unexpected in the codebase and one instance of errstr.
This gets rid of the horrible `errstr` function :).
In miniscript and in policy we had near-identical logic dealing with : and @ separators on certain nodes. The differences were: * In Miniscript we had special handling for aliases, where we would synthetically munge the wrappers (characters before ':'). This was unnecessary since we can just handle the aliases directly. (Because of our munging code, we did some extra error-checking to ensure that a PkK fragment always fits into a Check. It does. This checking is completely unnecessary.) * In Policy we forbade the @ character if we were outside of an Or context. Also unnecessary. The @ character does not appear in any other fragment, so the "unknown fragment" error is already sufficient. Removes two variants from the giant Error enum.
|
Benchmarks with this PR (see #775 for previous numbers): Benchmarks with 778You can see from comparing to the previous PR that the numbers are basically identical. |
sanket1729
approved these changes
Nov 27, 2024
There was a problem hiding this comment.
ACK 33a60e2
Awesome. Thanks for the super helpful commit messages. I felt I was a part of the journey hunting down all the errstrs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a series of commits which cleans up the expression parsing module. After the last couple PRs, which substantially rewrote the parser and introduce a new parsing-error module, we can get rid of many uses of the
Error::Unexpectedvariant and its constructor, theerrstrfunction.This PR should have no visible effects, and does not even change any algorithms. The next one will return to the process of rewriting the expression parser, by replacing the recursive
Treetype with a non-recursive one.Will post benchmarks once they are done.