Cargo-culted, but then Swift-ified and cleaned-up ModuleDependencyGraph #283
Conversation
# Conflicts: # Sources/SwiftDriver/Driver/Driver.swift # Sources/SwiftDriver/Driver/ToolExecutionDelegate.swift # Sources/SwiftDriver/Incremental Compilation/IncrementalCompilationState.swift
| @swift-ci please test |
| let swiftDeps: String | ||
| let destination: ModuleDependencyGraph | ||
| | ||
| /// When done, changeDependencyKeys contains a list of keys that changed |
There was a problem hiding this comment.
I think this comment is stale, I do not see changeDependencyKeys in this PR.
There was a problem hiding this comment.
Thanks! I'll update it.
| /// | ||
| /// Use a class, not a struct because otherwise it would be duplicated for each thing it uses | ||
| | ||
| @_spi(Testing) public final class ModuleDepGraphNode { |
There was a problem hiding this comment.
This doesn't seem to need reference semantics.
There was a problem hiding this comment.
Right! It doesn't. I'm worried about the memory footprint of using a struct. Each Decl and each use of a Decl would need a copy. If someone has a huge module it might add up.
Or is it still reasonable?
| // MARK: - comparing, hashing | ||
| extension ModuleDepGraphNode: Equatable, Hashable { | ||
| /// Excludes hasBeenTraced... | ||
| static public func == (lhs: ModuleDepGraphNode, rhs: ModuleDepGraphNode) -> Bool { |
There was a problem hiding this comment.
Nit: access level first.
public static func
There was a problem hiding this comment.
Also, can this conformance be derived?
There was a problem hiding this comment.
Thanks! Will fix.
The compiler doesn't seem to derive that conformance for a class.
| && lhs.swiftDeps == rhs.swiftDeps | ||
| } | ||
| | ||
| public func hash(into hasher: inout Hasher) { |
There was a problem hiding this comment.
It doesn't seem to like deriving those conformance is for classes.
| @@ -0,0 +1,101 @@ | |||
| //===------------------------- Multidictionary.swift ----------------------===// | |||
There was a problem hiding this comment.
Filename has typo Multdictionary. Missing i
| What would y'all say to splitting this into its own package target (say, |
| } | ||
| } | ||
| | ||
| private func integrateANewDef(_ integrand: SourceFileDependencyGraph.Node) |
There was a problem hiding this comment.
Nit: This has a bunch of needless words. integrate(definition:) reads better.
There was a problem hiding this comment.
I can eliminate the "A", but "new" and "def" are significant here. It's integrating something new to the graph, and it's a def, not a use. But if you have something else equally expressive...
| The indentation in all of these files is very higgledy-piggledy. Could you try to match the style of the rest of the driver? |
| /// Returns a nil graph if there's a error | ||
| private static func getSourceFileDependencyGraphs(job: Job, | ||
| diagnosticEngine: DiagnosticsEngine) | ||
| -> [(graph: SourceFileDependencyGraph?, swiftDeps: String)] |
There was a problem hiding this comment.
This is crying out for a type.
private enum LoadedDependencyGraph { case success(SourceFileDependencyGraph, String) case failure(String) } | return nil | ||
| } | ||
| | ||
| let (foundChange: foundChange, integratedNode: integratedNode) = |
There was a problem hiding this comment.
Let's use a plain tuple here instead of a shuffle.
There was a problem hiding this comment.
Do you mean not immediately decompose the tuple? If so, what's a good name for the whole tuple? I'm having trouble coming up with one.
| func testBasicLoad() { | ||
| let graph = ModuleDependencyGraph(mock: de) | ||
| | ||
| graph.simulateLoad(job0, [.topLevel: ["a->", "b->"]]) |
There was a problem hiding this comment.
Much as I think the DSL you built for dependency edges is cute, I would like us to rebuild this with something more maintainable at some point in the future.
| import Foundation | ||
| import TSCBasic | ||
| | ||
| import Foundation |
There was a problem hiding this comment.
I don't think this needs to import Foundation.
There was a problem hiding this comment.
Yeah, and not TSCBasic, either. Both gone. I could say I was importing it twice to be sure everything got imported, but that would just be a bad joke.
| private var jobsBySwiftDeps: [String: Job] = [:] | ||
| | ||
| | ||
| func getJob(_ swiftDeps: String) -> Job { |
There was a problem hiding this comment.
I would like a stronger type than String for the SwiftDeps file here and in the integrator.
There was a problem hiding this comment.
yes, good idea! Excellent suggestion.
| /// The former comes from a frontend job, and the latter is used by the driver. | ||
| @_spi(Testing) public struct DepGraphIntegrator { | ||
| /// nil means there was an error | ||
| @_spi(Testing) public typealias Changes = Set<ModuleDepGraphNode>? |
There was a problem hiding this comment.
This is a nit, but I think it may be more-readable to have Changes be non-optional here and then changedNodes can have type Changes and the signature of integrate would return Changes? and convey that the result may be nil to the reader?
| let (foundChange: foundChange, integratedNode: integratedNode) = | ||
| integrate(integrand, reconcilingWith: preexistingMatchHereOrExpat) | ||
| | ||
| let hasNewExternalDependency = recordWhatIsDependendedUpon( |
| switch (lhs, rhs) { | ||
| case (.end, .end): return false | ||
| case (_, .end): return true | ||
| case (end, _): return false |
There was a problem hiding this comment.
Great catch! Yet it compiled and ran...?
| } | ||
| // MARK: - comparing, hashing | ||
| extension ModuleDepGraphNode: Equatable, Hashable { | ||
| /// Excludes hasBeenTraced... |
There was a problem hiding this comment.
nit: Looks like this comment might not be relevant to the new implementation
It would be nice to hide some of implementation details, but I think the use of |
There was a problem hiding this comment.
I don't have much to add to the other reviews, but it's really great to see this coming together! Once the organization is final all the new files should be added to the CMake build too.
Thanks! Yes, thank you for that reminder. (I don't understand how I got into review mode!)
| } | ||
| } | ||
| | ||
| private func integrateANewDef(_ integrand: SourceFileDependencyGraph.Node) |
There was a problem hiding this comment.
I can eliminate the "A", but "new" and "def" are significant here. It's integrating something new to the graph, and it's a def, not a use. But if you have something else equally expressive...
| switch (lhs, rhs) { | ||
| case (.end, .end): return false | ||
| case (_, .end): return true | ||
| case (end, _): return false |
There was a problem hiding this comment.
Great catch! Yet it compiled and ran...?
| import Foundation | ||
| import TSCBasic | ||
| | ||
| import Foundation |
There was a problem hiding this comment.
Yeah, and not TSCBasic, either. Both gone. I could say I was importing it twice to be sure everything got imported, but that would just be a bad joke.
| private var jobsBySwiftDeps: [String: Job] = [:] | ||
| | ||
| | ||
| func getJob(_ swiftDeps: String) -> Job { |
There was a problem hiding this comment.
yes, good idea! Excellent suggestion.
| func testBasicLoad() { | ||
| let graph = ModuleDependencyGraph(mock: de) | ||
| | ||
| graph.simulateLoad(job0, [.topLevel: ["a->", "b->"]]) |
| @DougGregor @CodaFi @artemcm At this point, I think I've addressed the great suggestions. Would you care to have a look? TIA. |
| @swift-ci please test |
| @swift-ci please test |
A protocol could provide a sufficient abstraction boundary here. We could sink it and common data structures into a Core or Basics library and pull that into both targets. |
5 participants
This is a bit of a big PR, sorry. It's the driver's dependency graph, along with the unit tests, which run!