Report error on circular local dependencies (#731)

If a stack overflow is found during project evaluation, present any
circular imports found in the dependency graph.

pkl-core/src/main/java/org/pkl/core/project/Project.java

@@ -26,6 +26,7 @@ import java.util.Objects;
 import java.util.function.Function;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
+import org.pkl.core.Analyzer;
 import org.pkl.core.Composite;
 import org.pkl.core.Duration;
 import org.pkl.core.Evaluator;
@@ -49,6 +50,9 @@ import org.pkl.core.packages.PackageLoadError;
 import org.pkl.core.packages.PackageUri;
 import org.pkl.core.packages.PackageUtils;
 import org.pkl.core.resource.ResourceReaders;
+import org.pkl.core.runtime.VmException;
+import org.pkl.core.runtime.VmExceptionBuilder;
+import org.pkl.core.util.ImportGraphUtils;
 import org.pkl.core.util.IoUtils;
 import org.pkl.core.util.Nullable;
 
@@ -108,16 +112,7 @@ public final class Project {
 
   /** Loads a project from the given source. */
   public static Project load(ModuleSource moduleSource) {
-    try (var evaluator =
+    try (var evaluator = evaluatorBuilder().build()) {
-        EvaluatorBuilder.unconfigured()
-            .setSecurityManager(SecurityManagers.defaultManager)
-            .setStackFrameTransformer(StackFrameTransformers.defaultTransformer)
-            .addModuleKeyFactory(ModuleKeyFactories.standardLibrary)
-            .addModuleKeyFactory(ModuleKeyFactories.file)
-            .addModuleKeyFactory(ModuleKeyFactories.classPath(Project.class.getClassLoader()))
-            .addResourceReader(ResourceReaders.environmentVariable())
-            .addResourceReader(ResourceReaders.file())
-            .build()) {
       return load(evaluator, moduleSource);
     }
   }
@@ -126,11 +121,105 @@ public final class Project {
     try {
       var output = evaluator.evaluateOutputValueAs(moduleSource, PClassInfo.Project);
       return Project.parseProject(output);
+    } catch (StackOverflowError e) {
+      var cycles = findImportCycle(moduleSource);
+      VmException vmException;
+      if (!cycles.isEmpty()) {
+        if (cycles.size() == 1) {
+          vmException =
+              new VmExceptionBuilder()
+                  .evalError(
+                      "cannotHaveCircularProjectDependenciesSingle",
+                      renderCycle(cycles.stream().toList().get(0)))
+                  .withCause(e)
+                  .build();
+        } else {
+          var renderedCycles = renderMultipleCycles(cycles);
+          vmException =
+              new VmExceptionBuilder()
+                  .evalError("cannotHaveCircularProjectDependenciesMultiple", renderedCycles)
+                  .withCause(e)
+                  .build();
+        }
+        // stack frame transformer never used; this exception has no stack frames.
+        throw vmException.toPklException(StackFrameTransformers.defaultTransformer);
+      }
+      throw e;
     } catch (URISyntaxException e) {
       throw new PklException(e.getMessage(), e);
     }
   }
 
+  private static String renderMultipleCycles(List<List<URI>> cycles) {
+    var sb = new StringBuilder();
+    var i = 0;
+    for (var cycle : cycles) {
+      if (i > 0) {
+        sb.append('\n');
+      }
+      sb.append("Cycle ").append(i + 1).append(":\n");
+      renderCycle(sb, cycle);
+      sb.append('\n');
+      i++;
+    }
+    return sb.toString();
+  }
+
+  private static void renderCycle(StringBuilder sb, List<URI> cycle) {
+    sb.append("┌─>");
+    var isFirst = true;
+    for (URI uri : cycle) {
+      if (isFirst) {
+        isFirst = false;
+      } else {
+        sb.append("\n│");
+      }
+      sb.append("\n│  ");
+      sb.append(uri.toString());
+    }
+    sb.append("\n└─");
+  }
+
+  private static String renderCycle(List<URI> cycle) {
+    var sb = new StringBuilder();
+    renderCycle(sb, cycle);
+    return sb.toString();
+  }
+
+  private static List<List<URI>> findImportCycle(ModuleSource moduleSource) {
+    var builder = evaluatorBuilder();
+    var analyzer =
+        new Analyzer(
+            StackFrameTransformers.defaultTransformer,
+            SecurityManagers.defaultManager,
+            builder.getModuleKeyFactories(),
+            builder.getModuleCacheDir(),
+            builder.getProjectDependencies(),
+            builder.getHttpClient());
+    var importGraph = analyzer.importGraph(moduleSource.getUri());
+    var ret = ImportGraphUtils.findImportCycles(importGraph);
+    // we only care about cycles in the same scheme as `moduleSource`
+    return ret.stream()
+        .filter(
+            (cycle) ->
+                cycle.stream()
+                    .anyMatch(
+                        (uri) ->
+                            uri.getScheme().equalsIgnoreCase(moduleSource.getUri().getScheme())))
+        .collect(Collectors.toList());
+  }
+
+  private static EvaluatorBuilder evaluatorBuilder() {
+    return EvaluatorBuilder.unconfigured()
+        .setSecurityManager(SecurityManagers.defaultManager)
+        .setStackFrameTransformer(StackFrameTransformers.defaultTransformer)
+        .addModuleKeyFactory(ModuleKeyFactories.standardLibrary)
+        .addModuleKeyFactory(ModuleKeyFactories.file)
+        .addModuleKeyFactory(ModuleKeyFactories.classPath(Project.class.getClassLoader()))
+        .addResourceReader(ResourceReaders.environmentVariable())
+        .addResourceReader(ResourceReaders.file());
+  }
+
   private static DeclaredDependencies parseDependencies(
       PObject module, URI projectFileUri, @Nullable PackageUri packageUri)
       throws URISyntaxException {
@@ -280,7 +369,7 @@ public final class Project {
     var sourceCodeUrlScheme = (String) getNullableProperty(pObj, "sourceCodeUrlScheme");
     var license = (String) getNullableProperty(pObj, "license");
     var licenseText = (String) getNullableProperty(pObj, "licenseText");
-    var issueTracker = (URI) getNullableURI(pObj, "issueTracker");
+    var issueTracker = getNullableURI(pObj, "issueTracker");
     var apiTestStrs = (List<String>) getProperty(pObj, "apiTests");
     var apiTests = apiTestStrs.stream().map(Path::of).collect(Collectors.toList());
     var exclude = (List<String>) getProperty(pObj, "exclude");

pkl-core/src/main/java/org/pkl/core/runtime/VmExceptionRenderer.java

@@ -136,6 +136,10 @@ public final class VmExceptionRenderer {
       if (!frames.isEmpty()) {
         builder.append('\n');
         stackTraceRenderer.render(frames, hint, builder);
+      } else if (hint != null) {
+        // render hint if there are no stack frames
+        builder.append('\n');
+        builder.append(hint);
       }
     }
   }

pkl-core/src/main/java/org/pkl/core/util/ImportGraphUtils.java

@@ -0,0 +1,64 @@
+/*
+ * Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.pkl.core.util;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.List;
+import org.pkl.core.ImportGraph;
+
+public class ImportGraphUtils {
+
+  private ImportGraphUtils() {}
+
+  /** Find import cycles inside the graph. */
+  public static List<List<URI>> findImportCycles(ImportGraph importGraph) {
+    var res = new ArrayList<List<URI>>();
+    for (var uri : importGraph.imports().keySet()) {
+      if (res.stream().anyMatch((it) -> it.contains(uri))) {
+        continue;
+      }
+      var cycle = doFindCycle(uri, importGraph, new ArrayList<>(List.of(uri)));
+      if (cycle != null) {
+        res.add(cycle);
+      }
+    }
+    return res;
+  }
+
+  private static @Nullable List<URI> doFindCycle(
+      URI currentUri, ImportGraph importGraph, List<URI> path) {
+    var imports = importGraph.imports().get(currentUri);
+    var startingUri = path.get(0);
+    for (var imprt : imports) {
+      var uri = imprt.uri();
+      if (uri.equals(startingUri)) {
+        return path;
+      }
+      if (path.contains(uri)) {
+        // there is a cycle, but it doesn't start at `startUri`
+        return null;
+      }
+      path.add(uri);
+      var cycle = doFindCycle(uri, importGraph, path);
+      if (cycle != null) {
+        return cycle;
+      }
+      path.remove(path.size() - 1);
+    }
+    return null;
+  }
+}

pkl-core/src/main/resources/org/pkl/core/errorMessages.properties

@@ -827,6 +827,21 @@ Microbenchmark is a constant expression.
 stackOverflow=\
 A stack overflow occurred.
 
+cannotHaveCircularProjectDependenciesSingle=\
+Local project dependencies cannot be circular.\n\
+\n\
+Cycle:\n\
+{0}
+
+cannotHaveCircularProjectDependenciesMultiple=\
+Local project dependencies cannot be circular.\n\
+\n\
+The following circular imports were found.\n\
+Not all of them are necessarily problematic.\n\
+The problematic cycles are those declared as local dependencies.\n\
+\n\
+{0}
+
 multipleUnionDefaults=\
 A type union cannot have more than one default type.
 

pkl-core/src/test/kotlin/org/pkl/core/project/ProjectTest.kt

@@ -21,9 +21,11 @@ import java.util.regex.Pattern
 import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatCode
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.api.io.TempDir
 import org.pkl.commons.test.FileTestUtils
 import org.pkl.commons.test.PackageServer
+import org.pkl.commons.toPath
 import org.pkl.commons.writeString
 import org.pkl.core.*
 import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
@@ -188,4 +190,58 @@ class ProjectTest {
         )
     }
   }
+
+  @Test
+  fun `fails if project has cyclical dependencies`() {
+    val projectPath = javaClass.getResource("projectCycle1/PklProject")!!.toURI().toPath()
+    val e = assertThrows<PklException> { Project.loadFromPath(projectPath) }
+    val cleanMsg = e.message!!.replace(Regex("file:///.*/resources/test"), "file://")
+    assertThat(cleanMsg)
+      .isEqualTo(
+        """
+        –– Pkl Error ––
+        Local project dependencies cannot be circular.
+        
+        Cycle:
+        ┌─>
+        │  file:///org/pkl/core/project/projectCycle2/PklProject
+        │
+        │  file:///org/pkl/core/project/projectCycle3/PklProject
+        └─
+        """
+          .trimIndent()
+      )
+  }
+
+  @Test
+  fun `fails if a project has cyclical dependencies -- multiple cycles found`() {
+    val projectPath = javaClass.getResource("projectCycle4/PklProject")!!.toURI().toPath()
+    val e = assertThrows<PklException> { Project.loadFromPath(projectPath) }
+    val cleanMsg = e.message!!.replace(Regex("file://.*/resources/test"), "file://")
+    assertThat(cleanMsg)
+      .isEqualTo(
+        """
+        –– Pkl Error ––
+        Local project dependencies cannot be circular.
+        
+        The following circular imports were found.
+        Not all of them are necessarily problematic.
+        The problematic cycles are those declared as local dependencies.
+        
+        Cycle 1:
+        ┌─>
+        │  file:///org/pkl/core/project/projectCycle2/PklProject
+        │
+        │  file:///org/pkl/core/project/projectCycle3/PklProject
+        └─
+        
+        Cycle 2:
+        ┌─>
+        │  file:///org/pkl/core/project/projectCycle4/PklProject
+        └─
+        
+        """
+          .trimIndent()
+      )
+  }
 }

