Update dsc.exe to have single process exit point - #1693
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the dsc CLI to avoid calling std::process::exit() throughout the codebase, instead propagating std::process::ExitCode via Result so the process exits through a single return path (improving Windows code coverage collection).
Changes:
- Replaces many
exit(<code>)calls withResult<_, ExitCode>propagation across CLI helper functions and subcommand handlers. - Updates CLI exit code constants to
u8to align withExitCode::from(u8). - Adjusts output and input utilities (
write_object,get_input, config root initialization) to returnResultrather than terminating the process.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dsc/src/util.rs | Converts exit-code constants to u8 and updates utility helpers to return Result<_, ExitCode> instead of exiting. |
| dsc/src/subcommand.rs | Updates subcommand entrypoints/helpers to return Result<(), ExitCode> and propagate failures upward. |
| dsc/src/resource_command.rs | Changes resource command handlers to return Result<(), ExitCode> and replace exit() with propagated ExitCode. |
| dsc/src/main.rs | Makes main() return ExitCode and routes subcommands through dsc_main() -> Result<(), ExitCode>. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Mikey Lombardi (He/Him) (michaeltlombardi)
left a comment
There was a problem hiding this comment.
Everything here looks good to me, I do think that switching to handling exit at the top level will be more maintainable in the long run.
Looking at the updated logic for the functions, especially the emitting of errors in-place and selecting an exit code to return, made me think that in a future PR (I will file an issue if we agree this is coherent) we could:
- Define a mapping for
DscErrortoExitCodeby implementingTryFrom<DscError> for ExitCodeto minimize needing to do so at all these callsites. - Return the
DscErrortype instead of anExitCode - Emit the final terminating error in the root handler before exiting with the required code.
This would also make it easier from a maintainer perspective to work with terminating errors and will eventually be very useful when we want to emit miette diagnostics for more verbose and useful errors (much easier to define handling/emitting in one place for the terminating error).
PR Summary
There's a known issue using
std::process::exit()on Windows that prevents code coverage data from being collected. Fix is to switch to havingmain()returnExitCodewhich accomplishes the same thing. The previous code indsc.exehad many calls toexit()which is not ideal since there are now multiple ways for the process to exit. Changed many functions in the exe to return aResult<>instead which wraps the exit code it would have returned so now there's a single place where that gets returned.ExitCode::from()takes a u8 so changed the consts to u8 instead of i32.