Skip to content
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

Core: ADT-ize all errors #139

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Core: ADT-ize all errors #139

merged 1 commit into from
Jun 6, 2024

Conversation

jmcardon
Copy link
Member

@jmcardon jmcardon commented Jun 4, 2024 •
edited by rsoeldner
Loading

This PR aims to do a few things:

  • Add stack frames back to user emitted errors (via VError propagating frames in the CEK machine, and the UserRecoverableError type in the direct style)
  • Fix the pretty printing of all of our errors (a longstanding TODO)
  • Clean up code related to messy errors and unused error constructors
  • Reevaluate what our actual "Invariant Failures" are, and turn those into an ADT as well.

Post-this PR, the repl output becomes:

pact>(ff 1)
(interactive):1:43: boom
 1 | (ff 1)
   |                                            ^^^^^^^^^^^^^^^^^^^^^^
  at (m.f)
  at (m.ff 1)

PR checklist:

  • Test coverage for the proposed changes
  • PR description contains example output from repl interaction or a snippet from unit test output

@jmcardon jmcardon force-pushed the jose/user-error-stackframes branch from a780164 to 069021c Compare June 4, 2024 15:52
rsoeldner
rsoeldner reviewed Jun 4, 2024
View reviewed changes
pact-tests/Pact/Core/Test/StaticErrorTests.hs Show resolved Hide resolved
pact/Pact/Core/Capabilities.hs Outdated Show resolved Hide resolved
pact/Pact/Core/DefPacts/Types.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CEK.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CEK.hs Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CEK.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CEK.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CoreBuiltin.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CoreBuiltin.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/Direct/ReplBuiltin.hs Outdated Show resolved Hide resolved
@jmcardon jmcardon force-pushed the jose/user-error-stackframes branch from 069021c to 5a421fc Compare June 4, 2024 21:27
@jmcardon jmcardon marked this pull request as ready for review June 5, 2024 00:54
@jmcardon jmcardon force-pushed the jose/user-error-stackframes branch from 5a421fc to 9f512c3 Compare June 5, 2024 01:05
rsoeldner
rsoeldner approved these changes Jun 5, 2024
View reviewed changes
pact-tests/Pact/Core/Test/StaticErrorTests.hs Outdated
runStaticTest :: String -> Text -> (PactErrorI -> Bool) -> Assertion
runStaticTest label src predicate = do
isUserRecoverableError :: Prism' UserRecoverableError a -> PactErrorI -> Bool
isUserRecoverableError p s = isJust $ preview (_PEUserRecoverableError . _1 . p) s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isUserRecoverableError p s = isJust $ preview (_PEUserRecoverableError . _1 . p) s
isUserRecoverableError p = has (_PEUserRecoverableError . _1 . p)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix it in a separate PR :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has to be a "lens master" reaction!

imalsogreg
imalsogreg reviewed Jun 5, 2024
View reviewed changes
pact/Pact/Core/Repl/Compile.hs Outdated
@@ -17,6 +17,8 @@ module Pact.Core.Repl.Compile
, interpretEvalBigStep
, interpretEvalSmallStep
, interpretEvalDirect
, interpretReplProgram'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Slight preference against ' names. Renaming interpretReplProgram' to interpretReplProgramWithBuiltinEnv, or renaming interpretReplProgram to interpretReplProgramDefaultBuiltins is slightly clearer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the thing is that it takes an Interpreter now, so probably should just be interpretReplProgram. The ' version should be purely internal to the module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 👍

imalsogreg
imalsogreg reviewed Jun 5, 2024
View reviewed changes
pact/Pact/Core/Repl/Compile.hs
@@ -203,7 +205,7 @@ interpretReplProgram' interpreter (SourceCode _ source) display = do
docs <- uses replUserDocs (M.lookup qn)
displayValue (RUserDoc d docs)
Nothing ->
failInvariant varI "repl invariant violated: resolved to a top level free variable without a binder"
throwExecutionError varI $ EvalError "repl invariant violated: resolved to a top level free variable without a binder"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ADT-ize this error, too? Or is that going to far?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't address this one, because it's a repl-only error and it's an invariant. It's not worth it.

