Improve exceptions reported by PolyglotEngine

[chumer]

We should start classify errors that can occur in guest languages and instruments and report them in a checkable form. We can and should at least differentiate between ParseErrors, RuntimeErrors and InternalErrors. All of these errors that are reported to the outside should be equipped with structured information so they can relate to the given Source in PolyglotEngine#eval. They should have a base class which guest languages should be able to subclass. If guest languages cannot use these error classes for whatever reason they have to translate the errors in TruffleLanguage.

Discussion item: I think we should avoid checked exceptions (IOException) for generic errors in PolyglotEngine (I think checked exceptions only make sense if we need to ensure that they are always checked immediatly after using a method ie. UnexpectedResultException). Instead we should provide a runtime exception base class for Parse and Runtime errors.

[woess]

I think we should avoid checked exceptions (IOException) for generic errors in PolyglotEngine

+1

[ldaynes]

+1

On 3/10/16 12:20 PM, Andreas Woess wrote:

I think we should avoid checked exceptions (IOException) for
generic errors in PolyglotEngine

+1

—
Reply to this email directly or view it on GitHub
#116 (comment).

[jtulach]

Right now the PolyglotEngine.eval method throws IOException. This was OK as there could have been problems reading files or doing some other I/O. It is however a bit strange to have an IOException to signal problem with parsing and moreover these days the IOException is supposed to be thrown from Source.fromFile method, not from the eval itself.

JDK's javax.script API is using its own checked ScriptException. This of course leads to problems where one needs to multi-catch both.... Just creating some new exception goes against my attempt to re-use as much as possible from the existing underlying JDK libraries. E.g. rather than having IllegalExecutionException, it is better to use java.lang.IllegalStateException. The same goes for IOException, it is better to use it rather to have proprietary SourceCannotBeLoadedException.

The only case when it makes sense to add new exceptions is when they also provide some additional information. For example the ScriptException adds methods for obtaining location of a possible error. If we need to deliver such kind of information, we certainly need new exception (sub)type. For example
we can create our own

public class ParsingFailedException extends IOException
  public SourceLocation getWhere();
}

E.g. even with current signature of eval we are able to extend list of thrown exceptions (via subclassing) whenever we need in a compatible way. Those that don't care can just catch IOException. Those who care about better reports, can catch ParsingFailedException. The try construct of Java supports a form of pattern matching.

[chumer]

I agree with your arguments. We should go ahead and define standard exceptions for that. Thats the topic of this issue.

Re IOException: I think the primary criticism is that we have a checked exception as opposed to having a RuntimeException as a base type for eval exceptions.
I find it more intuitive for those who don't care to just not have to catch any exception. So I think we should get rid of IOException in the signature and introduce or reuse some other runtime exception.

Another issue is KillException and QuitException which should extend the eval base exception but doesn't. The reason is that KillException needs to be propagated through the AST. This should also be adressed with this issue.

[mlvdv]

Another issue is KillException and QuitException which should extend the eval base exception but doesn't.

In a pending PR QuitException is deprecated and no longer used.

[mlvdv]

For the record, here's a summary of Debugger requirements (so far):

• Something like the current KillException, when thrown from within an Instrumentation notification, must terminate guest language execution immediately. GL implementations must be allowed to perform cleanups along the way, there must be no more Instrumentation notifications, and the Debugger must be able to detect the "kill" in return from calls to PolyglotEngine.eval() and TruffleLanguage.evalInContext().
• In returns from those same two calls, the Debugger must be also able to distinguish among the various exceptional returns that @chumer mentions: parse, runtime, and internal errors.

[mlvdv]

Should we restrict access to the "kill" option to the Debugger or some subset of privileged clients? Any client of Instrumentation could throw KillException, since the Instrumentation framework treats the Debugger the same as any other Instrument.

[chumer]

No I don't think we should make a difference between the debugger and other instrumentations. Instrumentations in general are not completely safe because they need performance.

[jtulach]

IOException is thrown by Source.newBuilder since 0.15 - it cannot occur when doing eval. I'll try to prototype what it would take to replace IOException by a runtime one.