syntax error highlighting quirks

[dtwelch]

This issue was migrated from this jetbrains dev forum post; you can the whole discussion up to this point there. I kind of hijacked that thread -- sorry about that! Probably should've moved stuff here sooner :)

Here are some of the latest syntax error highlighting quirks I've stumbled across.

I have a uses/import list rule that looks like:

usesList
    :   'uses' ID (',' ID)* ';'
    ;

So I type

Precis X;
     uses y

end X;

doesn't point out the missing semicolon after y.
If I do something like uses x, y it'll complain -- so it's only for lists with a single element then I guess?

Another one too:

Precis X;

     //error, should be something (not semi colon) immediately after ':'
     Definition x : ;
end X;

The grammar for this lang can be read here. It has a fairly big expression hierarchy and just two types of declarations: definitions and theorems. Definition's can take on several signature styles such prefix, postfix, outfix. I've only tried the prefix right now, before I ran into the issue mentioned above.

The rules composing the definition construct mentioned above starts at mathStandardDefinitionDecl (the signature referenced will be mathPrefixDefinitionSig).

[kshchepanovskyi]

I faced with the same problem, some syntax errors are not highlighted.

As far as I understand, parseTree created inside of org.antlr.jetbrains.adapter.parser.ANTLRParserAdaptor#parse(com.intellij.psi.tree.IElementType, com.intellij.lang.PsiBuilder) does not contain error nodes for some types of syntax errors. Later, error nodes are highlighted using org.antlr.jetbrains.adapter.parser.ANTLRParseTreeToPSIConverter#visitErrorNode, but it does not happen as they do not exist.

[parrt]

Thanks guys. I hope to get back to the plugin in October.

[kshchepanovskyi]

In the org.antlr.jetbrains.adapter.parser.ANTLRParseTreeToPSIConverter, we do not intercept all errors:

• #visitErrorNode is not invoked for non-existing ErrorNode-s (missing tokens, in this case)
• #enterEveryRule does not check ctx.exception value - it is set to org.antlr.v4.runtime.NoViableAltException

I will try to mark errors in #enter/exitEveryRule, probably it is the solution.

[parrt]

thanks!

[kshchepanovskyi]

Following code solves the problem, but I'm not 100% sure that it is the best solution.

diff --git a/src/main/java/org/antlr/jetbrains/adapter/parser/ANTLRParseTreeToPSIConverter.java b/src/main/java/org/antlr/jetbrains/adapter/parser/ANTLRParseTreeToPSIConverter.java
index bf2f787..e12351e 100644
--- a/src/main/java/org/antlr/jetbrains/adapter/parser/ANTLRParseTreeToPSIConverter.java
+++ b/src/main/java/org/antlr/jetbrains/adapter/parser/ANTLRParseTreeToPSIConverter.java
@@ -6,6 +6,7 @@ import com.intellij.openapi.progress.ProgressIndicatorProvider;
 import org.antlr.jetbrains.adapter.lexer.PSIElementTypeFactory;
 import org.antlr.jetbrains.adapter.lexer.RuleIElementType;
 import org.antlr.jetbrains.adapter.lexer.TokenIElementType;
+import org.antlr.runtime.RecognitionException;
 import org.antlr.v4.runtime.ANTLRErrorListener;
 import org.antlr.v4.runtime.Parser;
 import org.antlr.v4.runtime.ParserRuleContext;
