Fixing refactoring placing code outside class issues (constant and pu…

…ll member up)
HaxeFoundation · Aug 8, 2023 · 4007b6f · 4007b6f
1 parent 716c56e
commit 4007b6f
Show file tree

Hide file tree

Showing 6 changed files with 92 additions and 62 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,8 +1,10 @@
 # Changelog
 
 ## [Unreleased]
-* Bugfix: missing import class quickfix (#1132) 
+* Bugfix: "import class quickfix" missing in some cases (#1132) 
 * Bugfix: "Find Usages" was not checking catch blocks correctly(#929)
+* Bugfix: "Introduce Constant"  placed outside of class
+* Bugfix: "Pull member up" placed code outside of class
 * Change: hiding haxe context menus in non-haxe projects
 
 ## 1.4.11

diff --git a/src/main/java/com/intellij/plugins/haxe/ide/refactoring/introduce/HaxeIntroduceHandler.java b/src/main/java/com/intellij/plugins/haxe/ide/refactoring/introduce/HaxeIntroduceHandler.java
@@ -478,40 +478,43 @@ private PsiElement performReplace(@NotNull final PsiElement declaration, final H
     final HaxeExpression expression = operation.getInitializer();
     final Project project = operation.getProject();
 
-    WriteCommandAction.writeCommandAction(project, expression.getContainingFile()).compute(() -> {
+    PsiElement result = WriteCommandAction.writeCommandAction(project, expression.getContainingFile()).compute(() -> {
 
-        final PsiElement createdDeclaration = addDeclaration(operation, declaration);
+      final PsiElement createdDeclaration = addDeclaration(operation, declaration);
 
-        if (createdDeclaration != null) {
-          modifyDeclaration(createdDeclaration);
-        }
+      if (createdDeclaration == null) return null;
 
-        PsiElement newExpression = createExpression(project, operation);
-        if (null == newExpression) {
-          Logger.getInstance(this.getClass()).warn("Could not create replaceable expression for '" + operation.getName() + "'.");
-          return createdDeclaration;
-        }
+      modifyDeclaration(createdDeclaration);
 
-        if (operation.isReplaceAll()) {
-          List<PsiElement> newOccurrences = new ArrayList<PsiElement>();
-          for (PsiElement occurrence : operation.getOccurrences()) {
-            final PsiElement replaced = replaceExpression(occurrence, newExpression, operation);
-            if (replaced != null) {
-              newOccurrences.add(replaced);
-            }
+      PsiElement newExpression = createExpression(project, operation);
+      if (null == newExpression) {
+        Logger.getInstance(this.getClass()).warn("Could not create replaceable expression for '" + operation.getName() + "'.");
+        return createdDeclaration;
+      }
+
+      if (operation.isReplaceAll()) {
+        List<PsiElement> newOccurrences = new ArrayList<PsiElement>();
+        for (PsiElement occurrence : operation.getOccurrences()) {
+          final PsiElement replaced = replaceExpression(occurrence, newExpression, operation);
+          if (replaced != null) {
+            newOccurrences.add(replaced);
           }
-          operation.setOccurrences(newOccurrences);
-        }
-        else {
-          final PsiElement replaced = replaceExpression(expression, newExpression, operation);
-          operation.setOccurrences(Collections.singletonList(replaced));
         }
+        operation.setOccurrences(newOccurrences);
+      }
+      else {
+        final PsiElement replaced = replaceExpression(expression, newExpression, operation);
+        operation.setOccurrences(Collections.singletonList(replaced));
+      }
 
-        postRefactoring(operation.getElement());
+      postRefactoring(operation.getElement());
       return createdDeclaration;
-      });
-
+    });
 
+    // if we failed to extract return null;
+    if (result == null) {
+      return null;
+    }
 
     // We have added the new declaration, but the new element gets invalidated by
     // the reformatting that is triggered at the end of the write action (the execute above).

diff --git a/...om/intellij/plugins/haxe/ide/refactoring/introduceField/HaxeIntroduceConstantHandler.java b/...om/intellij/plugins/haxe/ide/refactoring/introduceField/HaxeIntroduceConstantHandler.java
@@ -57,28 +57,25 @@ public static PsiElement doIntroduceVariable(PsiElement expression,
                                                PsiElement declaration,
                                                List<PsiElement> occurrences,
                                                boolean replaceAll) {
-    //PsiElement anchor = replaceAll ? findAnchor(occurrences) : findAnchor(expression);
-    //assert anchor != null;
-    //final PsiElement parent = anchor.getParent();
-    //return parent.addBefore(declaration, anchor);
     HaxeClass haxeClass = PsiTreeUtil.getParentOfType(expression, HaxeClass.class, false);
-    if (haxeClass != null) {
-      //haxeClass.getFieldDeclarations().get(0)
-      HaxeClassBody classBody = PsiTreeUtil.getChildOfType(haxeClass, HaxeClassBody.class);
-
-      if (classBody != null) {
-        PsiElement child = classBody.getFirstChild();
-
-        if (child != null) {
-          return classBody.addBefore(declaration, child);
-        }
-        else {
-          classBody.add(declaration);
-        }
-      }
+    if (haxeClass == null) return null;
+
+    HaxeClassBody classBody = PsiTreeUtil.getChildOfType(haxeClass, HaxeClassBody.class);
+    if (classBody == null) return null;
+
+    PsiElement child = lastFieldDeclarationInList(classBody);
+
+    if (child == null) {
+      child = classBody.getFirstChild();
     }
 
-    return null;
+    return child != null ? classBody.addAfter(declaration, child) : null;
+  }
+
+  @Nullable
+  private static HaxeFieldDeclaration lastFieldDeclarationInList(HaxeClassBody classBody) {
+    List<HaxeFieldDeclaration> list = classBody.getFieldDeclarationList();
+    return list.isEmpty() ? null : list.get(list.size() - 1);
   }
 
   @Nullable

diff --git a/src/main/java/com/intellij/plugins/haxe/ide/refactoring/memberPullUp/HaxePullUpHelper.java b/src/main/java/com/intellij/plugins/haxe/ide/refactoring/memberPullUp/HaxePullUpHelper.java
@@ -29,6 +29,7 @@
 import com.intellij.plugins.haxe.HaxeLanguage;
 import com.intellij.plugins.haxe.lang.psi.HaxeMethod;
 
+import com.intellij.plugins.haxe.lang.psi.impl.AbstractHaxePsiClass;
 import com.intellij.plugins.haxe.util.HaxeElementGenerator;
 import com.intellij.psi.*;
 import com.intellij.psi.codeStyle.CodeStyleManager;
@@ -67,8 +68,8 @@ public class HaxePullUpHelper implements PullUpHelper<MemberInfo> {
   private static final Key<Boolean> PRESERVE_QUALIFIER = Key.create("PRESERVE_QUALIFIER");
 
 
-  private final PsiClass mySourceClass;
-  private final PsiClass myTargetSuperClass;
+  private final AbstractHaxePsiClass mySourceClass;
+  private final AbstractHaxePsiClass myTargetSuperClass;
   private final boolean myIsTargetInterface;
   private final DocCommentPolicy myJavaDocPolicy;
   private Set<PsiMember> myMembersAfterMove = null;
@@ -82,8 +83,8 @@ public HaxePullUpHelper(PullUpData data) {
     myProject = data.getProject();
     myMembersToMove = data.getMembersToMove();
     myMembersAfterMove = data.getMovedMembers();
-    myTargetSuperClass = data.getTargetClass();
-    mySourceClass = data.getSourceClass();
+    myTargetSuperClass = (AbstractHaxePsiClass)data.getTargetClass();
+    mySourceClass = (AbstractHaxePsiClass)data.getSourceClass();
     myJavaDocPolicy = data.getDocCommentPolicy();
     myIsTargetInterface = myTargetSuperClass.isInterface();
 
@@ -209,7 +210,7 @@ private void doMoveField(PsiSubstitutor substitutor, MemberInfo info) {
     if (myIsTargetInterface) {
       PsiUtil.setModifierProperty(field, PsiModifier.PUBLIC, true);
     }
-    final PsiMember movedElement = (PsiMember)myTargetSuperClass.addBefore(convertFieldToLanguage(field, myTargetSuperClass.getLanguage()), myTargetSuperClass.getRBrace());
+    final PsiMember movedElement = (PsiMember)myTargetSuperClass.getBody().addBefore(convertFieldToLanguage(field, myTargetSuperClass.getLanguage()), myTargetSuperClass.getRBrace());
     myMembersAfterMove.add(movedElement);
     field.delete();
   }
@@ -239,6 +240,7 @@ private  void doMoveMethod(PsiSubstitutor substitutor, MemberInfo info) {
     }
     boolean isOriginalMethodAbstract = method.hasModifierProperty(PsiModifier.ABSTRACT) || method.hasModifierProperty(PsiModifier.DEFAULT);
     boolean isOriginalMethodPrototype = method instanceof HaxeMethod;
+    PsiElement rightBrace = myTargetSuperClass.getRBrace();
     if (myIsTargetInterface || info.isToAbstract()) {
       ChangeContextUtil.clearContextInfo(method);
 
@@ -261,8 +263,10 @@ private  void doMoveMethod(PsiSubstitutor substitutor, MemberInfo info) {
       else {
         methodCopy = HaxeElementGenerator.createMethodDeclaration(myProject, methodCopy.getText().trim() + ";");
 
+        PsiElement superClassBody = myTargetSuperClass.getBody();
         movedElement =
-          anchor != null ? (PsiMember)myTargetSuperClass.addBefore(methodCopy, anchor) : (PsiMember)myTargetSuperClass.addBefore(methodCopy, myTargetSuperClass.getRBrace());
+          anchor != null ? (PsiMember)superClassBody.addBefore(methodCopy, anchor)
+                         : (PsiMember)superClassBody.addBefore(methodCopy, rightBrace);
 
         reformat(movedElement);
       }
@@ -299,11 +303,10 @@ private  void doMoveMethod(PsiSubstitutor substitutor, MemberInfo info) {
         superClassMethod.replace(convertMethodToLanguage(methodCopy, language));
       }
       else {
+        PsiElement superClassBody = myTargetSuperClass.getBody();
         final PsiMember movedElement =
-          anchor != null ? (PsiMember)myTargetSuperClass.addBefore(convertMethodToLanguage(methodCopy,
-                                                                                           language), anchor) : (PsiMember)myTargetSuperClass.addBefore(
-            convertMethodToLanguage(
-              methodCopy, language), myTargetSuperClass.getRBrace());
+          anchor != null ? (PsiMember)superClassBody.addAfter(convertMethodToLanguage(methodCopy, language), anchor)
+                         : (PsiMember)superClassBody.addBefore(convertMethodToLanguage(methodCopy, language), rightBrace);
         reformat(movedElement);
         myMembersAfterMove.add(movedElement);
       }

diff --git a/src/main/java/com/intellij/plugins/haxe/lang/psi/impl/AbstractHaxeNamedComponent.java b/src/main/java/com/intellij/plugins/haxe/lang/psi/impl/AbstractHaxeNamedComponent.java
@@ -269,19 +269,26 @@ public final PsiElement findChildByRoleAsPsiElement(int role) {
     if (element == null) return null;
     return SourceTreeToPsiMap.treeElementToPsi(element);
   }
-
   @Nullable
-  public ASTNode findChildByRole(int role) {
-    // assert ChildRole.isUnique(role);
-    PsiElement firstChild = getFirstChild();
+  public final PsiElement findChildByRoleAsPsiElementIn(PsiElement body, int role) {
+    ASTNode element = findChildByRole(body.getFirstChild(), role);
+    if (element == null) return null;
+    return SourceTreeToPsiMap.treeElementToPsi(element);
+  }
 
+  @Nullable
+  public ASTNode findChildByRole(PsiElement firstChild, int role) {
     if (firstChild == null) return null;
 
     for (ASTNode child = firstChild.getNode(); child != null; child = child.getTreeNext()) {
       if (getChildRole(child) == role) return child;
     }
     return null;
   }
+  @Nullable
+  public ASTNode findChildByRole(int role) {
+    return findChildByRole(getFirstChild(), role);
+  }
 
   public int getChildRole(ASTNode child) {
     if (child.getElementType() == HaxeTokenTypes.PLCURLY) {

diff --git a/src/main/java/com/intellij/plugins/haxe/lang/psi/impl/AbstractHaxePsiClass.java b/src/main/java/com/intellij/plugins/haxe/lang/psi/impl/AbstractHaxePsiClass.java
@@ -510,14 +510,32 @@ public PsiTypeParameter[] getTypeParameters() {
 
   @Override
   public PsiElement getLBrace() {
-    return findChildByRoleAsPsiElement(ChildRole.LBRACE);
+    PsiElement body = getBody();
+    return findChildByRoleAsPsiElementIn(body, ChildRole.LBRACE);
   }
 
   @Override
   public PsiElement getRBrace() {
-    return findChildByRoleAsPsiElement(ChildRole.RBRACE);
+    PsiElement body = getBody();
+    return findChildByRoleAsPsiElementIn(body, ChildRole.RBRACE);
+  }
+
+  public PsiElement getBody() {
+      if (this instanceof HaxeClassDeclaration classDeclaration) {  // concrete class
+        return classDeclaration.getClassBody();
+      } else if (this instanceof HaxeAbstractTypeDeclaration typeDeclaration) {  // abstract
+        return typeDeclaration.getAbstractBody();
+      } else if (this instanceof HaxeExternClassDeclaration externClassDeclaration) { // extern class
+        return externClassDeclaration.getExternClassDeclarationBody();
+      } else if (this instanceof HaxeTypedefDeclaration typedefDeclaration) {  // typedef
+        return typedefDeclaration.getTypeOrAnonymous();
+      } else if (this instanceof HaxeInterfaceDeclaration interfaceDeclaration) { // interface
+        return interfaceDeclaration.getInterfaceBody();
+      } else if (this instanceof HaxeEnumDeclaration enumDeclaration) { // enum
+        return enumDeclaration.getEnumBody();
+      }
+    return this;
   }
-
   private boolean isPrivate() {
     if(_isPrivate == null) {
       HaxePrivateKeyWord privateKeyWord = null;