Fix/bad syntax binding error variants #6343

jcnelson · 2025-07-31T21:37:59Z

Description

This PR improves the expressiveness of CheckErrors::BadSyntaxBinding and 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 as CheckErrors::BadSyntaxBinding while reporting the inner CheckErrors error message and innermost faulty binding in the signature.

For example, in develop today, these errors are reported.

$ echo '(let ((u1 u2)) true)' | clarity-cli eval_raw - | jq -r '.error.analysis' | head -n 1
BadSyntaxBinding
$ echo '(define-public (foo (string -19)) (ok true))' | clarity-cli eval_raw - | jq -r '.error.analysis' | head -n 1
BadSyntaxBinding
$ echo '(tuple oops (foo u1))' | clarity-cli eval_raw - | jq -r '.error.analysis' | head -n 1
TupleExpectsPairs

In this PR, these same errors are as follows:

$ echo '(let ((u1 u2)) true)' | /tmp/clarity-cli eval_raw - | jq -r '.error.analysis' | head -n 1
BadSyntaxBinding(NotAtom(Let, 0, SymbolicExpression { expr: LiteralValue(UInt(1)), id: 5 }))
$ echo '(define-public (foo (string -19)) (ok true))' | /tmp/clarity-cli eval_raw - | jq -r '.error.analysis' | head -n 1
BadSyntaxBinding(BadTypeSignature(0, SymbolicExpression { expr: LiteralValue(Int(-19)), id: 7 }, "supplied type description is invalid"))
$ echo '(tuple oops (foo u1))' | /tmp/clarity-cli eval_raw - | jq -r '.error.analysis' | head -n 1
BadSyntaxBinding(NotList(TupleCons, 0, SymbolicExpression { expr: Atom(ClarityName("oops")), id: 3 }))

Right now, this PR does not touch CheckErrors::BadMayTypeDefinition, but I'm happy to add that as well.

Applicable issues

partially fixes [VM] Anemic VM errors in type-checking bindings #6337

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 CheckErrors value affected by this PR.

Checklist

Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

… 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

…ssage text

…type-check a list of (name, value) pairs

…kind of binding being checked so an error will result in a meaningful message

…SyntaxBinding(..)

…ue) pairs

…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 · 2025-08-01T18:17:06Z

Codecov Report

❌ Patch coverage is 92.34973% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.11%. Comparing base (073d2f4) to head (e047935).
⚠️ Report is 61 commits behind head on develop.

Files with missing lines	Patch %	Lines
clarity/src/vm/analysis/errors.rs	84.61%	12 Missing ⚠️
...y/src/vm/analysis/type_checker/v2_1/natives/mod.rs	87.50%	7 Missing ⚠️
clarity/src/vm/tests/mod.rs	0.00%	4 Missing ⚠️
clarity/src/vm/representations.rs	57.14%	3 Missing ⚠️
.../src/vm/analysis/type_checker/v2_05/natives/mod.rs	94.73%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6343      +/-   ##
===========================================
+ Coverage    75.26%   80.11%   +4.85%     
===========================================
  Files          555      555              
  Lines       351247   351471     +224     
===========================================
+ Hits        264353   281598   +17245     
+ Misses       86894    69873   -17021

Files with missing lines	Coverage Δ
clarity/src/vm/analysis/read_only_checker/mod.rs	`87.39% <100.00%> (+0.75%)`	⬆️
clarity/src/vm/analysis/tests/mod.rs	`100.00% <100.00%> (+11.90%)`	⬆️
clarity/src/vm/analysis/type_checker/v2_05/mod.rs	`90.41% <100.00%> (+0.48%)`	⬆️
...ty/src/vm/analysis/type_checker/v2_05/tests/mod.rs	`99.89% <100.00%> (+9.54%)`	⬆️
clarity/src/vm/analysis/type_checker/v2_1/mod.rs	`90.42% <100.00%> (+0.18%)`	⬆️
...ity/src/vm/analysis/type_checker/v2_1/tests/mod.rs	`99.84% <100.00%> (+6.71%)`	⬆️
clarity/src/vm/errors.rs	`74.68% <100.00%> (+0.99%)`	⬆️
clarity/src/vm/functions/define.rs	`98.86% <100.00%> (+0.02%)`	⬆️
clarity/src/vm/functions/mod.rs	`98.01% <100.00%> (+0.97%)`	⬆️
clarity/src/vm/functions/tuples.rs	`83.33% <100.00%> (ø)`
... and 8 more

