Lambda support for BeanShell by Net-0 · Pull Request #766 · beanshell/beanshell

Net-0 · 2024-07-07T00:47:51Z

Hello @nickl- , here I'm again 😁

This is probably my last PR, but it's a great one.

Introduction

The basic idea of this PR is implement support for lambdas by lambda expressions and method references, as in the snippet bellow for reference:

import java.util.function.Supplier;

Runnable lambda1 = () -> {};
Supplier<String> lambda2 = myObj::toString;

I read entirely the issue 645, and here I'm with my solution to this issue :D

Implementation

The implementation is kinda tricky, so I'll first explain the main idea and after explain the details.

The basic idea is that lambda expressions and method references must return an instance of bsh.BshLambda, and when we use this lambda ( call some method, set to a variable, etc... ) we convert this bsh.BshLambda to an instance of that specific functional interface, and we do it basically calling the method bsh.BshLambda.convertTo(Class) ( I'll explain this method but you can see the method implementation here ).

Here is a BeanShell snippet showing what I'm talking about:

var lambda1 = () -> System.out.println("Hello World :D"); // Create an instance of bsh.BshLambda and store in a dynamic variable

Runnable runnable = lambda1; // Convert from bsh.BshLambda to java.lang.Runnable

new Thread(lambda1); // Convert from bsh.BshLambda to java.lang.Runnable and call java.lang.Thread constructor

The code above is a step-by-step code, but it also works:

// Create the bsh.BshLambda instance and already convert to java.lang.Runnable and then call java.lang.Thread constructor
new Thread(() -> System.out.println("Hello World :D"));

Okay, but you might be thinking "how the bsh.BshLambda.convertTo() works ?", and here we go. The implementation idea is the tricky part as I already said, because we have a instance of bsh.BshLambda, so we can just call bsh.BshLambda.invoke() to run the lambda, but unfortunatelly Java is strong typed, so we must have a strict type to lambda works, so to solve that, we make a wrapper of bsh.BshLambda that implements our desired functional interface.

Here is a code snippet showing the idea of wrapper:

import java.lang.Runnable;

public class MyRunnable implements Runnable {
    private BshLambda bshLamba;

    public MyClass(BshLambda bshLambda) {
        this.bshLambda = bshLambda;
    }

    public void run() {
        this.bshLambda.invoke(new Object[0], new Class[0]);
    }
}

So, basically, the method bsh.BshLambda.convertTo() create the wrapper class using ASM and then the method return an instance of the wrapper.
Note: I used ASM to created the class byte code and load it in JVM because this way seems more lightweight than using java.lang.invoke.LambdaMetafactory.

It's the basic explanation about how I implemented this Lambda Support, but there is a bunch of details that I did to make this Lambda Support "java strict" that would take a bunch of paragraphs to explain, so if you wanna know all features of this Lambda Support, you can read the test file because I did JUnit tests to them all 😅

The "method overload with lambda types alike" issue

This lambda support works really well, but there is just one case that it wouldn't work as clean as in Java, that is in method overload with lambda types where the difference is just the lambda return type. Here is a code showing this:

@FunctionalInterface
interface ListSupplier {
    List<?> getList();
}

@FunctionalInterface
interface MapSupplier {
    Map<?, ?> getMap();
}

class MyClass {
    static void doSomething(ListSupplier supplier) {} // first method
    static void doSomething(MapSupplier supplier) {} // second method
}

MyClass.doSomething(() -> new ArrayList<>()); // Will call the first method
MyClass.doSomething(() -> new HashMap<>()); // Will also call the first method, because the code can't know that the lambda body return is just valid for the second method

But there are some disclaimers:

It can be solved with casts, so (MapSupplier) () -> new HashMap<>() would solve this issue
It's a lambda expression problem, method references works 100%, so using HashMap::new would also solve this problem
It's a really rare case, and to be honest, I'd never seen any method in any real code with this specific kind of overload, I just discovered this problem while I was testing the standard java lambda behavior.
It's not impossible to fix that, but to fix that, we would have to re-implement the .eval() method for all nodes in a different way where we don't wanna really evaluate the code, but instead, we wanna evaluate the return type of the code, but it's a HUGE PROBLEM to solve, I tried but it's too much work, so being honest, I don't wanna solve that because of the previous disclaimers: it's an extremelly rare scenario and there is simple workarrounds to solve that.

Tests and Code coverage

The code coverage is 100%, I make all kind of tests to ensure that the behaviour is as in standard Java.
Note: I did ~1400 lines of tests 🫠

Conclusion

I guess that this Lambda Support is really good, and will be perfect for users, and if you see something wrong or that isn't that good, I can fix that, but it's probably my last PR 😢, because I shoud be using my free time after my job to study to go to college/university :V

after writing this big text, now I'll waiting your response 😊

Note: my PR for Security Guard is also waiting for you ;)

