[Sema] Pare down operator candidates during protocol type checking #18831
Conversation
|
Don't forget class inheritance. I'm not sure why someone would implement |
|
Or to use an |
Yup, that still works AFAICT. This is invalid either way: class A {
static func == (_: A, _: A) -> Bool { return true }
}
class B: A, Equatable {}This is valid either way: class A: Equatable {
static func == (_: A, _: A) -> Bool { return true }
}
class B: A {}
I'm not sure what you mean by this. 🤔 Could you elaborate? |
|
Ah, I forgot that we still don't do covariant witness matching. I'm not sure it's a bug exactly but it is still something I could see changing. Here's the protocol value example: protocol BadlyStringRepresentable {
var rawValue: String { get }
}
extension BadlyStringRepresentable {
static func ==(left: BadlyStringRepresentable, right: BadlyStringRepresentable) -> Bool {
return left.rawValue == right.rawValue
}
}
enum Example: String, BadlyStringRepresentable, Equatable {
case a, b
}In theory, that |
|
Oh, you can do that with protocol BadlyStringRepresentable {
var rawValue: String { get }
}
extension BadlyStringRepresentable {
static func ==(left: Self, right: Self) -> Bool {
return left.rawValue == right.rawValue
}
}
class Example: BadlyStringRepresentable, Equatable {
var rawValue: String = ""
init() {}
}(I used a class to avoid a synthesized That still works too. |
|
Yes, I saw that you handled the |
|
Ohhhhh… right. 🤦🏻♂️ |
|
Does this need anything then? I'm assuming this logic would be updated anyway as part of the work to support covariant witness matching. |
|
I'm sorry, I should have tagged others in sooner. I'm not quite familiar enough with the code to approve this. (I can kick off the tests, though.) @swift-ci Please test |
|
@swift-ci Please test source compatibility |
|
I'm not very familiar with the compatibility suite, but it looks like those failures are pre-existing known failures. Is that right? |
|
The failure is an unexpected pass. We need to update the suite. |
lib/Sema/TypeCheckProtocol.cpp
Outdated
|
|
||
| // Is it the same nominal type? | ||
| auto nominal = paramType->getAnyNominal(); | ||
| if ((!nominal && paramType->getKind() != TypeKind::Tuple) || |
There was a problem hiding this comment.
I’m not sure I understand the check here. Other than nominals, what are you accepting here? The ‘Self’ generic parameter? Then you might want to make this more precise and check for a GenericTypeParamType. Also as a general stylistic comment, instead of checking getKind() it’s better to use ->is(), etc.
There was a problem hiding this comment.
No, I'm trying to check for generic parameters that might fulfill the requirement:
infix operator <~>
protocol P1 {
static func <~>(x: Self, y: Self)
}
protocol P2 {}
struct ConformsWithMoreGenericWitnesses : P1, P2 {
}
func <~> <P: P2>(x: P, y: P) {}It looks like is<GenericTypeParamType>() does what I want. I couldn't find the right way to express that.
lib/Sema/TypeCheckProtocol.cpp
Outdated
| witnesses.push_back(candidate.getValueDecl()); | ||
| auto decl = candidate.getValueDecl(); | ||
| if (!conformance || | ||
| isMemberOperator(dyn_cast<FuncDecl>(decl), conformance->getType())) { |
There was a problem hiding this comment.
dyn_cast returns nullptr if the cast fails. If you’re sure you have a function here, use cast<>; otherwise check the return value.
There was a problem hiding this comment.
Gotcha. I was confused by the presence of dyn_cast_or_null in autocompletion. 🤦🏻♂️
lib/Sema/TypeCheckProtocol.cpp
Outdated
| WitnessChecker::lookupValueWitnesses(ValueDecl *req, bool *ignoringNames) { | ||
| bool WitnessChecker::isMemberOperator(FuncDecl *decl, Type type) { | ||
| auto *DC = decl->getDeclContext(); | ||
| auto selfNominal = DC->getSelfNominalTypeDecl(); |
There was a problem hiding this comment.
Wouldn’t it be better to use the nominal type of the conformance itself to compare against? Adoptee->getAnyNominal()
There was a problem hiding this comment.
I stole this code from TypeCheckDecl.cpp:checkMemberOperator. 🤔
lib/Sema/TypeCheckProtocol.cpp
Outdated
| if (!paramType) | ||
| break; | ||
|
|
||
| // Look through a metatype reference, if there is one. |
There was a problem hiding this comment.
I don’t think you want to do that. An operator== where both parameters are Foo.Type cannot witness == on Foo for example.
There was a problem hiding this comment.
This also came from TypeCheckDecl.cpp:checkMemberOperator.
Without it, this test fails:
prefix operator ~~~
public protocol _CyclicAssociated {
associatedtype Assoc = CyclicImpl
}
public protocol CyclicAssociated : _CyclicAssociated {
static prefix func ~~~(_: Self.Type)
}
prefix public func ~~~ <T: _CyclicAssociated>(_: T.Type) {}
public struct CyclicImpl : CyclicAssociated {
public typealias Assoc = CyclicImpl
public init() {}
}I'm still trying to digest this one.
lib/Sema/TypeCheckProtocol.cpp
Outdated
| TC.conformsToProtocol(type, dyn_cast<ProtocolDecl>(selfNominal), DC, | ||
| ConformanceCheckFlags::InExpression); | ||
|
|
||
| for (auto param : *decl->getParameters()) { |
There was a problem hiding this comment.
You’re checking if at least one parameter is of the nominal type — but is that the right check?
There was a problem hiding this comment.
I think so. That seems to be the requirement for adding an operator to a protocol in the first place—it needs to have at least one Self argument. I was trying to replicate that requirement here.
lib/Sema/TypeCheckProtocol.cpp
Outdated
| // Check the parameters for a reference to 'Self'. | ||
| bool isProtocol = | ||
| selfNominal && isa<ProtocolDecl>(selfNominal) && | ||
| TC.conformsToProtocol(type, dyn_cast<ProtocolDecl>(selfNominal), DC, |
There was a problem hiding this comment.
You know it conforms to the protocol, because you found the operator via name lookup on the type
mdiep
force-pushed
the
pare-down-operator-candidates
branch
from
8ecb57a to
5188407
Compare
|
How does that look? I should have left a better description when I opened the PR. The intent here is to use the same requirements that are required to include an operator as part of a protocol: the operator must use
I believe this correctly cuts down on the number of possible matches, but it might not cut down as many as it could. |
|
@mdiep Would it make sense to extract the common checks into a shared function? That would have made the PR much clearer. |
|
Also it would be nice to have some new tests that explicitly demonstrate various cases where operators are included and excluded from the diagnostics emitted. |
Yeah, I think I could do that. They're looking a bit more similar after your feedback. How does this look? // Pass a null `type` when checking the protocol itself
bool isMemberOperator(FuncDecl *decl, Type type) {
if (decl->isInvalid())
return true;
auto *DC = decl->getDeclContext();
auto selfNominal = DC->getSelfNominalTypeDecl();
// Check the parameters for a reference to 'Self'.
bool isProtocol = selfNominal && isa<ProtocolDecl>(selfNominal);
for (auto param : *decl->getParameters()) {
auto paramType = param->getInterfaceType();
if (!paramType)
break;
// Look through a metatype reference, if there is one.
paramType = paramType->getMetatypeInstanceType();
auto nominal = paramType->getAnyNominal();
if (type) {
// Is it the same nominal type? Or a generic (which may or may not match)?
if (paramType->is<GenericTypeParamType>() ||
nominal == type->getAnyNominal())
return true;
} else {
// Is it the same nominal type?
if (nominal == selfNominal)
return true;
}
if (isProtocol) {
// For a protocol, is it the 'Self' type parameter?
if (auto genericParam = paramType->getAs<GenericTypeParamType>())
if (genericParam->isEqual(DC->getSelfInterfaceType()))
return true;
}
}
return false;
}Where's the right place to put this so it's visible from both files?
👍🏻 I think |
|
A top-level function in the swift namespace defined in TypeCheckDecl.cpp, or you could even move it to a method on FuncDecl. |
mdiep
force-pushed
the
pare-down-operator-candidates
branch
from
5188407 to
a07244c
Compare
|
Alright, I just gave it another shot. Let me know how that looks. |
lib/Sema/TypeCheckProtocol.cpp
Outdated
| WitnessChecker::lookupValueWitnesses(ValueDecl *req, bool *ignoringNames) { | ||
| SmallVector<ValueDecl *, 4> | ||
| WitnessChecker::lookupValueWitnesses(ValueDecl *req, bool *ignoringNames, | ||
| NormalProtocolConformance *conformance) { |
There was a problem hiding this comment.
Passing the conformance down feels like a hack. Is there no other way to get the type you need?
mdiep
force-pushed
the
pare-down-operator-candidates
branch
from
a07244c to
5e9da14
Compare
|
You're right, I didn't need to pass in the conformance. The type was available on the |
|
@swift-ci Please smoke test |
Thanks for reviewing! I'm always happy to iterate with a little direction. 🙂 Speaking of which… I'm not sure what's going on with the Linux smoke test failure and I don't have a linux box handy these days. Any idea what's going on there? Edit: Oh, I guess it's expected. |
|
@swift-ci Please smoke test Linux |
1 similar comment
|
@swift-ci Please smoke test Linux |
|
I think this resolved https://bugs.swift.org/browse/SR-4318 🎉 |
This cuts down the number of witnesses returned when looking up a protocol's member operator.
It does this by excluding operators that couldn't be valid—i.e. that only use nominal types that aren't the conforming type.
The end goal of this was to reduce the number of
candidate has non-matching typediagnostics when you're missing a==implementation. This is a first step towards providing an improved diagnostic when a==can't be synthesized.This code:
would return these diagnostics before:
Now it returns these diagnostics:
There's some more room for improvement here, but I'm not sure (1) how to properly check to exclude unbound generics that wouldn't be valid or (2) if this is desirable, since you might have forgotten to declare conformance to a protocol.