Skip to content

Warn about actions not being run by the interpreter #547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Warn about actions not being run by the interpreter #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38d5f8b
feat: #523 warn users that predicates and actions are not run by the …
bjansen Apr 27, 2022
5b70501
feat: #523 allow resizing the error console in the Preview window
bjansen Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/main/java/org/antlr/intellij/plugin/parsing/PreviewParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ public class PreviewParser extends GrammarParserInterpreter {

protected int lastSuccessfulMatchState = ATNState.INVALID_STATE_NUMBER; // not sure about error nodes

private boolean warnedAboutPredicates = false;

public PreviewParser(Grammar g, ATN atn, TokenStream input) {
super(g, atn, input);
lexerWatchdog = new LexerWatchdog(input, this);
Expand All @@ -33,6 +35,7 @@ public void reset() {
super.reset();
if ( inputTokenToStateMap!=null ) inputTokenToStateMap.clear();
lastSuccessfulMatchState = ATNState.INVALID_STATE_NUMBER;
warnedAboutPredicates = false;
}

@Override
Expand Down Expand Up @@ -75,4 +78,23 @@ public Token matchWildcard() throws RecognitionException {
lastSuccessfulMatchState = getState();
return super.matchWildcard();
}

@Override
public void action(RuleContext _localctx, int ruleIndex, int actionIndex) {
warnAboutPredicatesAndActions();
}

@Override
public boolean sempred(RuleContext _localctx, int ruleIndex, int actionIndex) {
warnAboutPredicatesAndActions();
return super.sempred(_localctx, ruleIndex, actionIndex);
}

private void warnAboutPredicatesAndActions() {
if (!warnedAboutPredicates) {
notifyErrorListeners("WARNING: predicates and actions are not run by this interpreter. " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using correct location of action/sempred (pass it to the method), not the first line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that it's already the case:

	public final void notifyErrorListeners(String msg)	{
		notifyErrorListeners(getCurrentToken(), msg, null);
	}

I can see the correct line:column in the generated message, as you can see in #523 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but I'm not sure it's correct. It should point line:column in the grammar not in the input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking of a generic message saying that there are actions or predicates in the grammar. If there are multiple, I'm not sure a single line and call them from the grammar will be useful and could be misleading.

The mechanism @bjansen proposes is better in the sense that it warns only if an action/pred is seen during the parse, rather than generically if there are ever any actions visible.

@KvanTTT 's Point is still valid though that it's pointing into the input stream with getCurrentToken(). On the other hand, maybe it makes sense to simply make it more specific that upon reaching input token foo, we bypassed an action?

Copy link
Collaborator Author

@bjansen bjansen Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it warns only if an action/pred is seen during the parse

That was indeed what I wanted to achieve.

I guess if an action is not run, there's a high probability there will be parse errors. So if we use getCurrentToken(), doesn't it make sense to "group" two error messages on the same token? One saying "hey we didn't expect this token here", and the other saying "but wait we skipped an action so it might be what's causing the error".

OTOH if we point to some location in the grammar, we're not actually "linking" the warning to any problem in the input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more likely that a predicate what caused problems but of course predicate usually don't make sense without other actions to set state. I don't think we have handy access to map an action back into the grammar file/line/column. I think it's fine to simply identify the problem and indicate that, at this point at the input, we have skipped something that could be important.

"Results may vary from the actual generated parser!");
warnedAboutPredicates = true;
}
}
}
10 changes: 10 additions & 0 deletions src/main/java/org/antlr/intellij/plugin/preview/InputPanel.form
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@
</grid>
</children>
</grid>
<component id="57495" class="com.intellij.openapi.ui.Splitter" binding="editorSplitter">
<constraints border-constraint="Center"/>
<properties>
<dividerWidth value="3"/>
<firstComponent value="4d6c1"/>
<orientation value="true"/>
<proportion value="0.8"/>
<secondComponent value="37477"/>
</properties>
</component>
<component id="4d6c1" class="javax.swing.JTextArea" binding="placeHolder">
<constraints border-constraint="East"/>
<properties>
Expand Down
21 changes: 3 additions & 18 deletions src/main/java/org/antlr/intellij/plugin/preview/InputPanel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.intellij.openapi.fileEditor.FileDocumentManager;
import com.intellij.openapi.project.Project;
import com.intellij.openapi.ui.ComponentWithBrowseButton;
import com.intellij.openapi.ui.Splitter;
import com.intellij.openapi.ui.TextComponentAccessor;
import com.intellij.openapi.ui.TextFieldWithBrowseButton;
import com.intellij.openapi.util.Key;
Expand Down Expand Up @@ -70,13 +71,7 @@ public class InputPanel {
private JPanel startRuleAndInputPanel;
private TextFieldWithBrowseButton fileChooser;
private JPanel outerMostPanel;

/**
* switchToGrammar() was seeing an empty slot instead of a previous
* editor or placeHolder. Figured it was an order of operations thing
* and synchronized add/remove ops. Works now w/o error.
*/
private final Object swapEditorComponentLock = new Object();
private Splitter editorSplitter;

private final PreviewPanel previewPanel;

Expand Down Expand Up @@ -266,17 +261,7 @@ public void switchToGrammar(PreviewState previewState, VirtualFile grammarFile)
}

public void setEditorComponent(JComponent editor) {
BorderLayout layout = (BorderLayout) outerMostPanel.getLayout();
String EDITOR_SPOT_COMPONENT = BorderLayout.CENTER;
// atomically remove old
synchronized (swapEditorComponentLock) {
Component editorSpotComp = layout.getLayoutComponent(EDITOR_SPOT_COMPONENT);
if ( editorSpotComp!=null ) {
editorSpotComp.setVisible(false);
outerMostPanel.remove(editorSpotComp); // remove old editor if it's there
}
outerMostPanel.add(editor, EDITOR_SPOT_COMPONENT);
}
editorSplitter.setFirstComponent(editor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the simplification of this code!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that was a pleasant surprise!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed a problem though: the bottom console grows automatically with each new line of text that is added inside. That's not really desired

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to set a maximum height?

}

public Editor getInputEditor() {
Expand Down