Fixed #2296

[aermicioi]

Did a manual test with two grammars importing each other, and it didn't end up in stack overflow.
If you know where I should add the unit test for this, please share the info, I'll eventually add the unit test as well, if needed.

[KvanTTT]

Yes, a unit-test would be very useful. You can use the following (place it to the end of TestCompositeGrammars:

// ISSUE: https://github.com/antlr/antlr4/issues/2296
@Test
public void testCircularGrammarInclusion() {
	String g1 =
			"grammar G1;\n" +
			"import  G2;\n" +
			"r : 'R1';";
	String g2 =
			"grammar G2;\n" +
			"import  G1;\n" +
			"r : 'R2';";
	RuntimeTestUtils.mkdir(getTempDirPath());
	writeFile(getTempDirPath(), "G1.g4", g1);
	String found = execParser("G2.g4", g2, "G2Parser", "G2Lexer",
			null, null, "r", "R2", debug);
	assertEquals(null, found);
	// TODO: add circular inclusion error checking
}

Also, I think it should be a error something like CIRCULAR_GRAMMAR_INCLUSION. Add it to ErrorType.java.

[aermicioi]

Thx for the pointer to right unit test.

Could you explain about it being an error part?
Would that imply that if there is a circular reference in grammar A and B (i.e. A imports B and B, A) then you won't be able to compile the grammar in this case?

[KvanTTT]

Would that imply that if there is a circular reference in grammar A and B (i.e. A imports B and B, A) then you won't be able to compile the grammar in this case?

Exactly, because it's incorrect and confusing (similar to circular class hierarhy in Java or C#).

[aermicioi]

I don't think it is incorrect and confusing. It is similar to how headers in C work with #ifndef #def directives are employed for making header inclusion done once per compilation unit, or how in php require_once statement works.

This is quite useful when you have two logical sets of rules that can be split in two grammars, say A and B, yet their implementation require couple for rules from either of sub-grammars. Disallowing circular dependencies in grammars, would simply force people to split the grammars to smaller chunks in order to avoid these circular dependencies (i.e. have a DAG in import relationship between grammars). This doesn't necessarily map to the logical splitting of grammars that is desired, and may also spawn lots of subgrammars with awkward and long names, just to accommodate DAG rule in imports.

Furthermore, from the source code it seems that original intent was to skip import of a grammar if it was already imported and merged into existing grammar. It simply wasn't finished, as the imported set of grammars wasn't propagated alongside subsequent calls of loadImportedGrammar, making original visited set useless.

[KvanTTT]

Ok, sounds convincing. Let's do it without error.

[parrt]

Could you add the unit test? Thanks

[KvanTTT]

I suggest closing in favor of #3771 Commit is preserved.

[parrt]

Closing in favor of #3771