Convert lambdas to method references when javac compiles a method reference to a lambda#532
Convert lambdas to method references when javac compiles a method reference to a lambda#532coehlrich wants to merge 4 commits into
Conversation
sschr15
left a comment
sschr15
left a comment
There was a problem hiding this comment.
I have some small nitpicks, but overall it looks good to me.
manual bytecode parsing is scary
| && newExprent.isLambda() | ||
| && !newExprent.isMethodReference()) { | ||
| if (stat.getTopParent().mt.getName().contains("<init>")) { | ||
| System.out.println(); |
| argument++; | ||
| } | ||
|
|
||
| if (next == null) |
There was a problem hiding this comment.
This should never be true (FullInstructionSequence seems to only ever have non-null instructions). Did you mean !iterator.hasNext()?
| } | ||
|
|
||
| private static boolean convertToMethodReference(Statement stat, int i, Exprent exp) throws IOException { | ||
| if (exp instanceof NewExprent newExprent |
There was a problem hiding this comment.
It's probably reasonable to flip this if to reduce a layer of nesting
|
|
||
| public class MethodReferenceHelper { | ||
| public static boolean convertToMethodReference(RootStatement root) throws IOException { | ||
| return convertToMethodReferenceRec(root); |
There was a problem hiding this comment.
This appears to also modify existing lambdas which could be represented with a method reference. Perhaps it should be behind a decompiler option
| method.expandData(struct); | ||
| FullInstructionSequence seq = method.getInstructionSequence(); | ||
| Iterator<Instruction> iterator = seq.iterator(); | ||
| Instruction next = null; |
jaskarth
left a comment
jaskarth
left a comment
There was a problem hiding this comment.
Thanks a lot for working on this! Overall, I don't really understand the approach taken here. I'll look at the generated code and see if there might be a better way.
| public Optional<String> field3 = Optional.of("").map(s -> s + "3");// 12 | ||
| public Stream<String> field4 = Stream.of("1", "2").sorted(Comparator.<String, Integer>comparing(s -> s.length()).thenComparing(i -> i.toString()));// 13 14 | ||
| public Comparator<String> field5 = Comparator.comparing(String::length).thenComparing(i -> i.toString());// 15 | ||
| public Stream<String> field4 = Stream.of("1", "2").sorted(Comparator.<String, Integer>comparing(s -> s.length()).thenComparing(String::toString));// 13 14 |
There was a problem hiding this comment.
I believe this output is wrong. In the original source, these are regular lambdas and not references.
There was a problem hiding this comment.
They would have to match the lambda that javac generates when converting a method reference to a lambda.
I could require the variable to exist but then it would only work for java 25.
| argument++; | ||
| } | ||
|
|
||
| boolean varargs = false; |
There was a problem hiding this comment.
The varargs variable is used for checking if the amount of method parameters of the lambda is what is expected.
| StructMethod method = struct.getMethod(info.content_method_key); | ||
| if (!method.hasModifier(CodeConstants.ACC_STATIC)) | ||
| return false; | ||
| method.expandData(struct); |
There was a problem hiding this comment.
Hmm, does this need to manually parse the instructions or would going through the exprents suffice?
There was a problem hiding this comment.
The ClassWrapper on the ClassNode isn't set at this point and even if it was the exprents for lambdas are almost certainly not generated at this point since they are the last methods in the class file.
I did consider doing it when the lambda is written to the output like with array constructor references but that would prevent inlining the variable since if it happened when the lambda is written nothing in codeToJava would see that the variable assignment is removed whereas if the variable is removed during codeToJava then there I don't think there would be anyway for the variable to be inlined correctly since lambdas can't call a method on an instance without the instance being referenced via a variable.
|
Any updates on this PR? As the last comment is from over 4 months ago and this still doesn’t have any assigned priority and well, the changes made here are somewhat crucial for proper decompilation of bytecode emitted by java 25+ |
jaskarth
added
Type: Enhancement
|
The fix for the synthetic variable inlining has been pushed separately to develop/1.12. The fix for converting lambdas to method references needs further thought on coming up with a fix that doesn't impact non-reference lambdas. |
4 participants
Converts lambdas to method references when javac compiles a method reference to a lambda, removes synthetic instance variable, and removes
Objects.requireNonNull.