Net-0 · 2024-07-29T00:57:03Z

Update: I migrated the tests of src/test/resources/test-scripts/casts.bsh to src/test/java/bsh/CastTest.java, and I also added tests for arrays and matrix, because there is no one and I noticed that while I was implementing the Lambda grammar parser, so I guess that it's important to add.

nickl- · 2024-08-10T13:34:15Z

Wow!! Very nice!!

This is going to take a while to go through but it seem to be working with everything I throw at it. =)

I moved your discussion on implementation to #675 where we can talk more in detail.

Will keep PR related stuff here.

nickl- · 2024-08-10T13:38:27Z

The grammar is not happy. We will need to massage that until it is and also get both the slices and lambdas to be happy.

Reading from file /home/intriazen/git/beanshell-securityguard/target/javacc-1723296930414/node/bsh.jj ...
Warning: Choice conflict in [...] construct at line 798, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "="
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in [...] construct at line 817, column 42.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "?"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 825, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "??"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 834, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "||"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 843, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "&&"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 852, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "|"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 861, column 7.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "^"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 870, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "&"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 879, column 6.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "=="
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in [...] construct at line 889, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "instanceof"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 896, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "<"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 906, column 3.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "<<"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 916, column 4.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "+"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 925, column 6.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "*"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict in (...)* construct at line 934, column 6.
         Expansion nested within construct and expansion following construct
         have common prefixes, one of which is: "**"
         Consider using a lookahead of 2 or more for nested expansion.
Warning: Choice conflict involving two expansions at
         line 983, column 5 and line 983, column 15 respectively.
         A common prefix is: "{" <IDENTIFIER>
         Consider using a lookahead of 3 or more for earlier expansion.

nickl- · 2024-08-10T13:44:34Z

Update: I migrated the tests of src/test/resources/test-scripts/casts.bsh to src/test/java/bsh/CastTest.java, and I also added tests for arrays and matrix, because there is no one and I noticed that while I was implementing the Lambda grammar parser, so I guess that it's important to add.

Not sure why you did this, I see the old test case (casts.bsh) still succeeds.

Would be nice if we can start some lambda tests in a test-script for reference purposes. This will not be code coverage tests per se but useful for reference and documentation supplements.

nickl- · 2024-08-10T13:47:05Z

I shoud be using my free time after my job to study to go to college/university :V

Be grateful you still have such a thing as free time =)

Thank you for your efforts this is awesome!!

nickl- · 2024-08-10T14:05:00Z

I not we have conflicts now, may need to rebase from fresh HEAD

codecov · 2024-08-17T22:30:12Z

Codecov Report

Attention: Patch coverage is 96.37462% with 12 lines in your changes missing coverage. Please review.

Project coverage is 75.66%. Comparing base (0ed20ed) to head (d412c42).
Report is 31 commits behind head on master.

Files	Patch %	Lines
src/main/java/bsh/RuntimeEvalError.java	55.55%	8 Missing ⚠️
src/main/java/bsh/Variable.java	40.00%	2 Missing and 1 partial ⚠️
src/main/java/bsh/NameSpace.java	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #766      +/-   ##
============================================
+ Coverage     74.26%   75.66%   +1.39%     
- Complexity     3114     3196      +82     
============================================
  Files           112      115       +3     
  Lines          9572     9872     +300     
  Branches       1880     1931      +51     
