Multiple declarations highlighting issues in IDEA

I'm making CMake highlighter plugin for IDEA/CLion.

Right now I'm trying to implement variable declarations/references highlighting and navigation. With navigation, I found few issues that look for me to be produced by IDEA. Need help from the community and JB experts.

At CMake, the variable definition is declaration as well. That lead to the necessity of implementing multiResolve() for variable references.

For similarity to PSI structure to the builtin CLion cmake support plugin, I don't split variable references from an Unquoted argument at the Lexer stage.

As a result Unquoted_argument PsiElement can be both containing variable references `${var1}` (implement getReferences() ) and variable declaration `var1` (implement PsiNameIdentifierOwner interface). 

Quoted_argument PsiElement `"${var1}"` can only contain variable references (implement getReferences() ).

With code like that:

if()
set(var1 1)
else()
set(var1 3)
endif()
set(var2 2)
message(${var1}
"${var1}"
${var2}
${var2})

Find Usages from declarations of var1 and var2  Ctrl+Alt+7 / Ctrl+B / Ctrl+click works as expected (show the list of multi usages or jump to single usage).

Go to Declaration (Ctrl+B) and Ctrl+click works for all references (show the list of declarations or jump to single declaration).

Highlighting issues:

Placing the cursor into var1 at "${var1}" line show expected results (highlight all declarations and usages of var1):

Placing the cursor into var2 at {var2} line show expected results (highlight all declarations and usages of var2):

Issue: Placing the cursor into var1 at ${var1} line highlight only that element itself:

Show Definition Ctrl+Shift+I shows correct definition(s) for ${var2} and "${var1}"

Issue: Show Definition Ctrl+Shift+I show same current code line only is shown for ${var1}.

Issue: Also, Ctrl+Alt+7 works a bit wired for both "${var1}" and ${var1}: shown first Find Usages dialog and then immediately Usages Of dialog. I didn't explore that but believe it linked to the above highlighter issue.

 

My research:

Playing with debugger through all internal IDEA invocations I found that all combinations invoke TargetElementUtil.findTargetElement()->...->findReference() method which return PsiReference to PsiElement with correct declaration from resolve() for single declaration case. And return Null from resolve() method for multi references (as expected).

The different is that Ctrl+click check for PsiPolyVariantReference instance at CtrlMouseHandler.getInfoAt()->resolve().

And highlighting process use IdentifierHighlighterPass.doCollectInformation() and using the flag TargetElementUtil.ELEMENT_NAME_ACCEPTED that leads an invocation of getNamedElement() that return posion of current caret PsiNameIdentifierOwner element (i.e current var reference we trying to find declaration for) for some reasons I can't understand...

So, declaration nature of PsiElement (PsiNameIdentifierOwner interface) "overlapping" reference nature of PsiElement (from implementation getReferences() ), but only for  element with multi declarations.

If this is a feature, not a bug, why it's not "overlapping" for single declaration elements?

It looks to me that using TargetElementUtil.ELEMENT_NAME_ACCEPTED when invoking IdentifierHighlighterPass.doCollectInformation() is wrong for elements that implement PsiPolyVariantReference and has multiple declarations.

Sorry for a bit chaotic explanation. Hope you got the idea of the problem (bug?). Will give any required additional explanation if needed.

Would be great if someone who knows any other multi declarations support plugin can check that behavior there.