... and 233 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 073d2f4...e047935. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

obycode

This looks good to me. I just noted a couple of import clippy warnings that need to be fixed.

clarity/src/vm/tests/datamaps.rs

clarity/src/vm/tests/defines.rs

clarity/src/vm/analysis/read_only_checker/mod.rs

clarity/src/vm/functions/mod.rs

…ntax-binding-error-variants

clarity/src/vm/analysis/errors.rs

kantai

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 CheckError struct). Another option is that rather than wrap the SyntaxBindingError into a CheckErrors variant, we do something like make each of the SyntaxBindingError variants into a new CheckErrors variant, and then define From<SyntaxBindingError> for CheckError which pulls the expression out of the SyntaxBindingError (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 the CheckError's diagnostic/expression field is the right thing to do.
BadTypeSignature should be dropped, and the underlying error should just be returned. I think the pain of returning the underlying error is that previously the CheckError expression 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 the parse_name_type_pairs function, but it certainly seems doable.

obycode

LGTM

clarity/src/vm/analysis/read_only_checker/mod.rs

clarity/src/vm/functions/mod.rs

clarity/src/vm/types/signatures.rs

…_type_pairs() return Result<_, E> where E: for<'a> From<(CheckErrors, &'a SymbolicExpression)> so these functions can be used in both analysis and runtime contexts with the appropriate error types (this patch also adds conversions for them)

…b.com/jcnelson/stacks-core into fix/bad-syntax-binding-error-variants

…ntax-binding-error-variants

jcnelson added 20 commits July 31, 2025 16:46

Add new SyntaxBindingError and SyntaxBindingErrorType variants (incl.…

fb56db8

… formatting code) to specialize CheckErrors::BadSyntaxBinding. Also, remove tuple-specific bad syntax binding errors since they are now captured by BadSyntaxBinding

feat: when encountering an error while checking let-bindings and tupl…

01452b7

…e-bindings, report the specific kind of error instead of the generic BadSyntaxBinding

chore: update tuple construction and pair tests with the new error me…

810330f

…ssage text

chore: pass BadSyntaxErrorType to use in the event the code fails to …

7b5dc8e

…type-check a list of (name, value) pairs

chore: update calls to handle_binding_list() to include the specific …

22a0a78

…kind of binding being checked so an error will result in a meaningful message

chore: update tests to check the specific variant of CheckErrors::Bad…

24fbf5d

…SyntaxBinding(..)

chore: pass the specific kind of type-checking when parsing (name,val…

8fed346

…ue) pairs

chore: update calls to handle_binding_list() to include the kind of t…

664b648

…ype-check being performed so error messages can reflect it

chore: export SyntaxBindingError and SyntaxBindingErrorType

2730aee

chore: report syntax binding errors as originating from function checks

9707f78

chore: update handle_binding_list() to report the specific kind of bi…

c75c8d6

…nding error and the specific context in which it occurs

chore: update call to parse_eval_bindings() to report that it was a t…

af57eee

…uple being checked

feat: add as_error_string() to SymbolicExpression for readable error …

93854b5

…reporting, and consolidate Display implementation for SymbolicExpression and SymbolicExpressionType

chore: update tests to use the specific variant of BadSyntaxBinding, …

38529ae

…and update tests to expect when BadSyntaxBinding wraps another CheckErrors variant

chore: update tests to use the new BadSyntaxBinding variant, includin…

e1ddf25

…g expecting it to wrap an inner CheckErrors variant which lead to a bad binding

chore: add with_id() test constructor method to SynmbolicExpression

ce8bcc5

feat: update parse_name_type_pairs() to report BadSyntaxBinding(Synta…

5bc3312

…xBindingError::BadTypeSignature(..) ..), instead of BadSyntaxExpectedListOfPairs, so the caller has a better idea about what is structurally deficient about the binding or type signature

Merge remote-tracking branch 'stacks-network/develop' into fix/bad-sy…

7ee976d

…ntax-binding-error-variants

chore: update changelog

1396f49

jcnelson requested review from a team as code owners July 31, 2025 21:38

