From 17eee72f20be96a61d5da2cad9bcba8e903a579e Mon Sep 17 00:00:00 2001 From: mmews-n4 Date: Thu, 15 Aug 2024 09:46:38 +0200 Subject: [PATCH] GH-2636: Improve handling of dependency cycles within dependencies (#2637) * another way to deal late with dependency cycles * adjust test case * fixes / adjustments --- .../N4JSStatefulIncrementalBuilder.java | 5 -- .../build/DefaultBuildRequestFactory.java | 10 --- .../ide/server/build/ProjectBuilder.java | 5 -- .../xtext/workspace/BuildOrderIterator.java | 16 ++--- .../utils/ProjectImportEnablingScope.java | 17 ++++- ...yclicDependenciesBuilderNoRebuildTest.java | 4 +- .../CyclicDependenciesBuilderTest.java | 68 +++++++++++++++++-- 7 files changed, 86 insertions(+), 39 deletions(-) diff --git a/plugins/org.eclipse.n4js.ide/src/org/eclipse/n4js/ide/server/N4JSStatefulIncrementalBuilder.java b/plugins/org.eclipse.n4js.ide/src/org/eclipse/n4js/ide/server/N4JSStatefulIncrementalBuilder.java index 73028a0cff..2a61932543 100644 --- a/plugins/org.eclipse.n4js.ide/src/org/eclipse/n4js/ide/server/N4JSStatefulIncrementalBuilder.java +++ b/plugins/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/plugins/org.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/DefaultBuildRequestFactory.java b/plugins/org.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/DefaultBuildRequestFactory.java index 30a78240cf..5a846ed57b 100644 --- a/plugins/org.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/DefaultBuildRequestFactory.java +++ b/plugins/org.eclipse.n4js.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/plugins/org.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/ProjectBuilder.java b/plugins/org.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/ProjectBuilder.java index 60e4cf256b..f155107a36 100644 --- a/plugins/org.eclipse.n4js.xtext.ide/src/org/eclipse/n4js/xtext/ide/server/build/ProjectBuilder.java +++ b/plugins/org.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 currSourceFileURIsToConsider, Set currSourceFileURIsOnDisk) { - if (workspaceManager.getWorkspaceConfig().isInDependencyCycle(getProjectID())) { - // see DefaultBuildRequestFactory#createBuildRequest(...) - currSourceFileURIsOnDisk.retainAll(getProjectConfig().getProjectDescriptionUris()); - } - ResourceChangeSet result = new ResourceChangeSet(); ImmutableProjectState oldProjectState = this.projectStateSnapshot.get(); Map 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 index f66ba94174..f3f1fb1d25 100644 --- 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 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 index 7b906c1710..812844271a 100644 --- 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/org.eclipse.n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderNoRebuildTest.java b/tests/org.eclipse.n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderNoRebuildTest.java index 78204d0b3c..5ad6aea9f7 100644 --- a/tests/org.eclipse.n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderNoRebuildTest.java +++ b/tests/org.eclipse.n4js.ide.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/tests/org.eclipse.n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderTest.java b/tests/org.eclipse.n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderTest.java index a7a2cdc39b..1bce273936 100644 --- a/tests/org.eclipse.n4js.ide.tests/src/org/eclipse/n4js/ide/tests/builder/CyclicDependenciesBuilderTest.java +++ b/tests/org.eclipse.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> 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'.)"))); + + } }