Skip to content
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.

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

[new-parser] Fix super_key failed predicate #5748

Merged

Conversation

yurloc
Copy link
Contributor

@yurloc yurloc commented Feb 27, 2024

How it works on main

This is how the selector rule and its first alternative looks like in the old parser:

selector
    :   (DOT super_key)=>DOT { helper.emit($DOT, DroolsEditorType.SYMBOL); } super_key superSuffix
    |   ...

The whole first alternative is guarded by a syntactic predicate that is used to make the decision whether this alternative is viable. The syntactic predicate relies on the super_key rule, which looks like this:

super_key
    : {(helper.validateIdentifierKey(DroolsSoftKeywords.SUPER))}? id=IDENTIFIER { helper.emit($id, DroolsEditorType.KEYWORD); }

If the next token is not super, validateIdentifierKey() will evaluate to false, rendering the first alternative of the selector rule non-viable.

What is the problem on the dev-new-parser branch?

Syntactic predicates were removed in ANTLR 4 so the selector rule became:

selector
    :   DOT { helper.emit($DOT, DroolsEditorType.SYMBOL); } super_key superSuffix
    |   ...

The semantic predicate {(helper.validateIdentifierKey(DroolsSoftKeywords.SUPER))}? now cannot be used to make the decision about the viability of the first alternative because it is now "invisible". See https://github.com/antlr/antlr4/blob/master/doc/predicates.md#finding-visible-predicates that explains how predicates can become invisible. The thing that makes the predicate invisible here is the action after DOT. And since the super_key rule matches the IDENTIFIER type of token, this alternative seems viable and the parser takes it even if the next token is not super.

When the decision has been made and the alternative is taken, the parser executes the action after DOT and the semantic predicate inside super_key is no longer invisible. It is now evaluated as well. But if the next token is not super it evaluates to false, which is an error condition because there is no other alternative inside super_key.

The fix is to replace the IDENTIFIER token type with SUPER.

Fixes #5703.

@yurloc
Copy link
Contributor Author

yurloc commented Feb 27, 2024

CC @gitgabrio @tkobayas @mariofusco.

@gitgabrio
Copy link
Contributor

Many thanks @yurloc for the explanation.
I've only a couple of question:

  1. would it be possible to somehow document, very briefly, the reason of the modification inside the code, somewhere ? With @tkobayas, in the past, we discussed a bit that all this code is pretty hard to understand
  2. there is a new test: is this relevant for the modification introduced ? If so, maybe it would be possible to add a small comment there, just to explain the relationship between the modification in the DRL6Expression.g4, and the new test

My personal focus, here, is to improve the "readability" (at least in general terms) also by someone that does not work on it daily: does this make sense ?

@yurloc yurloc force-pushed the fix-super-key-failed-predicate branch from b26c82a to ecdb23b Compare February 28, 2024 00:35
@yurloc
Copy link
Contributor Author

yurloc commented Feb 28, 2024

Thanks @gitgabrio. While working on your suggestions, I realized that my original fix was completely wrong.

I've pushed a new fix which is more self-explanatory and I'll see tomorrow if it can be improved with some comments.

@mariofusco mariofusco merged commit 5cc2371 into apache:dev-new-parser Feb 29, 2024
0 of 3 checks passed
@yurloc yurloc deleted the fix-super-key-failed-predicate branch February 29, 2024 11:00
@yurloc
Copy link
Contributor Author

yurloc commented Mar 4, 2024

Fixing progress: There were 549 failed tests (-0).

@yurloc yurloc changed the title Fix super_key failed predicate [new-parser] Fix super_key failed predicate Mar 4, 2024
tkobayas pushed a commit to tkobayas/drools that referenced this pull request Jun 11, 2024
tkobayas pushed a commit to tkobayas/drools that referenced this pull request Oct 2, 2024
tkobayas pushed a commit to tkobayas/drools that referenced this pull request Oct 11, 2024
tkobayas pushed a commit that referenced this pull request Oct 11, 2024
rgdoliveira pushed a commit to rgdoliveira/drools that referenced this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants