Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing injecting in constructor #30

Closed
wants to merge 6 commits into from

Conversation

liach
Copy link

@liach liach commented May 2, 2020

It is bad to assume that everyone does business logic outside of constructor. We don't want redirect conflict hell

Fixes #26

It is bad to assume that everyone does business logic outside of constructor. We don't want redirect conflict hell
@Juuxel
Copy link
Member

Juuxel commented May 2, 2020

Doesn't this allow cancelling final field initialization?

@liach
Copy link
Author

liach commented May 2, 2020

Will do a few tweaks to prevent cancellable then, I guess

@kvverti
Copy link

kvverti commented May 2, 2020

How about static injections before the super/this delegation?

Signed-off-by: liach <[email protected]>
@liach
Copy link
Author

liach commented May 2, 2020

You cannot call static methods before super/this delegation in constructors unless you are calling it for parameters. Imo in that case redirects are better. Redirect, Modify arg, Modify variable is already ALLOW_ALL when passed into checking.

@liach
Copy link
Author

liach commented May 2, 2020

This patch works.
image
Mixin code:

@Mixin(MinecraftClient.class)
public abstract class ClientMixin {
    @Inject(method = "<init>", at = @At(value = "INVOKE", target = "Lnet/minecraft/client/MinecraftClient;initializeSearchableContainers()V"))
    private void injectInit(RunArgs args, CallbackInfo ci) {
        System.out.println("HELLO I AM INITIALIZING SEARCHABLE CONTAINERS");
    }
}

Also the cancellable = true check works as well:
image

@Mixin(MinecraftClient.class)
public abstract class ClientMixin {
    @Inject(method = "<init>", cancellable = true,
            at = @At(value = "INVOKE", target = "Lnet/minecraft/client/MinecraftClient;initializeSearchableContainers()V"))
    private void injectInit(RunArgs args, CallbackInfo ci) {
        System.out.println("HELLO I AM INITIALIZING SEARCHABLE CONTAINERS");
    }
}

@i509VCB
Copy link

i509VCB commented May 2, 2020

Hmm, what's does the output look like (Dmixin.debug.output=true)

@liach
Copy link
Author

liach commented May 2, 2020

@i509VCB javap result of MinecraftClient here (Note: Fabric API patches exist as well)

@liach
Copy link
Author

liach commented May 2, 2020

@sfPlayer1 the before super injection does properly fail i.e. HEAD
image

build.gradle Outdated Show resolved Hide resolved
@Runemoro
Copy link

Runemoro commented Jun 28, 2020

Disallowing cancellation is unnecessary. There's nothing wrong with final fields being left uninitialized. Here's some vanilla Java code that accesses a final field before it's initialized:

public class Test {
    private final int a;

    public Test() {
        a = f();
        System.out.println(a); // prints 1
    }

    private int f() {
        System.out.println(a); // a is accessed before being assigned, prints 0
        return 1;
    }
}

Also, injecting before the super() call should be allowed, but with the restriction that the Mixin isn't given the this instance, similar to the way we can run any static code before a super() call:

public class Test extends Something {
    public Test() {
        super(f());
    }

    private static int f() {
        // do anything here
        return 0;
    }
}

Copy link

@Devan-Kerman Devan-Kerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rune is rite

@sylv256
Copy link

sylv256 commented Aug 3, 2020

pls merge w/ master tanqs

i need feature :(

@modmuss50
Copy link
Member

Going to test and merge this in via: #40

@modmuss50 modmuss50 closed this Oct 26, 2020
modmuss50 added a commit that referenced this pull request Oct 26, 2020
* Allowing injecting in constructor

It is bad to assume that everyone does business logic outside of constructor. We don't want redirect conflict hell

* This should work

Signed-off-by: liach <[email protected]>

* Add notes for easy portin in the future

Signed-off-by: liach <[email protected]>

* Restore build number

* Adopt new versioning scheme

Signed-off-by: liach <[email protected]>

Co-authored-by: liach <[email protected]>
Co-authored-by: liach <[email protected]>
LlamaLad7 added a commit to LlamaLad7/FabricMixin that referenced this pull request May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let's Talk: Constructor Injection