0xd34df00d
0xd34df00d approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@0xd34df00d 0xd34df00d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work! I left a few very nitpicky comments, please feel free to ignore any!

pact-lsp/Pact/Core/LanguageServer.hs
@@ -254,7 +254,7 @@ sendDiagnostics nuri mv content = liftIO runPact >>= \case
PEParseError{} -> "Parse"
PEDesugarError{} -> "Desugar"
PEExecutionError{} -> "Execution"
PERecoverableError{} -> "Execution"
PEUserRecoverableError{} -> "Execution"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be the same as Execution?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question, @rsoeldner does it matter? I assumed so, it's essentially an error that happens during execution.

pact-tests/Pact/Core/Test/StaticErrorTests.hs Outdated
runStaticTest :: String -> Text -> (PactErrorI -> Bool) -> Assertion
runStaticTest label src predicate = do
isUserRecoverableError :: Prism' UserRecoverableError a -> PactErrorI -> Bool
isUserRecoverableError p s = isJust $ preview (_PEUserRecoverableError . _1 . p) s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has to be a "lens master" reaction!

pact/Pact/Core/Type.hs
DKDefConst -> "defconst"
DKDefCap -> "defcap"
DKDefPact -> "defpact"
DKDefSchema _ -> "defscema"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DKDefSchema _ -> "defscema"
DKDefSchema _ -> "defschema"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good catch 👍 I've overseen it

pact/Pact/Core/StableEncoding.hs
@@ -131,7 +131,7 @@ instance J.Encode (StableEncoding KeySetName) where
instance J.Encode (StableEncoding KeySet) where
build (StableEncoding (KeySet keys predFun)) =J.object
[ "pred" J..= StableEncoding predFun
, "keys" J..= J.Array (Set.map StableEncoding keys) -- TODO: is this valid?
, "keys" J..= J.Array (S.map StableEncoding keys) -- TODO: is this valid?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this line is touched — what kind of validity is implied here? Functoriality of this map or smth else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Don't quite remember lol.

pact-tests/Pact/Core/Test/StaticErrorTests.hs Show resolved Hide resolved
pact/Pact/Core/Errors.hs Outdated Show resolved Hide resolved
pact/Pact/Core/Errors.hs Outdated Show resolved Hide resolved
pact/Pact/Core/Errors.hs Outdated Show resolved Hide resolved
pact/Pact/Core/IR/Eval/CEK.hs
Comment on lines +1582 to +1583
v' <- enforcePactValue i v
maybeTCType i mty v'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved out to a separate function to avoid having to call maybeTCType (which can be forgotten)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made an asana ticket for this

pact/Pact/Core/IR/Eval/Runtime/Utils.hs Outdated Show resolved Hide resolved
@jmcardon jmcardon force-pushed the jose/user-error-stackframes branch from 9f512c3 to c44cf38 Compare June 6, 2024 19:55
@jmcardon jmcardon force-pushed the jose/user-error-stackframes branch from c44cf38 to 390ec3c Compare June 6, 2024 19:56
@jmcardon jmcardon merged commit 2882eac into master Jun 6, 2024
3 checks passed
@jmcardon jmcardon deleted the jose/user-error-stackframes branch August 27, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsoeldner rsoeldner rsoeldner approved these changes

@imalsogreg imalsogreg imalsogreg left review comments

@0xd34df00d 0xd34df00d 0xd34df00d approved these changes

@jwiegley jwiegley Awaiting requested review from jwiegley jwiegley is a code owner

@emilypi emilypi Awaiting requested review from emilypi emilypi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jmcardon @0xd34df00d @imalsogreg @rsoeldner