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.

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

[ refactor ] Improve error handling in Core #3434

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

spcfox
Copy link
Contributor

@spcfox spcfox commented Dec 3, 2024

Description

  • Added utility functions to handle errors in Core.
  • Added new error types: NonZeroExitCode, SystemError, CodegenNotFound, UnsupportedOpertaion.
  • Added error handling in many places where they were ignored.
  • All coreLift_ calls replaced by explicit result processing.
  • Removed Maybe from compileExpr and incCompileFile signatures. Instead of returning Nothing, throw is used. Earlier both variants were found in the code. Using throw allows you to pass a more clear error.
  • fix Successful exit code after error in --exec #3427: the --exec command returns the program exit code. If several --exec options are passed, the result of the last execution will be returned.
  • Removed CompilationFailed from REPLResult. More specific errors using throw are returned instead.

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG_NEXT.md (and potentially also
    CONTRIBUTORS.md).

@spcfox spcfox marked this pull request as draft December 3, 2024 13:41
src/Compiler/ES/Node.idr Outdated Show resolved Hide resolved
src/Compiler/RefC/RefC.idr Outdated Show resolved Hide resolved
ok <- coreLift $ system cmd
if ok == 0
then pure (Just execPath)
else pure Nothing
safeSystem cmd
pure (Just execPath)
Copy link
Member

Choose a reason for hiding this comment

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

This is not logically equivalent. Why is it legitimate to drop the Nothing branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

safeSystem throws an error if the exit code is not zero. This makes the errors more clear

Comment on lines 453 to 460
when mkexec $ logTime 1 "Build racket" $
safeSystem $ raco ++ ["-o", outBinAbs, outRktAbs]
-- TODO: add launcher script
let outShRel = outputDir </> outfile
makeShPlatform isWindows (if mkexec then "" else racket ++ " ") outShRel appDirRel outRktFile
handleFileError outShRel $ chmodRaw outShRel 0o755
pure (Just outShRel)
Copy link
Member

Choose a reason for hiding this comment

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

Again we're also dropping the Nothing branch

Copy link
Contributor Author

@spcfox spcfox Dec 3, 2024

Choose a reason for hiding this comment

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

safeSystem throw NonZeroExitCode. I replaced all Nothing with throw. Currently Nothing just wraps around to CompilationFailed. A more specific cause of the error is passed to throw.

Idris2/src/Idris/REPL.idr

Lines 815 to 818 in ec74792

ok <- compile cg tm_erased outfile
maybe (pure CompilationFailed)
(pure . Compiled)
ok

src/Compiler/ES/Node.idr Outdated Show resolved Hide resolved
@spcfox spcfox marked this pull request as ready for review December 4, 2024 12:41
(tmpDir : String) -> ClosedTerm -> Core ExitCode
executeExpr c s tmpDir tm = do
out <- compileExpr False c s tmpDir tmpDir tm "_tmpchez"
system $ escapeArg out
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think an explicit escapeArg is clearer than a single element list

@spcfox
Copy link
Contributor Author

spcfox commented Dec 9, 2024

This PR is ready for review. The lsp build is failing because I deleted CompilationFailed. I have not yet made a PR fixing this because I allow for the possibility of revising this decision during review.

@spcfox spcfox requested a review from gallais December 9, 2024 15:28
@spcfox
Copy link
Contributor Author

spcfox commented Dec 9, 2024

If my refactoring is questionable, I can put everything related to #3427 in a separate PR to simplify the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Successful exit code after error in --exec
3 participants