jcnelson changed the base branch from master to develop July 31, 2025 21:38

jcnelson requested review from kantai and obycode August 1, 2025 15:32

Merge branch 'develop' into fix/bad-syntax-binding-error-variants

f72830d

obycode reviewed Aug 11, 2025

View reviewed changes

clarity/src/vm/tests/datamaps.rs Outdated Show resolved Hide resolved

clarity/src/vm/tests/defines.rs Outdated Show resolved Hide resolved

obycode added this to Stacks Core Eng Aug 15, 2025

kantai reviewed Aug 15, 2025

View reviewed changes

clarity/src/vm/analysis/read_only_checker/mod.rs Outdated Show resolved Hide resolved

kantai reviewed Aug 15, 2025

View reviewed changes

clarity/src/vm/analysis/read_only_checker/mod.rs Outdated Show resolved Hide resolved

kantai reviewed Aug 15, 2025

View reviewed changes

clarity/src/vm/functions/mod.rs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'stacks-network/develop' into fix/bad-sy…

eae0acd

…ntax-binding-error-variants

kantai reviewed Aug 15, 2025

View reviewed changes

clarity/src/vm/analysis/errors.rs Outdated Show resolved Hide resolved

kantai requested changes Aug 15, 2025

View reviewed changes

github-project-automation bot moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Aug 15, 2025

chore: address PR feedback

1a55530

jcnelson requested review from kantai and obycode August 15, 2025 23:22

jcnelson and others added 2 commits August 15, 2025 22:38

chore: clippy

61daffe

Merge branch 'develop' into fix/bad-syntax-binding-error-variants

cf9a2e5

obycode previously approved these changes Aug 19, 2025

View reviewed changes

kantai reviewed Aug 19, 2025

View reviewed changes

clarity/src/vm/analysis/read_only_checker/mod.rs Outdated Show resolved Hide resolved

kantai reviewed Aug 19, 2025

View reviewed changes

clarity/src/vm/analysis/read_only_checker/mod.rs Outdated Show resolved Hide resolved

kantai reviewed Aug 19, 2025

View reviewed changes

clarity/src/vm/functions/mod.rs Outdated Show resolved Hide resolved

kantai reviewed Aug 19, 2025

View reviewed changes

clarity/src/vm/types/signatures.rs Outdated Show resolved Hide resolved

jcnelson added 2 commits August 21, 2025 13:59

Merge branch 'fix/bad-syntax-binding-error-variants' of https://githu…

7947db4

…b.com/jcnelson/stacks-core into fix/bad-syntax-binding-error-variants

jcnelson dismissed obycode’s stale review via 7947db4 August 21, 2025 18:01

jcnelson and others added 2 commits August 21, 2025 18:02

Merge branch 'develop' into fix/bad-syntax-binding-error-variants

1ab877b

Merge remote-tracking branch 'stacks-network/develop' into fix/bad-sy…

e047935

…ntax-binding-error-variants

obycode approved these changes Aug 21, 2025

View reviewed changes

kantai approved these changes Aug 25, 2025

View reviewed changes

jcnelson added this pull request to the merge queue Aug 26, 2025

Merged via the queue into stacks-network:develop with commit ed22100 Aug 26, 2025
834 of 850 checks passed

jcnelson deleted the fix/bad-syntax-binding-error-variants branch August 26, 2025 14:34

github-project-automation bot moved this from Status: 💻 In Progress to Status: ✅ Done in Stacks Core Eng Aug 26, 2025

hugoclrd mentioned this pull request Aug 27, 2025

chore: update clarity-vm hirosystems/clarinet#1961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix/bad syntax binding error variants #6343

Fix/bad syntax binding error variants #6343

Uh oh!

jcnelson commented Jul 31, 2025

Uh oh!

codecov bot commented Aug 1, 2025 •

edited

Loading

Uh oh!

obycode left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kantai left a comment

Uh oh!

obycode left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix/bad syntax binding error variants #6343

Fix/bad syntax binding error variants #6343

Uh oh!

Conversation

jcnelson commented Jul 31, 2025

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

Uh oh!

codecov bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

obycode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kantai left a comment

Choose a reason for hiding this comment

Uh oh!

obycode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 1, 2025 •

edited

Loading