Feature/lambda selection - #235
Conversation
Whitespace is only allowed between components. Also temporarily disallow `->` in selectors.
c435576 to
53596cb
Compare
53596cb to
959aa07
Compare
959aa07 to
cc05add
Compare
cc05add to
653270b
Compare
We do not have a `MixinInfo` to use.
653270b to
a2f1c7c
Compare
modmuss50
left a comment
There was a problem hiding this comment.
This looks great, happned to notice one minor thing when I was skim reading the code, I havent done an indepth review as im not familar with the mixin codebase, and its likely not worth my time.
I assume you are planning a set of tests for this in your test project, and I expect the best way to test it is to get it into peoples hands to play with.
Please do let me know when you are ready to release this and we can come up with a plan to get it out. Likely as part of Loom 1.18 and Loader 0.20 initally as a beta.
There was a problem hiding this comment.
Opted to remove the equals and hashCode instead because they never really made any sense semantically, aren't implemented in Mixin's other selector types, and aren't needed.
Both were horribly asymmetric and had unclear semantics. The only usage of these methods in the first place is to construct `Set<ITargetSelector>`s but in such cases the user has specified each target selector and so there is no reason to desire deduplication. The *results* of target selectors are deduplicated at a later time anyway.
Warning
This is ready for review but should not be merged until support is finalised in MCDev and tiny-remapper
This PR adds support for targeting lambdas within methods. A basic example is the following:
which will modify all
5s within the lexical scope ofouterMethod, i.e. including inside arbitrarily nestedlambdas (though note this does not include local or anonymous classes).
Parsing
See MemberInfo for pre-knowledge.
The general format is, in order:
MemberInfo->{1})MemberInfoThis nesting can be repeated multiple times.
Prior to this PR all whitespace is stripped from selectors before they are parsed. This naively allows selectors such as
method = "r e n d e r", and prevents whitespace being meaningful. This PR therefore disallows arbitrary whitespace, allowing only whitespace between the elements of a MemberInfo (and at the start/end).The parsing for nested selectors is whitespace-sensitive, allowing differentiation between
outerMethod -> *()V(any top-level()Vlambda)outerMethod ->* ()V(any()Vlambda or the root if it is also()V)A space is enforced before the arrow and after the arrow (and optional depth quantifier). The depth quantifier, if present, must directly follow the arrow. This ensures selectors remain clearly intelligible by readers.
Note that empty selectors are traditionally valid (e.g.
method = ""selects the first method). We do not change this, but we do disallow any nesting components being empty, which prevents cursed things likemethod = " -> ". Empty selectors can always be replaced at the top-level by{0,1}or at an inner level by*Root Methods
When a target selector involves nesting (i.e. contains
->), lambda methods in a class are not considered as possible root methods. This is contrary to the default behaviour ofmethod = "*"selecting all methods including lambdas.This is required for intuitive behaviour, so that for example
* -> *selects only top-level lambdas instead of all lambdas.If a particular selector wishes, for some reason, to include all lambdas in its root methods,
*can simply be swapped for* ->* *.Depth Quantifiers
Depth quantifiers restrict the possible nesting levels at which the inner selector can match. E.g.:
outer ->* *permits any nesting level, including 0, so selectsouteritself along with all its lambdasouter ->+ *permits only nesting levels >=1, so selects onlyouter's lambdasouter ->{2, 4} *permits nesting levels from 2-4 inclusive* ->{0} *permits no nesting, i.e. selects all non-lambda methods within the target classIt is possible in obscure cases for lambdas to be reached at multiple depths. In such cases, lambdas are included as long as they appear at some depth which is permitted. E.g.
* ->* * ->{2} *will match any lambda which is at least doubly nested.Nested Owner, Name and Desc
For a top-level method, the selector matches:
For a nested method, this is adjusted as follows:
outer ->+ Ljava/lang/Runnable;to mean anyRunnablelambdaouter ->+ runto mean any lambda implementing arunmethodouter ->+ Ljava/util/function/Supplier;get()Ljava/lang/String;to mean anySupplier<String>lambdaNote that when the depth quantifier permits
0(e.g.*), the outer method is only included if it matches the nested selector. E.g.:outer ->* *includesoutersince it matches*outer ->* *()Vincludesouteronly if it has the desc()V* ->* run()Vmatches the top-level methodrun()V, if it exists, along with any lambdas implementing arun()Vmethodouter ->* Ljava/lang/Runnable;never matches the outer method (unless somehow mixing intoRunnableitself). It is thereby equivalent toouter ->+ Ljava/lang/Runnable;, which should be preferred for clarity.Nested Quantifiers
Inner selectors can themselves have arbitrary quantifiers, e.g.
outer ->* {2,3}. Recall that the behaviour of a quantifier is toFor nested selectors, matches are found depth-first. Consider the following example method:
outer ->+ {3}matchesa,bandc.Additionally, nested quantifiers are evaluated cumulatively across all parent methods. E.g.
* ->+ {5}matches the first 5 lambdas in the class (with depth-first ordering as above).Remapping
This section applies to refmaps / the AP only. Future work will be needed in tiny-remapper.
Selectors are remapped as a whole, i.e.
outer -> innergets a standalone refmap entry rather than separate ones forouterandinner.Inner method names are remapped differently from root method names. This is necessary because the specified specialised descriptor may not match the method's actual descriptor. Thankfully, when an inner selector is qualified (e.g.
* -> Ljava/lang/Runnable;run), the owner is enough to uniquely identify the SAM method (since, by definition, there can only be 1). It is remapped therefore to* -> Ljava/lang/Runnable;. When a descriptor is included, e.g.* -> Ljava/util/function/Supplier;get()Ljava/lang/String;, the descriptor is preserved (and possibly remapped), leaving* -> Ljava/util/function/Supplier;()Ljava/lang/String;, which behaves the same. When no owner is present, the name is left as-is and cannot be remapped, as is already expected for unqualified references in Mixins.Recall that injectors allow disabling the AP's remapping via
remap = false. This option is not granular enough to enable remapping for only specific parts of a nested selector. E.g. when targeting a Minecraft class withsomeMethod -> Ljava/lang/Runnable;, we would wantsomeMethodto be remapped, but no mapping would exist forRunnable. In light of this, nested components of a selector are always remapped and do not respectremap = false. As such, if mappings for nested parts cannot be found, an error is never raised. This tradeoff means the new feature matches the behaviour users would expect from tiny-remapper, but keepsremap = true/falseworking for root methods so users can benefit from associated errors.Backwards Compatibility
Nested selectors have been partially implemented for a long time in upstream, meaning they are parsed but not properly validated or respected. This means that a mod today can write for example
someMethod -> [garbage*{to mean simplysomeMethod. A mod is unlikely to do this, but the behaviour is preserved out of caution, meaning mods must opt into the new compatibility version to use nested selectors properly.As for selector parsing, whitespace is still stripped unless a mod opts into the new compatibility version. Nested selectors for old mods are parsed as they were before, but they remain inactive.
Upstream Divergence
As mentioned, upstream has had this feature partially implemented for a long time, but the implementation in this PR diverges very significantly from what upstream appears to have intended. Most notably:
I believe that all these divergences are for good reason, and do not intend to support upstream's half-baked approach if it ever gets released. I will however attempt to convince them to take ours instead.