Skip to content

Simplest fix to a dependency bug. #32219

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 6, 2020
Merged

Simplest fix to a dependency bug. #32219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de7033c
Simplest fix to a dependency bug.
Jun 6, 2020
da14e31
Fix tests to correspond
Jun 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions lib/AST/AbstractSourceFileDepGraphFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,30 @@ void AbstractSourceFileDepGraphFactory::addAUsedDecl(
const DependencyKey &defKey, const DependencyKey &useKey) {
auto *defNode =
g.findExistingNodeOrCreateIfNew(defKey, None, false /* = !isProvides */);

// If the depended-upon node is defined in this file, then don't
// create an arc to the user, when the user is the whole file.
// Otherwise, if the defNode's type-body fingerprint changes,
// the whole file will be marked as dirty, losing the benefit of the
// fingerprint.
if (defNode->getIsProvides() &&
useKey.getKind() == NodeKind::sourceFileProvide)
return;

// if (defNode->getIsProvides() &&
// useKey.getKind() == NodeKind::sourceFileProvide)
// return;

// Turns out the above three lines cause miscompiles, so comment them out
// for now. We might want them back if we can change the inputs to this
// function to be more precise.

// Example of a miscompile:
// In main.swift
// func foo(_: Any) { print("Hello Any") }
// foo(123)
// Then add the following line to another file:
// func foo(_: Int) { print("Hello Int") }
// Although main.swift needs to get recompiled, the commented-out code below
// prevents that.

auto nullableUse = g.findExistingNode(useKey);
assert(nullableUse.isNonNull() && "Use must be an already-added provides");
auto *useNode = nullableUse.get();
Expand Down
7 changes: 1 addition & 6 deletions test/Frontend/Fingerprints/class-fingerprint.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,4 @@

// only-run-for-debugging: cp %t/usesB.swiftdeps %t/usesB4.swiftdeps

// RUN: %FileCheck -check-prefix=CHECK-MAINB-RECOMPILED %s < %t/output4

// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}
// CHECK-MAINB-RECOMPILED: Queuing because of dependencies discovered later: {compile: usesA.o <= usesA.swift}
// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}

// RUN: %FileCheck -check-prefix=CHECK-MAINAB-RECOMPILED %s < %t/output4
7 changes: 1 addition & 6 deletions test/Frontend/Fingerprints/enum-fingerprint.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,4 @@

// only-run-for-debugging: cp %t/usesB.swiftdeps %t/usesB4.swiftdeps

// RUN: %FileCheck -check-prefix=CHECK-MAINB-RECOMPILED %s < %t/output4

// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}
// CHECK-MAINB-RECOMPILED: Queuing because of dependencies discovered later: {compile: usesA.o <= usesA.swift}
// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}

// RUN: %FileCheck -check-prefix=CHECK-MAINAB-RECOMPILED %s < %t/output4
6 changes: 1 addition & 5 deletions test/Frontend/Fingerprints/protocol-fingerprint.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,5 @@

// only-run-for-debugging: cp %t/usesB.swiftdeps %t/usesB4.swiftdeps

// RUN: %FileCheck -check-prefix=CHECK-MAINB-RECOMPILED %s < %t/output4

// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}
// CHECK-MAINB-RECOMPILED: Queuing because of dependencies discovered later: {compile: usesA.o <= usesA.swift}
// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}
// RUN: %FileCheck -check-prefix=CHECK-MAINAB-RECOMPILED %s < %t/output4

7 changes: 1 addition & 6 deletions test/Frontend/Fingerprints/struct-fingerprint.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,4 @@

// only-run-for-debugging: cp %t/usesB.swiftdeps %t/usesB4.swiftdeps

// RUN: %FileCheck -check-prefix=CHECK-MAINB-RECOMPILED %s < %t/output4

// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}
// CHECK-MAINB-RECOMPILED: Queuing because of dependencies discovered later: {compile: usesA.o <= usesA.swift}
// CHECK-MAINB-RECOMPILED-NOT: Queuing because of dependencies discovered later: {compile: usesB.o <= usesB.swift}

// RUN: %FileCheck -check-prefix=CHECK-MAINAB-RECOMPILED %s < %t/output4
4 changes: 2 additions & 2 deletions unittests/Driver/TypeBodyFingerprintsDependencyGraphTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,10 +807,10 @@ TEST(ModuleDepGraph, UseFingerprints) {
{
const auto jobs =
simulateReload(graph, &job0, {{NodeKind::nominal, {"A1@11", "A2@2"}}});
EXPECT_EQ(2u, jobs.size());
EXPECT_EQ(3u, jobs.size());
EXPECT_TRUE(contains(jobs, &job0));
EXPECT_TRUE(contains(jobs, &job1));
EXPECT_FALSE(contains(jobs, &job2));
EXPECT_TRUE(contains(jobs, &job2));
EXPECT_FALSE(contains(jobs, &job3));
}
}
Expand Down