antlr / antlr5

BSD 3-Clause "New" or "Revised" License
57 stars 5 forks source link

Squashed errors for mutually left-recursive rules with incorrect positions #34

Closed KvanTTT closed 9 months ago

KvanTTT commented 2 years ago

Grammar

grammar Test;

a: e;
e: a;

b: c;
c: d;
d: c;

A: A 'B';
C: D;
D: E;
E: D;

Actual

error(119): Test.g4::: The following sets of rules are mutually left-recursive [A] and [D, E]
error(119): Test.g4::: The following sets of rules are mutually left-recursive [a, e] and [c, d]

Expected

error(119): Test.g4:10:0: The following sets of rules are mutually left-recursive [A]
error(119): Test.g4:12:0: The following sets of rules are mutually left-recursive [D, E]
error(119): Test.g4:3:0: The following sets of rules are mutually left-recursive [a, e]
error(119): Test.g4:7:0: The following sets of rules are mutually left-recursive [c, d]
KvanTTT commented 2 years ago

Moreover, I think it's more correctly to raise the error for every looped rule. For example, Kotlin compiler throws two errors instead of one for the sample:

open class A: B()
open class B: A()
parrt commented 2 years ago

I guess I'm OK with the change in the err messages although it should be singular:

error(119): Test.g4:12:0: The following set of rules are mutually left-recursive [D, E]

On the other hand, I'm not sure that it's worth me reviewing a big change to the CodeBase for this. I'm not sure what the motivation is here. What's wrong with existing and why are we focusing on this when I'm trying to allocate my time to building the online Grammar debugger?

KvanTTT commented 2 years ago

On the other hand, I'm not sure that it's worth me reviewing a big change to the CodeBase for this.

PR contains around 100 lines of code, our test coverage is quite good to detect regressions.

What's wrong with existing

Error messages without position look awkward and it complicates grammar development in IDE.

I'm not sure what the motivation is here.

I fixed it some time ago and eventually decided to suggest changes since I'm putting my computer in order because I'm in the process of relocation.

KvanTTT commented 2 years ago

I've encountered the error while working on https://github.com/antlr/antlr4/issues/3105 The simplest case:

a: B a; // infinite recursion

That issue was detected in our ANTLR grammars and ideally should be fixed. All issues relate to infinite recursion.

parrt commented 2 years ago

Could be useful, but let's leave it here for now. Glad you're getting it into the open during your relocation! Hopefully it's to a fancy new computer (and apartment?)!

KvanTTT commented 2 years ago

Hopefully it's to a fancy new computer (and apartment?)!

Apartment for only 1 month, full desktop computer will be transferred later, but nevermind.