[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing #664

davidungar · 2021-05-14T18:47:13Z

A refactoring of inputDependencySourceMap prior to really fixing the below:

There is a remaining lurking bug here that involves an extra step.
Suppose you build in a normal way with foo.swift/foo.c setup. Then you
rebuild and cancel it after a frontend has emitted a new swiftdeps file,
finally, you crash the driver by exploiting this bug. The driver is now
in a state where swiftdeps disagree with the priors which disagree with
the output file map.

All this to say, we should take the earliest opportunity to remove
BidirectionalMap and its uses. But this commit is not the one to make
such a structural change to the driver.

Start on that.

rdar://77998890

davidungar · 2021-05-14T18:47:38Z

@swift-ci please test macOS Platform

davidungar · 2021-05-21T20:08:39Z

@swift-ci please test

CodaFi · 2021-05-21T20:16:37Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+         inputsAddedSincePriors }
+  @_spi(Testing) public mutating func addEntry(_ input: TypedVirtualPath,
+                                               _ dependencySource: DependencySource,
+                                               `for` _ : AdditionPurpose) {


Nit: You don't need to quote keywords when they're external labels.

Thanks, will do.

CodaFi · 2021-05-21T20:20:29Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+// MARK: - Populating
+extension InputDependencySourceMap {
+  public enum AdditionPurpose {
+    case mocking,


Please document these. Their names alone don't reveal much at the point of usage.

CodaFi · 2021-05-21T20:23:05Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

+
+  /// Returns: false on error
+  func populateInputDependencySourceMap(
+    `for` purpose: InputDependencySourceMap.AdditionPurpose


Nit: Same here. Don't need the quotes around for.

CodaFi · 2021-05-21T20:25:49Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+    biMap[source]
+  }
+
+  @_spi(Testing) public func enumerateToSerializePriors(


This name conflates the API's purpose with its usage. I think the swift-ier name is something closer to just enumerate(_:) or forEach(_:).

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.

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.

CodaFi · 2021-05-21T20:26:23Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+
+// MARK: - Accessing
+extension InputDependencySourceMap {
+  @_spi(Testing) public func getSourceIfKnown(for input: TypedVirtualPath) -> DependencySource? {


Just source(for:) would do here.

Yes, it would. And I'll remove the "get" prefix.

The idea here was to draw a very explicit distinction with ModuleDependencyGraph.getRequiredSource. In an earlier draft, both were named getSource and it was not easy to search for one as opposed to the other, because the only difference in usage was the result type. So, I decided to rename at least one, for the sake of clarity. Then, for the sake of symmetry (and clarity), I renamed them both.
So, I would rather keep the "IfKnown" part.

Hmm... after eliminating the "get"s, the "IfKnown" etc., moves to the keyword. You'll see after I push. Does that look better?

CodaFi · 2021-05-21T20:26:33Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+    biMap[input]
+  }
+
+  @_spi(Testing) public func getInputIfKnown(for source: DependencySource) -> TypedVirtualPath? {


just input(for:) as well

CodaFi

Some documentation of this new interface would be welcome. API notes are in-line.

davidungar · 2021-05-21T20:48:45Z

Some documentation of this new interface would be welcome. API notes are in-line.

Yes, I'll add.

davidungar · 2021-05-21T21:04:09Z

Thanks for the comments; they helped! Have addressed them, and now will fix the conflicts.

davidungar · 2021-05-21T21:09:29Z

@swift-ci please test

davidungar · 2021-05-21T21:12:20Z

Thanks, @CodaFi , for productive comments. I've made the changes requested, modulo keeping some intentionality in the function names. This rev may not be there yet, so please let me know what you see.

CodaFi · 2021-05-25T17:01:37Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+  ///
+  /// - 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? {


ifKnownFor has the same issue as below. Just for suffices.

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.

CodaFi · 2021-05-25T17:01:58Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/Tracer.swift

@@ -121,7 +121,8 @@ extension ModuleDependencyGraph.Tracer {
        path.compactMap { node in
          node.dependencySource.map {
            source in
-            graph.inputDependencySourceMap[source].map { input in
+            graph.inputDependencySourceMap.input(ifKnownFor: source).map {


Any reason this is here? Seems superfluous.

The "ifKnownFor" is there because it makes two aspects more salient:

That the function might not return a value. (In this use, it reinforces that the map is to handle an optional, rather than a collection.), and

That it depends on if the mapping is "known". I'd be happy to find a better word than "known". "Has been seen" feels too wordy.

CodaFi · 2021-05-25T17:02:56Z

...SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InputDependencySourceMap.swift

+  /// - 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) {


It looks like the for parameter is dead. Is that intentional?

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.

CodaFi · 2021-05-25T17:05:43Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

+  }
+}
+extension OutputFileMap {
+  fileprivate func getDependencySource(


Nit from before: get prefixes usually imply indirect return values in Cocoa and Swift. Just dependencySource(for:diagnosticEngine:) suffices.

Thanks--done!

CodaFi · 2021-05-25T17:07:02Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

+      if let source = ofm.getDependencySource(for: input, diagnosticEngine: diags) {
+        inputDependencySourceMap.addEntry(input, source, for: purpose)
+      }
+      else {


Nit: OTBS here please

Suggested change

else {

if let source = ofm.getDependencySource(for: input, diagnosticEngine: diags) {

inputDependencySourceMap.addEntry(input, source, for: purpose)

} else {

I did a look-up on "OTBS" and it came back with "off track horse betting"! :)

Looks like the request is to put the else on the same line as the closing brace--no prob!

CodaFi · 2021-05-25T17:07:48Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

-                                       file: String = #file,
-                                       line: Int = #line) -> DependencySource {
-    guard let source = inputDependencySourceMap[input] else {
+  @_spi(Testing) public func source(requiredFor input: TypedVirtualPath,


The requirement part 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 think for suffices.

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) vs let 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.

CodaFi · 2021-05-25T17:08:30Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -329,7 +345,8 @@ extension ModuleDependencyGraph {
  ) -> TransitivelyInvalidatedInputSet? {
    var invalidatedInputs = TransitivelyInvalidatedInputSet()
    for invalidatedSwiftDeps in collectSwiftDepsUsingInvalidated(nodes: directlyInvalidatedNodes) {
-      guard let invalidatedInput = getInput(for: invalidatedSwiftDeps) else {
+      guard let invalidatedInput = input(neededFor: invalidatedSwiftDeps)
+      else {


Put this back if you could. This fits onto 80 columns just fine.

What's the rule you follow in general for the "else" in a guard? 80-columns?

davidungar · 2021-05-25T17:41:42Z

@CodaFi Thank you for your review. I have responded to your comments and made changes where I agree. You have a wealth of experience with Swift style, and that helps my code fit in. To clarify, I think we agree that this PR makes no functional change, and that (for me) is the most critical part of your review, because I don't want to break the driver. There are some remaining disagreements on function naming, but I think they follow from my practice of choosing salience at the point-of-use. If Xcode were better at showing inferred types, or Doxygen documentation, I'd be in agreement with you.

Would you be able to approve this PR at this point?

davidungar · 2021-05-25T17:43:46Z

@swift-ci please test

CodaFi

I remain unconvinced that this is a meaningful change, and moreover that the new API surface here is an improvement over the status quo. But this PR has also been blocked on my review and I don’t see a reason for that much, so

davidungar · 2021-05-27T18:31:50Z

I remain unconvinced that this is a meaningful change, and moreover that the new API surface here is an improvement over the status quo. But this PR has also been blocked on my review and I don’t see a reason for that much, so

I understand. Thank you so very much for compromising here.

Revert #664

davidungar force-pushed the replace-bijection-on-vacation branch 2 times, most recently from c679036 to c2687c8 Compare May 21, 2021 18:11

davidungar changed the title ~~[DNM, WIP, Incremental] Replace the inputDependencySourceMap with the right thing~~ [NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing May 21, 2021

davidungar force-pushed the replace-bijection-on-vacation branch from e7c52e9 to b567bec Compare May 21, 2021 20:07

davidungar requested a review from CodaFi May 21, 2021 20:10

CodaFi reviewed May 21, 2021

View reviewed changes

CodaFi requested changes May 21, 2021

View reviewed changes

David Ungar added 3 commits May 21, 2021 14:05

Step 1: Isolate the map to be replaced

8a3fc05

Redo interface to inputDependencySourceMap.

72d9da7

Address review comments.

41237bf

davidungar force-pushed the replace-bijection-on-vacation branch from 4022e1a to 41237bf Compare May 21, 2021 21:08

CodaFi reviewed May 25, 2021

View reviewed changes

Address review comments.

0dc8704

CodaFi approved these changes May 27, 2021

View reviewed changes

davidungar merged commit 3f68e62 into swiftlang:main May 27, 2021

varungandhi-apple added a commit that referenced this pull request May 27, 2021

Merge pull request #681 from CodaFi/depshire

f0abc07

Revert #664

This was referenced May 28, 2021

[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing #682

Closed

[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing #685

Merged

[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing #664

[NFC, Incremental] Refactoring to facilitate replacing the inputDependencySourceMap with the right thing #664

Uh oh!

Conversation

davidungar commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented May 14, 2021

Uh oh!

davidungar commented May 21, 2021

Uh oh!

CodaFi May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

davidungar commented May 21, 2021

Uh oh!

davidungar commented May 21, 2021

Uh oh!

davidungar commented May 21, 2021

Uh oh!

davidungar commented May 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

davidungar commented May 14, 2021 •

edited

Loading

CodaFi May 21, 2021 •

edited

Loading

CodaFi May 21, 2021 •

edited

Loading

davidungar May 21, 2021 •

edited

Loading

CodaFi May 25, 2021 •

edited

Loading

davidungar May 25, 2021 •

edited

Loading