beanshell / beanshell

Beanshell scripting language
Apache License 2.0
815 stars 183 forks source link

Cannot create array using Integer as dimension #652

Closed opeongo closed 1 year ago

opeongo commented 1 year ago

BeanShell will not unbox and widen Integer objects when instantiating arrays. For example this will fail:

i = Integer.valueOf(3);
a = new int[i];

This is allowed in Java, and should be also allowed in BeanShell.

I have a trivial fix for this in my fork but I cannot push it in because of conflicts.

nickl- commented 1 year ago

It looks like 3313d2b has everything needed for this without any dependencies on the operator preferences issue, could we not look at that separately?

Is Byte, Short and Int the only boxed types we should consider? Won't it be easier to test for instance of Number or not instance of Primitive perhaps, to allow for all types. Or am I missing something?

opeongo commented 1 year ago

Yes, this is a stand alone change that does not depend on anything else.

The maximum size of an array is 2^31-1, so Java only allows int values. byte and short will be widened to an int. long will throw an error because narrowing might lose precision. float and double are not integral valued and are a code smell. it would be a better idea to throw a syntax error than silently truncate. If you want to use a long, float or double then explicitly cast to an int.

The same logic applies to the primitive case... should throw an error on long, float or double. Not trying to make a nanny programming model, but using these data types is a stinky code smell and that probably indicates something is wrong.

Sometimes having a few additional guards is a better idea than letting people run with scissors.

nickl- commented 1 year ago

Yes, this is a stand alone change that does not depend on anything else.

Good, so we can get it to a PR, awesome.

The maximum size of an array is 2^31-1, so Java only allows int values.

Correct and we are giving it Number.intValue() so that is perfect, regardless of the type of number, no need to cast.

[snip ... widened ...] long will throw an error because narrowing might lose precision [ snip ... not an intValue]

The reason for the issue is that Integer throws an error, right? If we don't include Long, or the any of the others, it will throw an error, just like Integer now does.

Since we are supporting auto widening it may very well happen that your int is already a long, and if you are using a Float or Double then you know the risk of precision. Bottom line, as long as your value is less than Integer.MAX_VALUE it works as expected, and won't generate an error.

Sometimes having a few additional guards is a better idea than letting people run with scissors.

We can still run valid java just like java does. That is our first goal. After that there are many kewl scissors to run around with. =)

If we are to hide scissors it will fall under strict mode implementation, meaning additional checks after we allowed everything. Can't remember but I have a feeling tests and fixes for strict mode is currently incomplete. You can add it if you want but I would just keep it simple for now, perhaps strict mode is not even a priority anymore.

jimjag commented 1 year ago

I'm not seeing a PR to fold this in.

nickl- commented 1 year ago

I'm not seeing a PR to fold this in.

Open pull request #658 thanx @jimjag

nickl- commented 1 year ago

Final note on allowing the other boxed types, this will conform to the existing Primitive assignment which also allows any primitive as array index.

nickl- commented 1 year ago

Found the BeanShell policy statement #501 RE: BeanShell doesn't fail like java

Excerpt:

The fact of the matter is BeanShell is not the same as java. For starters there is no compiler so no compile time warnings or type parameter assignments. Syntax problems begets parser errors which has no similarity or comparison to any faults reported in java. Even when BeanShell produces similar runtime exceptions to those found in java, these should be considered as being for informational purposes only. BeanShell does not make any attempt to emulate the way java fails. In some cases BeanShell may even choose not to fail by design. Just because it fails in java does not mean it is expected to fail in BeanShell. It is the developers responsibility to ensure that the code doesn't fail and BeanShell will attempt to work like it does in java.

nickl- commented 1 year ago

Sometimes having a few additional guards is a better idea than letting people run with scissors.

My sincere apologies, time makes memory foggy, and lots of time has passed again. The uptake time is going to be significant! I just re-read #71 and realized we already have overflow protection for narrowing Number types.

That said we also have the unwrapping Primitives and type checking for Number done already so no need for checking instanceof.

The change can be implemented with:

definedDimensions[i] = (int) Primitive.castWrapper(Integer.TYPE, length);