[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing #664
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| //===------------- InputDependencySourceMap.swift ---------------- --------===// | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2021 Apple Inc. and the Swift project authors | ||
| // Licensed under Apache License v2.0 with Runtime Library Exception | ||
| // | ||
| // See https://swift.org/LICENSE.txt for license information | ||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
| import Foundation | ||
| import TSCBasic | ||
|
|
||
| /// Maps input source files to- and from- `DependencySource`s containing the swiftdeps paths. | ||
| /// Deliberately at the level of specific intention, for clarity and to facilitate restructuring later. | ||
| @_spi(Testing) public struct InputDependencySourceMap: Equatable { | ||
|
|
||
| /// Maps input files (e.g. .swift) to and from the DependencySource object. | ||
| /// | ||
| // FIXME: The map between swiftdeps and swift files is absolutely *not* | ||
| // a bijection. In particular, more than one swiftdeps file can be encountered | ||
| // in the course of deserializing priors *and* reading the output file map | ||
| // *and* re-reading swiftdeps files after frontends complete | ||
| // that correspond to the same swift file. These cause two problems: | ||
| // - overwrites in this data structure that lose data and | ||
| // - cache misses in `getInput(for:)` that cause the incremental build to | ||
| // turn over when e.g. entries in the output file map change. This should be | ||
| // replaced by a multi-map from swift files to dependency sources, | ||
| // and a regular map from dependency sources to swift files - | ||
| // since that direction really is one-to-one. | ||
|
|
||
| /// Holds the mapping for now. To be replaced later. | ||
| public typealias BiMap = BidirectionalMap<TypedVirtualPath, DependencySource> | ||
| @_spi(Testing) public var biMap = BiMap() | ||
| } | ||
|
|
||
| // MARK: - Accessing | ||
| extension InputDependencySourceMap { | ||
| /// Find where the swiftdeps are stored for a given source file. | ||
| /// | ||
| /// - Parameter input: A source file path | ||
| /// - Returns: the corresponding `DependencySource`, or nil if none known. | ||
| @_spi(Testing) public func sourceIfKnown(for input: TypedVirtualPath) -> DependencySource? { | ||
| biMap[input] | ||
| } | ||
|
|
||
| /// Find where the source file is for a given swiftdeps file. | ||
| /// | ||
| /// - Parameter source: A `DependencySource` (containing a swiftdeps file) | ||
| /// - Returns: the corresponding input source file, or nil if none known. | ||
| @_spi(Testing) public func input(ifKnownFor source: DependencySource) -> TypedVirtualPath? { | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Please see my comment for the next issue. I have a rule: Correlate salience with importance in order to ease the comprehension for new maintainer. |
||
| biMap[source] | ||
| } | ||
|
|
||
| /// Enumerate the input <-> dependency source pairs to be serialized | ||
| /// | ||
| /// - Parameter eachFn: a function to be called for each pair | ||
| @_spi(Testing) public func enumerateToSerializePriors( | ||
|
There was a problem hiding this comment. This name conflates the API's purpose with its usage. I think the swift-ier name is something closer to just There was a problem hiding this comment. Yes, it does. The conflation is quite deliberate here. The enclosing type is meant to operate at the level of intention. This level distinction paves the way for the greater structural change to come. There was a problem hiding this comment. I understand that, but Swift does not use the name of the API in the same way that Objective-C or Smalltalk might encourage the use of intention-revealing selectors. |
||
| _ eachFn: (TypedVirtualPath, DependencySource) -> Void | ||
| ) { | ||
| biMap.forEach(eachFn) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Populating | ||
| extension InputDependencySourceMap { | ||
| /// For structural modifications-to-come, reify the various reasons to add an entry. | ||
| public enum AdditionPurpose { | ||
| /// For unit testing | ||
| case mocking | ||
|
|
||
| /// When building a graph from swiftDeps files without priors | ||
| case buildingFromSwiftDeps | ||
|
|
||
| /// Deserializing the map stored with the priors | ||
| case readingPriors | ||
|
|
||
| /// After reading the priors, used to add entries for any inputs that might not have been in the priors. | ||
| case inputsAddedSincePriors | ||
| } | ||
|
|
||
| /// Add a mapping to & from input source file to swiftDeps file path | ||
| /// | ||
| /// - Parameter input: the source file path | ||
| /// - Parameter dependencySource: the dependency source (holding the swiftdeps path) | ||
| /// - Parameter why: the purpose for this addition. Will be used for future restructuring. | ||
| @_spi(Testing) public mutating func addEntry(_ input: TypedVirtualPath, | ||
| _ dependencySource: DependencySource, | ||
| for why: AdditionPurpose) { | ||
|
There was a problem hiding this comment. It looks like the There was a problem hiding this comment. Yes. As the documentation says, it will be used in the future. For now, it makes clear at the point-of-use what the purpose is. |
||
| assert(input.type == .swift && dependencySource.typedFile.type == .swiftDeps) | ||
| biMap[input] = dependencySource | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
The
requirementpart of this API is communicated by the type signature - being that it has a non-optional return value. You could reinforce this in documentation, but I thinkforsuffices.Uh oh!
There was an error while loading. Please reload this page.
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.
The requirement is in the function name, because it makes the required nature more salient at the point of use, without clicking to see the documentation, or the type of the assigned-to variable.
Consider:
let foo = source(requiredFor: someInput)vslet foo = source(for: someInput).The first construction makes the required nature of the mapping more salient.
I have a rule: that when some distinction is important, I make it salient at the point of use, and the correlation between importance and salience trumps most other considerations.