-
Notifications
You must be signed in to change notification settings - Fork 29
Fix crash on IntelliJ import with dependsOn(RootProject(...)) (#146) #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,20 +16,53 @@ trait SbtProjectStructureExtractorBase extends ProjectStructureExtractor { | |
| val buildDependencies: BuildDependencies | ||
| val projectsData: Seq[ProjectDataType] | ||
|
|
||
| /** | ||
| * Maps each [[ProjectRef]] of the current build to its extracted project data. | ||
| * | ||
| * Only projects that belong to this build (i.e. present in `projectsData`) are included. | ||
| * Projects pulled in via `dependsOn(RootProject(...))` live in their own builds and don't | ||
| * run sbt-idea-plugin, so we have no data for them and they are deliberately absent here. | ||
| * That is why every lookup against `projectMap` in this trait is guarded with | ||
| * `projectMap.contains` / `projectMap.get`: an unguarded `projectMap(externalRef)` is exactly | ||
| * the `NoSuchElementException` reported in issue #146. | ||
| * | ||
| * The filtering behaviour is covered by `SbtProjectStructureExtractorExternalRefsTest`. | ||
| */ | ||
| protected lazy val projectMap: Map[ProjectRef, ProjectDataType] = projectsData.iterator.map(x => x.thisProject -> x).toMap | ||
| protected lazy val revProjectMap: Seq[(ProjectRef, ProjectRef)] = projectsData.flatMap(x => buildDependencies.classpathRefs(x.thisProject).map(_ -> x.thisProject)) | ||
| // `filter(projectMap.contains)` drops reverse edges to external projects (dependsOn(RootProject(...))): | ||
| // they are not in projectMap, so leaving them in would later crash collectParents' `projectMap(ref)` | ||
| // lookup (the original #146 bug). See SbtProjectStructureExtractorExternalRefsTest. | ||
| protected lazy val revProjectMap: Seq[(ProjectRef, ProjectRef)] = projectsData.flatMap(x => buildDependencies.classpathRefs(x.thisProject).filter(projectMap.contains).map(_ -> x.thisProject)) | ||
| protected lazy val projectCache: mutable.Map[ProjectRef, NodeType] = mutable.HashMap.empty | ||
|
|
||
| def findProjectRef(project: Project): Option[ProjectRef] = projectMap.find(_._1.project == project.id).map(_._1) | ||
|
|
||
| protected def topoSortRefs(root: ProjectRef, queue: Seq[ProjectRef] = Seq.empty): Seq[ProjectRef] = { | ||
| val data = projectMap(root) | ||
| if (!queue.contains(root)) { | ||
| val newQueue = queue :+ root | ||
| val direct = buildDependencies.classpathRefs(root).foldLeft(newQueue) { case (q, r) => topoSortRefs(r, q) } | ||
| val additional = collectAdditionalProjects(data, direct) | ||
| additional | ||
| } else { queue } | ||
| projectMap.get(root) match { | ||
| case None => | ||
| // `root` is an external project (dependsOn(RootProject(...))) that isn't part of this | ||
| // build, so we have no data for it and can't place it in the graph — skip it. See #146. | ||
| log.warn(s"skipping external project ref not part of the current build: $root") | ||
| queue | ||
| case Some(data) => | ||
| if (queue.contains(root)) queue | ||
| else enqueueWithDependencies(data, root, queue) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Appends `root` to the topo-sort `queue`, then recursively visits its classpath | ||
| * dependencies and any additional projects contributed by subclasses. | ||
| * | ||
| * External classpath refs are filtered out here for the same reason as in [[projectMap]]: | ||
| * they belong to other builds and have no entry to recurse into. | ||
| */ | ||
| private def enqueueWithDependencies(data: ProjectDataType, root: ProjectRef, queue: Seq[ProjectRef]): Seq[ProjectRef] = { | ||
| val newQueue = queue :+ root | ||
| val direct = buildDependencies.classpathRefs(root) | ||
| .filter(projectMap.contains) | ||
| .foldLeft(newQueue) { case (q, r) => topoSortRefs(r, q) } | ||
| collectAdditionalProjects(data, direct) | ||
| } | ||
|
|
||
| protected def collectAdditionalProjects(data: ProjectDataType, direct: Seq[ProjectRef]): Seq[ProjectRef] = direct | ||
|
|
@@ -69,7 +102,10 @@ trait SbtProjectStructureExtractorBase extends ProjectStructureExtractor { | |
| } | ||
|
|
||
| override def collectChildren(node: NodeType, data: ProjectDataType): Seq[NodeType] = { | ||
| val childRefs = buildDependencies.classpathRefs(node.ref) | ||
| // Same rationale as revProjectMap/topoSortRefs: external classpath refs | ||
| // (dependsOn(RootProject(...))) have no cached stub, so exclude them before lookup. See #146 | ||
| // and SbtProjectStructureExtractorExternalRefsTest. | ||
| val childRefs = buildDependencies.classpathRefs(node.ref).filter(projectCache.contains) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, a some small comment for the filtering would be nice to have, with reference to example
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a short comment on the |
||
| assert(childRefs.forall(projectCache.contains), s"Child stubs incomplete: ${childRefs.filterNot(projectCache.contains)}") | ||
| childRefs.map(projectCache) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| package org.jetbrains.sbtidea.structure.sbtImpl | ||
|
|
||
| import org.jetbrains.sbtidea.PluginLogger | ||
| import org.jetbrains.sbtidea.structure._ | ||
| import org.scalatest.funsuite.AnyFunSuite | ||
| import org.scalatest.matchers.should.Matchers | ||
| import sbt._ | ||
| import sbt.internal.BuildDependencies | ||
|
|
||
| import java.io.File | ||
| import java.net.URI | ||
|
|
||
| /** | ||
| * Verifies how [[SbtProjectStructureExtractorBase]] handles '''external''' project references — | ||
| * those introduced by `dependsOn(RootProject(...))`, which belong to a different sbt build and | ||
| * are therefore absent from `projectsData` / `projectMap`. | ||
| * | ||
| * Regression coverage for issue #146: before the fix an external ref reaching `projectMap(ref)` | ||
| * threw a `NoSuchElementException` during IntelliJ import. The cases below exercise the guarded | ||
| * lookups in `topoSortRefs`, `revProjectMap` and `collectChildren`. | ||
| * | ||
| * These scenarios can't be reused from the existing integration-style structure tests: those load | ||
| * a real sbt build where every project shares one build, so no ref is ever "external". Here we | ||
| * drive the trait directly with a synthetic dependency graph and a hand-built [[BuildDependencies]] | ||
| * (via the public `BuildDependencies.apply` factory), which is the only way to introduce a ref | ||
| * that is on the classpath yet missing from `projectMap`. | ||
| */ | ||
| class SbtProjectStructureExtractorExternalRefsTest extends AnyFunSuite with Matchers { | ||
|
|
||
| private def mkRef(name: String, uri: String = "file:///tmp/build/"): ProjectRef = | ||
| ProjectRef(new URI(uri), name) | ||
|
|
||
| /** Builds a real [[BuildDependencies]] from a plain `ref -> classpath deps` adjacency map. */ | ||
| private def mkBuildDependencies(classpathDeps: Map[ProjectRef, Seq[ProjectRef]]): BuildDependencies = { | ||
| val classpath: Map[ProjectRef, Seq[ClasspathDep[ProjectRef]]] = | ||
| classpathDeps.map { case (ref, deps) => | ||
| ref -> deps.map(dep => ResolvedClasspathDependency(dep, None): ClasspathDep[ProjectRef]) | ||
| } | ||
| BuildDependencies(classpath, Map.empty) | ||
| } | ||
|
|
||
| private case class StubProjectData( | ||
| thisProject: ProjectRef, | ||
| cp: sbt.Def.Classpath = Nil, | ||
| definedDeps: Seq[ModuleID] = Nil, | ||
| productDirs: Seq[File] = Nil, | ||
| report: UpdateReport = null | ||
| ) extends CommonSbtProjectData | ||
|
|
||
| private class StubNode( | ||
| val ref: ProjectRef, | ||
| val name: String | ||
| ) extends SbtProjectNode { | ||
| override type T = StubNode | ||
| var parents: Seq[StubNode] = Nil | ||
| var children: Seq[StubNode] = Nil | ||
| var libs: Seq[Library] = Nil | ||
| } | ||
|
|
||
| /** | ||
| * Concrete extractor over the stub types. It implements only the genuinely abstract members | ||
| * (`buildStub`, `updateNode`, `collectLibraries`); all dependency traversal runs against the | ||
| * real [[BuildDependencies]] passed in, so the production `topoSortRefs` / `revProjectMap` / | ||
| * `collectChildren` logic is exercised verbatim — nothing is re-implemented here. | ||
| */ | ||
| private class TestExtractor( | ||
| override val rootProject: ProjectRef, | ||
| override val projectsData: Seq[StubProjectData], | ||
| override val buildDependencies: BuildDependencies | ||
| ) extends SbtProjectStructureExtractorBase { | ||
| override type ProjectDataType = StubProjectData | ||
| override type NodeType = StubNode | ||
|
|
||
| override implicit val log: PluginLogger = PluginLogger | ||
|
|
||
| override def buildStub(data: StubProjectData): StubNode = | ||
| new StubNode(data.thisProject, data.thisProject.project) | ||
|
|
||
| override def updateNode(node: StubNode, data: StubProjectData): StubNode = { | ||
| node.children = collectChildren(node, data) | ||
| node.parents = collectParents(node, data) | ||
| node.libs = Nil | ||
| node | ||
| } | ||
|
|
||
| override def collectLibraries(data: StubProjectData): Seq[Library] = Nil | ||
|
|
||
| // expose protected members for testing | ||
| def testTopoSortRefs(root: ProjectRef): Seq[ProjectRef] = topoSortRefs(root) | ||
| def testRevProjectMap: Seq[(ProjectRef, ProjectRef)] = revProjectMap | ||
| } | ||
|
|
||
| test("topoSortRefs skips external ProjectRefs not in projectMap") { | ||
| val internalRef = mkRef("internal-project") | ||
| val externalRef = mkRef("external-project", "file:///tmp/external-build/") | ||
|
|
||
| val projectsData = Seq(StubProjectData(internalRef)) | ||
| val extractor = new TestExtractor(internalRef, projectsData, | ||
| mkBuildDependencies(Map(internalRef -> Seq(externalRef))) | ||
| ) | ||
| val sorted = extractor.testTopoSortRefs(internalRef) | ||
|
|
||
| sorted should contain(internalRef) | ||
| sorted should not contain externalRef | ||
| } | ||
|
|
||
| test("topoSortRefs returns empty queue when root is external") { | ||
| val externalRef = mkRef("external-project", "file:///tmp/external-build/") | ||
|
|
||
| val extractor = new TestExtractor(externalRef, Seq.empty, mkBuildDependencies(Map.empty)) | ||
| val sorted = extractor.testTopoSortRefs(externalRef) | ||
|
|
||
| sorted shouldBe empty | ||
| } | ||
|
|
||
| test("topoSortRefs works normally with all-internal refs") { | ||
| val refA = mkRef("a") | ||
| val refB = mkRef("b") | ||
| val refC = mkRef("c") | ||
|
|
||
| val projectsData = Seq(StubProjectData(refA), StubProjectData(refB), StubProjectData(refC)) | ||
| val extractor = new TestExtractor(refA, projectsData, | ||
| mkBuildDependencies(Map(refA -> Seq(refB), refB -> Seq(refC), refC -> Nil)) | ||
| ) | ||
| val sorted = extractor.testTopoSortRefs(refA) | ||
|
|
||
| sorted should contain allOf(refA, refB, refC) | ||
| sorted.size shouldBe 3 | ||
| } | ||
|
|
||
| test("revProjectMap filters out external refs") { | ||
| val internalA = mkRef("a") | ||
| val internalB = mkRef("b") | ||
| val externalRef = mkRef("external", "file:///tmp/external/") | ||
|
|
||
| val projectsData = Seq(StubProjectData(internalA), StubProjectData(internalB)) | ||
| val extractor = new TestExtractor(internalA, projectsData, | ||
| mkBuildDependencies(Map(internalA -> Seq(internalB, externalRef), internalB -> Nil)) | ||
| ) | ||
| val revMap = extractor.testRevProjectMap | ||
|
|
||
| revMap should contain((internalB, internalA)) | ||
| revMap.map(_._1) should not contain externalRef | ||
| } | ||
|
|
||
| test("extract succeeds with mixed internal and external dependencies") { | ||
| val internalRef = mkRef("plugin") | ||
| val externalRef = mkRef("library", "file:///tmp/external/") | ||
|
|
||
| val projectsData = Seq(StubProjectData(internalRef)) | ||
| val extractor = new TestExtractor(internalRef, projectsData, | ||
| mkBuildDependencies(Map(internalRef -> Seq(externalRef))) | ||
| ) | ||
| val result = extractor.extract | ||
|
|
||
| result.size shouldBe 1 | ||
| result.head.name shouldBe "plugin" | ||
| result.head.children shouldBe empty | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave a line comment here with explanation why filtering as per the
projectMapis needed? For people not in the context this bit can be usefulThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ideally also needs a comment (Scaladoc?) to the
projectMapThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment might also refer to the existing tests (see example here...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, all three:
revProjectMapexplaining the.filter(projectMap.contains)drops reverse edges to externaldependsOn(RootProject(...))projects — theyaren't in
projectMap, so keeping them would crash the laterprojectMap(ref)lookup incollectParents(the dependsOn(RootProject(...)) crashes on IntelliJ import when depending on a non-IntelliJ SBT project #146 bug).projectMapitself: it holds only refs from the current build (projectsData); external refs are deliberately absent, which is why every lookup inthe trait is guarded with
projectMap.contains/projectMap.get.SbtProjectStructureExtractorExternalRefsTestas the example.