============================================
+ Hits           7109     7470     +361     
+ Misses         2115     2056      -59     
+ Partials        348      346       -2

Flag	Coverage Δ
unittests	`75.66% <96.37%> (+1.39%)`	⬆️

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.

Net-0 · 2024-08-31T21:03:50Z

Just some updates:

The grammar was fixed to support method references and also arrays slices
I restored the old cast tests ( src/test/resources/test-scripts/casts.bsh ) and added new tests inside the file; Note: I removed the file because I guessed that tests at .bsh files are legacy tests xD
I rebased the branch to remove the conflicts

Net-0 · 2024-08-31T21:14:29Z

I'm also working in some things right now:

Static method reference to non-static methods

I forgot to implement a support for some things like Object::toString xD, but the main problem here is with advanced type validation for some cases like List::toString, and actually I'm struggling with the classes that are being generated by BeanShell because they have a poor signature ( no throws, no generics, etc... ), and I'll probably need to fix those signature issues

Grammar warnings

I fixed the gramar problem of method references and slices but I still didn't see why there is so many warning messages in the console, I'm going to see that after resolving the issues with method references and signature stuff.

SecurityGuard

It's probably the simplest thing to do in this list, but explaining, I'm going to add support of Lambda into SecurityGuard; I mean, a call for bsh,BshLambda.convertTo() is technically creating a instance for a Functional Interface, so I need to validate if there is some security block for that, an example is that some projects wanna to disable the creation of some lambdas as java.lang.Runnable for some reasons.

Code fixes and improvements

I'm improving the tests too and also reviewing the code to make some improvements, an example is removing all 'invoke' methods of bsh,BshLambda and just have one, because the method must be public so if we just have a single public method, I guess that it's better because it's less contract to maintain.

Net-0 force-pushed the lambda-support branch 2 times, most recently from 28e4f75 to 22f6f33 Compare July 7, 2024 01:12

nickl- mentioned this pull request Aug 10, 2024

Implementing lambdas #675

Open

Net-0 force-pushed the lambda-support branch from 08310cc to 6a3b8e1 Compare August 17, 2024 18:47

Patrick Niemeyer and others added 10 commits August 17, 2024 15:53

*** empty log message ***

52e5bec

*** empty log message ***

0f6933a

*** empty log message ***

28c899f

Lambda Support

f7fc761

Style fix: removing trailing spaces

91ed326

Small style fix

fa46087

Migrating cast tests + Adding tests for casting arrays and matrix

9bebd2c

fix: array slice parsing

c9ffbd6

fixing spaces

6b8b00d

rework in casts tests

4b42ffc

Net-0 force-pushed the lambda-support branch from 6a3b8e1 to 4b42ffc Compare August 17, 2024 18:56

style fix

d412c42

Saving progress, needing to fix bsh generated classes

17b38e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lambda support for BeanShell#766

Lambda support for BeanShell#766
Net-0 wants to merge 12 commits intobeanshell:masterfrom
Net-0:lambda-support

Net-0 commented Jul 7, 2024 •

edited

Loading

Uh oh!

Net-0 commented Jul 29, 2024

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

nickl- commented Aug 10, 2024 •

edited

Loading

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

codecov bot commented Aug 17, 2024 •

edited

Loading

Uh oh!

Net-0 commented Aug 31, 2024

Uh oh!

Net-0 commented Aug 31, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Net-0 commented Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduction

Implementation

The "method overload with lambda types alike" issue

Tests and Code coverage

Conclusion

Uh oh!

Net-0 commented Jul 29, 2024

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

nickl- commented Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

nickl- commented Aug 10, 2024

Uh oh!

codecov bot commented Aug 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Net-0 commented Aug 31, 2024

Uh oh!

Net-0 commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Static method reference to non-static methods

Grammar warnings

SecurityGuard

Code fixes and improvements

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Net-0 commented Jul 7, 2024 •

edited

Loading

nickl- commented Aug 10, 2024 •

edited

Loading

codecov bot commented Aug 17, 2024 •

edited

Loading

Net-0 commented Aug 31, 2024 •

edited

Loading