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

Add support for MixinExtras expressions #2274

Open
wants to merge 87 commits into
base: dev
Choose a base branch
from

Conversation

Earthcomputer
Copy link
Member

See Llama's gist for a (slightly outdated) overview of what MixinExtras expressions are. Without IDE support, ME expressions would be really inconvenient to write, more so than other mixin features.

ME expression syntax is support through a custom language in IntelliJ, through the use of language injection. To make this nicer to work with, I have slightly extended the ME expression language. Definitions are prefixed with the class keyword, and then a boolean literal for whether they are a type or not. Expressions are surrounded with do { ... }. For example:

@Definition(id = "Integer", type = Integer.class)
@Definition(id = "x", local = @Local(type = Object.class, ordinal = 0))
@Expression("(Integer) x")

would produce the following injected ME expression file:

class true Integer
class false x
do { (Integer) x }

This feature is basically done now, the only thing left is to switch away from the JitPack version of MixinExtras and use the library they'll provide for us, which will hopefully have the same interface.

Don't worry about the amount of commits, I'll squash them all on merge.

Earthcomputer and others added 30 commits January 20, 2024 22:39
# Conflicts:
#	src/main/resources/messages/MinecraftDevelopment.properties
Copy link
Member

@DenWav DenWav left a comment

Choose a reason for hiding this comment

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

Well this PR is insane. I don't really know anything about MixinExtras, and can barely follow parts of this PR, and can't follow other parts at all. The code seems fine, it's just going over my head in the more complicated places, but I don't have a problem with trusting you. I did look at (most) of the code in this PR at least, even if I don't really know all of what's going on.

I apologize that the only comments I have are unimportant nitpicks, again I'm going mostly on trust here. Get the tests passing and address these and I'm happy to call this one good to go.

build.gradle.kts Show resolved Hide resolved
src/main/grammars/MEExpressionLexer.flex Outdated Show resolved Hide resolved
src/main/grammars/MEExpressionLexer.flex Show resolved Hide resolved
src/main/grammars/MEExpressionLexer.flex Outdated Show resolved Hide resolved
Copy link
Contributor

@RedNesto RedNesto left a comment

Choose a reason for hiding this comment

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

This looks really good.

I don't have much to say about the features added as I'm not familiar with MixinExtras.

Also not sure about those invokeLater { WriteCommandAction[...] } patterns. It might be the only way to achieve this today, but it might age poorly because of https://youtrack.jetbrains.com/issue/IJPL-53

@@ -432,6 +438,7 @@
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.folding.MixinObjectCastFoldingBuilder"/>
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.folding.MixinTargetDescriptorFoldingBuilder"/>
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.folding.AccessorMixinFoldingBuilder"/>
<lang.foldingBuilder language="JAVA" implementationClass="com.demonwav.mcdev.platform.mixin.expression.MEDefinitionFoldingBuilder"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: why is this not with the other ME EPs a bit further down?

Comment on lines +267 to +275
cacheLock.readLock().lock()
try {
val value = getUserData(key)?.upToDateOrNull
if (value != null) {
return value.get()
}
} finally {
cacheLock.readLock().unlock()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not super consequential, but using Kotlin's withLock extension function avoids that Java try/finally-unlock boilerplate.

Copy link
Member Author

Choose a reason for hiding this comment

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

It didn't let me do this for some reason, prompting me to add kotlin stdlib to the classpath, but don't we already have stdlib-jdk8?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps more to the point kt also has ReentrantReadWriteLock().read { } and a similar write method.

@Earthcomputer
Copy link
Member Author

I think once write actions are allowed outside of the EDT we'll have to make a general sweep of the codebase to look for cases like these where we use invokeLater for write actions.

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.

None yet

4 participants