Skip to content

IRGen: Fix subtle issue with "circular" conformance access paths #39588

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

Merged

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Oct 5, 2021

There are three things going on in rdar://83687967:

  • An accidental ABI break between Swift 5.4 and 5.5
  • A bug in getConformanceAccessPath()
  • A bug in IRGen's conformance access path evaluation

The ABI break

This part is actually not the root cause of the problem, but it is the reason why the test case used to work in Swift 5.4. Since we already shipped Swift 5.5, I think changing the ABI back to match the old behavior in this particular case would risk further regressions. The new ABI seems to make more sense, and I'm not changing it in this PR. But it could be changed back if we need to.

Here is the code:

public protocol P1 {}

public protocol P2 {
  associatedtype A: P1
}

public protocol P3 {
  associatedtype B: P2
  associatedtype A where B.A == A
}

public struct G<A: P1>: P2 {}

public func callee<T: P1>(_: T.Type) {}

public func caller11<Child: P3>(_: Child)
    where Child.B == G<Child.A> {
  callee(Child.A.self)
}

Requirement inference on the concrete type G<Child.A> in the where clause introduces the explicit conformance requirement Child.A: P1 from the requirement A: P1 in the signature of G.

In 5.4 and 5.5, we discover the alternate conformance access path (Child: P3)(Self.B: P2)(Self.A: P1) for Child.A. Note that the canonical type of Child.B.A is Child.A.

In 5.4, the "derived via concrete" check evaluates to true for this path, because the prefix Child.B is fixed to a concrete type. This excludes this conformance path from consideration.

In 5.5, the code was rewritten in an attempt to preserve existing behavior while fixing additional cases involving concrete types that the "derived via concrete" check didn't handle. This is where the "signature rebuilding after dropping conformances" stage of the GSB came from.

Clearly, I failed at preserving existing behavior in all cases, because in this case, it turns out that the "derived via concrete" check was too conservative. It was supposed to only consider a path "derived via concrete" if the root conformance was also redundant. This would happen if Child : P3 was made redundant by a requirement like Child == SomeConcreteType<...>. In this example though, (Child: P3) is not redundant, so the original conformance access path is actually valid for Child.A: P1. This means that the inferred requirement Child.A : P1 is actually redundant, according to the actual semantics of conformance access paths.

Indeed, 5.4 had a further inconsistency which highlights why the old behavior was wrong. In protocols, we don't perform requirement inference, since the idea is that the user should explicitly state all conformance requirements on associated types in a protocol, for readability.

You could write these two protocols in 5.4:

public protocol X1 {
  associatedtype Child: P3
    where Child.B == G<Child.A>
}

public protocol X2 {
  associatedtype Child: P3
    where Child.B == G<Child.A>, Child.A : P1
}

In the first example, there's no explicit Child.A : P1 requirement. In the second, there was. Reformulating the original test case into the protocol requirement signature equivalent would still trigger the same IRGen crash with protocol X1, but not X2 in Swift 5.4:

public func caller21<T : X1>(_: T) {
  callee(T.Child.A.self)
}

The conformance access path bug

There was a new implementation of conformance access paths in 5.5. It would skip recursing into the children of a conformance access path if its subject type was fixed to a concrete type. This is actually wrong because of exactly this case that we never tested: the witness table for Child.A: P1 is recovered from Child.B: P2, and Child.B is a concrete type.

Concrete types were skipped here because the use of getCanonicalTypeInContext() would replace the type with its concrete type, but really what we want is the "canonical anchor", which is the most canonical type parameter of a type parameter (even if it happens to be concrete). After making that change a couple of redundant term/type round-trips become apparent, so simplifying the algorithm by using Term throughout fixes this problem.

The IRGen bug

The final problem is revealed when IRGen attempts to recover the witness table from the conformance path (Child: P3)(Self.B: P2)(Self.A: P1). The associated conformance access function for the final step (Self.A : P1) takes the metadata for Child.B together with the witness table for Child.B : P2 as input. Since Child.B is concrete, IRGen would prefer to construct it directly, as the known concrete type G<Child.A>. The metadata for G is instantiated with the type metadata Child.B and the conformance Child.A: P1. Unfortunately, recovering Child.A: P1 from the generic environment of caller11() requires recursively evaluating the same conformance access path that is being evaluated now.

However, IRGen can also recover Child.B from the conformance Child: P3, which is immediately available, and the metadata for Child, by calling the associated type metadata access function. This function also constructs G<Child.A>, however it does it from the concrete nominal type metadata for the Child: P3 conformance, which must store Child.A: P1. By using abstract metadata access here, we avoid the circularity.

This last part is a bit of a hack because a more principled implementation would amend the conformance access path representation to encode access paths for each metadata component as well. I'll clean this up later.

Next steps

If we do need to change back to the 5.4 ABI, we can bring back the "derived via concrete" check in the GSB, and try to simulate it in the requirement machine's in-progress minimization algorithm.

The added test case covers several variations of this pattern to ensure the minimized set of conformance requirements does not change again.

The "generating conformances" algorithm that I'm working on to replace the GSB's signature minimization builds a set of simultaneous equations relating conformance access paths algebraically, and uses this to find a minimal set. These equations also encode the information describing how to recover the type metadata for the subject type of a conformance requirement. By serializing this information in type metadata, we will in the future be able to recover witness tables using conformance access paths encoded in the mangling format, avoiding swift_conformsToProtocol() calls at runtime on newer deployment targets. This will build on @al45tair's work in #39246.

Fixes rdar://83687967.

…ceAccessPath()

We would skip recording a conformance access path if the subject
type canonicalized to a concrete type, but this was incorrect.

The correct formulation is to use the _canonical anchor_ and not
the canonical type as the caching key; that is, we always want it
to be a type parameter, even if it is fixed to a concrete type,
because type parameters fixed to concrete types can appear in
the middle of conformance access paths, as the example in the
radar demonstrates.

Fixes rdar://problem/83687967.
This fixes the IRGen side of rdar://problem/83687967.
@slavapestov slavapestov marked this pull request as ready for review October 5, 2021 22:35
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov slavapestov requested a review from jckarter October 5, 2021 22:39
@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility Debug

@slavapestov slavapestov merged commit 92520d5 into swiftlang:main Oct 6, 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.

1 participant