Skip to content

Get rid of prismatic error handling? #6160

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.

Sign up for GitHub

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

Jump to bottom
Closed
effectfully opened this issue May 30, 2024 · 2 comments · Fixed by #7111 · May be fixed by #7116
Closed

Get rid of prismatic error handling? #6160

effectfully opened this issue May 30, 2024 · 2 comments · Fixed by #7111 · May be fixed by #7116
Assignees
@SeungheonOh
Labels
Errors Good first issue Internal status: triaged tech debt User experience

Comments

@effectfully
Copy link
Contributor

We use prisms for error handling a lot, an example:

instance AsEvaluationError UnliftingEvaluationError UnliftingError UnliftingError where
    _EvaluationError = coerced
    {-# INLINE _EvaluationError #-}

-- | An 'UnliftingEvaluationError' /is/ an 'EvaluationError', hence for this instance we only
-- require both @operational@ and @structural@ to have '_UnliftingError' prisms, so that we can
-- handle both the cases pointwisely.
instance (AsUnliftingError operational, AsUnliftingError structural) =>
        AsUnliftingEvaluationError (EvaluationError operational structural) where
    _UnliftingEvaluationError = go . coerced where
        go =
            prism'
                (bimap
                    (review _UnliftingError)
                    (review _UnliftingError))
                (bitraverse
                    (reoption . matching _UnliftingError)
                    (reoption . matching _UnliftingError))
    {-# INLINE _UnliftingEvaluationError #-}

instance AsUnliftingEvaluationError BuiltinError where
    _UnliftingEvaluationError = _BuiltinUnliftingEvaluationError . _UnliftingEvaluationError
    {-# INLINE _UnliftingEvaluationError #-}

This all is pretty complex and I've got parts of it wrong in the past. Do we really need it? I don't think we use the matching part of any of these prisms anywhere, only the review one? So can we just make those into regular functions?

We'll lose some Template Haskell convenience that comes with prismatic error handling, but given the amount of hand-written code, maybe even that will be net positive.

Or do we use the matching part somewhere?
@SeungheonOh SeungheonOh self-assigned this May 15, 2025
@SeungheonOh
Copy link
Collaborator

SeungheonOh commented May 21, 2025 •
edited
Loading

I went over the codebase briefly and 99% of the time Prism is used for constructing data. As you said, we can probably just kill prisms entirely and write helpers/throw errors using constructors directly.

More concretely, writing

throwingWithCause (StructuralEvaluationError BuiltinTermArgumentExpectedMachineError) (Just term')

instead of

throwingWithCause _MachineError BuiltinTermArgumentExpectedMachineError (Just term')

looks a lot more straight forward IMO despite it being slightly more verbose.

I also found few wrappers(like UnliftingEvaluationError) that seems extraneous, maybe those can be purged alongside with the prism.

Since error handling spans across very large part of codebase, I want to hear some thoughts. At the moment, I'm inclined to kill all prisms and make all error invocation using raw constructors.

@effectfully
Copy link
Contributor Author

effectfully commented May 21, 2025 •
edited
Loading

looks a lot more straight forward IMO despite it being slightly more verbose.

It's gonna be the same level of verbosity once we merge #7110.

If you've already started writing code and my PR conflicts with yours, it took me 3 find-and-replaces to make those changes, so feel free to close the PR and make the changes yourself.

I also found few wrappers(like UnliftingEvaluationError) that seems extraneous, maybe those can be purged alongside with the prism.

Please don't change those yet, I'm not convinced it's a good idea. Let's get rid of the prisms first.

Since error handling spans across very large part of codebase, I want to hear some thoughts. At the moment, I'm inclined to kill all prisms and make all error invocation using raw constructors.

This seems to be a fairly uncontroversial opinion, but you may ask the team in Slack.

This was referenced May 21, 2025
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SeungheonOh SeungheonOh

Labels
Errors Good first issue Internal status: triaged tech debt User experience
Projects
None yet
Milestone
No milestone
2 participants
@effectfully @SeungheonOh