might be useful to have an assert somewhere that hadWarningOutput is false if errorsAreFatal and the tool exit cleanly.

Sorry, I'm not quite sure I'm following: you want to make sure that if errorsAreFatal is set that hadWarningOutput is false in the case where we're going to exit cleanly? So, in other words, making sure that there were no warnings if there were no fatal errors?

I was intending for it to be the case that if errorsAreFatal is set, but not warningsAreFatal that only errors cause failure exit, and if warningsAreFatal is set, that either warnings or errors would cause a failure exit (basically that fatal warnings implies fatal errors too). Does that sound right to you?

I was intending for it to be the case that if errorsAreFatal is set, but not warningsAreFatal that only errors cause failure exit, and if warningsAreFatal is set, that either warnings or errors would cause a failure exit (basically that fatal warnings implies fatal errors too). Does that sound right to you?

I would only have one flag: did we print a warning/error? If so, exit with code 1 when --warnings-are-fatal, otherwise, exit with code 0 unless something in the code already told us to exit 1. Actually, I think I would probably have had only one API (printError), with "warnings" being errors that aren't immediately followed by a termination. I have no objection to having both as you have implemented it, though. If we have both, then I think the logical behaviour would be for it to be invalid use of the API to call printError and not quit the app with exit 1 shortly afterwards, and for printWarning to be like printStatus (except for color etc) and then at the end of program execution, if there's been a printWarning and --warnings-are-fatal, exit 1.

Or alternatively, instead of printError, printWarning, and ToolExit, we could have a reportError API and a reportWarning API. reportError always terminates (by throwing ToolExit), and reportWarning terminates if the --warnings-are-fatal flag is set, and otherwise continues.

The question is really whether we want the flag's semantics to be "run to completion then report failure" or "treat warnings like fatal errors that terminate the tool".

(I don't feel strongly about any of this; I think there are many viable approaches. I think there is some benefit to the approaches that lead to the tool overall being simpler architecturally than it is now, but that's certainly not a requirement, especially if the more complex approaches can give us features we need that we wouldn't otherwise be able to get.)

cc @christopherfujino

OK, we can just eliminate --fatal-log-errors, with the assumption that errors should already be fatal.

Ideally, I agree: we should just replace the combination of printError and throwToolExit with reportError and rename printWarning to reportWarning. It's basically swapping the precedence for throwToolExit and printError, since currently the former calls the latter. However, that's a pretty extensive change to make, and I don't feel like doing that in this PR, if that's OK. :-)

I do like having a separate printWarning, since it indicates better whether the author thought about how fatal the problem is. If you just have "printError" without an exit instead, then it's not clear if the exit was just overlooked.

There's probably some cases where it's nice to be able to do some work after printing the error, but before exiting, but that could also be addressed by rearranging when the error is reported.

I'm not sure how we would enforce the "exits shortly after printError is called" guideline. Perhaps printError could start a timer that calls throwToolExit after a second or two. That might just make the failures appear more flaky though. I will make it a stronger statement in the API docs for printError.

Actually, in thinking about this some more, it's not that big of a change to modify this. I could do the following:

1. Rename Logger.printError to Logger.reportError and have it take an exit code
2. Rename throwToolExit to reportError.
3. reportError would just call globals.logger.reportError, which would always throw ToolExit after printing its message.
4. Rename printWarning to reportWarning.
5. Fix up all the places where we throwToolExit after calling printError to just move the message to be part of a reportError message.
6. Fix up all the unknown places where we don't currently exit after an error message. (this is the tricky bit).

What do you think?

It might be that Logger isn't really the right name for this in that case, since it does more than just log data.

OK, I took a closer look at this, and it feels like a pretty risky change. There are a lot of places where throwing a ToolExit when we call reportError (née printError) would bypass some kind of shutdown/cleanup of resources.

Another practical consideration is that the analyzer considers throwToolExit to be equivalent to a throw statement, whereas having reportError throw a ToolExit when called isn't considered to be a throw, so there are analysis problems where it assumes that execution could continue when it can't. This could probably be fixed by just doing throw logger.reportError(...) instead, but is clunkier.

For now, I'm just going to do a quick audit of everywhere that we do a printError and see if we exit shortly thereafter, and if we don't, do so or convert it to a warning.

Does it seem like a good idea to add a timer to printError that prints an additional error when the tool hasn't exited after one second or so? Or will that just add error spam that will confuse our users?

Of course, those places where it bypasses cleanup should probably be considered bugs, since we should already clean up well in the face of exceptions. It is hard to verify that there is a finally or catch clause around each invocation that might need cleanup, however.

FWIW we can make the analyzer save us by having reportError return Never so that every implementation must always throw and every call site must always assume it throws.

Like I said, I don't have a strong opinion here. We could also just check that if printError has been called, the tool doesn't exit cleanly. It doesn't have to be on a timer or anything. We could also just do the change you're describing but over a longer period of time, not in this PR.

Ahh, Never was what I was missing. That would work.