throw Result.Failure for prettier error messages

[lolgab]

Since now Result.Failure has a special catcher which allows to show errors the same as if they were returned as Result[T], we can start throw Result.Failures instead of generic Exceptions for errors.

Pull Request: #3406

[lihaoyi]

I'm not sure if we want to do this in the general case, since the lack of stack trace may well make things harder to debug. For low frequency errors in uncommon code paths, having the stack trace present seems preferable to me

[lolgab]

@lihaoyi I left only the ones that are clearly user errors, the PublishModule error I added myself and I wanted it to be ideally a Result.Failure from the start, but we didn't have the special handling.

[lolgab]

Calling .getMessage() on a Result.Failure would return null. I overridden it to return msg

[lihaoyi]

Sounds good. We need to be careful with throwing Result.Failure, just like we need to be careful with early-returns, nonlocal-returns, and other kinds of nonlocal-control-flow in general. But I think it's worth having it as an option, and this kind of "short circuit return error" is pretty prevalent across the industry so there's probably some value in it

[lefou]

TBH, I lack a clear opinion on this change. I tend to hesitate, though. What I don't like is that it blurs the purpose of the API. Before, Result.Failure was a Result case, so it directly guides the API user when to use it. In the same spirit, we have a MillException hierarchy for some internal issues we need to transport as exceptions because they can happen before task evaluation. This PR add the exception type to Result.Failure, overloading it's utility. Although choice is always nice, it's also less clear what's the best way to use it from the API user perspective. Should I throw or return?

[lefou]

I'm aware of Exception being added earlier into Result.Failing, and @lolgab is just trying to improve what we have. @lihaoyi Why we added Exception to Failure again? It would be really nice, if we could better separate the two concept: exceptions and result types. Why do we have a Result.Exception being itself an Exception? How to properly display the message in the thrown case. How complex can extracting a proper message be?

So, I propose to remove exception from Result.Failure type hierarchy again. Instead we should evolve the MillException hierarchy, which is already properly unpacked and reported from the launcher. When caught at evaluation time, we can already wrap it in a Result.Exception.

[lolgab]

I know you are already aware of it, but let's clarify for other people reading.
case class Failure already extends java.lang.Exception because it extends the Failing trait which extends java.lang.Exception. This PR just changes the constructor used by Failure so it can pass the message to the underlying Exception class.
I agree that it's easier to understand when the two concepts are separate.
Result.Failure to return and Result.Exception to wrap thrown exceptions. However, this requires to refactor whole call-chains when you want to start return a Result.Failure deep in a method previously returning T.
I think the fact that you can return Result.Failure is still useful for the easy case of a single Task when you know conditionally what to return. Not throwing exceptions is also slightly more efficient.
Example:

def foo = T {
  if (bar()) { Result.Success("result") }
  else { Result.Failure("failure") }
}

vs

def foo = T {
  if (bar()) { "result" }
  else { throw Result.Failure("failure") }
}

However, having the possibility to throw a Result.Failure allows to display a nice error message also in cases where it is not possible, like when computing moduleDeps for PublishModule, which before displayed an ugly stack trace for no interesting reason.

[lefou]

@lolgab I think you made a good case for the motivation of Result.Failure being an Exception. Whereas I tried to point out the implications. Once, you are not directly in the task implementation body but in some helper method, you can't oversee how it will be used later. I think it even results in throw Result.Failure handled differently depending on whether it's execution flow is below a task evaluation by an Evaluator (in which case it's handled like a return Result.Failure) or outside of an evaluation (in which case it's an ordinary exception bubbling up to the next try-catch block).

So essentially, the motivation is to bypass the type inference of the compiler, which no longer can apply an implicit conversion from T to Result[T], once we return a Result.Failure[_]. Instead, we could try to explore alternatives.

For example, what about the task context API? We could use a T.fail(msg) as shortcut for T.ctx().fail(msg) which is only available inside an evaluation execution flow. It could do whatever is necessary to lift the msg into the correct Result.Failure, if necessary by throwing an exception.

[lihaoyi]

@lefou the original idea was to provide an easier way to short circuit return nice error messages. Working with Result is OK, but is definitely more clunky than letting exceptions be thrown. Allowing Result.Failure to be thrown and caught and handled specially like a non-local return gives the best of both worlds, at the cost of some magic (which is the case for all non-local returns). Lots of other frameworks have something similar - e.g. every web framework has some kind of short-circuit return-404 - so I thought it could be very useful in Mill as well. And com.lihaoyi Scala has never been shy about throwing exceptions, unlike other parts of the Scala community.

TBH I'm not totally confident that it definitely will be the right thing to do, but it seems plausible enough. So when I needed such a "nice" API in #3330 I went and added this capability (or tried to, and failed and so @lolgab ended up fixing it haha)

[lefou]

I guess I need to point something out. I'm not against having an return/fail-mechanism based on exceptions! I'm against reusing the existing Result.Failure type for it. Result is a sealed trait structure meant to be used as (return) values. We end up with some API with isn't self-explaining without the addition of extra docs. We should try a little bit harder to achieve what we want without such scarifies. What do you think of using some T.fail (like e.g. sys.error) that creates and throws a class TaskFailureException extends MillException. Looks clean to me. We could special handle it in the evaluator (convert it to a Result type), and already handle it in all other cases in the Mill launchers (which try-catches MillExceptions already).

[lefou]

I could also imagine some try-catch-block in the implicit conversion from T to Result[T], such that each task implicitly converts inner caught ResultFailureExceptions to a Result type and we can continue the control flow as nice as we could with a directly returned Result.Failure.

[lihaoyi]

I'm indifferent as to what we call it. If you think it should be a separate TaskFailureException, rather than reusing Result.Failure, that works for me

[lolgab]

Going to this route, I would also reconsider having Result.Exception part of the sealed hierarchy of Result.
Today it is possible to do the following:

def foo: T[String] = T {
  Result.Exception(new Exception("BOOM"), new Result.OuterStack(Seq.empty))
}

which is not a great API to expose.

If we want to separate the return API from the exception API, then Result.Exception could become a standalone exception class detached from the Result trait and handled only as an exception type in catch blocks, when displaying error messages.

[lihaoyi]

I'd be ok with refactoring the Result hierarchy, though any binary incompatible changes will have to wait for 0.13.0

[lefou]

Result.Exception needs to stay until we start to work on 0.13. But I too think, its API is not that useful but hard to use. Instead, we could add an optional exception parameter to Result.Failure, with the semantics, that the proper high level error message in still in the Failure message, but the causing exception is also retained, e.g. for reporting with --debug or in log files. That would make Result.Exception superfluous. And we could still keep any implicitly caught and wrapped exceptions in it.

I'm open to the name/fqn of the new Exception we want to introduce as replacement for Result.Failure: Exception, but i think, we should make it part of the MillException hierarchy, since that it best practice in Exception handling land. I named it ResultFailureException, but without much thinking. Not sure, whether an isolated mill.api.Result.Exception extends MillException clarifies or obfuscates its intended use case, as it's as far away from mill.eval as mill.api.MillException. So we need some Scaladoc anyways.

[lefou]

How about introducing the discussed mill.api.ResultFailureException extends MillException as part of this PR, and throw it instead of Result.Failure?

We can add any automatic lifting of it into a Result.Failure and some convenience API in a separate PR.