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

Validate shadow methods/fields and injections at compile time #40

Closed
stephan-gh opened this issue Apr 22, 2015 · 3 comments
Closed

Validate shadow methods/fields and injections at compile time #40

stephan-gh opened this issue Apr 22, 2015 · 3 comments
Assignees

Comments

@stephan-gh
Copy link

I'm not completely sure if this exists already, but it would be great if the annotation processor could validate the shadow fields and methods as well as injections at compile time and warn if they actually don't exist. While you'll notice it at runtime, it would be better if that could be reported already at compile time instead of only noticing once the mixin target class gets loaded.

For example, this commit by @gabizou in SpongeCommon compiles without any problems or warnings, but does actually add an injection on a method added by Forge which will fail when running in Vanilla.

It would be nice if we could have it fail the build, or at least warn about problems like this at compile time so we notice when adding Forge specific stuff to Common directly.

@Mumfrey
Copy link
Member

Mumfrey commented Apr 22, 2015

Warning is definitely possible. It could never fail a build because there are perfectly valid circumstances in which a shadow field or method may not exist at compile time, or may not be visible via Mirror.

It's worth bearing in mind that the information the AP has to work with is kind of limited. Mirror is pretty shite and also can't account for things which are added at runtime (such as fields added to a target class by another mixin) and doesn't provide access to anything synthetic and/or inner. I could in the future simulate some of this up to a point, but recreating the entire mixin architecture inside the AP isn't really feasible.

I can try to add what output I can, but failing a build with these checks would be absolutely unworkable so it would rely on people actually reading the build output, and I'm not sure this has much in the way of advantages over developers just getting into the habit of running an audit before committing changes.

@Mumfrey Mumfrey self-assigned this Apr 22, 2015
@stephan-gh
Copy link
Author

@Mumfrey Speaking of running an audit, is there a way to force-load all the classes (to check for apply errors) but without generating all the implementation reports? This is spamming the console quite a lot, but it is not running the audit if CHECK_IMPLEMENTS is not set.

@Mumfrey
Copy link
Member

Mumfrey commented Apr 22, 2015

I could probably remove the CHECK_IMPLEMENTS requirement on the audit pass.

i509VCB pushed a commit to QuiltMC/Mixin that referenced this issue Apr 21, 2021
* 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 <liach@users.noreply.github.com>

* Add notes for easy portin in the future

Signed-off-by: liach <liach@users.noreply.github.com>

* Restore build number

* Adopt new versioning scheme

Signed-off-by: liach <liach@users.noreply.github.com>

Co-authored-by: liach <7806504+liach@users.noreply.github.com>
Co-authored-by: liach <liach@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants