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

Compiler should not call System.exit #5674

Closed
scabug opened this issue Apr 16, 2012 · 10 comments
Closed

Compiler should not call System.exit #5674

scabug opened this issue Apr 16, 2012 · 10 comments

Comments

@scabug
Copy link

@scabug scabug commented Apr 16, 2012

...except under very specific/clear circumstances. To do so otherwise seriously hampers interoperation with maven, sbt, osgi, any situation where we don't completely own the running jvm.

@scabug
Copy link
Author

@scabug scabug commented Apr 16, 2012

@scabug
Copy link
Author

@scabug scabug commented Jan 17, 2013

@gkossakowski said:
I looked into this ticket one night. We are not that far from being System.exit free. I'll follow up on this some time in a future.

@scabug
Copy link
Author

@scabug scabug commented Feb 26, 2015

@adriaanm said:
We're currently at:

scala/tools/cmd/Demo.scala:51	      override def errorFn(msg: String) = { println("Error: " + msg) ; sys.exit(0) }
scala/tools/cmd/package.scala:19	    sys.exit(0)
scala/tools/nsc/Driver.scala:68	    sys.exit(if (reporter.hasErrors) 1 else 0)
scala/tools/nsc/MainGenericRunner.scala:106	      sys.exit(1)
scala/tools/nsc/MainTokenMetric.scala:53	    sys.exit(if (reporter.hasErrors) 1 else 0)
scala/tools/nsc/interactive/REPL.scala:66	    /*sys.*/exit(if (reporter.hasErrors) 1 else 0)// Don't use sys yet as this has to run on 2.8.2 also.
scala/tools/nsc/interactive/REPL.scala:189	          exit(1) // Don't use sys yet as this has to run on 2.8.2 also.
scala/tools/nsc/interactive/tests/InteractiveTest.scala:127	    sys.exit(0)
scala/tools/nsc/interactive/tests/Tester.scala:206	    sys.exit(0)
scala/tools/util/SocketServer.scala:21	  def fatal(msg: String) = { warn(msg) ; sys.exit(1) }
scala/tools/util/VerifyClass.scala:51	    System.exit(if(errors.size > 0) 1 else 0)

@SethTisue SethTisue added this to the Backlog milestone Mar 3, 2018
@scala scala deleted a comment from scabug Mar 3, 2018
@scala scala deleted a comment from scabug Mar 3, 2018
@scala scala deleted a comment from scabug Mar 3, 2018
@scala scala deleted a comment from scabug Mar 3, 2018
@scala scala deleted a comment from scabug Mar 3, 2018
@SethTisue
Copy link
Member

@SethTisue SethTisue commented Mar 3, 2018

/me shudders

is there a good samaritan who wants to tackle this?

@NthPortal
Copy link

@NthPortal NthPortal commented Mar 3, 2018

I could have a look into this.

What does exit get replaced with?

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Mar 6, 2018

well, that might depend on context. in general compiler code, just throw an exception, e.g. with Global.require. or whatever. there is a lot of diversity in the codebase on this: throw new IllegalArgumentException, throw new IllegalStateException, throw new FatalError, you'll see all sorts of stuff

perhaps there will be cases where there would be room to do even better, e.g. by reporting some kind of proper compiler error? but you needn't go there unless you really feel like it, feel free to scope the issue narrowly. System.exit and sys.exit are so bad that basically anything is better. (just don't launch the proverbial missiles.)

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Mar 6, 2018

thought: it's conceivable that one or more of the remaining calls is actually legit, like maybe in an fsc context or something? something to keep in mind, but you needn't get paranoid about it

@NthPortal
Copy link

@NthPortal NthPortal commented Mar 6, 2018

@SethTisue Looking more closely at some of the uses, a lot of them are exit(0), which is supposed to indicate a clean exit; this necessarily cannot be replaced with an exception indicating failure. Should there be a new CleanExit exception (ControlThrowable?), to replace exit(0)? Or something else? In some of these cases (e.g. scala.tools.cmd.runAndExit), it is not possible to merely remove the clean exit, as it actually short-circuits the code.

Edit: scala.tools.cmd.runAndExit is actually the only place I've found which uses exit(0) to short-circuit

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Mar 13, 2018

@NthPortal sorry but I haven't had time to take a closer look at this and won't this week either. we can look at it on a case-by-case basis on the PR.

@NthPortal
Copy link

@NthPortal NthPortal commented Mar 13, 2018

@SethTisue No worries

For all of the ones where exit(0) was at the end of a main method, I just removed it. It's really just runAndExit that I'm not sure how to remove

@lrytz lrytz closed this in #6407 Jun 6, 2018
@SethTisue SethTisue removed this from the Backlog milestone Jun 6, 2018
@SethTisue SethTisue added this to the 2.13.0-M5 milestone Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants