Fix/bad syntax binding error variants #6343
Conversation
… formatting code) to specialize CheckErrors::BadSyntaxBinding. Also, remove tuple-specific bad syntax binding errors since they are now captured by BadSyntaxBinding
…e-bindings, report the specific kind of error instead of the generic BadSyntaxBinding
…type-check a list of (name, value) pairs
…kind of binding being checked so an error will result in a meaningful message
…SyntaxBinding(..)
…ype-check being performed so error messages can reflect it
… to now check for the specific variant of BadSyntaxBinding. Also, add comprehensive checks for each variant of BadSyntaxBinding, as well as test to verify that error messages do not grow unbound with the type's nesting depth
…nding error and the specific context in which it occurs
…uple being checked
…reporting, and consolidate Display implementation for SymbolicExpression and SymbolicExpressionType
…and update tests to expect when BadSyntaxBinding wraps another CheckErrors variant
…g expecting it to wrap an inner CheckErrors variant which lead to a bad binding
…xBindingError::BadTypeSignature(..) ..), instead of BadSyntaxExpectedListOfPairs, so the caller has a better idea about what is structurally deficient about the binding or type signature
…ntax-binding-error-variants
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (72.73%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6343 +/- ##
===========================================
- Coverage 81.00% 72.73% -8.28%
===========================================
Files 541 541
Lines 347855 348261 +406
===========================================
- Hits 281772 253293 -28479
- Misses 66083 94968 +28885
... and 285 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
clarity/src/vm/analysis/errors.rs
Outdated
| /// Helper constructor for NotList(SyntaxBindingErrorType::Let, item_no, item) | ||
| pub fn let_binding_not_list(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::NotList(SyntaxBindingErrorType::Let, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for InvalidLength(SyntaxBindingErrorType::Let, item_no, item) | ||
| pub fn let_binding_invalid_length(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::InvalidLength(SyntaxBindingErrorType::Let, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for NotAtom(SyntaxBindingErrorType::Let, item_no, item) | ||
| pub fn let_binding_not_atom(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::NotAtom(SyntaxBindingErrorType::Let, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for NotList(SyntaxBindingErrorType::Eval, item_no, item) | ||
| pub fn eval_binding_not_list(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::NotList(SyntaxBindingErrorType::Eval, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for InvalidLength(SyntaxBindingErrorType::Eval, item_no, item) | ||
| pub fn eval_binding_invalid_length(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::InvalidLength(SyntaxBindingErrorType::Eval, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for NotAtom(SyntaxBindingErrorType::Eval, item_no, item) | ||
| pub fn eval_binding_not_atom(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::NotAtom(SyntaxBindingErrorType::Eval, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for NotList(SyntaxBindingErrorType::TupleCons, item_no, item) | ||
| pub fn tuple_cons_not_list(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::NotList(SyntaxBindingErrorType::TupleCons, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for InvalidLength(SyntaxBindingErrorType::TupleCons, item_no, item) | ||
| pub fn tuple_cons_invalid_length(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::InvalidLength(SyntaxBindingErrorType::TupleCons, item_no, item) | ||
| } | ||
|
|
||
| /// Helper constructor for NotAtom(SyntaxBindingErrorType::TupleCons, item_no, item) | ||
| pub fn tuple_cons_not_atom(item_no: usize, item: SymbolicExpression) -> Self { | ||
| Self::NotAtom(SyntaxBindingErrorType::TupleCons, item_no, item) | ||
| } |
There was a problem hiding this comment.
I think these helper constructors could return a CheckErrors::BadSyntaxBinding(..) which would simplify their invocations (I think every invocation of one of these functions wraps the result in a CheckErrors::BadSyntaxBinding). Alternatively, you could define a From<SyntaxBindingError> for CheckErrors and achieve similar cleanups.
clarity/src/vm/analysis/errors.rs
Outdated
| NotList(SyntaxBindingErrorType, usize, SymbolicExpression), | ||
| /// binding list item has an invalid length (e.g. not 2) | ||
| InvalidLength(SyntaxBindingErrorType, usize, SymbolicExpression), | ||
| /// binding name is not an atom | ||
| NotAtom(SyntaxBindingErrorType, usize, SymbolicExpression), | ||
| /// second binding item is a type signature, and the type signature itself is bad. | ||
| /// NOTE: type signature parsing returns CheckErrors, so we cannot include a CheckErrors here | ||
| /// directly without creating a recursive type. Instead, we just report the `Display` | ||
| /// representation of the error here as the third item. | ||
| BadTypeSignature(usize, SymbolicExpression, String), |
There was a problem hiding this comment.
I'm a little wary of including the SymbolicExpression in this error. Not just because it'll end up duplicating a portion of the AST, but CheckError also will contain the SymbolicExpression (I think as it is now, that ends up being the outer function, whether its a let or tuple or whatever). Importantly for debugging, the CheckError's symbolic expression is the one that is used to set the diagnostic struct in that error (which sets the line spans for the error) -- this is what tells LSPs where the error is.
I think the better thing to do would be to drop the SymbolicExpression from this enum, and instead set the SymbolicExpression on the assembled CheckError. This could be kind of a pain to do, except that you already have helper functions, so you could change the return type of the helper constructors to CheckError, and make sure that they invoke the .set_expression():
impl CheckError {
with_expression(err: CheckErrors, expr: &SymbolicExpression) -> CheckError {
let mut r = Self::new(err);
r.set_expression(expr);
r
}
}
...
impl SyntaxBindingError {
/// Helper constructor for NotList(SyntaxBindingErrorType::Let, item_no, item)
pub fn let_binding_not_list(item_no: usize, item: &SymbolicExpression) -> CheckError {
let inner_err = Self::NotList(SyntaxBindingErrorType::Let, item_no);
CheckError::with_expression(CheckErrors::BindingSyntaxError(inner_err))
}Note that the current set_expression function takes a reference arg and then clones internally, so to avoid excessive clones, we should either make sure that we pass a reference and never clone beforehand or we refactor set_expression to take an owned argument, so that the caller can take care of cloning-if-necessary.
| return Err(CheckErrors::BadSyntaxBinding( | ||
| SyntaxBindingError::let_binding_invalid_length( | ||
| i, | ||
| SymbolicExpression::list(pair_expression.to_vec()), |
There was a problem hiding this comment.
| SymbolicExpression::list(pair_expression.to_vec()), | |
| pair.clone(), |
| return Err(CheckErrors::BadSyntaxBinding( | ||
| SyntaxBindingError::tuple_cons_invalid_length( | ||
| i, | ||
| SymbolicExpression::list(pair_expression.to_vec()), |
There was a problem hiding this comment.
| SymbolicExpression::list(pair_expression.to_vec()), | |
| pair.clone() |
| where | ||
| F: FnMut(&ClarityName, &SymbolicExpression) -> std::result::Result<(), E>, | ||
| E: From<CheckErrors>, |
There was a problem hiding this comment.
Okay -- this is where I think my prior suggestion becomes problematic. This function returns E: From<CheckErrors> error types, which would be a problem if we're trying to use the CheckError to handle the symbolic expression. But the usage of this makes me even more skeptical of including portions of the AST in the error struct -- handle_binding_list also generates runtime errors (runtime-error wrapped CheckErrors). I don't think runtime errors should be cloning the AST. But anyways, I think a maybe cleaner thing to do here would be to update this function's signature:
| where | |
| F: FnMut(&ClarityName, &SymbolicExpression) -> std::result::Result<(), E>, | |
| E: From<CheckErrors>, | |
| where | |
| F: FnMut(&ClarityName, &SymbolicExpression) -> std::result::Result<(), E>, | |
| E: From<(SyntaxBindingError, &SymbolicExpression)>, |
That conveys (correctly) that the handler only returns BindingErrors, so E just needs to know how to convert from those, and then we'd define the From<(SyntaxBindingError, &SymbolicExpression)> impls explicitly -- that way this function no longer sometimes implicitly relies on the From<CheckErrors> implementation for runtime errors.
There was a problem hiding this comment.
Alternatively, we force the caller to supply a convert_fn:
pub fn handle_binding_list<F, E>(
bindings: &[SymbolicExpression],
binding_error_type: SyntaxBindingErrorType,
on_err: G,
mut handler: F,
) -> std::result::Result<(), E>
where
G: Fn(SyntaxBindingError, &SymbolicExpression) -> E,
F: FnMut(&ClarityName, &SymbolicExpression) -> std::result::Result<(), E> {
…ntax-binding-error-variants
clarity/src/vm/analysis/errors.rs
Outdated
| /// NOTE: type signature parsing returns CheckErrors, so we cannot include a CheckErrors here | ||
| /// directly without creating a recursive type. Instead, we just report the `Display` | ||
| /// representation of the error here as the third item. | ||
| BadTypeSignature(usize, SymbolicExpression, String), |
There was a problem hiding this comment.
Why not include a SyntaxBindingErrorType for BadTypeSignature?
There was a problem hiding this comment.
After looking at the case where BadTypeSignature arises (in parse_name_type_pairs), I'm actually not convinced that this should be a binding error at all. It seems like it should just be the type definition error (with the CheckError span/diagnostic data pointing at the expression that has the invalid definition). I think the only reason this wouldn't be the case is that it makes the function signature of parse_name_type_pairs simpler (i.e., it avoids needing to do more error conversion logic), but from a user/debugging perspective, it seems like this case really should just be whatever the underlying error was (and the debuggability over the existing implementation would be improved by making sure that the CheckError diagnostic/expression actually points to the offending signature).
There was a problem hiding this comment.
The VM currently treats a type-check error within a syntax binding as a CheckErrors::BadSyntaxBinding instead of the underlying type error. For example:
$ cat /tmp/bad-type-definition.clar
(define-private (foo (bar (string-ascii -12))) (ok true))
$ clarity-cli check /tmp/bad-type-definition.clar | jq
{
"error": {
"analysis": {
"level": "Error",
"message": "invalid syntax binding",
"spans": [
{
"end_column": 0,
"end_line": 0,
"start_column": 0,
"start_line": 0
}
],
"suggestion": "binding syntax example: ((supply int) (ttl int))"
}
},
"message": "Checks failed."
}The BadTypeSignature variant captures this specific class of error and produces a much more useful message:
$ ./target/debug/clarity-cli check /tmp/bad-type-definition.clar | jq
{
"error": {
"analysis": {
"level": "Error",
"message": "invalid syntax binding: Type definition item #1 has an invalid type signature: ( string-ascii -12 ) (reason: created a type which value size was out of defined bounds)",
"spans": [
{
"end_column": 0,
"end_line": 0,
"start_column": 0,
"start_line": 0
}
],
"suggestion": null
}
},
"message": "Checks failed."
Specifically, string S (reason: {S}) is the DiagnosableError::message() output for the inner error (in this case, a CheckErrors::ValueOutOfBounds).
This PR keeps the current behavior of treating type-check errors within syntax bindings as CheckErrors::BadSyntaxBinding.
There was a problem hiding this comment.
After looking at the case where BadTypeSignature arises (in parse_name_type_pairs), I'm actually not convinced that this should be a binding error at all.
I agree in principle, but I'd rather avoid risking a consensus-breaking change by changing the error variant.
There was a problem hiding this comment.
The error variant is not consensus critical here.
There was a problem hiding this comment.
To answer the original question:
Why not include a SyntaxBindingErrorType for BadTypeSignature?
This error variant is meant to capture the inner CheckErrors error which caused the BadSyntaxBinding, so the developer can deduce what's wrong with the syntax binding. Right now, this variant takes a String to capture the DiagnosableError::message() output for the inner error. It could alternatively take a Box<CheckErrors>, but I didn't do this because we're really only interested in reporting to the developer what the inner error was (i.e. there's no value in preserving the type). In addition, the code avoids producing a very long String (or alternatively, a series of nested Box<CheckErrors>) by dropping any inner intermediate BadSyntaxBinding errors, and only reporting the outermost error which was not a BadSyntaxBinding. This is handled by this code here, in parse_name_type_pairs():
let type_info = TypeSignature::parse_type_repr(epoch, type_symbol, accounting)
.map_err(|e| {
SyntaxBindingError::BadTypeSignature(
i,
(*type_symbol).clone(),
// if the inner error is itself a BadTypeSignature error, and it's
// `message` came from a BadSyntaxBinding, then just use its
// message directly so we don't get a tower of nested BadTypeSignature
// messages. We only want one level of nesting, so something like
// `(string-ascii -19)` gets reported instead of `-19` (so the caller gets
// some context, but not an unreasonably large amount)
if let CheckErrors::BadSyntaxBinding(
SyntaxBindingError::BadTypeSignature(_, _, message),
) = &e
{
if CheckErrors::has_nested_bad_syntax_binding_message(message) {
message.clone()
} else {
e.message()
}
} else {
e.message()
},
).into()There was a problem hiding this comment.
If it were consensus critical, this changeset already changes the error variant for this case -- parse_name_type_pairs is used by other callers than just the function argument binding (in particular, its used for parsing tuple type definitions). So for something like from-consensus-buff? your changeset would alter the error variant from the underlying one to a BadSyntaxBinding error.
Try this contract:
(from-consensus-buff? (tuple (a (string-ascii -12))) 0x00)There was a problem hiding this comment.
If it were consensus critical, this changeset already changes the error variant for this case
That was unintentional on my part.
I'm happy to follow your recommendation and update parse_name_type_pairs() to return the inner CheckErrors as it is. I'm pretty sure it's not consensus-critical since BadSyntaxBinding is not used in any code path in stackslib or stacks-node (but other CheckErrors are).
There was a problem hiding this comment.
I have two high-level pieces of feedback on the new error type:
-
The new error struct shouldn't keep a copy of the expression. Either the struct and the binding handlers should be refactored so that this can be avoided (making sure to pass the correct expression to the
CheckErrorstruct). Another option is that rather than wrap theSyntaxBindingErrorinto aCheckErrorsvariant, we do something like make each of theSyntaxBindingErrorvariants into a newCheckErrorsvariant, and then defineFrom<SyntaxBindingError> for CheckErrorwhich pulls the expression out of theSyntaxBindingError(so rather than wrapping a SyntaxBindingError, we're converting from a SyntaxBindingError). I'm a little agnostic between those, but I definitely think that using theCheckError's diagnostic/expression field is the right thing to do. -
BadTypeSignatureshould be dropped, and the underlying error should just be returned. I think the pain of returning the underlying error is that previously theCheckErrorexpression that is constructed used the whole binding as the expression (e.g., the tuple type definition) rather than the just the offending expression. So instead, we should just make sure that we're setting the offending expression on the returned CheckError. This will probably involve some careful type handling in theparse_name_type_pairsfunction, but it certainly seems doable.
github-project-automation
bot
moved this from Status: In Review
to Status: 💻 In Progress
in Stacks Core Eng
Description
This PR improves the expressiveness of
CheckErrors::BadSyntaxBindingand some related binding errors to indicate both the kind of binding considered, as well as the specific structural deficiency. Specifically, it differentiates between faulty let-bindings, function definitions, and tuple constructors, and it treats all instance of invalid type signatures found in top-level definitions asCheckErrors::BadSyntaxBindingwhile reporting the innerCheckErrorserror message and innermost faulty binding in the signature.For example, in
developtoday, these errors are reported.In this PR, these same errors are as follows:
Right now, this PR does not touch
CheckErrors::BadMayTypeDefinition, but I'm happy to add that as well.Applicable issues
Additional info (benefits, drawbacks, caveats)
Looking at where the errors were used, it does not appear that any changes are consensus critical. I could not find any place in the codebase where the caller matches on a
CheckErrorsvalue affected by this PR.Checklist
docs/rpc/openapi.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepobitcoin-tests.yml