[5.5, NFC, Incremental] Create a domain-specific type for the inputDependencySourceMap #710

davidungar · 2021-06-11T05:54:10Z

Cherry-pick of #685 for the `release/5.5` branch.

This PR paves the way for bug fixes by adding an intentions layer to the usage of a bidirectional map to go from input files (.swift) to and from dependency files (.swiftdeps).

Wrap the BidirectionalMap into its own type, and put that into InputDependencySourceMap.swift
Make the interface very explicit as to intentions:

Add an enum to explain why entries are being added (AdditionPurpose) and pass that through calls that add entries

addMapEntry in the ModuleDependencyGraph becomes addEntry on the new type

The new type does lookups via sourceIfKnown(for:) and inputIfKnown(for:) to contrast the possibility of failure with sourceRequired(for:), which is the new name for getSource(for:) in the graph method.

Fixes a misnomer in a local variable in collectInputsInvalidatedBy(input:)

The reduce in populateInputDependencySourceMap has been rewritten for clarity.

In preparation for: rdar://77998890
rdar://79211265

davidungar · 2021-06-11T05:54:22Z

@swift-ci please test

…ncySourceMap Will facilitate making it immutable

davidungar · 2021-06-11T06:30:32Z

@swift-ci please test

artemcm

LGTM for the 5.5 branch.

CodaFi

It is not clear to me what invariants are captured by this data structure and why this refactoring needs to go into 5.5.

CodaFi · 2021-06-21T19:04:29Z

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

+  public typealias BiMap = BidirectionalMap<TypedVirtualPath, DependencySource>
+  @_spi(Testing) public var biMap = BiMap()
+
+  private let simulateGetInputFailure: Bool


Why is this a format part of the API? We know how to create this failure mode in tests for the current implementation, and the new one should structurally avoid the issue altogether.

simulateGetInputFailure is gone on the main branch. It remains here in the interest of minimal changes to this branch. If you insist, I'm happy with removing it. Whatever the folks in charge of this branch prefer.

CodaFi · 2021-06-21T19:05:01Z

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

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


Please document these

Good point! Will do.

CodaFi · 2021-06-21T19:05:24Z

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

+extension InputDependencySourceMap {
+  @_spi(Testing) public func sourceIfKnown(for input: TypedVirtualPath) -> DependencySource? {
+    biMap[input]
+  }
+
+  @_spi(Testing) public func inputIfKnown(for source: DependencySource) -> TypedVirtualPath? {
+    simulateGetInputFailure ? nil : biMap[source]
+  }
+
+  @_spi(Testing) public func enumerateToSerializePriors(
+    _ eachFn: (TypedVirtualPath, DependencySource) -> Void
+  ) {
+    biMap.forEach(eachFn)
+  }
+}
+


Would very much prefer we not conflate the point of use with the names of these APIs.

Could you please elaborate? I want to be sure I understand the point here. What would you suggest instead?
If you feel that enumerateToSerializePriors is too specific, then we probably disagree about the value of intention-revealing names.

If that is the source of disagreement, here is a nice quote I found that explains what I am trying to accomplish:

If a developer must consider the implementation of a component in order to use it, the value of encapsulation is lost. If someone other than the original developer must infer the purpose of an object or operation based on its implementation, that new developer may infer a purpose that the operation or class fulfills only by chance. If that was not the intent, the code may work for the moment, but the conceptual basis of the design will have been corrupted, and the two developers will be working at cross-purposes.

Therefore,

Name classes and operations to describe their effect and purpose, without reference to the means by which they do what they promise. This relieves the client developer of the need to understand the internals.

From http://ddd.fed.wiki.org/view/intention-revealing-interfaces

davidungar · 2021-06-21T20:01:52Z

LGTM for the 5.5 branch.

Thank you!

davidungar · 2021-06-21T20:05:55Z

It is not clear to me what invariants are captured by this data structure and why this refactoring needs to go into 5.5.

I understand your preference, and the custom type does add one more term to the vocabulary here. Here's why I think that this type eases maintenance:

Because it:

