beanshell / beanshell

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

Changes related to issue #731 method lookup regression. #732

Open opeongo opened 1 year ago

opeongo commented 1 year ago

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:

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

sonatype-lift[bot] commented 1 year ago

🛠 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.[^1]

# 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. [^1]: You can preview the patch by opening the patch URL in the browser.

codecov[bot] commented 1 year ago

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 :warning:
src/main/java/bsh/BshClassManager.java 65.73% 48 Missing and 13 partials :warning:
src/main/java/bsh/NameSpace.java 87.87% 40 Missing and 17 partials :warning:
src/main/java/bsh/ClassGeneratorUtil.java 89.59% 35 Missing and 14 partials :warning:
src/main/java/bsh/BshMethod.java 76.73% 26 Missing and 21 partials :warning:
src/main/java/bsh/Name.java 86.76% 20 Missing and 23 partials :warning:
src/main/java/bsh/LHS.java 74.35% 28 Missing and 12 partials :warning:
src/main/java/bsh/Invocable.java 83.03% 23 Missing and 15 partials :warning:
src/main/java/bsh/DelayedEvalBshMethod.java 61.97% 26 Missing and 1 partial :warning:
src/main/java/bsh/BSHPrimarySuffix.java 84.41% 17 Missing and 7 partials :warning:
... and 30 more
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/beanshell/beanshell/pull/732/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=beanshell) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/beanshell/beanshell/pull/732/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=beanshell) | `74.35% <87.54%> (+0.11%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=beanshell#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

opeongo commented 1 year ago

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.

opeongo commented 4 months ago

Hopefully this can be merged

nickl- commented 4 months ago

Hopefully this can be merged

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