EvalError vs dynamic language exception causes

[nickl-]

The purpose of EvalError is designed to abort script when syntax or language errors occur which are considered unrecoverable. These are meant to include the JAVA equivalent of conditions which normally result in compile time errors.

There were arguments raised by @drosenbauer and @opeongo that while BeanShell aims to be a dynamic scripting language it enforces unnecessarily strict conditions by conforming to all the same language parameters as JAVA does. There may be some of the static typed exceptions which should not apply to a dynamic typed language in general.

BeanShell affirmed its policy on the JAVA likeness #501 which clarified that while BeanShell aims to "work like java" it must not be mistaken to also mean that BeanShell will "break like java does". Which implies that should BeanShell not fail like JAVA for the conditions of a certain compile time error it would not be in violation of the policy including where StrictJava compliance is concerned.

For the purpose of finding an appropriate BeanShell centric solution the scope of the problem needs to be clearly defined by producing a comprehensive list of all the causes that previously were considered EvalError, identifying clear distinctions and associate appropriate types which may lead to the allocation of the appropriately typed dynamic language exceptions.

The initial analysis of the previous errors can be collected at the BeanShell EvalError list google docs sheet. Anyone who wants to assist can email me for write access.

[nickl-]

Updated the working spreadsheet to include all throw new EvalError instances in source code.

UtilEvalError also results in EvalError and needs to be mapped as well. Some of the non AST eval errors thrown should probably be UtilEvalError and needs to be investigated.

ReflectError as produced by Reflect.java also appears to result in EvalError which needs to be investigated as well.

Edit: add files from google docs BeanShell EvalError list.xlsx

BeanShell EvalError list - Sheet1.csv

[opeongo]

This is not so bad, your spreadsheet lists that there are only 54 of them. This is not unmanageable.

Is the issue here to go through this list and for each case determine if the error is truly non-recoverable, or if it is not so bad and that if caught then the intepreter could continue? Do I understand this correctly?

If so, then we will need a new type of Exception that is less serious than EvalError that is catchable within scripts. If we could start from scratch I would suggest that we use

• NonrecoverableEvalError instead of the existing EvalError, and
• EvalException for situations that could be dealt with.

However, legacy code... so maybe keep EvalError as is and add EvalException. It is almost too similar to EvalError but it does indicate a lesser severity. (Note that EvalError is actually a subclass of Exception, so out of whack from the beginning).

Can I get write access to the spreadsheet to add some annotations to it? How can I pm you my email address?

[nickl-]

@opeongo I can agree with your assessment of EvalError should remain and adding EvalException as recoverable evaluation exceptions.

You can drop me a mail: github at jigsoft dot co.za

Please reference this issue in your mail.

[nickl-]

From #262

The parser error is a separate issue, this is valid java and should parse.

What the OP is actually after is for beanshell to report an incomparable types error.

Ex:

jshell> new Double(1) == "test"
|  Error:
|  incomparable types: java.lang.Double and java.lang.String
|  new Double(1) == "test"
|  ^---------------------^

Bad operand types may also suffice:

jshell> 1 == "test"
|  Error:
|  bad operand types for binary operator '=='
|    first type:  int
|    second type: java.lang.String
|  1 == "test"
|  ^---------^

Edit: Although this treads close to "will not fail like java does"

Added: Yes this falls under won't break like JAVA because the following is valid BeanShell code

BeanShell 3.0.0-SNAPSHOT.2403
BigInteger.ONE == 1

--> $0 = true :boolean

[opeongo]

I am getting started to have a look at this one. As mentioned previously the approach will be:

• create a new exception named EvalException. This will be used for exceptions arising within BeanShell that do not corrupt interpreter state and where recovery within the calling script is possible.
• examine each location where EvalError is currently being thrown and asses the impact of the error. For example, a name not being found during a lookup does not corrupt the interpreter. It does mean that evaluation of the current statement/block cannot continue. Annotate the list of source locations with the result of the assessents.
• change EvalError to EvalException in these locations.
• change the handling within BSHTryStatement.java so that catch and finally blocks are run for EvalException's

[opeongo]

I made some progress working through the working spreadsheet of EvalError sites. I was working off of the master branch so a few of the call sites and locations have changed. It is slow but easy going to have a look at what is happening at each site.

Many of the call sites are handling runtime issues of syntactical or semantic errors, or name lookup misses as the code is being dynamically executed. For these I have added a column to the spreadsheet with a snippet of code to trigger the error. It is easy to see what is happening: the dynamic evaluation comes across an error (e.g. name not found) and processing of the node cannot continue so an exception is thrown. Nothing else is wrong in the interpreter. These are the sites that can be converted in to recoverable exceptions.

Other call sites are covering issues that cannot normally be triggered by user code. I suspect that some of them cover conditions that will never occur. For example, in BSHMethod there is a test to make sure not to invoke an abstract method, but I cannot figure out a way to trick the interpreter into doing this. There are also exceptions guarding locations that cannot be reached, like BSHArrayInitializer.eval(). There are many other sites guarding against utility methods throwing exceptions which normally should not happen. These are the sites that should be left alone as non-recoverable exceptions.

Since this will only changed audited sites there is no real big damage if I miss some recoverable problems on this first go around. In other words, if I don't capture 100% of the recoverable errors it is no big loss. These can always be picked up and added later. The thing to watch for is to not mark any non-recoverable errors a recoverable. So false negatives are ok, but false positives are not acceptable.

So for the handful of confirmed site where the node cannot be evaluated but nothing else is wrong we can change the EvalError to be an EvalException.

The signature for Node.eval() is

    Object eval(CallStack callstack, Interpreter interpreter) throws EvalError;

The easiest way to make this conversion is to make EvalException extend EvalError, then the signatures throughout the code base do not have to change.

Then in BSHTryStatement add a catch for EvalException and allow this to be handled in BeanShell code. EvalError exceptions will be handled as before.

Any comments so far?

[opeongo]

I submitted a PR that handles most of the cases listed in the spreadsheet. This is pretty low impact PR as you can see from the diffs.

A couple of notes:

1. A few of the cases were not appropriate to be converted from errors to exceptions. These often cover sanity checks that probably wont ever get triggers. For example, testing if trying to invoke an abstract method. I think that this would require an instantiated abstract class, so impossible? Maybe if a method lookup went wrong, but that would mean an error in BeanShell. It is best to leave these ones as EvalError's because if they ever do trigger it is something extraordinary.
2. There are many cases where UtilEvalError's are converted to EvalError and I did not look in to all of these. It is quite likely that there are many more cases that could be reliably be handled as EvalException rather than as EvalError. I think we should see how this first iteration works in practice before beginning another sweep for more cases.

[nickl-]

Awesome great job, thanks for the work you put into this. I am merging #758 but if I understand correctly we are leaving this open for a round 2? Please advise...