Discourage brittle usages of shifting by n instructions

minecraft-dev · Jul 21, 2024 · 3a055f8 · 3a055f8
1 parent 91d44de
commit 3a055f8
Show file tree

Hide file tree

Showing 11 changed files with 133 additions and 25 deletions.
diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/AtResolver.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/AtResolver.kt
@@ -25,24 +25,29 @@ import com.demonwav.mcdev.platform.mixin.reference.isMiscDynamicSelector
 import com.demonwav.mcdev.platform.mixin.reference.parseMixinSelector
 import com.demonwav.mcdev.platform.mixin.reference.target.TargetReference
 import com.demonwav.mcdev.platform.mixin.util.MixinConstants.Annotations.SLICE
+import com.demonwav.mcdev.platform.mixin.util.MixinConstants.Classes.SHIFT
 import com.demonwav.mcdev.platform.mixin.util.findSourceElement
 import com.demonwav.mcdev.util.computeStringArray
 import com.demonwav.mcdev.util.constantStringValue
 import com.demonwav.mcdev.util.constantValue
+import com.demonwav.mcdev.util.equivalentTo
 import com.demonwav.mcdev.util.fullQualifiedName
 import com.intellij.codeInsight.lookup.LookupElementBuilder
+import com.intellij.psi.JavaPsiFacade
 import com.intellij.psi.PsiAnnotation
 import com.intellij.psi.PsiAnnotationMemberValue
 import com.intellij.psi.PsiArrayInitializerMemberValue
 import com.intellij.psi.PsiClass
 import com.intellij.psi.PsiClassType
 import com.intellij.psi.PsiElement
+import com.intellij.psi.PsiEnumConstant
 import com.intellij.psi.PsiExpression
 import com.intellij.psi.PsiModifierList
 import com.intellij.psi.PsiQualifiedReference
 import com.intellij.psi.PsiReference
 import com.intellij.psi.PsiReferenceExpression
 import com.intellij.psi.search.GlobalSearchScope
+import com.intellij.psi.util.PsiUtil
 import com.intellij.psi.util.parentOfType
 import com.intellij.psi.util.parents
 import org.objectweb.asm.tree.ClassNode
@@ -142,6 +147,21 @@ class AtResolver(
                 .filterIsInstance<PsiAnnotation>()
                 .firstOrNull { it.parent is PsiModifierList }
         }
