[Typechecker] Check for generic signature when checking for overrides

[theblixguy]

We currently do not check for generic signatures when performing override checking. This PR adds a check for generic signature to OverrideMatcher::checkOverride().

Resolves SR-4206, SR-4986, SR-7573, SR-10076, SR-10198 & SR-10603
Resolves rdar://problem/23626260
Resolves rdar://problem/32378463
Resolves rdar://problem/39868032
Resolves rdar://problem/49339618

[theblixguy]

cc @jrose-apple @slavapestov

[slavapestov]

Nice!

[theblixguy]

Hmm building the stdlib is producing the new diagnostics. Here's an reduced example of code that triggers it:

protocol CodingKey {}

protocol Container {
  associatedtype Key: CodingKey
}

class Base<Key: CodingKey> {
  func foo(forKey key: Key) throws {}
}

class Derived<Concrete: Container> : Base<Concrete.Key> {
  typealias Key = Concrete.Key
  override func foo(forKey key: Key) throws {}
}

Concrete.Key conforms to CodingKey so it should satisfy the requirement Key: CodingKey on Base.

[slavapestov]

Oh, I see. I guess it's not sufficient to ask if the base signature has any requirements not satisfied by the derived signature. You need to apply the superclass type context substitutions (derivedClass->getSuperclass()->getContextSubstitutionMap(M, baseClass)) to each base requirement before checking it against the derived signature.

Adding a SubstitutionMap argument to requirementsNotSatisfiedBy() is your best bet. Do you also mind adding a reduced test case for the stdlib failure to the test suite, so that in the future we can catch a regression here without rebuilding the stdlib?

[theblixguy]

Thanks @slavapestov, that seems to work! I have added the above example to the test file.

[jrose-apple]

