fixed identical path analysis to consider prefixes

org.eclipse.xtext.tests/src/org/eclipse/xtext/xtext/generator/hoisting/HoistingProcessorTest.java

@@ -21,7 +21,6 @@ import org.eclipse.xtext.testing.GlobalRegistries;
 import org.eclipse.xtext.tests.AbstractXtextTests;
 import org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.HoistingProcessor;
 import org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.exceptions.TokenAnalysisAbortedException;
-import org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.exceptions.UnsupportedConstructException;
 import org.eclipse.xtext.xtext.generator.parser.antlr.hoisting.guards.HoistingGuard;
 import org.junit.After;
 import org.junit.Before;
@@ -435,9 +434,8 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		assertEquals("((" + getSyntaxForKeywordToken("a", 1) + " || (p0)) && (" + getSyntaxForKeywordToken("b", 1) + " || (p1)))", guard.render());
 	}
 	
-	// TODO: this is no longer unsupported
-	// @Test(expected = UnsupportedConstructException.class)
-	public void testUnorderedGroupWithEmptyPathsWithoutContext_expectUnsupportedConstruct() throws Exception {
+	@Test(expected = TokenAnalysisAbortedException.class)
+	public void testUnorderedGroupWithEmptyPathsWithoutContext_expectTokenAnalysisAbortedException() throws Exception {		
 		// @formatter:off
 		String model =
 			MODEL_PREAMBLE +
@@ -448,11 +446,14 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		hoistingProcessor.init(grammar);
 		AbstractRule rule = getRule(grammar, "S");
 		
-		hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		assertFalse(guard.isTrivial());
+		assertTrue(guard.hasTerminal());
+		assertEquals("((" + getSyntaxForKeywordToken("a", 1) + " || (p0)) && (" + getSyntaxForKeywordToken("b", 1) + " || (p1)))", guard.render());
 	}
 	
-	//@Test
-	public void testUnorderedGroupWithoutMandatoryContent() throws Exception {
+	@Test
+	public void testUnorderedGroupWithoutMandatoryContentWithContext() throws Exception {
 		// @formatter:off
 		String model =
 			MODEL_PREAMBLE +
@@ -466,12 +467,13 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
 		assertFalse(guard.isTrivial());
 		assertTrue(guard.hasTerminal());
-		assertEquals("((" + getSyntaxForKeywordToken("s", 1) + " || (p2)) && (" + getSyntaxForKeywordToken("a", 1) + " || (p0)) && (" + getSyntaxForKeywordToken("b", 1) + " || (p1)))", guard.render());
+		assertEquals("((" + getSyntaxForKeywordToken("a", 1) + " || (p0)) && (" + getSyntaxForKeywordToken("b", 1) + " || (p1)))", guard.render());
 	}
 	
-	// TODO: this is no longer unsupported
-	@Test(expected = UnsupportedConstructException.class)
-	public void testUnorderedGroupWithoutMandatoryContentWithoutContext_expectUnsupportedConstruct() throws Exception {
+	@Test
+	public void testUnorderedGroupWithoutMandatoryContentWithoutContext_expectNoEofCheck() throws Exception {
+		// no eof check needed since that case is covered by the two alternatives
+		
 		// @formatter:off
 		String model =
 			MODEL_PREAMBLE +
@@ -482,7 +484,11 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		hoistingProcessor.init(grammar);
 		AbstractRule rule = getRule(grammar, "S");
 		
-		hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		assertFalse(guard.isTrivial());
+		assertTrue(guard.hasTerminal());
+		System.out.println(guard.toString());
+		assertEquals("((" + getSyntaxForKeywordToken("a", 1) + " || (p0)) && (" + getSyntaxForKeywordToken("b", 1) + " || (p1)))", guard.render());
 	}
 	
 	@Test
@@ -888,8 +894,45 @@ public class HoistingProcessorTest extends AbstractXtextTests {
 		Grammar grammar = ((Grammar) resource.getContents().get(0));
 		hoistingProcessor.init(grammar);
 		AbstractRule rule = getRule(grammar, "S");
+
+		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		assertFalse(guard.isTrivial());
+		assertTrue(guard.hasTerminal());
+		System.out.println(guard.render());
+	}
+	
+	@Test
+	public void testNestedAlternativesWithIdenticalPrefix_bugPathIdentityCheckDoesNotConsiderContext_expectContextCheckInResult() throws Exception {
+		// @formatter:off
+		String model =
+			MODEL_PREAMBLE +
+			"S: {S} $$ p0 $$?=> (\n" +
+			"      'a' \n" +
+			"    | 'a' 'b' \n" +
+			"   ) \n" +
+			" | {S} $$ p1 $$?=> (\n" +
+			"      'a' \n" + 
+			"    | 'a' 'c' \n" + 
+			"   ) \n" +
+			" | {S} $$ p2 $$?=> 'd' ;\n";
+		// @formatter:off
+		XtextResource resource = getResourceFromString(model);
+		Grammar grammar = ((Grammar) resource.getContents().get(0));
+		hoistingProcessor.init(grammar);
+		AbstractRule rule = getRule(grammar, "S");
 		
-		hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		HoistingGuard guard = hoistingProcessor.findHoistingGuard(rule.getAlternatives());
+		assertFalse(guard.isTrivial());
+		assertTrue(guard.hasTerminal());
+		System.out.println(guard.render());
+		assertEquals(
+				"(" + 
+					"(" + getSyntaxForKeywordToken("a", 1) + " || (" + getSyntaxForEofToken(2) + " && " + getSyntaxForKeywordToken("b", 2) + ") || (p0)) && " + 
+					"(" + getSyntaxForKeywordToken("a", 1) + " || (" + getSyntaxForEofToken(2) + " && " + getSyntaxForKeywordToken("c", 2) + ") || (p1)) && " + 
+					"(" + getSyntaxForKeywordToken("d", 1) + " || (p2))" + 
+				")", 
+			guard.render()
+		);
 	}
 	
 	// symbolic analysis not yet implemented

org.eclipse.xtext.xtext.generator/src/org/eclipse/xtext/xtext/generator/parser/antlr/hoisting/pathAnalysis/TokenAnalysis.java

@@ -30,6 +30,8 @@ import org.eclipse.xtext.CompoundElement;
 import org.eclipse.xtext.Grammar;
 import org.eclipse.xtext.GrammarUtil;
 import org.eclipse.xtext.Group;
+import org.eclipse.xtext.Keyword;
+import org.eclipse.xtext.ParserRule;
 import org.eclipse.xtext.RuleCall;
 import org.eclipse.xtext.UnorderedGroup;
 import org.eclipse.xtext.util.XtextSwitch;
@@ -96,12 +98,10 @@ public class TokenAnalysis {
 		log.info("getNext: " + abstractElementToShortString(last));
 		log.info("container: " + abstractElementToShortString(container));
 		
-		final AbstractElement finalLast = last;
-		
 		if (container instanceof UnorderedGroup) {
 			List<AbstractElement> result = new ArrayList<>();
 			result.addAll(container.getElements().stream()
-				.filter(e -> e != finalLast).collect(Collectors.toList())
+				.collect(Collectors.toList())
 			);
 			result.addAll(getNextElementsInContext(container));
 			return result;
@@ -156,11 +156,17 @@ public class TokenAnalysis {
 			// TODO: is this special case necessary?
 			throw new TokenAnalysisAbortedException("context analysis failed: no context");
 		}
+
+		int currentPosition = prefix.getMinPosition();
 		
 		for (AbstractElement element : context) {			
 			TokenAnalysisPaths path = new TokenAnalysisPaths(prefix);
 			path = getTokenPaths(element, path, false, false);
 			if (!path.isDone() && element != null) {
+				if (path.getMinPosition() == currentPosition) {
+					throw new TokenAnalysisAbortedException("no progress in context analysis");
+				}
+				
 				path = getTokenPathsContext(element, path);
 			}
 			if (path.isDone()) {
@@ -205,12 +211,18 @@ public class TokenAnalysis {
 			result = TokenAnalysisPaths.empty(prefix);
 		}
 		
+		int currentPosition = result.getMinPosition();
+		
 		do {
 			current = TokenAnalysisPaths.empty(result);
 			for (AbstractElement element : path.getElements()) {
 				current = current.merge(getTokenPaths(element, result, false, false));
 			}
 			
+			if (current.getMinPosition() == currentPosition) {
+				throw new TokenAnalysisAbortedException("no progress in loop");
+			}
+			
 			result = result.merge(current);
 		} while(!current.isDone());
 		
@@ -291,7 +303,9 @@ public class TokenAnalysis {
 		
 		boolean loop = isMultipleCardinality(path);
 		
-		do {
+		int currentPosition = result.getMinPosition();
+		
+		do {	
 			TokenAnalysisPaths tokenPaths = getTokenPathsTrivial(path, result);
 			
 			if (tokenPaths.isDone()) {
@@ -305,6 +319,14 @@ public class TokenAnalysis {
 			} else {
 				result = result.merge(tokenPaths);
 			}
+			
+			if (loop) {
+				if (result.getMinPosition() == currentPosition) {
+					throw new TokenAnalysisAbortedException("no progress in loop");
+				} else {
+					currentPosition = result.getMinPosition();
+				}
+			}
 		} while (loop);
 		
 		return result;
@@ -330,36 +352,43 @@ public class TokenAnalysis {
 	
 	private boolean arePathsIdenticalFallback(AbstractElement path1, AbstractElement path2) {
 		// + 1, because otherwise identical paths of length token limit can't be checked
-		for (int i = 0; i < config.getTokenLimit() + 1; i++) {
-			Set<List<Token>> tokenListSet1;
-			Set<List<Token>> tokenListSet2;
+		
+		log.info("path1: " + abstractElementToString(path1));
+		log.info("path2: " + abstractElementToString(path2));
+		
+		int i;
+		int limit = config.getTokenLimit() + 1;
+		for (i = 0; i < limit; i++) {
+			TokenAnalysisPaths tokenPaths1;
+			TokenAnalysisPaths tokenPaths2;
 			
 			List<Integer> range = range(0,  i);
 			
-			try {
-				tokenListSet1 = new HashSet<>(getTokenPaths(path1, range, false));
-			} catch (TokenAnalysisAbortedException e) {
-				tokenListSet1 = null;
-			}
+			// there shouldn't be a TokenAnalysisAbortedException if needsLength is false
+			tokenPaths1 = getTokenPaths(path1, new TokenAnalysisPaths(range), false, false);
+			tokenPaths2 = getTokenPaths(path2, new TokenAnalysisPaths(range), false, false);
 			
-			try {
-				tokenListSet2 = new HashSet<>(getTokenPaths(path2, range, false));
-			} catch (TokenAnalysisAbortedException e) {
-				tokenListSet2 = null;
-			}
+			Set<Set<Token>> tokenListSet1 = tokenPaths1.getTokenPaths().stream().map(HashSet::new).collect(Collectors.toSet());
+			Set<Set<Token>> tokenListSet2 = tokenPaths2.getTokenPaths().stream().map(HashSet::new).collect(Collectors.toSet());
+			
+			int maxPosition1 = tokenPaths1.getMaxPosition();
+			int maxPosition2 = tokenPaths2.getMaxPosition();
+			
+			log.info("set1: " + tokenListSet1 + ", " + maxPosition1);
+			log.info("set2: " + tokenListSet2 + ", " + maxPosition2);
 			
-			if (tokenListSet1 == null && tokenListSet2 == null) {
-				return true;
-			}
-			if (tokenListSet1 == null || tokenListSet2 == null) {
-				// the paths have different length
-				return false;
-			}
 			if (!tokenListSet1.equals(tokenListSet2)) {
 				return false;
 			}
+			
+			if (maxPosition1 < i + 1) {
+				// different max positions would have failed the equals-Operation
+				// if the max position is smaller than i + 1 the end of the path has been reached
+				return true;
+			}
 		}
 		
+		// token analysis limit reached
 		// we can't analyze the paths any further
 		// TODO maybe assume paths are equal and show warning instead of exception
 		throw new TokenAnalysisAbortedException("token limit exhausted while looking for identical paths");
@@ -383,7 +412,6 @@ public class TokenAnalysis {
 			}
 		}
 		// we tried all possible combinations
-		// TODO: can only happen if no symbolic analysis is implemented
 		// -> abort
 		if (limit.get().equals(config.getTokenLimit())) {
 			throw new TokenAnalysisAbortedException("token limit exhausted while searching for minimal differences");

org.eclipse.xtext.xtext.generator/src/org/eclipse/xtext/xtext/generator/parser/antlr/hoisting/pathAnalysis/TokenAnalysisPath.java

@@ -32,6 +32,10 @@ public class TokenAnalysisPath {
 		path = new LinkedList<>(prefix.path);
 		position = prefix.position;
 	}
+
+	int getPosition() {
+		return position - 1;
+	}
 	
 	public boolean isDone() {
 		return remainingIndexes.isEmpty();

org.eclipse.xtext.xtext.generator/src/org/eclipse/xtext/xtext/generator/parser/antlr/hoisting/pathAnalysis/TokenAnalysisPaths.java

@@ -63,6 +63,14 @@ public class TokenAnalysisPaths {
 		return empty;
 	}
 	
+	public int getMinPosition() {
+		return tokenPaths.stream().map(TokenAnalysisPath::getPosition).mapToInt(Integer::intValue).min().getAsInt();
+	}
+	
+	public int getMaxPosition() {
+		return tokenPaths.stream().map(TokenAnalysisPath::getPosition).mapToInt(Integer::intValue).max().getAsInt();
+	}
+	
 	@Override
 	public String toString() {
 		if (isEmpty) {