Provides a focus for an overall comment,
Moves the invariants into a dedicated initializer,
Replaces subscript invocations with domain-specific, intention-revealing names such as sourceIfKnown(for:)
Provides a separate place for an extension to OutputFileMap that is specific to this data structure
I believe that it enhances the maintainability of the code by making it easier for someone to understand what's going on.
The invariants are spelled out in the comment: ```
/// Maps input files (e.g. .swift) to and from the DependencySource object.
///
// This map caches the same information as in the OutputFileMap, but it
// optimizes the reverse lookup, and includes path interning via DependencySource.
// Once created, it does not change.

Most of all, it lays the foundation for #712, by isolating the locus of the change.

davidungar · 2021-06-21T20:14:19Z

@swift-ci please test

davidungar · 2021-06-21T20:20:38Z

@CodaFi Thanks a lot for taking a look at this. I was hoping you would verify that this refactoring introduces no functional change. From what I can tell your comments relate to: the maintenance value of the refactoring, and the choice of names. (I have added documentation per your request.) Is this assessment of your review correct? If so, I am heartened by your assessment. Thank you again!

davidungar · 2021-06-21T20:24:10Z

@swift-ci please test

davidungar · 2021-06-21T22:46:57Z

@swift-ci please test macos platform

CodaFi · 2021-06-22T01:41:45Z

I was hoping you would verify that this refactoring introduces no functional change.

This is sidestepping the primary issues with this patch(set). I have expressed concerns from the start about merging a large patchset including refactoring to the release branch without additional qualification or sufficient time to seed to users both internally and externally so we can detect issues before release. We should be shooting for narrow fixes to issues, as you yourself articulated upthread, and there is a much more narrow course of action here: Only using the output file map as a source of truth. I do not understand why the rest of the refactoring needs to come along for the ride.

davidungar · 2021-06-22T06:12:17Z

I was hoping you would verify that this refactoring introduces no functional change.

This is sidestepping the primary issues with this patch(set). I have expressed concerns from the start about merging a large patchset including refactoring to the release branch without additional qualification or sufficient time to seed to users both internally and externally so we can detect issues before release. We should be shooting for narrow fixes to issues, as you yourself articulated upthread, and there is a much more narrow course of action here: Only using the output file map as a source of truth. I do not understand why the rest of the refactoring needs to come along for the ride.

Do you mean that, although you cannot identify a functional change from the refactoring, in the interest of reducing risk for the beta, you advocate the functional change of making the OutputFileMap the source of truth without also doing the refactoring? If so, then I could agree. Although IMO, the refactoring adds value, and we have both looked at it, the greater value is getting the immutability and single-source-of-truth into the beta, ASAP.

davidungar requested a review from a team as a code owner June 11, 2021 05:54

[NFC, Incremental] Create a domain-specific type for the inputDepende…

fe7100a

…ncySourceMap Will facilitate making it immutable

davidungar force-pushed the 5.5-catchup-1 branch from d538883 to fe7100a Compare June 11, 2021 06:30

davidungar changed the title ~~[NFC, Incremental] Create a domain-specific type for the inputDependencySourceMap~~ [5.5, NFC, Incremental] Create a domain-specific type for the inputDependencySourceMap Jun 11, 2021

davidungar added the swift 5.5 label Jun 11, 2021

davidungar marked this pull request as draft June 11, 2021 06:45

davidungar marked this pull request as ready for review June 11, 2021 06:48

davidungar requested a review from artemcm June 11, 2021 06:51

artemcm approved these changes Jun 11, 2021

View reviewed changes

davidungar mentioned this pull request Jun 11, 2021

[5.5, Incremental] Harness immutability to make inputDependencySourceMap robust #712

Closed

CodaFi requested changes Jun 21, 2021

View reviewed changes

CodaFi reviewed Jun 21, 2021

View reviewed changes

documented AdditionPurpose cases

f875561

davidungar force-pushed the 5.5-catchup-1 branch from a0f4df5 to f875561 Compare June 21, 2021 20:23

davidungar closed this Jun 25, 2021

davidungar deleted the 5.5-catchup-1 branch July 1, 2021 20:47

[5.5, NFC, Incremental] Create a domain-specific type for the inputDependencySourceMap #710

[5.5, NFC, Incremental] Create a domain-specific type for the inputDependencySourceMap #710

Uh oh!

Conversation

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

Uh oh!

Cherry-pick of #685 for the release/5.5 branch.

Uh oh!

davidungar commented Jun 11, 2021

Uh oh!

davidungar commented Jun 11, 2021

Uh oh!

artemcm left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

davidungar Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

CodaFi Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

davidungar Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

CodaFi Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

davidungar Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

davidungar commented Jun 21, 2021

Uh oh!

davidungar commented Jun 21, 2021

Uh oh!

davidungar commented Jun 21, 2021

Uh oh!

davidungar commented Jun 21, 2021

Uh oh!

davidungar commented Jun 21, 2021

Uh oh!

davidungar commented Jun 21, 2021

Uh oh!

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

Uh oh!

Uh oh!

davidungar commented Jun 22, 2021

Uh oh!

Uh oh!

davidungar commented Jun 11, 2021 •

edited

Loading

Cherry-pick of #685 for the `release/5.5` branch.

CodaFi commented Jun 22, 2021 •

edited

Loading