Will this break any working code? (That's a little different from "code that compiles today".)

@swift-ci Please test source compatibility

[jrose-apple]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 4e57747dc4a1eeb6557d4b2b056e726f018e1bf8

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 4e57747dc4a1eeb6557d4b2b056e726f018e1bf8

[slavapestov]

Thank you. One more optional suggestion but you can address it in a follow up PR if you don't want to do it now.

[slavapestov]

@theblixguy It looks like you're checking that the base signature does not have any requirements not satisfied by the derived method signature. That is, we don't allow the derived method to have a more general generic signature.

We actually want to ban the other direction too, for example this crashes:

class A {
  func f<T : P>(_: T) {}
}

class B : A {
  override func f<T : P & Q>(_ t: T) {
    t.doit()
  }
}

protocol P {}
protocol Q {
  func doit()
}

let b: A = B()

struct R : P, Q {
  func doit() {
    print("hi")
  }
}

b.f(R())

I think we want the generic requirements to match exactly -- if the base is more general than derived, you can't call a base method on a derived value in a sound way. On the other hand if the base is less general than derived, then super.foo() calls can't be made from the derived method.

@jrose-apple What do you think? And yeah, it's plausible that this makes us now reject formerly-working code, but that's really iffy. We rely on the ABI of a derived method matching a base method exactly (if not, we emit a vtable thunk, but the ABI check does not look at generic requirements either!)

It's possible that things line up somehow even when the requirements don't match (eg, superclass constraints and @objc protocol constraints don't introduce witness table parameters) but... ugh.

[jrose-apple]

Liskov substitutability says we need Derived to be at least as general as Base. The super.foo() thing is interesting because we allow other types of covariance and restrict the super call when you try to make it, but we don't have a way to refine generic parameters that way—and as Slava points out, we'd have to go further to make sure the vtable builder added a new slot for the new signature. Let's just stick to exact matches for now.

[jrose-apple]

I suppose we should run this by the core team as an acceptable source-breaking change (I agree that it's worth it), and possibly add it to CHANGELOG as well.

[slavapestov]

@jrose-apple Right, I didn't realize that the super.foo() thing would come up if you, eg, made a parameter more optional as well, which we do allow. So I guess the right answer is for the vtable check to add a new entry if the base requirements are not satisfied by the derived signature. Only derived requirements not being satisfied by the base signature has to be rejected for soundness reasons.

@theblixguy This is totally optional, but if you wanted to try implementing that, take a look at requiresNewVTableEntry() in lib/AST/Decl.cpp. We strip off the generic signatures before calling TypeBase::matches() on the base and derived type, so you'll need to check for them separately before or after doing that. Then to test it, you'll need to write some SILGen tests (and execution tests too preferably). But like I said, it's totally reasonable to land this fix first (but note that you'll need to change the polarity of your 'requirements satisfied' check, since the case you want to ban is when the derived signature has requirements not satisfied by the base, not the other way around).

[theblixguy]

Sounds good to me, I have changed it so we now ensure there's an exact match.

[slavapestov]

@theblixguy My apologies for the confusion but "exact match" means you still have to areRequirementsSatisfiedBy() with the substitution map, but you're checking both signatures.

[slavapestov]

(Try running the tests with your current version, they should hopefully fail)

[slavapestov]

Sorry for neglecting this. I will take a look as this in more detail as soon as I can.

In the mean time, here's a brain dump. I think you're right that building a new signature is your best bet.

So consider you have a class hierarchy like

class Base<T : IteratorProtocol, U> {
  func foo<V : IteratorProtocol, V.Element == T.Element>(_: U) {}
}

class Derived<T : Collection> : Base<T.Iterator, Int> {
  // note: T.Element should work as well
  override func foo<V : IteratorProtocol, U`V.Element == T.Iterator.Element>(_: U) {}
}

The superclass substitutions are:

T_0_0 := T_0_0.Element
T_0_1 := Int
T_0_0 : Iterator := conformance of T_0_0.Iterator to IteratorProtocol, derived from conformance of T_0_0 to Collection

Instead of applying substitutions to each requirement of the base signature before testing if the derived signature satisfies it, which only works in one direction, we build a whole new signature from the base signature.

A generic signature has parameters and requirements. So you have a generic signature for the base method. Zero or more generic parameters at the start are from the base generic type; you drop them. In their place, you add the generic parameters from the derived generic type. Then, you take any remaining generic arguments from the base method signature, and you "shift" them by adjusting their depth as necessary. To see why you have to do this, consider these cases:

class Generic<T> { func foo<U>(_: ...) {} } // depth is 1
class Concrete : Generic<T> { override func foo<U>(_: ...) {} } // depth is 0

class Outer<T> { class Inner<U> : Generic<(T, U)> { ... } }

The requirements then get mapped by applying the substitution map to each one. However, note that the substitution map only maps the base type's generic parameters. You'll in fact need to build a new substitution map that handles all of the base method's generic parameters -- mapping the type's parameters to the corresponding replacements from the superclass substitutions, and mapping the method's parameters by 'shifting' the depth as needed while preserving the index. I think getOverrideSubstitutions() implements this already, so perhaps just grabbing the override substitutions (they might be computed in nearby code already) and applying them to the requirements will do the trick. Also I would not expect this substitution to fail, except on invalid code. It's fine to say the override doesn't match in this case.

So this is a bit more involved than I imagined. But now you will have a generic signature that combines the derived type's parameters with the base method's parameters and requirements.

It should then be possible to pass this generic signature together with the derived method signature to requirementsNotSatisfiedBy(), and critically, test both directions of the relation.

[theblixguy]

Thank you @slavapestov. Do you mean something like this?

static bool areRequirementsSatisfied(ValueDecl *base, ValueDecl *derived) {

  auto baseGenericCtx = base->getAsGenericContext();
  auto derivedGenericCtx = derived->getAsGenericContext();

  if (baseGenericCtx && derivedGenericCtx) {
    auto baseGenericSig = baseGenericCtx->getGenericSignature();
    auto derivedGenericSig = derivedGenericCtx->getGenericSignature();

    auto baseClass = baseGenericCtx->getInnermostTypeContext();
    auto derivedClass = derivedGenericCtx->getInnermostTypeContext();

    if (baseClass && derivedClass) {
      if (isa<ClassDecl>(baseClass) && isa<ClassDecl>(derivedClass)) {
        auto derivedSuperclass =
            derivedClass->getSelfClassDecl()->getSuperclass();
        auto derivedSubMap = derivedSuperclass
                                 ? derivedSuperclass->getContextSubstitutionMap(
                                       base->getModuleContext(), baseClass)
                                 : SubstitutionMap();

        auto subMap = SubstitutionMap::getOverrideSubstitutions(base, derived,
                                                                derivedSubMap);
        auto baseReqsSatisfied = baseGenericSig->requirementsNotSatisfiedBy(derivedGenericSig, subMap).empty();
        auto derivedReqsSatisfied = derivedGenericSig->requirementsNotSatisfiedBy(baseGenericSig, subMap).empty();
        return baseReqsSatisfied && derivedReqsSatisfied;
      }
    }
  }

  return true;
}

[slavapestov]

@theblixguy I don't think that's right because you're using the same substitution map with baseGenericSig and derivedGenericSig, which is an error.

Take a look at how configureGenericDesignatedInitOverride() builds the generic signature for the inherited designated initializer. I think this is what you want to do as well.

[theblixguy]

@slavapestov In your previous comment, you mentioned

I think getOverrideSubstitutions() implements this already, so perhaps just grabbing the override substitutions (they might be computed in nearby code already) and applying them to the requirements will do the trick.

which is what I was doing above - except I am also using the same subMap in both directions. Should I just use the subMap when calling baseGenericSig->requirementsNotSatisfiedBy but not the other way round?

Also, it seems like getOverrideSubstitutions is already doing most of what configureGenericDesignatedInitOverride does, so maybe I don't need to duplicate the code?

Sorry, I am just trying to gain some clarity about how exactly we are supposed to check the requirements (because it seems like there's many ways to do it according to previous comments).

[slavapestov]

@theblixguy We need to build a new generic signature using the same logic as configureGenericDesignatedInitOverride(). The signature has to combine the requirements of the derived method's generic type with the base method's generic requirements. Then we check if this signature has any requirements not satisfied by the derived method's signature, and vice versa.

We can avoid code duplication by factoring out the code from configureGenericDesignatedInitOverride() into a new method.

[theblixguy]

Hmm thank you @slavapestov :) I am still a bit confused because the logic in configureGenericDesignatedInitOverride method is based on things like the superclassCtor and superclassTy, so I have been trying to adjust that to accept the baseMethod and baseMethodTy instead. Could you take a look at commit bd2cfad?

[slavapestov]

configureGenericDesignatedInitOverride() is entirely analogous except that superclassCtor is the base class constructor instead of the overridden method, and superclassTy is the base class type whereas baseMethodTy is the (entire) type of the base method.

[slavapestov]

@swift-ci Please test

[slavapestov]

@swift-ci Please test source compatibility

[slavapestov]

@swift-ci Please test compiler performance

[slavapestov]

You need to add the derived method's generic params as before -- the problem is with the calculation of depth. Right now both depth and superclassDepth are computed from the base class's signature. However I think depth should be computed from the derived class's signature. For clarity you can rename depth to derivedDepth and superclassDepth to baseDepth.

[theblixguy]

Right now both depth and superclassDepth are computed from the base class's signature

Hmm that doesn't appear to be the case as depth is calculated from derivedClass->getGenericSignature() and superclassDepth from baseClassSig (which was set to baseClass->getGenericSignature() earlier) 🤔

Nevermind I just realised I changed it in the previous commit so it's no longer the case! baseDepth and derivedDepth are now calculated from base and derived class's generic signature respectively, as expected.

[theblixguy]

@slavapestov Seems like the problem is fixed because I no longer get the error diagnostic on the failing case I showed you before, so I think the compatibility tests should pass now. Let's run the CI to verify 🙏

[xedin]

@swift-ci please test source compatibility

[xedin]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 279d2b414b14a743ee452b1b1d7439564d592141

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 279d2b414b14a743ee452b1b1d7439564d592141

[theblixguy]

@xedin @slavapestov could you rerun source compat tests again? The failure is unrelated (two packages failed to build due due to 'url(with:)' is only available in macOS 10.11 or newer and 'NSTextView' is incompatible with 'weak' references). Seems like the tests are broken since yesterday: https://forums.swift.org/t/swift-ci-build-failure-2-swift-source-compatibility-suite-master-3865/26919 (same failure here)

[xedin]

Looks like it just has to be XFailed first, I am going to do it later today.

[slavapestov]

@swift-ci Please test compiler performance

[slavapestov]

@theblixguy My only remaining concern is that we're going to create more GenericSignatureBuilders than before. They're pretty heavy weight. If there's a performance regression here, we'll need to add caching in getOverrideGenericSignature(). Either way I'm happy merging this before we fix that, but there's no way to run performance tests on an already-merged PR.

[theblixguy]

Yep, there’s a small regression in NumGenericSignatureBuilders (release) and SuperclassTypeRequest (debug): https://ci.swift.org/job/swift-PR-compiler-performance-macOS/430/artifact/comment.md

lets get this merged first and I’ll put up a new PR to add caching :)

[slavapestov]

@theblixguy Yes, please take a look at the caching.

I think this fix is worth a changelog entry too.

[theblixguy]

@slavapestov Okay, I'll put up a new PR shortly :)

Is it worth cherry picking this to 5.1 as well? Or is it too risky?