@@ -30,7 +31,7 @@ import java.util.Map;
 public class ANTLRParseTreeToPSIConverter implements ParseTreeListener {
    protected final Language language;
    protected final PsiBuilder builder;
-   protected List<SyntaxError> syntaxErrors;
+   protected Map<RecognitionException, SyntaxError> syntaxErrors;
    protected final Deque<PsiBuilder.Marker> markers = new ArrayDeque<PsiBuilder.Marker>();

    protected final List<TokenIElementType> tokenElementTypes;
@@ -48,8 +49,8 @@ public class ANTLRParseTreeToPSIConverter implements ParseTreeListener {

        for (ANTLRErrorListener listener : parser.getErrorListeners()) {
            if (listener instanceof SyntaxErrorListener) {
-               syntaxErrors = ((SyntaxErrorListener)listener).getSyntaxErrors();
-               for (SyntaxError error : syntaxErrors) {
+               syntaxErrors = ((SyntaxErrorListener)listener).getErrorMap();
+               for (SyntaxError error : syntaxErrors.values()) {
                    // record first error per token
                    int StartIndex = error.getOffendingSymbol().getStartIndex();
                    if ( !tokenToErrorMap.containsKey(StartIndex) ) {
@@ -115,6 +116,7 @@ public class ANTLRParseTreeToPSIConverter implements ParseTreeListener {
     *  prediction started (which we use to find syntax errors). So,
     *  SyntaxError objects return start not offending token in this case.
     */
+   @Override
    public void visitErrorNode(ErrorNode node) {
        ProgressIndicatorProvider.checkCanceled();

@@ -159,6 +161,16 @@ public class ANTLRParseTreeToPSIConverter implements ParseTreeListener {
    public void exitEveryRule(ParserRuleContext ctx) {
        ProgressIndicatorProvider.checkCanceled();
        PsiBuilder.Marker marker = markers.pop();
-       marker.done(getRuleElementTypes().get(ctx.getRuleIndex()));
+       if (ctx.exception != null) {
+           SyntaxError error = syntaxErrors.get(ctx.exception);
+           if (error != null) {
+               marker.error(error.getMessage());
+           } else {
+               // should not happen
+               marker.error("syntax error");
+           }
+       } else {
+           marker.done(getRuleElementTypes().get(ctx.getRuleIndex()));
+       }
    }
 }
diff --git a/src/main/java/org/antlr/jetbrains/adapter/parser/SyntaxErrorListener.java b/src/main/java/org/antlr/jetbrains/adapter/parser/SyntaxErrorListener.java
index c7bc748..e2d15f8 100644
--- a/src/main/java/org/antlr/jetbrains/adapter/parser/SyntaxErrorListener.java
+++ b/src/main/java/org/antlr/jetbrains/adapter/parser/SyntaxErrorListener.java
@@ -14,12 +14,16 @@ import java.util.List;
  *  This swallows the errors as the PSI tree has error nodes.
  */
 public class SyntaxErrorListener extends BaseErrorListener {
-   private final List<SyntaxError> syntaxErrors = new ArrayList<SyntaxError>();
+   private final Map<RecognitionException, SyntaxError> syntaxErrors = new HashMap<RecognitionException, SyntaxError>();

    public SyntaxErrorListener() {
    }

-   public List<SyntaxError> getSyntaxErrors() {
+   public Collection<SyntaxError> getSyntaxErrors() {
+       return syntaxErrors.values();
+   }
+
+   public Map<RecognitionException, SyntaxError> getErrorMap() {
        return syntaxErrors;
    }

@@ -29,11 +33,13 @@ public class SyntaxErrorListener extends BaseErrorListener {
                            int line, int charPositionInLine,
                            String msg, RecognitionException e)
    {
-       syntaxErrors.add(new SyntaxError(recognizer, (Token)offendingSymbol, line, charPositionInLine, msg, e));
+       syntaxErrors.put(e, new SyntaxError(recognizer, (Token)offendingSymbol, line, charPositionInLine, msg, e));
    }

+
+
    @Override
    public String toString() {
-       return Utils.join(syntaxErrors.iterator(), "\n");
+       return Utils.join(syntaxErrors.values().iterator(), "\n");
    }
 }

Can somebody give me a hint why in some cases we have error nodes, in other cases - there is only an exception in the rule context?

[Shan1024]

@kshchepanovskyi @parrt Hi guys, can you please merge this fix to the master branch since it will be very useful to others?

[parrt]

Hi. I'm useless until mid October again and then can do antlr stuff. sorry!

[kshchepanovskyi]

@Shan1024 I can publish my fork to maven central, if you want.

It is used in https://plugins.jetbrains.com/plugin/8277-protobuf-support, it is quite stable and I think might be useful to others.

[Shan1024]

Thanks for the quick replies.

@parrt This is a really useful library and hope to see more features when you are available :)

@kshchepanovskyi I have added this repo as a sub-module. I will check whether I can use your fork as a sub-module. I think it would be better if you can send a PR to this repo so that @parrt can check and merge it when he is available. Thanks a lot for the fix.

[kshchepanovskyi]

@Shan1024 I published patch here; my fork contains many other changes, unfortunately I cannot make regular PR, it will take too much time.

[Shan1024]

@kshchepanovskyi I just sent a PR with your changes (#11). Terence can check and merge it when he is available.

I noticed that after applying this change, live templates stopped working. Do you have any idea why is that?

[kshchepanovskyi]

I have no idea, patch is quite small and has no direct relation to live templates.
Are there any exceptions in logs?

[Shan1024]

@kshchepanovskyi Found the issue. It was in the file context checking logic. Applying the fix creates a new PsiErrorElement which was not expected in file context. So updating the logic fixed the issue.

[kshchepanovskyi]

Can you please add more details how did you update the logic?

[Shan1024]

Issue was in the TemplateContextType subclass which I have implemented which identifies the context type to suggest live templates. By applying the fix, PSI tree structure changed because it adds PsiErrorElement to top. Previously it was not there and I did not account it to the file context checking logic. So the file context checking logic did not correctly identify the file context, hence did not suggest any live template. Sorry for not making that clear in the last message :(

[kshchepanovskyi]

Thanks for details. I was afraid that there is an issue in my code :)

[RAVEENSR]

@parrt can you look into this problem and give a solution. With the solution @Shan1024 and @kshchepanovskyi provided, sometimes some nodes are not build because of the error is shown on top of that node.

[sischnei]

I just ran into this same issue and scratched my head for about a day or so until stumbling upon this ticket. Could this please be merged into a new release? Thank you guys for your great work, really appreciated (migrating our old parser + IDE support over to ANTLR saved us a ton of work already!)

[parrt]

hi. Sorry about that. I just I haven't been able to get back to this project! Glad that the tool itself is useful :)

[bjansen]

Is this really the correct fix though?

If I try it on the grammar posted in the original comment, the error is highlighted but it feels like it's not at the right location:

OTOH the ANTLR preview seems correct:

@kshchepanovskyi did you notice this in your fork?

[bjansen]

Looking for errors in visitTerminal instead of exitEveryRule seems to yield better results:

The generated tree is also more correct:

FILE
  ANTLRPsiNode(block)
    PsiElement('start')('start ')
    PsiElement(ID)('X')
    PsiElement(';')(';\n    ')
    ANTLRPsiNode(usesList)
      PsiElement('uses')('uses ')
      PsiElement(ID)('ducks\n')
    PsiErrorElement:mismatched input 'end ' expecting {';', ','}
      PsiElement('end')('end ')
    PsiElement(ID)('X')
    PsiElement(';')(';')

Whereas with #19 it was:

FILE
  ANTLRPsiNode(block)
    PsiElement('start')('start ')
    PsiElement(ID)('X')
    PsiElement(';')(';\n    ')
    PsiErrorElement:mismatched input 'end ' expecting {';', ','}
      PsiElement('uses')('uses ')
      PsiElement(ID)('ducks\n')
    PsiElement('end')('end ')
    PsiElement(ID)('X')
    PsiElement(';')(';')