Skip to content

RequirementMachine: Another silly GenericSignatureBuilder compatibility hack for concrete contraction #41935

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
85 changes: 84 additions & 1 deletion lib/AST/RequirementMachine/ConcreteContraction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ class ConcreteContraction {
Type substType(Type type) const;
Requirement substRequirement(const Requirement &req) const;

bool preserveSameTypeRequirement(const Requirement &req) const;

bool hasResolvedMemberTypeOfInterestingParameter(Type t) const;

public:
ConcreteContraction(bool debug) : Debug(debug) {}

Expand Down Expand Up @@ -384,6 +388,73 @@ ConcreteContraction::substRequirement(const Requirement &req) const {
}
}

bool ConcreteContraction::
hasResolvedMemberTypeOfInterestingParameter(Type type) const {
return type.findIf([&](Type t) -> bool {
if (auto *memberTy = t->getAs<DependentMemberType>()) {
if (memberTy->getAssocType() == nullptr)
return false;

auto baseTy = memberTy->getBase();
if (auto *genericParam = baseTy->getAs<GenericTypeParamType>()) {
GenericParamKey key(genericParam);

Type concreteType;
{
auto found = ConcreteTypes.find(key);
if (found != ConcreteTypes.end() && found->second.size() == 1)
return true;
}

Type superclass;
{
auto found = Superclasses.find(key);
if (found != Superclasses.end() && found->second.size() == 1)
return true;
}
}
}

return false;
});
}

/// Another silly GenericSignatureBuilder compatibility hack.
///
/// Consider this code:
///
/// class C<T> {
/// typealias A = T
/// }
///
/// protocol P {
/// associatedtype A
/// }
///
/// func f<X, T>(_: X, _: T) where X : P, X : C<T>, X.A == T {}
///
/// The GenericSignatureBuilder would introduce an equivalence between
/// typealias A in class C and associatedtype A in protocol P, so the
/// requirement 'X.A == T' would effectively constrain _both_.
///
/// Simulate this by keeping both the original and substituted same-type
/// requirement in a narrow case.
bool ConcreteContraction::preserveSameTypeRequirement(
const Requirement &req) const {
if (req.getKind() != RequirementKind::SameType)
return false;

if (Superclasses.find(req.getFirstType()->getRootGenericParam())
== Superclasses.end())
return false;

if (hasResolvedMemberTypeOfInterestingParameter(req.getFirstType()) ||
hasResolvedMemberTypeOfInterestingParameter(req.getSecondType()))
return false;

return true;
}

/// Substitute all occurrences of generic parameters subject to superclass
/// or concrete type requirements with their corresponding superclass or
/// concrete type.
Expand Down Expand Up @@ -506,6 +577,18 @@ bool ConcreteContraction::performConcreteContraction(
llvm::dbgs() << "\n";
}

if (preserveSameTypeRequirement(req.req)) {
if (Debug) {
llvm::dbgs() << "@ Preserving original requirement: ";
req.req.dump(llvm::dbgs());
llvm::dbgs() << "\n";
}

// Make the duplicated requirement 'inferred' so that we don't diagnose
// it as redundant.
result.push_back({req.req, SourceLoc(), /*inferred=*/true});
}

// Substitute the requirement.
Optional<Requirement> substReq = substRequirement(req.req);

Expand All @@ -519,7 +602,7 @@ bool ConcreteContraction::performConcreteContraction(
llvm::dbgs() << "\n";
}

return false;
continue;
}

if (Debug) {
Expand Down
48 changes: 48 additions & 0 deletions test/Generics/concrete_contraction_unrelated_typealias.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// R/UN: %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-inferred-signatures=on 2>&1 | %FileCheck %s
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this RUN line disabled deliberately?

// RUN: %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-inferred-signatures=on -disable-requirement-machine-concrete-contraction 2>&1 | %FileCheck %s

// Another GenericSignatureBuilder oddity, reduced from RxSwift.
//
// The requirements 'Proxy.Parent == P' and 'Proxy.Delegate == D' in the
// init() below refer to both the typealias and the associated type,
// despite the class being unrelated to the protocol; it just happens to
// define typealiases with the same name.
//
// In the Requirement Machine, the concrete contraction pre-processing
// pass would eagerly substitute the concrete type into these two
// requirements, producing the useless requirements 'P == P' and 'D == D'.
//
// Make sure concrete contraction keeps these requirements as-is by
// checking the generic signature with and without concrete contraction.

class GenericDelegateProxy<P : AnyObject, D> {
typealias Parent = P
typealias Delegate = D

// CHECK-LABEL: .GenericDelegateProxy.init(_:)@
// CHECK-NEXT: <P, D, Proxy where P == Proxy.[DelegateProxyType]Parent, D == Proxy.[DelegateProxyType]Delegate, Proxy : GenericDelegateProxy<P, D>, Proxy : DelegateProxyType>
init<Proxy: DelegateProxyType>(_: Proxy.Type)
where Proxy: GenericDelegateProxy<P, D>,
Proxy.Parent == P,
Proxy.Delegate == D {}
}
Comment on lines +24 to +28
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Mar 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to wrap my head around this. If the class is unrelated to the protocol, and the Proxy subclass could have its own distinct witnesses for these associated types, why would it be correct to substitute GenericDelegateProxy<P, D> into Proxy.Parent == P and discard the requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the test in #42005


class SomeClass {}
struct SomeStruct {}

class ConcreteDelegateProxy {
typealias Parent = SomeClass
typealias Delegate = SomeStruct

// CHECK-LABEL: .ConcreteDelegateProxy.init(_:_:_:)@
// CHECK-NEXT: <P, D, Proxy where P == Proxy.[DelegateProxyType]Parent, D == Proxy.[DelegateProxyType]Delegate, Proxy : ConcreteDelegateProxy, Proxy : DelegateProxyType>
init<P, D, Proxy: DelegateProxyType>(_: P, _: D, _: Proxy.Type)
where Proxy: ConcreteDelegateProxy,
Proxy.Parent == P,
Proxy.Delegate == D {}
}

protocol DelegateProxyType {
associatedtype Parent : AnyObject
associatedtype Delegate
}