Skip to content

Avoid using std::process::exit(0) in ProjectCompiler::compile #10328

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Pronoss
Copy link

@Pronoss Pronoss commented Apr 17, 2025

This pull request replaces a call to std::process::exit(0) in the ProjectCompiler::compile method with a proper Ok(ProjectCompileOutput::new()) return.

PR Checklist

  • Added Tests (not required for this minor refactor)
  • Added Documentation (inline clarity improved)
  • Breaking changes (no)

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty, yeah we should def remove this.

this is legacy from when this existed directly in foundry and we wanted to exit early.

not sure if this has side-effects in foundry now,

we should check before we merge this @grandizzy

@grandizzy grandizzy self-assigned this Apr 17, 2025
@grandizzy
Copy link
Collaborator

@Pronoss thank you, please fix compilation errors

@Pronoss
Copy link
Author

Pronoss commented Apr 22, 2025

@grandizzy
Fixed the compilation error as requested

@Pronoss
Copy link
Author

Pronoss commented Apr 22, 2025

@grandizzy

After some investigation, I've decided to revert the change and keep the original std::process::exit(0) call.

While avoiding exit(0) would be more idiomatic in Rust, replacing it with a return value like Ok(ProjectCompileOutput::default()) isn't currently feasible without introducing a Default bound on C::Language, which breaks compatibility with downstream code (e.g., foundry-cli fails to compile with E0277).

I explored alternative approaches, including returning an Option or a custom Err, but they would require a wider refactor across the entire call chain. Given that, I've opted to keep the current behavior and leave a // TODO: Avoid process::exit(0) comment in the code for future cleanup when a broader change is more appropriate.

Should I add a more detailed comment in the code next to the exit(0) explaining these issues?

@grandizzy
Copy link
Collaborator

@grandizzy

After some investigation, I've decided to revert the change and keep the original std::process::exit(0) call.

While avoiding exit(0) would be more idiomatic in Rust, replacing it with a return value like Ok(ProjectCompileOutput::default()) isn't currently feasible without introducing a Default bound on C::Language, which breaks compatibility with downstream code (e.g., foundry-cli fails to compile with E0277).

I explored alternative approaches, including returning an Option or a custom Err, but they would require a wider refactor across the entire call chain. Given that, I've opted to keep the current behavior and leave a // TODO: Avoid process::exit(0) comment in the code for future cleanup when a broader change is more appropriate.

Should I add a more detailed comment in the code next to the exit(0) explaining these issues?

@Pronoss do I get this right that you propose to keep current behavior and only improve TODO comment? If so, can you draft the comment here to check if it makes sense? thanks!

@Pronoss
Copy link
Author

Pronoss commented Apr 24, 2025

@grandizzy Yes, that’s correct — I’ve decided to keep the current std::process::exit(0) call and added a detailed TODO comment in the code explaining why it remains for now.

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

Successfully merging this pull request may close these issues.

3 participants