Quickfix: Warnings don't (always) vanish when "Fix all for file" is called

In Mathematica this

a = 1
b = 3
c = 4


is just 3 separate expressions when this appears at file scope. On the other hand this

a = 1;
b = 3;
c = 4;


is on single compound expression. I have a quickfix (because the second version should be preferred) to append a semicolon to each expression at filescope. The problem is, when I run it on very large files (about 3000 lines) with probably several hundret missing semicolons, sometimes at a few places one of this happens

  • a semicolon is added at the end, but the inspections still shows that it is missing
  • the semicolon is not added


The first issue sometimes appears, when I apply the fix on only one place. One problem might be that by adding a semicolon, the Psi structure is changed in a greater context. I attach the small code for my quick-fix.
Additionally, do I have to change the PsiElements or would it be possible to insert the semicolons directly in the document?

 
public class ConsistentCompoundExpressionQuickFix extends LocalQuickFixBase {
  private static final Logger LOG = Logger.getInstance("#de.halirutan.mathematica.codeinsight.inspections.codestyle.ConsistentCompoundExpressionQuickFix");

  public
ConsistentCompoundExpressionQuickFix() {
    super("Fix missing semicolon");
  
}

  @Override
  public void applyFix
(@NotNull final Project project, @NotNull final ProblemDescriptor descriptor) {
    final PsiElement element = descriptor.getPsiElement();
    final
PsiElement newElement;

    if
(!FileModificationService.getInstance().preparePsiElementForWrite(element)) {
      LOG.warn("cannot write");
      return;
    
}

    if (element instanceof Expression) {
      final MathematicaPsiElementFactory factory = new MathematicaPsiElementFactory(project);
      
newElement = factory.createExpressionFromText(element.getText() + ";");
      
element.replace(newElement);
    
} else {
      LOG.warn("Element was no Expression " + element.getTextOffset());
    
}
  }
}
4 comments
Avatar
Patrick Scheibe
Comment actions Permalink

Additionally, when I don't *replace the Psi elements* but only add a semicolon with

final MathematicaPsiElementFactory factory = new MathematicaPsiElementFactory(project);
final
PsiElement semicolon = factory.createExpressionFromText("a;").getLastChild();
element.add(semicolon);


To run the quick fix on the whole file (1800 lines, 194 inspection warnings) requires several seconds. After this, some places still show the warning, although the semicolon was added.
A look at the PsiViewer reveals, that the Psi tree was not updated and as soon as I insert a character, the whole things is reparsed and the inspection warnings disappear.

0
Avatar
Patrick Scheibe
Comment actions Permalink

After a bit of thinking and searching it is appearently a bad idea to do this quick fix by changing the Psi tree directly, because as I learned, you have to ensure that all changes you
do leave the tree in a correct state. The proble is that inserting a semicolon can change the whole tree and fixing this manually isn't worth the effort.

I'm doing this now by inserting the semicolon directly into the Document. The issue that the quick fix is called for the same element twice sometimes persists. I'm looking into it and I'm reporting back

0
Comment actions Permalink

That's true that PSI changes should result in the same tree structure as if the document was completely reparsed with the new text. So, in your case, modifications via document seem reasonable. After that, the daemon should be restarted automatically, commit the document in background and after some time the stale warnings should disappear (when the new ones are ready). If it still doesn't work as expected, I'd start with putting lots of System.out.println in your inspection & quick fix code, printing expressions or offsets and trying to make sense of this output :)

AFAIK PsiViewer is misleading: it isn't looking into the file's PSI, it just builds a new PSI tree from the file's text.

0
Avatar
Patrick Scheibe
Comment actions Permalink

TL;DR Always register an inspection problem by giving the PsiElement of the largest context that is going to be changed.

Hi Peter,

I was debugging IDEAs routing of applying a quick fix to all found issues and I could find where it goes wrong. The main problem was indeed registering the problem during the inspection. For further references, let me give an example and explain how it can be fixed. Let's assume you have a very simple mathematical expression where the operation between var1 and var2 is missing:

var1 var2


Now, you know that after var1, there is something like a + or - or * missing. When you now register the problem in the inspection with

com.intellij.codeInspection.ProblemsHolder#registerProblem(
    com.intellij.psi.PsiElement, com.intellij.openapi.util.TextRange, 
    java.lang.String, com.intellij.codeInspection.LocalQuickFix...)


you have to think about what you give as PsiElement! The problem I had can be broken down to that I effectively registered the PsiElement of var1 and marking the last character as "error text range".
When we now build a QuickFix that inserts a + there, we have one big problem: After insertion, the whole expression "var1 + var2" is now parsed as a "plus expression". Since this affects the Psi element var2 too, the quick fix changed a greater Psi context than we have given by registering only var1. If you think about a situation like this

var1 var2 + var3 + var4 * var5


you instantly see, that it can indeed affect a large portion of the whole Psi tree. The bug turned out to be in

com.intellij.codeInspection.actions.CleanupInspectionIntention#invoke


where all found problem descriptors are first sorted by reverse text-range and then they are fixed in a loop. I can only speculate where exactly this goes wrong, but what happend is that after some issues were fixed, the list of all issues (it's called descriptions) was changed. I believe that the PsiElements of the elements changed since they are no longer valid after changing (and commiting) the document. As it turns out at some point the list "descriptions" contained duplicate entries which replaced some other elements. Therefore, what effectively happend for me was that something like this

var1 var2 var3 var4


it was turned into

var1 + var2 ++ var3 var4


The solution is simple once you knew that: When you register a proble, always use as PsiElement the largest context that is going to be changed. For me this was the PsiFile. I had to adjust the given TextRange because now it was relativ to the file, but that was it.

0

Please sign in to leave a comment.