+
+        fun getShift(at: PsiAnnotation): Int {
+            val shiftAttr = at.findDeclaredAttributeValue("shift") as? PsiExpression ?: return 0
+            val shiftReference = PsiUtil.skipParenthesizedExprDown(shiftAttr) as? PsiReferenceExpression ?: return 0
+            val shift = shiftReference.resolve() as? PsiEnumConstant ?: return 0
+            val containingClass = shift.containingClass ?: return 0
+            val shiftClass = JavaPsiFacade.getInstance(at.project).findClass(SHIFT, at.resolveScope) ?: return 0
+            if (!(containingClass equivalentTo shiftClass)) return 0
+            return when (shift.name) {
+                "BEFORE" -> -1
+                "AFTER" -> 1
+                "BY" -> at.findDeclaredAttributeValue("by")?.constantValue as? Int ?: 0
+                else -> 0
+            }
+        }
     }
 
     fun isUnresolved(): InsnResolutionInfo.Failure? {

diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/ConstantStringMethodInjectionPoint.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/ConstantStringMethodInjectionPoint.kt
@@ -86,6 +86,11 @@ class ConstantStringMethodInjectionPoint : AbstractMethodInjectionPoint() {
         editor.caretModel.moveToOffset(cursorElement.textRange.endOffset - 1)
     }
 
+    override fun isShiftDiscouraged(shift: Int): Boolean {
+        // allow shifting after the INVOKE_STRING
+        return shift != 0 && shift != 1
+    }
+
     override fun getArgsKeys(at: PsiAnnotation) = ARGS_KEYS
 
     override fun getArgsValues(at: PsiAnnotation, key: String): Array<Any> {

diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/FieldInjectionPoint.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/FieldInjectionPoint.kt
@@ -59,6 +59,11 @@ class FieldInjectionPoint : QualifiedInjectionPoint<PsiField>() {
         completeExtraStringAtAttribute(editor, reference, "target")
     }
 
+    override fun isShiftDiscouraged(shift: Int): Boolean {
+        // allow shift after the field access
+        return shift != 0 && shift != 1
+    }
+
     override fun getArgsKeys(at: PsiAnnotation) = ARGS_KEYS
 
     override fun getArgsValues(at: PsiAnnotation, key: String): Array<Any> =

diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/InjectionPoint.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/InjectionPoint.kt
@@ -22,13 +22,11 @@ package com.demonwav.mcdev.platform.mixin.handlers.injectionPoint
 
 import com.demonwav.mcdev.platform.mixin.reference.MixinSelector
 import com.demonwav.mcdev.platform.mixin.reference.toMixinString
-import com.demonwav.mcdev.platform.mixin.util.MixinConstants.Classes.SHIFT
 import com.demonwav.mcdev.platform.mixin.util.fakeResolve
 import com.demonwav.mcdev.platform.mixin.util.findOrConstructSourceMethod
 import com.demonwav.mcdev.util.constantStringValue
 import com.demonwav.mcdev.util.constantValue
 import com.demonwav.mcdev.util.createLiteralExpression
-import com.demonwav.mcdev.util.equivalentTo
 import com.demonwav.mcdev.util.findAnnotations
 import com.demonwav.mcdev.util.fullQualifiedName
 import com.demonwav.mcdev.util.getQualifiedMemberReference
@@ -47,17 +45,13 @@ import com.intellij.psi.PsiAnnotation
 import com.intellij.psi.PsiAnonymousClass
 import com.intellij.psi.PsiClass
 import com.intellij.psi.PsiElement
-import com.intellij.psi.PsiEnumConstant
-import com.intellij.psi.PsiExpression
 import com.intellij.psi.PsiLambdaExpression
 import com.intellij.psi.PsiLiteral
 import com.intellij.psi.PsiMember
 import com.intellij.psi.PsiMethod
 import com.intellij.psi.PsiMethodReferenceExpression
-import com.intellij.psi.PsiReferenceExpression
 import com.intellij.psi.PsiSubstitutor
 import com.intellij.psi.codeStyle.CodeStyleManager
-import com.intellij.psi.util.PsiUtil
 import com.intellij.psi.util.parentOfType
 import com.intellij.serviceContainer.BaseKeyedLazyInstance
 import com.intellij.util.ArrayUtilRt
@@ -108,7 +102,9 @@ abstract class InjectionPoint<T : PsiElement> {
 
     open fun isArgValueList(at: PsiAnnotation, key: String) = false
 
-    open val isDiscouraged: Boolean = false
+    open val discouragedMessage: String? = null
+
+    open fun isShiftDiscouraged(shift: Int): Boolean = shift != 0
 
     abstract fun createNavigationVisitor(
         at: PsiAnnotation,
@@ -146,20 +142,7 @@ abstract class InjectionPoint<T : PsiElement> {
     }
 
     protected open fun addShiftSupport(at: PsiAnnotation, targetClass: ClassNode, collectVisitor: CollectVisitor<*>) {
-        val shiftAttr = at.findDeclaredAttributeValue("shift") as? PsiExpression ?: return
-        val shiftReference = PsiUtil.skipParenthesizedExprDown(shiftAttr) as? PsiReferenceExpression ?: return
-        val shift = shiftReference.resolve() as? PsiEnumConstant ?: return
-        val containingClass = shift.containingClass ?: return
-        val shiftClass = JavaPsiFacade.getInstance(at.project).findClass(SHIFT, at.resolveScope) ?: return
-        if (!(containingClass equivalentTo shiftClass)) return
-        when (shift.name) {
-            "BEFORE" -> collectVisitor.shiftBy = -1
-            "AFTER" -> collectVisitor.shiftBy = 1
-            "BY" -> {
-                val by = at.findDeclaredAttributeValue("by")?.constantValue as? Int ?: return
-                collectVisitor.shiftBy = by
-            }
-        }
+        collectVisitor.shiftBy = AtResolver.getShift(at)
     }
 
     protected open fun addSliceFilter(at: PsiAnnotation, targetClass: ClassNode, collectVisitor: CollectVisitor<*>) {

diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/InvokeInjectionPoint.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/InvokeInjectionPoint.kt
@@ -44,6 +44,24 @@ abstract class AbstractInvokeInjectionPoint(private val assign: Boolean) : Abstr
         completeExtraStringAtAttribute(editor, reference, "target")
     }
 
+    override fun isShiftDiscouraged(shift: Int): Boolean {
+        if (shift == 0) {
+            return false
+        }
+        if (assign) {
+            // allow shifting before the INVOKE_ASSIGN
+            if (shift == -1) {
+                return false
+            }
+        } else {
+            // allow shifting after the INVOKE
+            if (shift == 1) {
+                return false
+            }
+        }
+        return true
+    }
+
     override fun createNavigationVisitor(
         at: PsiAnnotation,
         target: MixinSelector?,

diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/JumpInjectionPoint.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/JumpInjectionPoint.kt
@@ -57,7 +57,7 @@ class JumpInjectionPoint : InjectionPoint<PsiElement>() {
         )
     }
 
-    override val isDiscouraged = true
+    override val discouragedMessage = "Usage of JUMP is discouraged because it is brittle"
 
     override fun createNavigationVisitor(
         at: PsiAnnotation,

diff --git a/src/main/kotlin/platform/mixin/handlers/injectionPoint/LoadInjectionPoint.kt b/src/main/kotlin/platform/mixin/handlers/injectionPoint/LoadInjectionPoint.kt
@@ -67,6 +67,24 @@ abstract class AbstractLoadInjectionPoint(private val store: Boolean) : Injectio
         return LocalInfo.fromAnnotation(localType, modifyVariable)
     }
 
+    override fun isShiftDiscouraged(shift: Int): Boolean {
+        if (shift == 0) {
+            return false
+        }
+        if (store) {
+            // allow shift before the STORE
+            if (shift == -1) {
+                return false
+            }
+        } else {
+            // allow shift after the LOAD
+            if (shift == 1) {
+                return false
+            }
+        }
+        return true
+    }
+
     override fun createNavigationVisitor(
         at: PsiAnnotation,
         target: MixinSelector?,

diff --git a/src/main/kotlin/platform/mixin/inspection/injector/DiscouragedInjectionPointInspection.kt b/src/main/kotlin/platform/mixin/inspection/injector/DiscouragedInjectionPointInspection.kt
@@ -39,8 +39,9 @@ class DiscouragedInjectionPointInspection : MixinInspection() {
             }
             val atValue = annotation.findDeclaredAttributeValue("value") ?: return
             val atCode = atValue.constantStringValue ?: return
-            if (InjectionPoint.byAtCode(atCode)?.isDiscouraged == true) {
-                holder.registerProblem(atValue, "Usage of $atCode is discouraged")
+            val discouragedMessage = InjectionPoint.byAtCode(atCode)?.discouragedMessage
+            if (discouragedMessage != null) {
+                holder.registerProblem(atValue, discouragedMessage)
             }
         }
     }

diff --git a/src/main/kotlin/platform/mixin/inspection/injector/DiscouragedShiftInspection.kt b/src/main/kotlin/platform/mixin/inspection/injector/DiscouragedShiftInspection.kt
@@ -0,0 +1,50 @@
+/*
+ * Minecraft Development for IntelliJ
+ *
+ * https://mcdev.io/
+ *
+ * Copyright (C) 2024 minecraft-dev
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation, version 3.0 only.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package com.demonwav.mcdev.platform.mixin.inspection.injector
+
+import com.demonwav.mcdev.platform.mixin.handlers.injectionPoint.AtResolver
+import com.demonwav.mcdev.platform.mixin.handlers.injectionPoint.InjectionPoint
+import com.demonwav.mcdev.platform.mixin.inspection.MixinInspection
+import com.demonwav.mcdev.platform.mixin.util.MixinConstants
+import com.demonwav.mcdev.util.constantStringValue
+import com.intellij.codeInspection.ProblemsHolder
+import com.intellij.psi.JavaElementVisitor
+import com.intellij.psi.PsiAnnotation
+import com.intellij.psi.PsiElementVisitor
+
+class DiscouragedShiftInspection : MixinInspection() {
+    override fun getStaticDescription() = "Reports discouraged usages of shifting in injection points"
+
+    override fun buildVisitor(holder: ProblemsHolder): PsiElementVisitor = object : JavaElementVisitor() {
+        override fun visitAnnotation(annotation: PsiAnnotation) {
+            if (!annotation.hasQualifiedName(MixinConstants.Annotations.AT)) {
+                return
+            }
+            val atValue = annotation.findDeclaredAttributeValue("value") ?: return
+            val atCode = atValue.constantStringValue ?: return
+            val shift = AtResolver.getShift(annotation)
+            if (InjectionPoint.byAtCode(atCode)?.isShiftDiscouraged(shift) == true) {
+                val shiftElement = annotation.findDeclaredAttributeValue("shift") ?: return
+                holder.registerProblem(shiftElement, "Shifting like this is discouraged because it's brittle")
+            }
+        }
+    }
+}
diff --git a/src/main/kotlin/platform/mixin/reference/InjectionPointReference.kt b/src/main/kotlin/platform/mixin/reference/InjectionPointReference.kt
@@ -88,7 +88,7 @@ object InjectionPointReference : ReferenceResolver(), MixinReference {
         return (
             getAllAtCodes(context.project).keys.asSequence()
                 .filter {
-                    InjectionPoint.byAtCode(it)?.isDiscouraged != true
+                    InjectionPoint.byAtCode(it)?.discouragedMessage == null
                 }
                 .map {
                     PrioritizedLookupElement.withPriority(

diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml
@@ -1026,6 +1026,14 @@
                          level="WARNING"
                          hasStaticDescription="true"
                          implementationClass="com.demonwav.mcdev.platform.mixin.inspection.injector.DiscouragedInjectionPointInspection"/>
+        <localInspection displayName="Usage of discouraged shifting in injection point"
+                         shortName="DiscouragedShift"
+                         groupName="Mixin"
+                         language="JAVA"
+                         enabledByDefault="true"
+                         level="WARNING"
+                         hasStaticDescription="true"
+                         implementationClass="com.demonwav.mcdev.platform.mixin.inspection.injector.DiscouragedShiftInspection"/>
         <!--endregion-->
 
         <!--region Overwrite Inspections -->