PsiElement.replace() proper way to do?

What is the proper way to replace PsiElement?

The only found quick explanation is that:

Did you get an error message together with the exception? It should be "Must not change PSI outside command or undo-transparent action. See com.intellij.openapi.command.WriteCommandAction or com.intellij.openapi.command.CommandProcessor", which suggests which classes to use to fix it (unfortunately, not documented; we'll fix that).

The easiest way to get rid of the exception is to wrap the whole modification into WriteCommandAction.runWriteCommandAction(project, modificationRunnable).

 

When I trying to do that I've got next exception:

Must not start write action from within read action in the other thread - deadlock is coming
java.lang.Throwable: Must not start write action from within read action in the other thread - deadlock is coming
at com.intellij.openapi.diagnostic.Logger.error(Logger.java:136)
at com.intellij.openapi.command.WriteCommandAction.execute(WriteCommandAction.java:145)
at com.intellij.openapi.command.WriteCommandAction.runWriteCommandAction(WriteCommandAction.java:254)
at com.intellij.openapi.command.WriteCommandAction.runWriteCommandAction(WriteCommandAction.java:241)
at com.cmakeplugin.utils.CMakePSITreeSearch.findVarDefsAtDirScope(CMakePSITreeSearch.java:68)
at com.cmakeplugin.utils.CMakePSITreeSearch.findVariableDefinitions(CMakePSITreeSearch.java:36)
at com.cmakeplugin.psi.impl.CMakePsiImplUtil$MyPsiPolyVariantReferenceBase.multiResolve(CMakePsiImplUtil.java:91)
at com.intellij.psi.PsiPolyVariantReferenceBase.resolve(PsiPolyVariantReferenceBase.java:46)
at com.intellij.codeInsight.intention.AddAnnotationPsiFix.getContainer(AddAnnotationPsiFix.java:76)
at com.intellij.codeInsight.intention.impl.AddAnnotationIntention.isAvailable(AddAnnotationIntention.java:46)
at com.intellij.codeInsight.intention.impl.ShowIntentionActionsHandler.availableFor(ShowIntentionActionsHandler.java:130)
at com.intellij.codeInsight.daemon.impl.ShowIntentionsPass.lambda$getActionsToShow$2(ShowIntentionsPass.java:337)
at com.intellij.codeInsight.intention.impl.ShowIntentionActionsHandler.chooseBetweenHostAndInjected(ShowIntentionActionsHandler.java:155)
at com.intellij.codeInsight.daemon.impl.ShowIntentionsPass.getActionsToShow(ShowIntentionsPass.java:336)
at com.intellij.codeInsight.daemon.impl.ShowIntentionsPass.getIntentionActionsToShow(ShowIntentionsPass.java:269)
at com.intellij.codeInsight.daemon.impl.ShowIntentionsPass.doCollectInformation(ShowIntentionsPass.java:245)
at com.intellij.codeHighlighting.TextEditorHighlightingPass.collectInformation(TextEditorHighlightingPass.java:70)
at com.intellij.codeInsight.daemon.impl.PassExecutorService$ScheduledPass.lambda$null$1(PassExecutorService.java:437)
at com.intellij.openapi.application.impl.ApplicationImpl.tryRunReadAction(ApplicationImpl.java:1127)
at com.intellij.codeInsight.daemon.impl.PassExecutorService$ScheduledPass.lambda$doRun$2(PassExecutorService.java:430)
at com.intellij.openapi.progress.impl.CoreProgressManager.registerIndicatorAndRun(CoreProgressManager.java:543)
at com.intellij.openapi.progress.impl.CoreProgressManager.executeProcessUnderProgress(CoreProgressManager.java:488)
at com.intellij.openapi.progress.impl.ProgressManagerImpl.executeProcessUnderProgress(ProgressManagerImpl.java:94)
at com.intellij.codeInsight.daemon.impl.PassExecutorService$ScheduledPass.doRun(PassExecutorService.java:429)
at com.intellij.codeInsight.daemon.impl.PassExecutorService$ScheduledPass.lambda$run$0(PassExecutorService.java:405)
at com.intellij.openapi.application.impl.ReadMostlyRWLock.executeByImpatientReader(ReadMostlyRWLock.java:143)
at com.intellij.openapi.application.impl.ApplicationImpl.executeByImpatientReader(ApplicationImpl.java:229)
at com.intellij.codeInsight.daemon.impl.PassExecutorService$ScheduledPass.run(PassExecutorService.java:403)
at com.intellij.concurrency.JobLauncherImpl$VoidForkJoinTask$1.exec(JobLauncherImpl.java:170)
at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

I can see com.intellij.openapi.application.impl.ApplicationImpl.tryRunReadAction is invoked far deeper in the stack then all of my code. So how can I do my Write action out or Read action?...

PS Just in case I do something wrong, here is the code of my function. I'm looking for all CMakeUnquotedArgumentContainer elements that hold the name of Variable and then trying to replace them with CMakeVariableDeclaration(implements PsiNameIdentifierOwner) elements (that is needed to provide PsiNameIdentifierOwner interface only for Variable declarations):

  @NotNull
private static List<PsiElement> findVarDefsAtDirScope(@NotNull PsiElement varElement, String varName) {
Project project = varElement.getProject();
List<PsiElement> result = new ArrayList<>();
Collection<VirtualFile> virtualFiles =
FileBasedIndex.getInstance().getContainingFiles(FileTypeIndex.NAME, CMakeFileType.INSTANCE,
GlobalSearchScope.allScope(project)); // fixme: Directory Scope needed
for (VirtualFile virtualFile : virtualFiles) {
CMakeFile cmakeFile = (CMakeFile) PsiManager.getInstance(project).findFile(virtualFile);
if (cmakeFile != null) {
Collection<CMakeUnquotedArgumentContainer> unquotedArgs = PsiTreeUtil.findChildrenOfType(cmakeFile, CMakeUnquotedArgumentContainer.class);
for (CMakeUnquotedArgumentContainer unquotedArg : unquotedArgs) {
if (varName.equals(unquotedArg.getText())) {
WriteCommandAction.runWriteCommandAction(project, () -> {
unquotedArg.replace(createVariableDeclarationFromText(project, varName));
});
}
}
Collection<PsiElement> varDefs = PsiTreeUtil.findChildrenOfType(cmakeFile, CMakeVariableDeclaration.class);
for (PsiElement varDef : varDefs) {
if (varName.equals(varDef.getText())) {
result.add(varDef);
}
}
}
}
return result;
}
13 comments

Of course, a quick fix is not required to modify the PSI, in general.  What both Peter and I agree on is that the highlighting and semantic annotation passes should not be used to effect PSI changes.  Causing changes to run later, on a write thread, is appropriate.  I mention quick fixes as a proper way to signal issues because that places control back into the user's hands; they can decide whether or not to make such changes.

In general, you don't want to use highlighting and annotation passes to make changes because: 1) the always run on a background thread, 2) they are cancelable (and often canceled) by user activity or timeouts, 3) changes to the PSI cause the process to start over (see reason 2), which makes the process appear to be slow when the user is viewing the end of files.

1

Yes, modifying the code when a file is just opened in the editor and highlighted can be quite surprising for people. invokeLater+WriteCommandAction gets rid of the assertions, but it still leads to the code being modified when the user probably doesn't expect it to be. Normally, modifications are results of direct user actions, and as Eric points out, a quick fix or an intention action is one way to achieve that. It could be some AnAction as well.

Maybe the problem you're trying to solve doesn't require replacing project PSI? It might help if you explain that problem.

Actually, highlighting and annotating are very similar. Annotator is just one thing that's run during highlighting, others being HighlightVisitors, inspections, and other kinds of highlighting passes. I wouldn't distinguish between them very much. They're all run concurrently, and so I wouldn't also depend on them being in any kind of order. But they all run over PSI corresponding to the text in the file, so the file is definitely lexed and parsed before that. There are no guarantees about the thread that happens in, but it's definitely in a read action.

1

Replacing existing PSI is very fragile (e.g. it will be lost after reparse and possibly break the code that doesn't expect to see your PSI), and, as we see, you can't do it during highlighting.

I'd suggest to create a non-physical declaration (something extending PomNamedTarget, PomRenameableTarget or a related interface, maybe also PsiTarget), return it (via PomService#convertToPsi result) from your references' "resolve", and create a PomDeclarationSearcher that returns that target when the caret is on the declaration. Unfortunately this API is not documented at all (we'll be working on improving that), but you can check some usage examples in the IDEA source, and, of course, ask questions.

1

Indeed, there's no way currently to add custom references to just any PSI element. So CPP-15012 is the best way to achieve your goal.

You could try to work around that by implementing com.intellij.codeInsight.TargetElementEvaluatorEx2#adjustReferenceOrReferencedElement, which is called when "goto declaration" is invoked. You won't get Find Usages or refactorings from it though, but if you need them, there are other extensions which can be used. But it's not an easy workaround, unfortunately.

1

Having such generically reference-contributable PSI is a possibility, thanks. Your use case counts in its favor. Unfortunately it's not easy to implement, as all clients of "getReference(s)" (including third-party plugins) would have to be converted to call some new API (because "final defaults" are impossible on JVM). In addition, it's not clear which elements can contribute references at a specific location: traversing all tree can be expensive, and now there are APIs like "HintedReferenceHost#shouldAskParentForReferences" which prevent that. But that'd go against the "freedom of contributing references everywhere".

Besides, there are some reasons it hasn't been done before. E.g. this makes people believe that all resolve should be implemented and customized by reference contributors, even in cases where hardcoded references and a custom extension policy would be a better choice (which is the majority of programming languages). For example, plugins probably shouldn't provide its own references in Java code in addition to the ones we already have, that'd make navigation and completion crazy. Even crazier if several plugins do that in one place.

1

You shouldn't modify the code from within editor highlighting. The method name "findVarDefsAtDirScope" also suggests that it finds something, not that it modifies the text.

0

To be a little more helpful:  You should create an Annotator (https://www.jetbrains.org/intellij/sdk/docs/tutorials/custom_language_support/annotator.html) to run your analysis.  It should not make any changes to the PSI, either; it should register a "quick fix" (https://www.jetbrains.org/intellij/sdk/docs/tutorials/custom_language_support/quick_fix.html), and *that* should do the work.  Note, in the latter example, how the invoke() function calls invokeLater() -- that will cause the Runnable to run on the write thread.

0

Peter Gromov:

Thank you for your response. That's just very draft prototyping to see if the idea works at all. Sure PSI tree modification should be in a separate method.

At the moment function gindVar... is used in both annotator explicitly and in highlighter implicitly (through getReferences() -> multiResolve() - > findVar...). But the fact that the findVar... function been called from Highlighting process before annotating process surprise me too.

Is there any explanation of how the source file processing flow works? I understood that first (?) Lexer process is going, then Parser process and then?... annotating? inspections?... Unfortunately, Architectural Overview and Fundamentals doesn't really help a lot to understand that.

Eric Bishton:

Thank you for pointing me in the right direction! After reviewing your links I found that we don't really need to implement "quick fix" to modify PSI. All we need just wrap WriteCommandAction.runWriteCommandAction into ApplicationManager.getApplication().invokeLater call. Unfortunately, there is not too much information how it all works except that paragraph at General Threading Rules.

 

So the next code will do the job:

ApplicationManager.getApplication().invokeLater( () -> {
WriteCommandAction.runWriteCommandAction(project, () -> {
// Your code here to modify PSI tree like that:
// unquotedArg.replace(createVariableDeclarationFromText(project, varName));
});
});

PS For the reason that other plugging developer will probably find that thread in search for PSI modification, would be nice if Peter Gromov and Eric Bishton or other experts comment that solution.

0

Thanks, guys!

Your arguments against changing PSI while annotating document are clear and have points.

Lets me explain my problem, hope you might give me a better way to solve it then I found.

I have a cmake command to annotate. Very simplified grammar is:

cmd ::=  identifier "(" unquoted_argument ")"

unquoted_argument can contain variable_declaration | variable_reference | cmake_property | cmake_operator | path_url | unquoted_legacy | etc. I can't recognize all that subspecies of unquoted_argument at the lexing process for few reasons:

  1. Want to maintain maximum unification with builtin CLion cmake plugin, that doesn't split unquoted_argument into subspecies.
  2. Using a relatively large list of keywords for predefined variables and properties at .flex file will slow down (and even freeze)  lexer code generating process.

For making reference/resolve functionality for variable references/declarations, I have to make unquoted_argument implement PsiNameIdentifierOwner interface. That leads to few issues:

  1. Multiple declarations highlighting issues in IDEA

  2. All cmake_property | cmake_operator | path_url | unquoted_legacy | etc. will implement PsiNameIdentifierOwner interface as well. Which make them highlighted when the mouse (with pressed Ctr) is over the element or caret is placed inside that element. That mislead the user to presume that element has some reference to/from despite its just reference to itself.

As a solution I decided to inject variable_declaration element (implement PsiNameIdentifierOwner instead of implementing it by unquoted_argument) into grammar, that is not declared in the lexer (so could be created only explicitly by me with some dirty hack).

cmd ::=  identifier "(" unquoted_argument_container | variable_declaration ")"
unquoted_argument_container ::= unquoted_argument { methods=[getReferences]}
variable_declaration ::= unquoted_argument { methods=[getName setName getNameIdentifier] }

// dirty hack to explicitly create variable_declaration elemant at CMakePsiElementFactory.createVariableDeclarationFromText()
fake_cmd_for_var_delaration ::= "FAKE_COMMAND_NAME_FOR_VAR_DECLARATION_CREATION_1234567890(" variable_declaration ")"

Then during annotation process, I find a variable reference (${}) and look for all unquoted_arguments_container elements with the declaration of inspected variable reference and replace them with variable_declaration elements containing absolutely the same text (so for user nothing visually changed, all changes just inside PSI tree).  

It works, a bit slow for now, need to optimize it, but it works.

The problem with that solution: In the future, I will need to replace PSI element in CLion cmake plugin PSI tree without an ability to modify it parser code generated from BNF: create NewPsiElement that extend ExistingPsiElement.

Will highly appreciate any thought about that and any advices how to make it better.

0

Sorry for the long delay here...

After meditation for a few weeks on what Peter said above and digging into IDEA source code and its plugins, I found the next: It's not possible to extend any reference/resolve functionality for psiElements that not support any way of extension (i.e. if that PsiElements not supporting reference contributors or implementing PomTarget interface). Please, correct me if I'm wrong.

And if there is any way to create reference/resolve functionality "parallel" to the existing Psi tree (i.e. without a possibility to change any of existing class for current Psi tree), I would highly appreciate any link and class/interface name to looking for.

PS In particular, I'm looking the way to extend current CMake psi tree (com.jetbrains.cmake) to implement variables reference/resolve. The only way I can see that might help, will be support of reference contributors at com.jetbrains.cmake.psi.CMakeArgumentImpl: https://youtrack.jetbrains.com/issue/CPP-15012

0

Thank you! Will definitely look into TargetElementEvaluatorEx2 to see how it may help. (Actually, bumped into that class few times while wandering through IDEA source, but didn't carefully explore it yet...)

Just a general thought: Why all that reference/resolve functionality so much hardly "nailed" into particular PsiElement class implementation? Wouldn't it be easier if, by default, any psiElement be referencable and able to refer to any other psiElement(s)? That way will be no difference between language implementer and third party plugins. Also, that would better split(abstract) lexer/parser level of languages from it higher references/inspections/etc. levels.

That could be partially implemented, for example, through some kind of "final default" #getRefernces() method in PsiElement interface which calls all corresponding references contributors. So any third party plugin developer would not rely on (hope for) goodwill of language implementer to check for third parties references contributors (how that unfortunately made now), but language implementor would be acting as equal to others and just implement (if he willing to) his version of references contributor.

0

Playing around com.intellij.codeInsight.TargetElementEvaluatorEx2, got next questions:

Looks like its already implemented and loaded in builtin Cmake plugin:

<targetElementEvaluator language="CMake" implementationClass="com.jetbrains.cmake.resolve.CMakeElementEvaluator"/>

A possible workaround would be to extend my ElementEvaluator from it (with the implementation of adjustReferenceOrReferencedElement) and replace original CMakeElementEvaluator somehow. Sounds a bit hacky but... It's not an explicit extension, so  LanguageExtension#removeExplicitExtension/addExplicitExtension wouldn't work here. Diving a bit into com.intellij.openapi.extensions package, but been lost there :( Does that whole idea of replacing CMakeElementEvaluator make sense? And if so, is there any particular class(es) that might help?

PS I think even short draft general version of the Component Model article in http://www.jetbrains.org/intellij/sdk/docs/platform/fundamentals.html would be really helpful for me and fellow plugin developers...

0

Oh, indeed, I didn't expect someone already declaring an evaluator for this language. You could try registering your extension before the original one, by adding order="first". It's quite hacky indeed, I'd try pinging CMake support developers to increase the priority of custom reference contribution API.

0

Please sign in to leave a comment.