feat: Split analysis-only and runtime-only CheckErrors

[francesco-stacks]

Description

Some input would be appreciated!

• StaticCheckErrorKind: 98 total variants
• CheckErrorKind: 87 total variants
• Shared Variants (in both): 66
  • Shared Variants used in common code paths (CommonCheckErrorKind): 26
• Unique to StaticCheckErrorKind: 34
• Unique to CheckErrorKind: 22

Why Separating the two?

Separating static from "execution-time" Check errors would make the code explicit about what the contract analysis guarantees vs what can only be checked at runtime. Currently, all errors seem possible at any time.

More importantly, this split enables a second phase of cleanup: many CheckErrorKind at runtime are actually safeguards for unreachable code that's already protected by contract analysis. Even the relative tests bypass the contract_analysis to stimulate the unreachable code paths. Once separated, we can identify these and replace them with CheckError::Expects (or a new variant that accepts the transaction but marks it as failed for consensus safety).

We currently have 79 CheckErrorKind variants (58 shared with static). After this cleanup, we'd likely reduce this to ~40 actual runtime errors, making the codebase significantly clearer about what can actually fail at runtime.

WIP: currently the CommonCheckErrorKind enum is public, but should become private and invisible to users of the clarity-types and clarity crates (can't be truly private because the clarity crate also needs it. However, this is solvable by making it public but ensuring all public functions only return StaticCheckError or CheckError)

While trying to achieve this clean public API, I've tried multiple approaches, but each has significant problems (uuugly):

Duplicate public functions

impl ListTypeData {
    pub fn new_list(...) -> Result<ListTypeData, CheckError> {
        Self::inner_new_list(...).map_err(CheckError::from)
    }

    pub fn new_list_static(...) -> Result<ListTypeData, StaticCheckError> {
        Self::inner_new_list(...).map_err(StaticCheckError::from)
    }
    
    fn inner_new_list(...) -> Result<ListTypeData, CommonCheckError> {
        // actual implementation
    }
} 

Problems:

• Ugly and awkward
• Duplicates signatures for tens of functions
• Higher-level shared functions force inner_* functions to become public. (e.g. a function in clarty has to return a CommonCheckError and use a function in clarity-types)

Generic error type

impl ListTypeData {
    pub fn new_list<E: From<CommonCheckError>>(
        entry_type: TypeSignature, 
        max_len: u32
    ) -> Result<ListTypeData, E> {
        // implementation
    }
}

Problems:

• Sometimes the user needs to explicitly state the type, even when not needed e.g. ListTypeData :::new_list::<CheckError>(...).unwrap()
• Functions returning CommonCheckError can't call these generic functions

Open questions:

• Is there a cleaner architectural pattern I'm missing?
• Or should I accept that mixing CommonCheckError, StaticCheckError, and CheckError in the public API is acceptable?
• Is this split actually providing enough value in terms of type safety and code clarity to justify the complexity?

Looking for feedback on whether to push through with a better approach, or abandon this split entirely.

Applicable issues

• fixes #

Additional info (benefits, drawbacks, caveats)

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

[jacinta-stacks]

Still working through but I think I might rename CheckErrorKind to RuntimeCheckErrorKind as I have forgot a few times whether CheckErrorKind was runtime or static specific. Plus then its more consistent with the naming convention used for the other *CheckErrorKind. I would expect CommonCheckErrorKind to be renamed CheckErrorKind if anything but I am okay with it being called CommonCheckErrorKind to be extra explicit.

[jacinta-stacks]

I think this change is a net improvement and it does actually make it easier for me to understand what is happening at what point in the code base. I am not sure yet what a better architectural pattern might be available, but even so I think its still an improvement even if the public API is not super clean.

[federico-stacks]

This split definitely clarifies a lot, making the purpose of each error variant much clearer.

On the other hand, the side effects introduced by the shared variants don’t seem to have a clean solution. I agree that the alternatives you listed as “ugly” would likely make the code worse, so I wouldn’t pursue those either.

If we decide to stick with this architecture, I think we’ll have to accept that the common enum remains public. Perhaps we could give it a name that better reflects its role (not complitely sure about that), especially if we manage to separate the cost-related parts into their own enum (something I’ll try on the ParserErrorKind side).

The only alternative I see would be to revert to a single CheckErrorsKind enum, but keep the improvements you’ve made in documenting properly each error variant. If we can also find a way to merge the “expect-like” ones, we could also reduce the total count.

About the expect-like merge: the problem would be to be sure to preserve the same consensus behaviour (even if some unreachable). So as a first step, could we think to merge them in two different expect variants? One for the expect we are sure are rejectable and one for the one we are not, with a string the explain the kind of error (or a sub enum eventually).

[francesco-stacks]

@jacinta-stacks @federico-stacks Thanks both for the inputs!

re: Stick with a single CheckErrorKind enum but document each field. May be a valid alternative. I don't think it will be possible to merge many variants in that case, because most of them are actually unique. The merge seems to make sense for a decent amount of variants only for the "RuntimeCheckErrorKind", since they are mostly unreachable.

re: preserve same consensus behavior then before. Definitely makes sense. Actually, there already is such a variant CheckErrorKind::CheckerImplementationFailure that we may use

[aaronb-stacks]

Is this split actually providing enough value in terms of type safety and code clarity to justify the complexity?

Yes, I believe so. I think the split makes it much more clear where different errors can surface, and what is actually semantically possible at different phases of the interpreter. It makes the code more explicit, and makes writing new error variants safer: if you add a new CheckErrors today, the rust compiler won't necessarily highlight that handlers for wrapped versions of that variant need to be updated as well.

Is there a cleaner architectural pattern I'm missing?

I think you've basically found the major options. Of the options, I like the generic option the best, however, if we're implementing From<CommonCheckError> anyways, the function could just be:

    pub fn new_list(
        entry_type: TypeSignature, 
        max_len: u32
    ) -> Result<ListTypeData, CommonCheckError> {
        // implementation
    }

Then most callers would be able to just use ?, and it's not really ugly.

Or should I accept that mixing CommonCheckError, StaticCheckError, and CheckError in the public API is acceptable?

I think it's fine (and maybe even good?) to have all these different error types in the Public API.

More importantly, this split enables a second phase of cleanup: many CheckErrorKind at runtime are actually safeguards for unreachable code that's already protected by contract analysis. Even the relative tests bypass the contract_analysis to stimulate the unreachable code paths. Once separated, we can identify these and replace them with CheckError::Expects (or a new variant that accepts the transaction but marks it as failed for consensus safety).

I agree with this second phase too, so long as we go with the second option there (a new variant that accepts the transaction but marks it as failed). While we should be able to identify that some of these are unreachable without bypassing the analysis check, actually guaranteeing that this is the case is pretty challenging.