Current version branch for reproducing issues (sorry for a "dirty" code, work is in progress and also I'm not an expert in Java development, any bits of advice are welcome): https://github.com/ArtsiomCh/CMake/tree/var_refs_test

 

 

6 comments

Hi! Could you verify that you have the correct implementation of `com.intellij.psi.PsiReferenceBase#isReferenceTo`. In your case the default implementation could working wrong (for multi references).

0

Hi, Alexey!

Thnk you for your response! I extend com.intellij.psi.PsiPolyVariantReferenceBase which override isReferenceTo method in the proper way:

@Override
public boolean isReferenceTo(PsiElement element) {
final ResolveResult[] results = multiResolve(false);
for (ResolveResult result : results) {
if (getElement().getManager().areElementsEquivalent(result.getElement(), element)) {
return true;
}
}
return false;
}

The problem is in case of PsiElement implement PsiNameIdentifierOwner interface and has multiple references (i.e. getReferences().lehght > 1). Then in highlighting process com.intellij.codeInsight.daemon.impl.IdentifierHighlighterPass#doCollectInformation()

myTarget = TargetElementUtil.getInstance().findTargetElement(myEditor, flags, myCaretOffset);

should be null and then branch for multiResolve will be invoked:

if (myTarget != null) {
highlightTargetUsages(myTarget);
} else {
PsiReference ref = TargetElementUtil.findReference(myEditor);
if (ref instanceof PsiPolyVariantReference) {
if (!ref.getElement().isValid()) {
throw new PsiInvalidElementAccessException(ref.getElement(), "Invalid element in " + ref + " of " + ref.getClass() + "; editor=" + myEditor);
}
ResolveResult[] results = ((PsiPolyVariantReference)ref).multiResolve(false);
if (results.length > 0) {
for (ResolveResult result : results) {
PsiElement target = result.getElement();
if (target != null) {
if (!target.isValid()) {
throw new PsiInvalidElementAccessException(target, "Invalid element returned from " + ref + " of " + ref.getClass() + "; editor=" + myEditor);
}
highlightTargetUsages(target);
}
}
}
}

}

But com.intellij.codeInsight.TargetElementUtil.findTargetElement() -> doFindTargetElement() return current PsiElement insteed of desired null (from getNamedElement(), because ELEMENT_NAME_ACCEPTED flag is set):

PsiElement element = file.findElementAt(offset);
if (BitUtil.isSet(flags, REFERENCED_ELEMENT_ACCEPTED)) {
final PsiElement referenceOrReferencedElement = getReferenceOrReferencedElement(file, editor, flags, offset);
//if (referenceOrReferencedElement == null) {
// return getReferenceOrReferencedElement(file, editor, flags, offset);
//}
if (isAcceptableReferencedElement(element, referenceOrReferencedElement)) {
return referenceOrReferencedElement;
}
}

if (element == null) return null;

if (BitUtil.isSet(flags, ELEMENT_NAME_ACCEPTED)) {
if (element instanceof PsiNamedElement) return element;
return getNamedElement(element, offset - element.getTextRange().getStartOffset());
}
return null;

And  com.intellij.codeInsight.TargetElementUtil.getNamedElement() return current PsiElement because it extend PsiNamedElement.class

PsiElement parent;
if ((parent = PsiTreeUtil.getParentOfType(element, PsiNamedElement.class, false)) != null) {
boolean isInjected = parent instanceof PsiFile
&& InjectedLanguageManager.getInstance(parent.getProject()).isInjectedFragment((PsiFile)parent);
// A bit hacky depends on navigation offset correctly overridden
if (!isInjected && parent.getTextOffset() == element.getTextRange().getStartOffset()) {
if (evaluator == null || evaluator.isAcceptableNamedParent(parent)) {
return parent;
}
}
}

So, for me it looks like the problem is inside TargetElementUtil implementation. I would presume that ELEMENT_NAME_ACCEPTED flag should not be used in the cases similar to mine (PsiElement implement PsiNameIdentifierOwner interface and has multiple references). But to be honest, purpose of that flag is unclear for me so I might be wrong...

0

OK. I spent some time to your project.

Here are that I found:
1. If you are implementing `getNameIdentifier`, please overwrite `getName` and `getTextOffset` (com.cmakeplugin.psi.impl.CMakeUnquotedArgumentContainerImpl).
.....
  @Override
  public int getTextOffset() {
    return getNameIdentifier().getTextOffset();
  }


  @NotNull
  @Override
  public String getName() {
    return getNameIdentifier().getText();
  }  
 
  @NotNull
  public PsiElement getNameIdentifier() {
    return CMakePsiImplUtil.getNameIdentifier(this);
  }
....

2. To highlight ids right, you need implement the instance of `FindUsagesHandlerFactory` and register it as
<findUsagesHandlerFactory implementation="xxxx.CMakeFindUsagesHandlerFactory"/>

3. Maybe you need implement and register the instance of `TargetElementEvaluatorEx2`, but is is more for navigation purposes

0

Thank you, Alexey for taking a look!

1. setName, getName, getNameIdetifier already implemented in CMakePsiImplUtil:

public class CMakePsiImplUtil {
@NotNull
public static String getName(CMakeUnquotedArgumentContainer o) {
return ObjectUtils.assertNotNull(o.getUnquotedArgument()).getText();
}

@NotNull
public static CMakeUnquotedArgumentContainer setName(CMakeUnquotedArgumentContainer o, String newName) {
ObjectUtils.assertNotNull(o.getUnquotedArgument())
.replace(CMakePsiElementFactory.createUnquotedArgumentFromText(o.getProject(), newName));
return o;
}

@NotNull
public static PsiElement getNameIdentifier(CMakeUnquotedArgumentContainer o) {
return o.getUnquotedArgument();
}

And linked to CMakeUnquotedArgumentContainerImpl through cmake_v3.bnf:

unquoted_argument_container ::= unquoted_argument { methods=[getName setName getNameIdentifier getReferences] }

Also, We shouldn't directly modify CMakeUnquotedArgumentContainerImpl as its generated file, don't we?

 

getTextOffset already implemented in ancestor ASTDelegatePsiElement:

@Override
public int getTextOffset() {
return getNode().getStartOffset();
}

What the purpose to override it? Even after doing that I don't see any changes in highlighting.

2. What is FindUsagesHandlerFactory class for? I implemented FindUsagesProvider. Isn't it enough for Find Usage?

public class CMakeFindUsagesProvider implements FindUsagesProvider {
private static final DefaultWordsScanner WORDS_SCANNER =
new DefaultWordsScanner(new CMakeLexerAdapter(),
TokenSet.create(CMakeTypes.COMMAND_NAME),
TokenSet.create(CMakeTypes.LINE_COMMENT,
CMakeTypes.BRACKET_COMMENT),
TokenSet.EMPTY);
@Nullable
@Override
public WordsScanner getWordsScanner() {
return WORDS_SCANNER;
}

@Override
public boolean canFindUsagesFor(PsiElement psiElement) {
return isPossibleVarDefinition(psiElement.getText()); /*psiElement instanceof CMakeNamedElement;*/
}

@Nullable
@Override
public String getHelpId(PsiElement psiElement) {
return HelpID.FIND_OTHER_USAGES;
}

@NotNull
@Override
public String getType(PsiElement psiElement) {
return "test Type";//ElementDescriptionUtil.getElementDescription(psiElement, UsageViewTypeLocation.INSTANCE);
}

@NotNull
@Override
public String getDescriptiveName(@NotNull PsiElement psiElement) {
return "test DescriptiveName";//ElementDescriptionUtil.getElementDescription(psiElement, UsageViewLongNameLocation.INSTANCE);
}

@NotNull
@Override
public String getNodeText(@NotNull PsiElement psiElement, boolean b) {
return "test NodeText";//ElementDescriptionUtil.getElementDescription(psiElement, UsageViewNodeTextLocation.INSTANCE);
}
}

FindUsage works fine so far. The problem with highlighting declarations in case of PsiElement implement PsiNameIdentifierOwner interface and has multiple references. And as I describe above it in com.intellij.codeInsight.TargetElementUtil.

0

1. Please, read JavaDoc in community\platform\core-api\src\com\intellij\psi\PsiNameIdentifierOwner.java:

/**
* A Psi element which has a name given by an identifier token in the Psi tree.
* <p/>
* Implementors should also override {@link PsiElement#getTextOffset()} to return
* the relative offset of the identifier token.
*
* @author yole
*/

after that you get non-null myTarget in com.intellij.codeInsight.daemon.impl.IdentifierHighlighterPass#doCollectInformation

and come into the right branch of code for the case of cursor over message(${va|r1}

if (myTarget != null) {
highlightTargetUsages(myTarget); <--- this is the right way!
} else {

2. To execute highlightTargetUsages(myTarget) correctly  the language-specific FindUsagesHandlerFactory need to be implemented.
Your CMakeFindUsagesProvider implementation is used for different purpose - it helps to support full-text search and full-words index. That makes find procedures faster and get you the chance to rename ids in comments/string literals.

3. You need FindUsagesHandlerFactory implementation to distinct the same name local ids in different contexts.

1

1. Looks like we still have misunderstanding what we need to highlight in case of set( va|r1 1) and message(${va|r1}  

Because of dual nature of unquoted_argument it can be variable declaration (implemented through PsiNamedElement/PsinameIdetifierOwner ) or variable reference (throught getReferences() implementation) or neither of those.

In case of set( va|r1 1)  we work with variable declaration nature of unquoted_argument and that works fine.

In case of message(${va|r1} we need to highlight it variable refernce nature, i.e.highlight getReferences->relsolve->getElement for single variable declaration and that works fine. Problem happens when we deal with multi declaration , i.e. getReference->multiResolve.

So, for message(${va|r1} in com.intellij.codeInsight.daemon.impl.IdentifierHighlighterPass#doCollectInformation we need myTarget to be null and then correct branch for multiResolve will be invoked:

if (myTarget != null) {
highlightTargetUsages(myTarget);
} else {
PsiReference ref = TargetElementUtil.findReference(myEditor);
if (ref instanceof PsiPolyVariantReference) { // <---- This is the right way
if (!ref.getElement().isValid()) {
throw new PsiInvalidElementAccessException(ref.getElement(), "Invalid element in " + ref + " of " + ref.getClass() + "; editor=" + myEditor);
}
ResolveResult[] results = ((PsiPolyVariantReference)ref).multiResolve(false);
if (results.length > 0) {
for (ResolveResult result : results) {
PsiElement target = result.getElement();
if (target != null) {
if (!target.isValid()) {
throw new PsiInvalidElementAccessException(target, "Invalid element returned from " + ref + " of " + ref.getClass() + "; editor=" + myEditor);
}
highlightTargetUsages(target);
}
}
}
}

}

That how it works for message("${va|r1}") with mutliple declarations of var1. I checked that with debuger. Here, varaibale reference is inside quoted_urgument which is not inplementing PsiNamedElement/PsinameIdetifierOwner (quoted_urgument can't hold variable declration), and myTarget remains null.

And in case of only single declaration for message(${va|r1} highlighting works too as myTarget will be that single declaration psiElemet (unquited_declaration)) 

Also, all navigations (Ctr;+click, Ctrl+B, etc.) work properly for all cases (as you can see in my initial post) cause none of them use that IdentifierHighlighterPass#doCollectInformation method.

I'm trying to find any other plugin that implements a language that has an element with multiple declarations and implements  PsiNamedElement/PsinameIdetifierOwner to check that highlighting bug, but not yet succeed with that. Would highly appreciate if anyone could point me to such a class. Potentially, any PsiElement that implements PsiNamedElement/PsinameIdetifierOwner and use any of below implementation of multiResolve should be a candidate:

2-3. I still not understand the role of FindUsagesHandlerFactory in my case. As I can see no one plugin really implement it:

And it's not mentioned anywhere in docs... I only see a lot of implementations and usages of com.intellij.lang.findUsagesProvider... I'm a bit confused if my case so unique that require overriding such an internal class...

4. Would be nice to have something like PsiNamedElement#isEnabled method. And use it with the checks for PsiNamedElement instance:

if (element instanceof PsiNamedElement && ((PsiNamedElement)elemnet).isEnabled) ...

In that case, we could implement check if we want to treat that particular element as PsiNamedElement or not. Let say if the element represents some keyword (i.e. definitely can't be declaration), so it doesn't need even show highlighting (like blue underline when a mouse with pressed Ctrl over it). 

0

Please sign in to leave a comment.