Changes related to issue #731 method lookup regression. by opeongo · Pull Request #732 · beanshell/beanshell

opeongo · 2023-03-14T16:54:15Z

This is only a partial fix because each change uncovered another subtle issue. Still work to be done. I am posting this PR so that someone else who has a better idea of how this is supposed to work can at least see some of the issues that I found.

Fixing the issue for proper matching of the most specific method broke several other code paths that depended on the loose behaviour of the Types.castObject method.
Other issues that popped up that I also fixed included:

array dimensions occurring after a variable name being ignored (hidden problem that had not been tested yet).
arrays of different dimensions but same base type being incorrectly matched as being cast compatible.
Arrays of collections being cast to collections (surfaced by the change to Types.castObject, wasn't a problem before)

Unfortunately as I made changes more issues started to crop up and I just ran out of time. For example

new {{false}, true}; no longer compiles and executes correctly. This looks to me like it shouldn't work anyway, because it is not a proper 2D array.
entries = {{new Entry {"k"=3, "l"=5}, {6}}}; should get typed as Object[][][] but now is getting typed as 'MapEntry[][][]` and this fails.
I suspect that I might have been going down the wrong track with some of my changes. Perhaps there is a simpler way to fix the original problem that does not cause so much other collateral damage.

…a partial fix because each change uncovered another subtle issue. Still work to be done.

sonatype-lift · 2023-03-14T17:02:05Z

src/main/java/bsh/BSHArrayInitializer.java

-                || MapEntry.class == baseType )
-            baseType = Map.class;
+        if (dimensions < 2)
+            if ( MapEntry.class == inferType && Void.TYPE == baseType


OperatorPrecedence: Use grouping parenthesis to make the operator precedence explicit

Suggested change

if ( MapEntry.class == inferType && Void.TYPE == baseType

if ( (MapEntry.class == inferType && Void.TYPE == baseType)

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-14T17:02:08Z

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.¹

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/beanshell/beanshell/732.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/beanshell/beanshell/732.diff | git apply

Once you're satisfied, commit and push your changes in your project.

You can preview the patch by opening the patch URL in the browser. ↩

Adjust test to reflect new error message resulting from change.

codecov · 2023-03-15T22:43:30Z

Codecov Report

Attention: 652 lines in your changes are missing coverage. Please review.

Comparison is base (b1998b1) 74.24% compared to head (c59c722) 74.35%.
Report is 1026 commits behind head on master.

Files	Patch %	Lines
src/main/java/bsh/Interpreter.java	85.20%	50 Missing and 21 partials ⚠️
src/main/java/bsh/BshClassManager.java	65.73%	48 Missing and 13 partials ⚠️
src/main/java/bsh/NameSpace.java	87.87%	40 Missing and 17 partials ⚠️
src/main/java/bsh/ClassGeneratorUtil.java	89.59%	35 Missing and 14 partials ⚠️
src/main/java/bsh/BshMethod.java	76.73%	26 Missing and 21 partials ⚠️
src/main/java/bsh/Name.java	86.76%	20 Missing and 23 partials ⚠️
src/main/java/bsh/LHS.java	74.35%	28 Missing and 12 partials ⚠️
src/main/java/bsh/Invocable.java	83.03%	23 Missing and 15 partials ⚠️
src/main/java/bsh/DelayedEvalBshMethod.java	61.97%	26 Missing and 1 partial ⚠️
src/main/java/bsh/BSHPrimarySuffix.java	84.41%	17 Missing and 7 partials ⚠️
... and 30 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #732      +/-   ##
============================================
+ Coverage     74.24%   74.35%   +0.11%     
- Complexity     3041     3051      +10     
============================================
  Files           108      108              
  Lines          9354     9368      +14     
  Branches       1857     1864       +7     
============================================
+ Hits           6945     6966      +21     
+ Misses         2070     2063       -7     
  Partials        339      339

Flag	Coverage Δ
unittests	`74.35% <87.54%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

opeongo · 2023-03-15T22:58:44Z

I took another look and was able to fix the remaining issues, the fix just had to gestate. By the way new {{false}, true}; was explicitly test and checking the for an error, and my fix changed the error message (I think it made it better). Changing the expected error message fixed that part.

This PR should be ready to merge.

nickl-

More nitpicking than much else, over all looks good. Just worried about arrayElementType...

nickl- · 2023-04-01T11:16:29Z

src/main/java/bsh/BSHFormalParameter.java

+                else
+                    throw new EvalError(
+                       "Array dimensions not allowed on both type and name: "
+                       + name, this, null );


This seems like an easy enough test to add.

Codecov flagged as untested.

nickl- · 2023-04-01T11:18:50Z

src/main/java/bsh/BSHArrayInitializer.java

-            baseType = Map.class;
+        if (dimensions < 2)
+            if ( MapEntry.class == inferType && Void.TYPE == baseType
+                 || MapEntry.class == baseType )


Seems like a fair suggestion, I know the code didn't change but it came under review now.

Reference to Lift's suggestion, now seems to be lost.

OperatorPrecedence: Use grouping parenthesis to make the operator precedence explicit

nickl- · 2023-04-01T11:19:21Z

src/main/java/bsh/BSHArrayInitializer.java

-        if ( MapEntry.class == inferType && Void.TYPE == baseType
-                || MapEntry.class == baseType )
-            baseType = Map.class;
+        if (dimensions < 2)


Why does the dimensions matter again?

It cannot be coerced to a map if there are more than two dimensions

nickl- · 2023-04-01T11:21:28Z

src/main/java/bsh/BSHArrayInitializer.java

        // the values are set.
        dims[0] = jjtGetNumChildren();
        Object array =  Array.newInstance( baseType, dims );
+        Class<?> entryType = array.getClass().getComponentType();


Isn't this what arrayElementType is supposed to do? I am still curious if that is not broken.

Do you mean Types.arrayELementType()? That will return the ultimate base type of the array, so for int[][] it will return int. However here we want the type for the current dimension, which would be int[].

nickl- · 2023-04-01T11:22:55Z

src/main/java/bsh/Types.java

    public static Object castObject( Class<?> toType, Class<?> fromType, Object fromValue,
            int operation, boolean checkOnly ) throws UtilEvalError {
        // assignment to loose type, void type, or exactly same type
-        if ( toType == null || arrayElementType(toType) == arrayElementType(fromType) )


What happens if we fix arrayElementType, I am wondering if this does not cause problems elsewhere...

There is nothing wrong with the way the arrayElementType works, it is just that it is not appropriate here. You cannot just test for equality by comparing the ultimate base types because the number of dimensions might be different.

nickl- · 2023-04-01T11:23:35Z

src/main/java/bsh/Types.java

            int operation, boolean checkOnly ) throws UtilEvalError {
        // assignment to loose type, void type, or exactly same type
-        if ( toType == null || arrayElementType(toType) == arrayElementType(fromType) )
+        if ( toType == null || toType == fromType )


However this condition does satisfy the comments, for what that's worth.

Comment changed

nickl- · 2023-04-01T11:25:18Z

src/test/resources/test-scripts/expression_array.bsh

 assert(isEvalError("Class: Unknow.ObjectType not found in namespace", "new Unknow.ObjectType[0];"));
 assert(isEvalError("Incompatible initializer. Allocation calls for a 2 dimensional array, but initializer is a 1 dimensional array", "new int[][] {};"));
-assert(isEvalError("Incompatible type: boolean in initializer of array type: boolean", "new {{false}, true};"));
+assert(isEvalError("Error in array initializer: Cannot cast primitive value to object type [Z", "new {{false}, true};"));


Agreed more precise error now.

opeongo

Changes made

opeongo · 2024-01-04T22:24:52Z

Hopefully this can be merged

nickl- · 2024-01-06T01:34:58Z

Hopefully this can be merged

Been a while, will need to pick up again to see where we are at.

opeongo · 2024-08-10T20:45:51Z

Has this been merged? If so should be closed.

Changes related to issue #731 method lookup regression. This is only …

82e50e5

…a partial fix because each change uncovered another subtle issue. Still work to be done.

sonatype-lift bot reviewed Mar 14, 2023

View reviewed changes

Fix logic of when to make a collection.

ff499a7

Adjust test to reflect new error message resulting from change.

Update the test for the original error

687f2e4

nickl- requested changes Apr 1, 2023

View reviewed changes

patniemeyer force-pushed the master branch from 10e0089 to b35aac4 Compare December 6, 2023 15:58

patniemeyer force-pushed the fix_731 branch from 5830f75 to 687f2e4 Compare December 6, 2023 15:58

Update from review - no functional changes

c59c722

opeongo commented Jan 4, 2024

View reviewed changes

opeongo mentioned this pull request Jan 5, 2024

Fix issue #753 #754

Merged

	if ( MapEntry.class == inferType && Void.TYPE == baseType
	if ( (MapEntry.class == inferType && Void.TYPE == baseType)

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Conversation

opeongo commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonatype-lift bot Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot commented Mar 14, 2023

🛠 Lift Auto-fix

Footnotes

Uh oh!

codecov bot commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

opeongo commented Mar 15, 2023

Uh oh!

nickl- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opeongo Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opeongo left a comment

Choose a reason for hiding this comment

Uh oh!

opeongo commented Jan 4, 2024

Uh oh!

nickl- commented Jan 6, 2024

Uh oh!

opeongo commented Aug 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

opeongo commented Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

opeongo Jan 4, 2024 •

edited

Loading