diff --git a/pkl-core/src/main/java/org/pkl/core/project/Project.java b/pkl-core/src/main/java/org/pkl/core/project/Project.java index 950385ae..d0fc8341 100644 --- a/pkl-core/src/main/java/org/pkl/core/project/Project.java +++ b/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 = - 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()) { + try (var evaluator = evaluatorBuilder().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> 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 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 cycle) { + var sb = new StringBuilder(); + renderCycle(sb, cycle); + return sb.toString(); + } + + private static List> 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) getProperty(pObj, "apiTests"); var apiTests = apiTestStrs.stream().map(Path::of).collect(Collectors.toList()); var exclude = (List) getProperty(pObj, "exclude"); diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmExceptionRenderer.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmExceptionRenderer.java index 095d5b09..9f72bb0d 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmExceptionRenderer.java +++ b/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); } } } diff --git a/pkl-core/src/main/java/org/pkl/core/util/ImportGraphUtils.java b/pkl-core/src/main/java/org/pkl/core/util/ImportGraphUtils.java new file mode 100644 index 00000000..1010513e --- /dev/null +++ b/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> findImportCycles(ImportGraph importGraph) { + var res = new ArrayList>(); + 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 doFindCycle( + URI currentUri, ImportGraph importGraph, List 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; + } +} diff --git a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties index 3fcb9c1f..8a02a926 100644 --- a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties +++ b/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. diff --git a/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectTest.kt index ba9cac04..b60a75b2 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/project/ProjectTest.kt +++ b/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 { 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 { 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() + ) + } } diff --git a/pkl-core/src/test/kotlin/org/pkl/core/util/ImportGraphUtilsTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/util/ImportGraphUtilsTest.kt new file mode 100644 index 00000000..fffcb58b --- /dev/null +++ b/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))) + } +} diff --git a/pkl-core/src/test/resources/org/pkl/core/project/projectCycle1/PklProject b/pkl-core/src/test/resources/org/pkl/core/project/projectCycle1/PklProject new file mode 100644 index 00000000..3c4bf3ea --- /dev/null +++ b/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") +} diff --git a/pkl-core/src/test/resources/org/pkl/core/project/projectCycle2/PklProject b/pkl-core/src/test/resources/org/pkl/core/project/projectCycle2/PklProject new file mode 100644 index 00000000..35b73502 --- /dev/null +++ b/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") +} diff --git a/pkl-core/src/test/resources/org/pkl/core/project/projectCycle3/PklProject b/pkl-core/src/test/resources/org/pkl/core/project/projectCycle3/PklProject new file mode 100644 index 00000000..013edd42 --- /dev/null +++ b/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") +} diff --git a/pkl-core/src/test/resources/org/pkl/core/project/projectCycle4/PklProject b/pkl-core/src/test/resources/org/pkl/core/project/projectCycle4/PklProject new file mode 100644 index 00000000..af8ad15a --- /dev/null +++ b/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") +}