Skip to content

GSB: Rewrite redundant requirements algorithm for the second time #37078

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.

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

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Apr 27, 2021

This rewrites the existing redundant requirements algorithm to be simpler, and fix an incorrect behavior in the case where
we're building a protocol requirement signature.

Consider the following example:

protocol P {
  associatedtype A : P
  associatedtype B : P where A.B == B
}

The requirement B : P has two conformance paths here:

(B : P)
(A : P)(B : P)

The naive redundancy algorithm would conclude that (B : P) is redundant because it can be derived as (A : P)(B : P). However, if we drop (B : P), we lose the derived conformance path as well, since it involves the same requirement (B : P).

The above example actually worked before this change, because we handled this case in getMinimalConformanceSource() by dropping any derived conformance paths that involve the requirement itself appearing in the "middle" of the path.

However, this is insufficient because you can have a "cycle" here with length more than 1. For example,

protocol P {
  associatedtype A : P where A == B.C
  associatedtype B : P where B == A.C
  associatedtype C : P where C == A.B
}

The requirement A : P has two conformance paths here:

(A : P)
(B : P)(C : P)

Similarly, B : P has these two paths:

(B : P)
(A : P)(C : P)

And C : P has these two paths:

(C : P)
(A : P)(B : P)

Since each one of A : P, B : P and C : P has a derived conformance path that does not involve itself, we would conclude that all three were redundant. But this was wrong; while (B : P)(C : P) is a valid derived path for A : P that allows us to drop A : P, once we commit to dropping A : P, we can no longer use the other derived paths (A : P)(C : P) for B : P, and (A : P)(B : P) for C : P, respectively, because they involve A : P, which we dropped.

The problem is that we were losing information here. The explicit requirement A : P can be derived as (B : P)(C : P), but we would just say that it was implied by B : P alone.

For non-protocol generic signatures, just looking at the root is still sufficient.

However, when building a requirement signature of a self-recursive protocol, instead of looking at the root explicit requirement only, we need to look at all intermediate steps in the path that involve the same protocol.

This is implemented in a new getBaseRequirements() method, which generalizes the operation of getting the explicit requirement at the root of a derived conformance path by returning a vector of one or more explicit requirements that appear in the path.

Also the new algorithm computes redundancy online instead of building a directed graph and then computing SCCs. This is possible by recording newly-discovered redundant requirements immediately, and then using the set of so-far-redundant requirements when evaluating a path.

Fixes https://bugs.swift.org/browse/SR-14510 / rdar://problem/76883924.

@slavapestov slavapestov force-pushed the new-redundant-requirements-algorithm branch from d846c59 to 5df65ca Compare April 29, 2021 04:28
Every valid signature should have at least one requirement that can
be removed while still producing a valid signature.
This rewrites the existing redundant requirements algorithm
to be simpler, and fix an incorrect behavior in the case where
we're building a protocol requirement signature.

Consider the following example:

    protocol P {
      associatedtype A : P
      associatedtype B : P where A.B == B
    }

The requirement B : P has two conformance paths here:

    (B : P)
    (A : P)(B : P)

The naive redundancy algorithm would conclude that (B : P) is
redundant because it can be derived as (A : P)(B : P). However,
if we drop (B : P), we lose the derived conformance path
as well, since it involves the same requirement (B : P).

The above example actually worked before this change, because we
handled this case in getMinimalConformanceSource() by dropping any
derived conformance paths that involve the requirement itself
appearing in the "middle" of the path.

However, this is insufficient because you can have a "cycle"
here with length more than 1. For example,

    protocol P {
      associatedtype A : P where A == B.C
      associatedtype B : P where B == A.C
      associatedtype C : P where C == A.B
    }

The requirement A : P has two conformance paths here:

   (A : P)
   (B : P)(C : P)

Similarly, B : P has these two paths:

   (B : P)
   (A : P)(C : P)

And C : P has these two paths:

   (C : P)
   (A : P)(B : P)

Since each one of A : P, B : P and C : P has a derived conformance
path that does not involve itself, we would conclude that all three
were redundant. But this was wrong; while (B : P)(C : P) is a valid
derived path for A : P that allows us to drop A : P, once we commit to
dropping A : P, we can no longer use the other derived paths
(A : P)(C : P) for B : P, and (A : P)(B : P) for C : P, respectively,
because they involve A : P, which we dropped.

The problem is that we were losing information here. The explicit
requirement A : P can be derived as (B : P)(C : P), but we would
just say that it was implied by B : P alone.

For non-protocol generic signatures, just looking at the root is
still sufficient.

However, when building a requirement signature of a self-recursive
protocol, instead of looking at the root explicit requirement only,
we need to look at _all_ intermediate steps in the path that involve
the same protocol.

This is implemented in a new getBaseRequirements() method, which
generalizes the operation of getting the explicit requirement at
the root of a derived conformance path by returning a vector of
one or more explicit requirements that appear in the path.

Also the new algorithm computes redundancy online instead of building
a directed graph and then computing SCCs. This is possible by
recording newly-discovered redundant requirements immediately,
and then using the set of so-far-redundant requirements when
evaluating a path.

This commit introduces a small regression in an existing test case
involving a protocol with a 'derived via concrete' requirement.
Subsequent commits in this PR fix the regression and remove the
'derived via concrete' mechanism, since it is no longer necessary.

Fixes https://bugs.swift.org/browse/SR-14510 / rdar://problem/76883924.
@slavapestov slavapestov force-pushed the new-redundant-requirements-algorithm branch from 5df65ca to a007645 Compare April 29, 2021 19:15
…nce requirements

This fixes a regression from the new redundant requirements algorithm
and paves the way for removing the notion of 'derived via concrete'
requirements.
…ivalence classes

Literally an off-by-one error -- we were skipping over the first
requirement and not copying it over. I think this is because the
updateLayout() call used to be addLayoutRequirementDirect(),
which would add the first layout constraint, and I forgot to
remove the '+ 1' when I refactored this.
@slavapestov slavapestov force-pushed the new-redundant-requirements-algorithm branch from a007645 to b799942 Compare April 29, 2021 20:17
@slavapestov slavapestov marked this pull request as ready for review April 29, 2021 20:19
@slavapestov slavapestov force-pushed the new-redundant-requirements-algorithm branch from b799942 to aedba4d Compare April 29, 2021 20:50
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov
Copy link
Contributor Author

@swift-ci Please ASAN test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - aedba4d

@slavapestov
Copy link
Contributor Author

@swift-ci Please test macOS

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test macOS

1 similar comment
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test macOS

@slavapestov
Copy link
Contributor Author

CoreStore and swift-futures failed in source compat testing, so I need to investigate those first.

@slavapestov
Copy link
Contributor Author

Let's just have one PR instead of two.

@slavapestov slavapestov closed this May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants