SpongePowered
Big Trouble in Little Lambda
So far, a majority of use cases for injections have been in traditional Java code, be it explicit methods, or surrogated methods based on some alteration of the target injection point, but it's only come recently where I've wanted to target a lambda method.
There's potential for having a lambda being off limits, but in the context of an entire method block being within a lambda, the last resort ends up being a wildcarded method target with somewhat minimal guarantee of what is being transformed.
A clear example is the following method from forge:
private static boolean loadAdvancements(Map<ResourceLocation, Advancement.Builder> map, ModContainer mod)
{
return CraftingHelper.findFiles(mod, "assets/" + mod.getModId() + "/advancements", null,
(root, file) ->
{
String relative = root.relativize(file).toString();
if (!"json".equals(FilenameUtils.getExtension(file.toString())) || relative.startsWith("_"))
return true;
String name = FilenameUtils.removeExtension(relative).replaceAll("\\\\", "/");
ResourceLocation key = new ResourceLocation(mod.getModId(), name);
if (!map.containsKey(key))
{
BufferedReader reader = null;
try
{
reader = Files.newBufferedReader(file);
Advancement.Builder builder = JsonUtils.fromJson(AdvancementManager.GSON, reader, Advancement.Builder.class);
map.put(key, builder);
}
catch (JsonParseException jsonparseexception)
{
FMLLog.log.error("Parsing error loading built-in advancement " + key, (Throwable)jsonparseexception);
return false;
}
catch (IOException ioexception)
{
FMLLog.log.error("Couldn't read advancement " + key + " from " + file, (Throwable)ioexception);
return false;
}
finally
{
IOUtils.closeQuietly(reader);
}
}
return true;
},
true, true
);
}
Translates to the following in bytecode:
// access flags 0xA
// signature (Ljava/util/Map<Lnet/minecraft/util/ResourceLocation;Lnet/minecraft/advancements/Advancement$Builder;>;Lnet/minecraftforge/fml/common/ModContainer;)Z
// declaration: boolean loadAdvancements(java.util.Map<net.minecraft.util.ResourceLocation, net.minecraft.advancements.Advancement$Builder>, net.minecraftforge.fml.common.ModContainer)
private static loadAdvancements(Ljava/util/Map;Lnet/minecraftforge/fml/common/ModContainer;)Z
L0
LINENUMBER 1351 L0
ALOAD 1
NEW java/lang/StringBuilder
DUP
INVOKESPECIAL java/lang/StringBuilder.<init> ()V
LDC "assets/"
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
ALOAD 1
INVOKEINTERFACE net/minecraftforge/fml/common/ModContainer.getModId ()Ljava/lang/String; (itf)
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
LDC "/advancements"
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
ACONST_NULL
ALOAD 1
ALOAD 0
INVOKEDYNAMIC apply(Lnet/minecraftforge/fml/common/ModContainer;Ljava/util/Map;)Ljava/util/function/BiFunction; [
// handle kind 0x6 : INVOKESTATIC
java/lang/invoke/LambdaMetafactory.metafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
// arguments:
(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;,
// handle kind 0x6 : INVOKESTATIC
net/minecraftforge/common/ForgeHooks.lambda$loadAdvancements$0(Lnet/minecraftforge/fml/common/ModContainer;Ljava/util/Map;Ljava/nio/file/Path;Ljava/nio/file/Path;)Ljava/lang/Boolean;,
(Ljava/nio/file/Path;Ljava/nio/file/Path;)Ljava/lang/Boolean;
]
ICONST_1
ICONST_1
INVOKESTATIC net/minecraftforge/common/crafting/CraftingHelper.findFiles (Lnet/minecraftforge/fml/common/ModContainer;Ljava/lang/String;Ljava/util/function/Function;Ljava/util/function/BiFunction;ZZ)Z
IRETURN
L1
LOCALVARIABLE map Ljava/util/Map; L0 L1 0
// signature Ljava/util/Map<Lnet/minecraft/util/ResourceLocation;Lnet/minecraft/advancements/Advancement$Builder;>;
// declaration: map extends java.util.Map<net.minecraft.util.ResourceLocation, net.minecraft.advancements.Advancement$Builder>
LOCALVARIABLE mod Lnet/minecraftforge/fml/common/ModContainer; L0 L1 1
MAXSTACK = 6
MAXLOCALS = 2
// access flags 0x100A
private static synthetic lambda$loadAdvancements$0(Lnet/minecraftforge/fml/common/ModContainer;Ljava/util/Map;Ljava/nio/file/Path;Ljava/nio/file/Path;)Ljava/lang/Boolean;
TRYCATCHBLOCK L0 L1 L2 com/google/gson/JsonParseException
TRYCATCHBLOCK L0 L1 L3 java/io/IOException
TRYCATCHBLOCK L0 L1 L4 null
TRYCATCHBLOCK L2 L5 L4 null
TRYCATCHBLOCK L3 L6 L4 null
TRYCATCHBLOCK L4 L7 L4 null
L8
LINENUMBER 1355 L8
ALOAD 2
ALOAD 3
INVOKEINTERFACE java/nio/file/Path.relativize (Ljava/nio/file/Path;)Ljava/nio/file/Path; (itf)
INVOKEINTERFACE java/nio/file/Path.toString ()Ljava/lang/String; (itf)
ASTORE 4
L9
LINENUMBER 1356 L9
LDC "json"
ALOAD 3
INVOKEINTERFACE java/nio/file/Path.toString ()Ljava/lang/String; (itf)
INVOKESTATIC org/apache/commons/io/FilenameUtils.getExtension (Ljava/lang/String;)Ljava/lang/String;
INVOKEVIRTUAL java/lang/String.equals (Ljava/lang/Object;)Z
IFEQ L10
ALOAD 4
LDC "_"
INVOKEVIRTUAL java/lang/String.startsWith (Ljava/lang/String;)Z
IFEQ L11
L10
LINENUMBER 1357 L10
FRAME APPEND [java/lang/String]
ICONST_1
INVOKESTATIC java/lang/Boolean.valueOf (Z)Ljava/lang/Boolean;
ARETURN
L11
LINENUMBER 1359 L11
FRAME SAME
ALOAD 4
INVOKESTATIC org/apache/commons/io/FilenameUtils.removeExtension (Ljava/lang/String;)Ljava/lang/String;
LDC "\\\\"
LDC "/"
INVOKEVIRTUAL java/lang/String.replaceAll (Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;
ASTORE 5
L12
LINENUMBER 1360 L12
NEW net/minecraft/util/ResourceLocation
DUP
ALOAD 0
INVOKEINTERFACE net/minecraftforge/fml/common/ModContainer.getModId ()Ljava/lang/String; (itf)
ALOAD 5
INVOKESPECIAL net/minecraft/util/ResourceLocation.<init> (Ljava/lang/String;Ljava/lang/String;)V
ASTORE 6
L13
LINENUMBER 1362 L13
ALOAD 1
ALOAD 6
INVOKEINTERFACE java/util/Map.containsKey (Ljava/lang/Object;)Z (itf)
IFNE L14
L15
LINENUMBER 1364 L15
ACONST_NULL
ASTORE 7
L0
LINENUMBER 1368 L0
ALOAD 3
INVOKESTATIC java/nio/file/Files.newBufferedReader (Ljava/nio/file/Path;)Ljava/io/BufferedReader;
ASTORE 7
L16
LINENUMBER 1369 L16
GETSTATIC net/minecraft/advancements/AdvancementManager.GSON : Lcom/google/gson/Gson;
ALOAD 7
LDC Lnet/minecraft/advancements/Advancement$Builder;.class
INVOKESTATIC net/minecraft/util/JsonUtils.fromJson (Lcom/google/gson/Gson;Ljava/io/Reader;Ljava/lang/Class;)Ljava/lang/Object;
CHECKCAST net/minecraft/advancements/Advancement$Builder
ASTORE 8
L17
LINENUMBER 1370 L17
ALOAD 1
ALOAD 6
ALOAD 8
INVOKEINTERFACE java/util/Map.put (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; (itf)
POP
L1
LINENUMBER 1384 L1
ALOAD 7
INVOKESTATIC org/apache/commons/io/IOUtils.closeQuietly (Ljava/io/Reader;)V
L18
LINENUMBER 1385 L18
GOTO L14
L2
LINENUMBER 1372 L2
FRAME FULL [net/minecraftforge/fml/common/ModContainer java/util/Map java/nio/file/Path java/nio/file/Path java/lang/String java/lang/String net/minecraft/util/ResourceLocation java/io/BufferedReader] [com/google/gson/JsonParseException]
ASTORE 8
L19
LINENUMBER 1374 L19
GETSTATIC net/minecraftforge/fml/common/FMLLog.log : Lorg/apache/logging/log4j/Logger;
NEW java/lang/StringBuilder
DUP
INVOKESPECIAL java/lang/StringBuilder.<init> ()V
LDC "Parsing error loading built-in advancement "
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
ALOAD 6
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
ALOAD 8
INVOKEINTERFACE org/apache/logging/log4j/Logger.error (Ljava/lang/String;Ljava/lang/Throwable;)V (itf)
L20
LINENUMBER 1375 L20
ICONST_0
INVOKESTATIC java/lang/Boolean.valueOf (Z)Ljava/lang/Boolean;
ASTORE 9
L5
LINENUMBER 1384 L5
ALOAD 7
INVOKESTATIC org/apache/commons/io/IOUtils.closeQuietly (Ljava/io/Reader;)V
L21
LINENUMBER 1375 L21
ALOAD 9
ARETURN
L3
LINENUMBER 1377 L3
FRAME SAME1 java/io/IOException
ASTORE 8
L22
LINENUMBER 1379 L22
GETSTATIC net/minecraftforge/fml/common/FMLLog.log : Lorg/apache/logging/log4j/Logger;
NEW java/lang/StringBuilder
DUP
INVOKESPECIAL java/lang/StringBuilder.<init> ()V
LDC "Couldn't read advancement "
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
ALOAD 6
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
LDC " from "
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
ALOAD 3
INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
ALOAD 8
INVOKEINTERFACE org/apache/logging/log4j/Logger.error (Ljava/lang/String;Ljava/lang/Throwable;)V (itf)
L23
LINENUMBER 1380 L23
ICONST_0
INVOKESTATIC java/lang/Boolean.valueOf (Z)Ljava/lang/Boolean;
ASTORE 9
L6
LINENUMBER 1384 L6
ALOAD 7
INVOKESTATIC org/apache/commons/io/IOUtils.closeQuietly (Ljava/io/Reader;)V
L24
LINENUMBER 1380 L24
ALOAD 9
ARETURN
L4
LINENUMBER 1384 L4
FRAME SAME1 java/lang/Throwable
ASTORE 10
L7
ALOAD 7
INVOKESTATIC org/apache/commons/io/IOUtils.closeQuietly (Ljava/io/Reader;)V
L25
LINENUMBER 1385 L25
ALOAD 10
ATHROW
L14
LINENUMBER 1388 L14
FRAME CHOP 1
ICONST_1
INVOKESTATIC java/lang/Boolean.valueOf (Z)Ljava/lang/Boolean;
ARETURN
L26
LOCALVARIABLE builder Lnet/minecraft/advancements/Advancement$Builder; L17 L1 8
LOCALVARIABLE jsonparseexception Lcom/google/gson/JsonParseException; L19 L3 8
LOCALVARIABLE ioexception Ljava/io/IOException; L22 L4 8
LOCALVARIABLE reader Ljava/io/BufferedReader; L0 L14 7
LOCALVARIABLE mod Lnet/minecraftforge/fml/common/ModContainer; L8 L26 0
LOCALVARIABLE map Ljava/util/Map; L8 L26 1
LOCALVARIABLE root Ljava/nio/file/Path; L8 L26 2
LOCALVARIABLE file Ljava/nio/file/Path; L8 L26 3
LOCALVARIABLE relative Ljava/lang/String; L9 L26 4
LOCALVARIABLE name Ljava/lang/String; L12 L26 5
LOCALVARIABLE key Lnet/minecraft/util/ResourceLocation; L13 L26 6
MAXSTACK = 4
MAXLOCALS = 11
And while targeting the method lambda$loadAdvancements$0 would suffice in development environments, some production environments don't follow the same targeting circumstances.
Following on from our discussion the other night, I think the way to approach this looks like the following:
-
From the point of view of injection points, target selectors, etc. treat the lambda instructions as if they are inline with the regular method instructions. In other words, the
InsnListsupplied to injection points transparently recurses into lambda method bodies when encountered, wherever it's possible to do so. -
Pair this with an extension to target selectors which allows them to specify that they want this recursion to occur. This allows the lambas to be skipped when necessary since recursing into them may be expensive.
The first is self explanatory and means that existing InjectionPoints would not (necessarily) need to be lambda-aware, although markers (probably label nodes) could be inserted in the InsnList so that the start and end of lambdas could be identified if the Injection Point cared about them. This would also facilitate parallels for HEAD for example LAMBDA_HEAD which would inject at the lambda method's head instead of the containing method head for example.
The second would - for backward compatibility - default to regular explicit target selectors not recursing into lambdas. As an example we might use something like:
doSomething(Ljava/lang/String;II)V
as a traditional selector for the method and
doSomething(Ljava/lang/String;II)V -> (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
to designate a selector which recurses into lambdas in the method matching the specified signature. Or
doSomething(Ljava/lang/String;II)V -> *
to recurse into all lambdas.
This is just "thinking out loud" as it were, I'm open to suggestions on how the 'recurse into lambdas' should be expressed in the selector.
Side Note: I know I've chatted with you about this, but for the elucidation of others reading this. Targets which were previously identified solely via (and expressed as compatible representations of
MemberInfo) are being extended soon to include other more flexible declarations, with the important characteristic of being easily disambiguated from previous selectors. Thus, the artist formerly known asMemberInfois retroactively named an "explicit target selector" with other newer target selectors to come.This feature is still incubating at present but will likely include other selector types such as regular expressions:
/get[A-Z][a-z]+/and a dynamic selector format for other selectors where the selector string begins with@followed by the extended selector name.
It might be useful to be able to specify "recurse into lambda" versus "select this lambda" so alternative extensions to selector syntax might be to use alternative separators to designate "recurse into if found" versus "I mean this one". For example(s):
// Select method doSomething and recurse into lambda?
doSomething(ZII)V -> (ZZ)Ljava/lang/Object;
// Select the lambda method itself, not doSomething
doSomething(ZII)V :: (ZZ)Ljava/lang/Object;
// Or possibly
doSomething(ZII)V >> (ZZ)Ljava/lang/Object;
// Or even
doSomething(ZII)V +> (ZZ)Ljava/lang/Object;
// Or maybe
doSomething(ZII)V [ (ZZ)Ljava/lang/Object; ]
// though I don't like that because it looks like array syntax so maybe
doSomething(ZII)V < (ZZ)Ljava/lang/Object; >
I don't know. I think any of these would work but being able to both include a lambda (or all lambdas) in a method and directly select a lambda makes sense, since the name of the lambda method is not deterministic so selecting it directly is - I feel - a recipe for suffering. None of this actively prohibits targetting the lambda method directly though of course.
Interested to hear thoughts on syntax though.
I like the first 2 syntax ideas (-> and ::). Just a question on that, is it going to work with nested lambdas? Is a "recurse into all lambdas" going to work with nested lambdas? Would there be a way to limit it to one level? And will it recurse into method references? That could be bad, but the only difference in the bytecode is the actual method name it refers to
I like the first 2 syntax ideas (
->and::). Just a question on that, is it going to work with nested lambdas? Is a "recurse into all lambdas" going to work with nested lambdas? Would there be a way to limit it to one level?
I was imagining only going one level deep, but arbitrarily extending the syntax so that you can specify select/recurse into/recurse arbitrarily wouldn't be too much of a stretch, since using separators which can't legally appear in a descriptor makes the descriptor easy to tokenise and parse out.
Consider:
// Select the lambda method itself, do not recurse
doSomething(ZII)V :: (ZZ)Ljava/lang/Object;
// Select method doSomething and recurse into lambda but not children
doSomething(ZII)V -> (ZZ)Ljava/lang/Object;
// Select method doSomething and recurse into lambda and children
doSomething(ZII)V -> (ZZ)Ljava/lang/Object; >> *
// Select lambda and recurse into all children
doSomething(ZII)V :: (ZZ)Ljava/lang/Object; >> *
// Select doSomething and recurse into all children
doSomething(ZII)V >> *
// Select specific child lambda within first lambda
doSomething(ZII)V :: (ZZ)Ljava/lang/Object; :: (II)Z
// Select specific child lambda within first lambda and all *its* children
doSomething(ZII)V :: (ZZ)Ljava/lang/Object; :: (II)Z >> *
Obviously >> * could be anything as long as it uses symbols which can't appear in descriptors, so ++ would be equally valid, as would >> ?. Using -> * would not work since * is already a valid selector and -> already means (under this regime) "recurse into child" so -> * would mean "recurse into all children but not their children.
What we're basically doing here is creating a path of selectors, and choosing what to use as a wildcard. It would be nice to use / as a separator but this can already appear in descriptors so it's not valid.
And will it recurse into method references? That could be bad, but the only difference in the bytecode is the actual method name it refers to
The only difference in the calling bytecode yes, but the actual method invoked will be both synthetic and a member of the same class, so in terms of determining what to treat as a lambda and what to ignore, I think those two criteria allow enough certainty, if we exclude the fact that usually lamba methods have the actual word lambda in their name somewhere.
Would it be default to recurse into all children for standard descriptors up to this point?
Read the above. Short answer, "no". Because otherwise this represents a semantic change to existing selectors, which I don't want to do.
Then I'm fine with the consideration of syntax, would it be possible to declare the lambda types?