GH-2636: Improve handling of dependency cycles within dependencies (#…

…2637) * another way to deal late with dependency cycles * adjust test case * fixes / adjustments
eclipse-n4js · Aug 15, 2024 · 17eee72 · 17eee72
1 parent 76bcd6b
commit 17eee72
Show file tree

Hide file tree

Showing 7 changed files with 86 additions and 39 deletions.
diff --git a/.../org.eclipse.n4js.ide/src/org/eclipse/n4js/ide/server/N4JSStatefulIncrementalBuilder.java b/.../org.eclipse.n4js.ide/src/org/eclipse/n4js/ide/server/N4JSStatefulIncrementalBuilder.java
@@ -167,11 +167,6 @@ protected void unloadResource(URI uri) {
 	 */
 	@Override
 	protected XBuildRequest initializeBuildRequest(XBuildRequest initialRequest, XBuildContext context) {
-		if (workspaceManager.getWorkspaceConfig().isInDependencyCycle(initialRequest.getProjectName())) {
-			// see DefaultBuildRequestFactory#createBuildRequest(...)
-			return initialRequest;
-		}
-
 		ProjectBuilder projectBuilder = workspaceManager.getProjectBuilder(initialRequest.getProjectName());
 		N4JSProjectConfigSnapshot projectConfig = (N4JSProjectConfigSnapshot) projectBuilder.getProjectConfig();
 		ResourceDescriptionsData oldIndex = context.getOldIndex();

diff --git a/...4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/DefaultBuildRequestFactory.java b/...4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/DefaultBuildRequestFactory.java
@@ -12,7 +12,6 @@
 
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 
@@ -54,15 +53,6 @@ public XBuildRequest createBuildRequest(
 			boolean writeStorageResources) {
 
 		String projectID = projectConfig.getName();
-		if (workspaceConfig.isInDependencyCycle(projectID)) {
-			// remove all resources (except for the project description) from the build
-			// since cycles are not supported
-			dirtyFiles = new HashSet<>(dirtyFiles);
-			dirtyFiles.retainAll(projectConfig.getProjectDescriptionUris());
-			deletedFiles = new HashSet<>(deletedFiles);
-			deletedFiles.retainAll(projectConfig.getProjectDescriptionUris());
-		}
-
 		XBuildRequest request = new XBuildRequest(projectID, projectConfig.getPath(),
 				dirtyFiles, deletedFiles, externalDeltas,
 				index, resourceSet, fileMappings,

diff --git a/...rg.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/ProjectBuilder.java b/...rg.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/ProjectBuilder.java
@@ -267,11 +267,6 @@ public ResourceChangeSet scanForSourceFileChanges() {
 	protected ResourceChangeSet scanForSourceFileChanges(
 			Set<URI> currSourceFileURIsToConsider, Set<URI> currSourceFileURIsOnDisk) {
 
-		if (workspaceManager.getWorkspaceConfig().isInDependencyCycle(getProjectID())) {
-			// see DefaultBuildRequestFactory#createBuildRequest(...)
-			currSourceFileURIsOnDisk.retainAll(getProjectConfig().getProjectDescriptionUris());
-		}
-
 		ResourceChangeSet result = new ResourceChangeSet();
 		ImmutableProjectState oldProjectState = this.projectStateSnapshot.get();
 		Map<URI, HashedFileContent> oldHashes = oldProjectState.getFileHashes();

diff --git a/plugins/org.eclipse.n4js.xtext/src/org/eclipse/n4js/xtext/workspace/BuildOrderIterator.java b/plugins/org.eclipse.n4js.xtext/src/org/eclipse/n4js/xtext/workspace/BuildOrderIterator.java
@@ -52,16 +52,14 @@ public BuildOrderIterator visit(Collection<? extends ProjectConfigSnapshot> proj
 		for (ProjectConfigSnapshot pc : projectConfigs) {
 			String projectID = pc.getName();
 
-			if (!visitedAlready.contains(pc)
-					&& boi.sortedProjects.indexOf(pc) < boi.sortedProjects.indexOf(lastVisited)) {
-				String currentProjectID = current().getName();
-				throw new IllegalStateException("Dependency-inverse visit order not supported: from "
-						+ currentProjectID + " to " + projectID);
-			}
+			if (boi.sortedProjects.indexOf(pc) < boi.sortedProjects.indexOf(lastVisited)) {
+				// Dependency-inverse visit order not supported. Happens in dependency cycles
+			} else {
 
-			visitProjectIDs.add(projectID);
-			iteratorIndex = boi.sortedProjects.indexOf(lastVisited);
-			moveNext();
+				visitProjectIDs.add(projectID);
+				iteratorIndex = boi.sortedProjects.indexOf(lastVisited);
+				moveNext();
+			}
 		}
 		return this;
 	}

diff --git a/plugins/org.eclipse.n4js/src/org/eclipse/n4js/scoping/utils/ProjectImportEnablingScope.java b/plugins/org.eclipse.n4js/src/org/eclipse/n4js/scoping/utils/ProjectImportEnablingScope.java
@@ -193,7 +193,22 @@ public IEObjectDescription getSingleElement(QualifiedName name) {
 
 		if (size == 1) {
 			// main case
-			return result.get(0);
+			IEObjectDescription objDescr = result.get(0);
+
+			// references within a dependency cycle not supported
+			if (workspaceConfigSnapshot.isInDependencyCycle(contextProject.getName())) {
+				URI uri = objDescr.getEObjectURI();
+				N4JSProjectConfigSnapshot n4jsdProject = workspaceConfigSnapshot.findProjectContaining(uri);
+
+				if (workspaceConfigSnapshot.isInDependencyCycle(n4jsdProject.getName())) {
+					if (Objects.equals(n4jsdProject.getName(), contextProject.getName())) {
+						// allow references within the same project
+					} else {
+						return null;
+					}
+				}
+			}
+			return objDescr;
 		}
 
 		// use sorted entries and linked map for determinism in error message

diff --git a/....tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderNoRebuildTest.java b/....tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderNoRebuildTest.java
@@ -44,9 +44,7 @@ public void testStableCycleNoRebuild() {
 						Pair.of(CFG_DEPENDENCIES, "P2"))),
 				Pair.of("P2", List.of(
 						Pair.of("M2",
-								"import {C1} from 'M1'; \n" +
-										"const x: C1 = null;\n" +
-										"x;"),
+								"export public class C2 {}"),
 						Pair.of(CFG_DEPENDENCIES, "P1"))));
 
 		startAndWaitForLspServer();

diff --git a/....n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderTest.java b/....n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderTest.java
@@ -39,9 +39,7 @@ export public class C1 {
 							"""),
 			"P2", Map.of(
 					"M2", """
-							import {C1} from "M1";
-							const x: C1 = null;
-							x;
+							export public class C2 {}
 							""",
 					CFG_DEPENDENCIES, """
 							P1
@@ -58,10 +56,45 @@ export public class C1 {
 							"""),
 			"P2", Map.of(
 					"M2", """
-							import {C1} from "M1";
+							export public class C2 {}
+							""",
+					CFG_DEPENDENCIES, """
+							P1
+							"""));
+
+	private static Map<String, Map<String, String>> testData3 = Map.of(
+			"P1", Map.of(
+					"M1", """
+							export public class C1 {
+							}
+							""",
+					"I1A", """
+							import {C2} from "M2"; // expect error
+							const x: C2 = null;
+							x;
+							""",
+					"I1B", """
+							import {C1} from "M1"; // expect no error
+							const x: C1 = null;
+							x;
+							""",
+					CFG_DEPENDENCIES, """
+							P2
+							"""),
+			"P2", Map.of(
+					"M2", """
+									export public class C2 {}
+							""",
+					"I2A", """
+							import {C1} from "M1"; // expect error
 							const x: C1 = null;
 							x;
 							""",
+					"I2B", """
+							import {C2} from "M2"; // expect no error
+							const x: C2 = null;
+							x;
+							""",
 					CFG_DEPENDENCIES, """
 							P1
 							"""));
@@ -169,7 +202,8 @@ public void testOnlyDirtyStateBuildInsideCycle() {
 				"P1/package.json", List.of(
 						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)"),
 				"P2/package.json", List.of(
-						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)")));
+						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)"),
+				"M1", List.of("(Error, [0:25 - 1:0], extraneous input '#\\n' expecting '}')")));
 	}
 
 	@Test
@@ -210,7 +244,8 @@ public void testBuildDirtyAfterRemoveCycle1() {
 				"P1/package.json", List.of(
 						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)"),
 				"P2/package.json", List.of(
-						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)")));
+						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)"),
+				"M1", List.of("(Error, [0:25 - 1:0], extraneous input '#\\n' expecting '}')")));
 
 		// remove cycle
 		openFile("P1/package.json");
@@ -261,4 +296,25 @@ public void testBuildDirtyAfterRemoveCycle2() {
 		assertIssues2(Map.of(
 				"M1", List.of("(Error, [0:25 - 1:0], extraneous input '#\\n' expecting '}')")));
 	}
+
+	@Test
+	public void testCycleImportErrors() {
+		testWorkspaceManager.createTestOnDisk(testData3);
+
+		startAndWaitForLspServer();
+
+		assertIssues2(Map.of(
+				"P1/package.json", List.of(
+						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)"),
+				"P2/package.json", List.of(
+						"(Error, [14:18 - 14:22], Dependency cycle of the projects: yarn-test-project/packages/P1, yarn-test-project/packages/P2.)"),
+
+				"I1A",
+				List.of("(Error, [0:17 - 0:21], Couldn't resolve reference to TModule 'M2'.)",
+						"(Error, [1:9 - 1:11], Couldn't resolve reference to Type 'C2'.)"),
+				"I2A",
+				List.of("(Error, [0:17 - 0:21], Cannot resolve plain module specifier (without project name as first segment): no matching module found.)",
+						"(Error, [1:9 - 1:11], Couldn't resolve reference to Type 'C1'.)")));
+
+	}
 }