pkl-core/src/test/kotlin/org/pkl/core/util/ImportGraphUtilsTest.kt

@@ -0,0 +1,95 @@
+/*
+ * Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.pkl.core.util
+
+import java.net.URI
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+import org.pkl.core.ImportGraph
+
+class ImportGraphUtilsTest {
+  @Test
+  fun basic() {
+    val barUri = URI("file:///bar.pkl")
+    val fooUri = URI("file:///foo.pkl")
+    val graph =
+      ImportGraph(
+        mapOf(
+          fooUri to setOf(ImportGraph.Import(barUri)),
+          barUri to setOf(ImportGraph.Import(fooUri))
+        ),
+        // resolved URIs is not important
+        mapOf()
+      )
+    val cycles = ImportGraphUtils.findImportCycles(graph)
+    assertThat(cycles).isEqualTo(listOf(listOf(fooUri, barUri)))
+  }
+
+  @Test
+  fun `two cycles`() {
+    val barUri = URI("file:///bar.pkl")
+    val fooUri = URI("file:///foo.pkl")
+    val bizUri = URI("file:///biz.pkl")
+    val quxUri = URI("file:///qux.pkl")
+    val graph =
+      ImportGraph(
+        mapOf(
+          fooUri to setOf(ImportGraph.Import(barUri)),
+          barUri to setOf(ImportGraph.Import(fooUri)),
+          bizUri to setOf(ImportGraph.Import(quxUri)),
+          quxUri to setOf(ImportGraph.Import(bizUri))
+        ),
+        // resolved URIs is not important
+        mapOf()
+      )
+    val cycles = ImportGraphUtils.findImportCycles(graph)
+    assertThat(cycles).isEqualTo(listOf(listOf(fooUri, barUri), listOf(bizUri, quxUri)))
+  }
+
+  @Test
+  fun `no cycles`() {
+    val barUri = URI("file:///bar.pkl")
+    val fooUri = URI("file:///foo.pkl")
+    val bizUri = URI("file:///biz.pkl")
+    val quxUri = URI("file:///qux.pkl")
+    val graph =
+      ImportGraph(
+        mapOf(
+          barUri to setOf(ImportGraph.Import(fooUri)),
+          fooUri to setOf(ImportGraph.Import(bizUri)),
+          bizUri to setOf(ImportGraph.Import(quxUri)),
+          quxUri to setOf()
+        ),
+        // resolved URIs is not important
+        mapOf()
+      )
+    val cycles = ImportGraphUtils.findImportCycles(graph)
+    assertThat(cycles).isEmpty()
+  }
+
+  @Test
+  fun `self-import`() {
+    val fooUri = URI("file:///foo.pkl")
+    val graph =
+      ImportGraph(
+        mapOf(fooUri to setOf(ImportGraph.Import(fooUri))),
+        // resolved URIs is not important
+        mapOf()
+      )
+    val cycles = ImportGraphUtils.findImportCycles(graph)
+    assertThat(cycles).isEqualTo(listOf(listOf(fooUri)))
+  }
+}

pkl-core/src/test/resources/org/pkl/core/project/projectCycle1/PklProject

@@ -0,0 +1,12 @@
+amends "pkl:Project"
+
+package {
+  name = "projectCycle1"
+  version = "1.0.0"
+  packageZipUrl = "https://bogus.value"
+  baseUri = "package://localhost:0/projectCycle1"
+}
+
+dependencies {
+  ["projectCycle2"] = import("../projectCycle2/PklProject")
+}

pkl-core/src/test/resources/org/pkl/core/project/projectCycle2/PklProject

@@ -0,0 +1,12 @@
+amends "pkl:Project"
+
+package {
+  name = "projectCycle2"
+  version = "1.0.0"
+  packageZipUrl = "https://bogus.value"
+  baseUri = "package://localhost:0/projectCycle2"
+}
+
+dependencies {
+  ["projectCycle3"] = import("../projectCycle3/PklProject")
+}

pkl-core/src/test/resources/org/pkl/core/project/projectCycle3/PklProject

@@ -0,0 +1,12 @@
+amends "pkl:Project"
+
+package {
+  name = "projectCycle3"
+  version = "1.0.0"
+  packageZipUrl = "https://bogus.value"
+  baseUri = "package://localhost:0/projectCycle3"
+}
+
+dependencies {
+  ["projectCycle2"] = import("../projectCycle2/PklProject")
+}

pkl-core/src/test/resources/org/pkl/core/project/projectCycle4/PklProject

@@ -0,0 +1,7 @@
+amends "pkl:Project"
+
+import "PklProject"
+
+dependencies {
+  ["projectCycle1"] = import("../projectCycle1/PklProject")
+}