search

rrevenantt / antlr4rust

ANTLR4 parser generator runtime for Rust programming laguage
Other
404 stars 70 forks source link

Fix `enterXXX` listener calls for alternative labels #13

Closed 3c1u closed 3 years ago

3c1u commented 4 years ago

This will fix #12.

rrevenantt commented 4 years ago

Thanks, apparently Java target has the same issue https://github.com/antlr/antlr4/issues/802 that has not been fixed for 5 years, which makes me think that fix can't be as simple as you suggest, although at first glance I don't see any problem with it. So i will take some more time to find out if there is anything wrong.

3c1u commented 3 years ago

I checked most targets (Java, C#, C++, JavaScript, Go) and their runtime seem to have the identical code on enterRule and enterOuterAlt, and thus, they won't invoke enter_rule for alt labels (not sure).

Interestingly, Swift target goes otherwise. It triggers enter events in enterOuterAlt, not in enterRule (see https://github.com/antlr/antlr4/blob/master/runtime/Swift/Sources/Antlr4/Parser.swift#L637). It emits enter_rule event for alternative labels properly.

rrevenantt commented 3 years ago

Hmm..., triggering all enter events in enterOuterAlt instead of enterRule looks more logical. Also in this case triggering in enterRecursionRule will be also redundant (although, Swift target still do it, i am quite sure that it is a bug). Would you mind changing PR this way?

3c1u commented 3 years ago

Changed. I made a PR for other targets (https://github.com/antlr/antlr4/pull/2910) and it passed tests (on Travis CI; AppVeyor build failed because "Build execution time has reached the maximum allowed time for your plan (60 minutes)."), so it seems to be okay.

rrevenantt commented 3 years ago

Perhaps I wasn't clear. You also need to remove trigger_enter_rule_event from enter_recursive_rule because it replaces enter_rule for left recursive rules. I believe same can be applied to other target as well. Otherwise looks good.

3c1u commented 3 years ago

I tested with a grammar with recursion, and it was surely redundant. I'll fix the PR for other targets then.

Grammar:

grammar Test;

prog
    : ident EOF
    ;

ident
    : 'tohka'       # Tohka
    | ident 'tohka' # Tohka2
    ;

Expected:

enter rule
enter prog
enter rule
exit Tohka
enter rule
enter Tohka2
exit Tohka2
exit prog

Output before fix:

enter rule
enter prog
enter rule
enter rule    <- called twice
exit Tohka
enter rule
enter Tohka2
exit Tohka2
exit prog