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

[Sema] Pare down operator candidates during protocol type checking #18831

Merged
merged 1 commit into from Aug 29, 2018

Conversation

Projects
None yet
4 participants
@mdiep
Copy link
Contributor

mdiep commented Aug 19, 2018

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 type diagnostics when you're missing a == implementation. This is a first step towards providing an improved diagnostic when a == can't be synthesized.

This code:

struct Nope {}

struct B: Equatable {
	let a: Nope
	
	static func == (_: B, _: Int) -> Bool { return false }
}

would return these diagnostics before:

equatable.swift:3:8: error: type 'B' does not conform to protocol 'Equatable'
struct B: Equatable {
       ^
equatable.swift:6:14: note: candidate has non-matching type '(B, Int) -> Bool'
        static func == (_: B, _: Int) -> Bool { return false }
                    ^
Swift.==:1:13: note: candidate has non-matching type '(Any.Type?, Any.Type?) -> Bool'
public func == (t0: Any.Type?, t1: Any.Type?) -> Bool
            ^
Swift.==:1:13: note: candidate has non-matching type '<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool'
public func == <T>(lhs: T, rhs: T) -> Bool where T : RawRepresentable, T.RawValue : Equatable
            ^
Swift.==:1:13: note: candidate has non-matching type '((), ()) -> Bool'
public func == (lhs: (), rhs: ()) -> Bool
            ^
Swift.==:1:13: note: candidate has non-matching type '<A, B where A : Equatable, B : Equatable> ((A, B), (A, B)) -> Bool'
public func == <A, B>(lhs: (A, B), rhs: (A, B)) -> Bool where A : Equatable, B : Equatable
            ^
Swift.==:1:13: note: candidate has non-matching type '<A, B, C where A : Equatable, B : Equatable, C : Equatable> ((A, B, C), (A, B, C)) -> Bool'
public func == <A, B, C>(lhs: (A, B, C), rhs: (A, B, C)) -> Bool where A : Equatable, B : Equatable, C : Equatable
            ^
Swift.==:1:13: note: candidate has non-matching type '<A, B, C, D where A : Equatable, B : Equatable, C : Equatable, D : Equatable> ((A, B, C, D), (A, B, C, D)) -> Bool'
public func == <A, B, C, D>(lhs: (A, B, C, D), rhs: (A, B, C, D)) -> Bool where A : Equatable, B : Equatable, C : Equatable, D : Equatable
            ^
Swift.==:1:13: note: candidate has non-matching type '<A, B, C, D, E where A : Equatable, B : Equatable, C : Equatable, D : Equatable, E : Equatable> ((A, B, C, D, E), (A, B, C, D, E)) -> Bool'
public func == <A, B, C, D, E>(lhs: (A, B, C, D, E), rhs: (A, B, C, D, E)) -> Bool where A : Equatable, B : Equatable, C : Equatable, D : Equatable, E : Equatable
            ^
Swift.==:1:13: note: candidate has non-matching type '<A, B, C, D, E, F where A : Equatable, B : Equatable, C : Equatable, D : Equatable, E : Equatable, F : Equatable> ((A, B, C, D, E, F), (A, B, C, D, E, F)) -> Bool'
public func == <A, B, C, D, E, F>(lhs: (A, B, C, D, E, F), rhs: (A, B, C, D, E, F)) -> Bool where A : Equatable, B : Equatable, C : Equatable, D : Equatable, E : Equatable, F : Equatable
            ^
Swift.ContiguousArray<Element>:2:24: note: candidate has non-matching type '<Element> (ContiguousArray<ContiguousArray<Element>.Element>, ContiguousArray<ContiguousArray<Element>.Element>) -> Bool' (aka '<τ_0_0> (ContiguousArray<τ_0_0>, ContiguousArray<τ_0_0>) -> Bool')
    public static func == (lhs: ContiguousArray<ContiguousArray<Element>.Element>, rhs: ContiguousArray<ContiguousArray<Element>.Element>) -> Bool
                       ^
Swift.ArraySlice<Element>:2:24: note: candidate has non-matching type '<Element> (ArraySlice<ArraySlice<Element>.Element>, ArraySlice<ArraySlice<Element>.Element>) -> Bool' (aka '<τ_0_0> (ArraySlice<τ_0_0>, ArraySlice<τ_0_0>) -> Bool')
    public static func == (lhs: ArraySlice<ArraySlice<Element>.Element>, rhs: ArraySlice<ArraySlice<Element>.Element>) -> Bool
                       ^
Swift.Array<Element>:2:24: note: candidate has non-matching type '<Element> (Array<Array<Element>.Element>, Array<Array<Element>.Element>) -> Bool' (aka '<τ_0_0> (Array<τ_0_0>, Array<τ_0_0>) -> Bool')
    public static func == (lhs: [[Element].Element], rhs: [[Element].Element]) -> Bool
                       ^
Swift.Bool:3:24: note: candidate has non-matching type '(Bool, Bool) -> Bool'
    public static func == (lhs: Bool, rhs: Bool) -> Bool
                       ^
Swift.AutoreleasingUnsafeMutablePointer<Pointee>:2:24: note: candidate has non-matching type '<Pointee> (AutoreleasingUnsafeMutablePointer<Pointee>, AutoreleasingUnsafeMutablePointer<Pointee>) -> Bool'
    public static func == (lhs: AutoreleasingUnsafeMutablePointer<Pointee>, rhs: AutoreleasingUnsafeMutablePointer<Pointee>) -> Bool
                       ^
Swift.Character:2:24: note: candidate has non-matching type '(Character, Character) -> Bool'
    public static func == (lhs: Character, rhs: Character) -> Bool
                       ^
Swift.Character.UnicodeScalarView.Index:2:24: note: candidate has non-matching type '(Character.UnicodeScalarView.Index, Character.UnicodeScalarView.Index) -> Bool'
    public static func == (lhs: Character.UnicodeScalarView.Index, rhs: Character.UnicodeScalarView.Index) -> Bool
                       ^
Swift.CodingUserInfoKey:5:24: note: candidate has non-matching type '(CodingUserInfoKey, CodingUserInfoKey) -> Bool'
    public static func == (lhs: CodingUserInfoKey, rhs: CodingUserInfoKey) -> Bool
                       ^
Swift.ClosedRange<Bound>.Index:2:24: note: candidate has non-matching type '<Bound> (ClosedRange<Bound>.Index, ClosedRange<Bound>.Index) -> Bool'
    public static func == (lhs: ClosedRange<Bound>.Index, rhs: ClosedRange<Bound>.Index) -> Bool
                       ^
Swift.ClosedRange<Bound>:2:24: note: candidate has non-matching type '<Bound> (ClosedRange<Bound>, ClosedRange<Bound>) -> Bool'
    public static func == (lhs: ClosedRange<Bound>, rhs: ClosedRange<Bound>) -> Bool
                       ^
Swift.OpaquePointer:2:24: note: candidate has non-matching type '(OpaquePointer, OpaquePointer) -> Bool'
    public static func == (lhs: OpaquePointer, rhs: OpaquePointer) -> Bool
                       ^
Swift.Dictionary<Key, Value>:17:28: note: candidate has non-matching type '<Key, Value> (Dictionary<Key, Value>.Keys, Dictionary<Key, Value>.Keys) -> Bool'
        public static func == (lhs: Dictionary<Key, Value>.Keys, rhs: Dictionary<Key, Value>.Keys) -> Bool
                           ^
Swift.Dictionary<Key, Value>:2:24: note: candidate has non-matching type '<Key, Value> ([Key : Value], [Key : Value]) -> Bool'
    public static func == (lhs: [Key : Value], rhs: [Key : Value]) -> Bool
                       ^
Swift.Dictionary<Key, Value>.Index:2:24: note: candidate has non-matching type '<Key, Value> (Dictionary<Key, Value>.Index, Dictionary<Key, Value>.Index) -> Bool'
    public static func == (lhs: Dictionary<Key, Value>.Index, rhs: Dictionary<Key, Value>.Index) -> Bool
                       ^
Swift.LazyDropWhileCollection<Base>.Index:2:24: note: candidate has non-matching type '<Base> (LazyDropWhileCollection<Base>.Index, LazyDropWhileCollection<Base>.Index) -> Bool'
    public static func == (lhs: LazyDropWhileCollection<Base>.Index, rhs: LazyDropWhileCollection<Base>.Index) -> Bool
                       ^
Swift.EmptyCollection<Element>:2:24: note: candidate has non-matching type '<Element> (EmptyCollection<Element>, EmptyCollection<Element>) -> Bool'
    public static func == (lhs: EmptyCollection<Element>, rhs: EmptyCollection<Element>) -> Bool
                       ^
Swift.FlattenCollection<Base>.Index:2:24: note: candidate has non-matching type '<Base> (FlattenCollection<Base>.Index, FlattenCollection<Base>.Index) -> Bool'
    public static func == (lhs: FlattenCollection<Base>.Index, rhs: FlattenCollection<Base>.Index) -> Bool
                       ^
Swift.FloatingPointSign:6:24: note: candidate has non-matching type '(FloatingPointSign, FloatingPointSign) -> Bool'
    public static func == (a: FloatingPointSign, b: FloatingPointSign) -> Bool
                       ^
Swift.FloatingPoint:2:24: note: candidate has non-matching type '<Self> (Self, Self) -> Bool'
    public static func == (lhs: Self, rhs: Self) -> Bool
                       ^
Swift.AnyHashable:2:24: note: candidate has non-matching type '(AnyHashable, AnyHashable) -> Bool'
    public static func == (lhs: AnyHashable, rhs: AnyHashable) -> Bool
                       ^
Swift.BinaryInteger:2:24: note: candidate has non-matching type '<Self, Other> (Self, Other) -> Bool'
    public static func == <Other>(lhs: Self, rhs: Other) -> Bool where Other : BinaryInteger
                       ^
Swift.UInt8:11:24: note: candidate has non-matching type '(UInt8, UInt8) -> Bool'
    public static func == (lhs: UInt8, rhs: UInt8) -> Bool
                       ^
Swift.Int8:11:24: note: candidate has non-matching type '(Int8, Int8) -> Bool'
    public static func == (lhs: Int8, rhs: Int8) -> Bool
                       ^
Swift.UInt16:11:24: note: candidate has non-matching type '(UInt16, UInt16) -> Bool'
    public static func == (lhs: UInt16, rhs: UInt16) -> Bool
                       ^
Swift.Int16:11:24: note: candidate has non-matching type '(Int16, Int16) -> Bool'
    public static func == (lhs: Int16, rhs: Int16) -> Bool
                       ^
Swift.UInt32:11:24: note: candidate has non-matching type '(UInt32, UInt32) -> Bool'
    public static func == (lhs: UInt32, rhs: UInt32) -> Bool
                       ^
Swift.Int32:13:24: note: candidate has non-matching type '(Int32, Int32) -> Bool'
    public static func == (lhs: Int32, rhs: Int32) -> Bool
                       ^
Swift.UInt64:11:24: note: candidate has non-matching type '(UInt64, UInt64) -> Bool'
    public static func == (lhs: UInt64, rhs: UInt64) -> Bool
                       ^
Swift.Int64:13:24: note: candidate has non-matching type '(Int64, Int64) -> Bool'
    public static func == (lhs: Int64, rhs: Int64) -> Bool
                       ^
Swift.UInt:11:24: note: candidate has non-matching type '(UInt, UInt) -> Bool'
    public static func == (lhs: UInt, rhs: UInt) -> Bool
                       ^
Swift.Int:11:24: note: candidate has non-matching type '(Int, Int) -> Bool'
    public static func == (lhs: Int, rhs: Int) -> Bool
                       ^
Swift.AnyKeyPath:6:24: note: candidate has non-matching type '(AnyKeyPath, AnyKeyPath) -> Bool'
    public static func == (a: AnyKeyPath, b: AnyKeyPath) -> Bool
                       ^
Swift.ManagedBufferPointer:12:24: note: candidate has non-matching type '<Header, Element> (ManagedBufferPointer<Header, Element>, ManagedBufferPointer<Header, Element>) -> Bool'
    public static func == (lhs: ManagedBufferPointer<Header, Element>, rhs: ManagedBufferPointer<Header, Element>) -> Bool
                       ^
Swift.Unicode.Scalar:2:24: note: candidate has non-matching type '(Unicode.Scalar, Unicode.Scalar) -> Bool'
    public static func == (lhs: Unicode.Scalar, rhs: Unicode.Scalar) -> Bool
                       ^
Swift.ObjectIdentifier:2:24: note: candidate has non-matching type '(ObjectIdentifier, ObjectIdentifier) -> Bool'
    public static func == (x: ObjectIdentifier, y: ObjectIdentifier) -> Bool
                       ^
Swift.Optional<Wrapped>:2:24: note: candidate has non-matching type '<Wrapped> (Wrapped?, Wrapped?) -> Bool'
    public static func == (lhs: Wrapped?, rhs: Wrapped?) -> Bool
                       ^
Swift.Optional<Wrapped>:3:24: note: candidate has non-matching type '<Wrapped> (Wrapped?, _OptionalNilComparisonType) -> Bool'
    public static func == (lhs: Wrapped?, rhs: _OptionalNilComparisonType) -> Bool
                       ^
Swift.Optional<Wrapped>:5:24: note: candidate has non-matching type '<Wrapped> (_OptionalNilComparisonType, Wrapped?) -> Bool'
    public static func == (lhs: _OptionalNilComparisonType, rhs: Wrapped?) -> Bool
                       ^
Swift.LazyPrefixWhileCollection<Base>.Index:2:24: note: candidate has non-matching type '<Base> (LazyPrefixWhileCollection<Base>.Index, LazyPrefixWhileCollection<Base>.Index) -> Bool'
    public static func == (lhs: LazyPrefixWhileCollection<Base>.Index, rhs: LazyPrefixWhileCollection<Base>.Index) -> Bool
                       ^
Swift.Range<Bound>:2:24: note: candidate has non-matching type '<Bound> (Range<Range<Bound>.Bound>, Range<Range<Bound>.Bound>) -> Bool' (aka '<τ_0_0> (Range<τ_0_0>, Range<τ_0_0>) -> Bool')
    public static func == (lhs: Range<Range<Bound>.Bound>, rhs: Range<Range<Bound>.Bound>) -> Bool
                       ^
Swift.ReversedCollection<Base>.Index:2:24: note: candidate has non-matching type '<Base> (ReversedCollection<Base>.Index, ReversedCollection<Base>.Index) -> Bool'
    public static func == (lhs: ReversedCollection<Base>.Index, rhs: ReversedCollection<Base>.Index) -> Bool
                       ^
Swift.Set<Element>:2:24: note: candidate has non-matching type '<Element> (Set<Element>, Set<Element>) -> Bool'
    public static func == (lhs: Set<Element>, rhs: Set<Element>) -> Bool
                       ^
Swift.Set<Element>.Index:2:24: note: candidate has non-matching type '<Element> (Set<Element>.Index, Set<Element>.Index) -> Bool'
    public static func == (lhs: Set<Element>.Index, rhs: Set<Element>.Index) -> Bool
                       ^
Swift.Strideable:3:24: note: candidate has non-matching type '<Self> (Self, Self) -> Bool'
    public static func == (x: Self, y: Self) -> Bool
                       ^
Swift.StringProtocol:2:24: note: candidate has non-matching type '<Self, S> (Self, S) -> Bool'
    public static func == <S>(lhs: Self, rhs: S) -> Bool where S : StringProtocol
                       ^
Swift.String:2:24: note: candidate has non-matching type '(String, String) -> Bool'
    public static func == (lhs: String, rhs: String) -> Bool
                       ^
Swift.String.Index:2:24: note: candidate has non-matching type '(String.Index, String.Index) -> Bool'
    public static func == (lhs: String.Index, rhs: String.Index) -> Bool
                       ^
Swift._UIntBuffer<Storage, Element>:3:28: note: candidate has non-matching type '<Storage, Element> (_UIntBuffer<Storage, Element>.Index, _UIntBuffer<Storage, Element>.Index) -> Bool'
        public static func == (lhs: _UIntBuffer<Storage, Element>.Index, rhs: _UIntBuffer<Storage, Element>.Index) -> Bool
                           ^
Swift.UnsafeMutablePointer<Pointee>:2:24: note: candidate has non-matching type '<Pointee> (UnsafeMutablePointer<Pointee>, UnsafeMutablePointer<Pointee>) -> Bool'
    public static func == (lhs: UnsafeMutablePointer<Pointee>, rhs: UnsafeMutablePointer<Pointee>) -> Bool
                       ^
Swift.UnsafePointer<Pointee>:2:24: note: candidate has non-matching type '<Pointee> (UnsafePointer<Pointee>, UnsafePointer<Pointee>) -> Bool'
    public static func == (lhs: UnsafePointer<Pointee>, rhs: UnsafePointer<Pointee>) -> Bool
                       ^
Swift.UnsafeMutableRawPointer:2:24: note: candidate has non-matching type '(UnsafeMutableRawPointer, UnsafeMutableRawPointer) -> Bool'
    public static func == (lhs: UnsafeMutableRawPointer, rhs: UnsafeMutableRawPointer) -> Bool
                       ^
Swift.UnsafeRawPointer:2:24: note: candidate has non-matching type '(UnsafeRawPointer, UnsafeRawPointer) -> Bool'
    public static func == (lhs: UnsafeRawPointer, rhs: UnsafeRawPointer) -> Bool
                       ^
Swift.UnicodeDecodingResult:5:24: note: candidate has non-matching type '(UnicodeDecodingResult, UnicodeDecodingResult) -> Bool'
    public static func == (lhs: UnicodeDecodingResult, rhs: UnicodeDecodingResult) -> Bool
                       ^
Swift._ValidUTF8Buffer<Storage>:3:28: note: candidate has non-matching type '<Storage> (_ValidUTF8Buffer<Storage>.Index, _ValidUTF8Buffer<Storage>.Index) -> Bool'
        public static func == (lhs: _ValidUTF8Buffer<Storage>.Index, rhs: _ValidUTF8Buffer<Storage>.Index) -> Bool
                           ^
Swift._SwiftNSOperatingSystemVersion:2:24: note: candidate has non-matching type '(_SwiftNSOperatingSystemVersion, _SwiftNSOperatingSystemVersion) -> Bool'
    public static func == (lhs: _SwiftNSOperatingSystemVersion, rhs: _SwiftNSOperatingSystemVersion) -> Bool
                       ^
Swift.AnyIndex:2:24: note: candidate has non-matching type '(AnyIndex, AnyIndex) -> Bool'
    public static func == (lhs: AnyIndex, rhs: AnyIndex) -> Bool
                       ^
Swift.Equatable:2:24: note: protocol requires function '==' with type '(B, B) -> Bool'; do you want to add a stub?
    public static func == (lhs: Self, rhs: Self) -> Bool
                       ^

Now it returns these diagnostics:

/Users/mdiep/Repositories/apple/bugs/equatable.swift:3:8: error: type 'B' does not conform to protocol 'Equatable'
struct B: Equatable {
       ^
equatable.swift:6:14: note: candidate has non-matching type '(B, Int) -> Bool'
        static func == (_: B, _: Int) -> Bool { return false }
                    ^
Swift.==:1:24: note: candidate has non-matching type '<T where T : RawRepresentable, T.RawValue : Equatable> (T, T) -> Bool'
@inlinable public func == <T>(lhs: T, rhs: T) -> Bool where T : RawRepresentable, T.RawValue : Equatable
                       ^
Swift.FloatingPoint:2:24: note: candidate has non-matching type '<Self> (Self, Self) -> Bool'
    public static func == (lhs: Self, rhs: Self) -> Bool
                       ^
Swift.BinaryInteger:2:35: note: candidate has non-matching type '<Self, Other> (Self, Other) -> Bool'
    @inlinable public static func == <Other>(lhs: Self, rhs: Other) -> Bool where Other : BinaryInteger
                                  ^
Swift._Pointer:2:24: note: candidate has non-matching type '<Self> (Self, Self) -> Bool'
    public static func == (lhs: Self, rhs: Self) -> Bool
                       ^
Swift.Strideable:3:35: note: candidate has non-matching type '<Self> (Self, Self) -> Bool'
    @inlinable public static func == (x: Self, y: Self) -> Bool
                                  ^
Swift.StringProtocol:2:35: note: candidate has non-matching type '<Self, S> (Self, S) -> Bool'
    @inlinable public static func == <S>(lhs: Self, rhs: S) -> Bool where S : StringProtocol
                                  ^
Swift.Equatable:2:24: note: protocol requires function '==' with type '(B, B) -> Bool'; do you want to add a stub?
    public static func == (lhs: Self, rhs: Self) -> Bool
                       ^

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.

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Aug 20, 2018

Don't forget class inheritance. I'm not sure why someone would implement == in a base class and not conform to Equatable, but it is legal.

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Aug 20, 2018

Or to use an == that takes a protocol value instead of Self.

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 20, 2018

Don't forget class inheritance. I'm not sure why someone would implement == in a base class and not conform to Equatable, but it is legal.

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 {}

Or to use an == that takes a protocol value instead of Self.

I'm not sure what you mean by this. 🤔 Could you elaborate?

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Aug 20, 2018

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 == could satisfy the Equatable conformance. But because we need an exact match today, it won't. (And in the case of an enum, it's probably not a good idea.)

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 20, 2018

Oh, you can do that with Self:

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 Equatable implementation.)

That still works too.

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Aug 20, 2018

Yes, I saw that you handled the Self case, but the protocol value form is more general, since it lets you compare heterogeneous values as long as they conform to the protocol. But we won't accept that as a witness.

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 20, 2018

Ohhhhh… right. 🤦🏻‍♂️

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 21, 2018

Does this need anything then? I'm assuming this logic would be updated anyway as part of the work to support covariant witness matching.

@jrose-apple jrose-apple requested a review from DougGregor Aug 21, 2018

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Aug 21, 2018

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

@jrose-apple

This comment has been minimized.

Copy link
Member

jrose-apple commented Aug 21, 2018

@swift-ci Please test source compatibility

@jrose-apple jrose-apple requested a review from slavapestov Aug 21, 2018

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 21, 2018

I'm not very familiar with the compatibility suite, but it looks like those failures are pre-existing known failures. Is that right?

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 21, 2018

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) ||

This comment has been minimized.

@slavapestov

slavapestov Aug 21, 2018

Member

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.

This comment has been minimized.

@mdiep

mdiep Aug 22, 2018

Contributor

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())) {

This comment has been minimized.

@slavapestov

slavapestov Aug 21, 2018

Member

dyn_cast returns nullptr if the cast fails. If you’re sure you have a function here, use cast<>; otherwise check the return value.

This comment has been minimized.

@mdiep

mdiep Aug 22, 2018

Contributor

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();

This comment has been minimized.

@slavapestov

slavapestov Aug 21, 2018

Member

Wouldn’t it be better to use the nominal type of the conformance itself to compare against? Adoptee->getAnyNominal()

This comment has been minimized.

@mdiep

mdiep Aug 22, 2018

Contributor

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.

This comment has been minimized.

@slavapestov

slavapestov Aug 21, 2018

Member

I don’t think you want to do that. An operator== where both parameters are Foo.Type cannot witness == on Foo for example.

This comment has been minimized.

@mdiep

mdiep Aug 22, 2018

Contributor

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()) {

This comment has been minimized.

@slavapestov

slavapestov Aug 21, 2018

Member

You’re checking if at least one parameter is of the nominal type — but is that the right check?

This comment has been minimized.

@mdiep

mdiep Aug 22, 2018

Contributor

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,

This comment has been minimized.

@slavapestov

slavapestov Aug 21, 2018

Member

You know it conforms to the protocol, because you found the operator via name lookup on the type

@mdiep mdiep force-pushed the mdiep:pare-down-operator-candidates branch Aug 22, 2018

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 23, 2018

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 Self. I test that by checking that either

  1. it's defined in a protocol extension and uses Self
  2. it's defined on the same nominal type
  3. it's defined on a generic type, which might match (I'm not sure how to make this more precise)

I believe this correctly cuts down on the number of possible matches, but it might not cut down as many as it could.

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 24, 2018

@mdiep Would it make sense to extract the common checks into a shared function? That would have made the PR much clearer.

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 24, 2018

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.

@slavapestov slavapestov self-assigned this Aug 24, 2018

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 24, 2018

Would it make sense to extract the common checks into a shared function? That would have made the PR much clearer.

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?

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.

👍🏻 I think test/decl/protocol/req/func.swift covers the basics well, but I'll add a few more cases there.

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 24, 2018

A top-level function in the swift namespace defined in TypeCheckDecl.cpp, or you could even move it to a method on FuncDecl.

@mdiep mdiep force-pushed the mdiep:pare-down-operator-candidates branch Aug 25, 2018

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 25, 2018

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) {

This comment has been minimized.

@slavapestov

slavapestov Aug 28, 2018

Member

Passing the conformance down feels like a hack. Is there no other way to get the type you need?

@mdiep mdiep force-pushed the mdiep:pare-down-operator-candidates branch to 5e9da14 Aug 28, 2018

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 28, 2018

You're right, I didn't need to pass in the conformance. The type was available on the WitnessChecker.

@slavapestov
Copy link
Member

slavapestov left a comment

Thanks for taking the time to iterate on this!

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 28, 2018

@swift-ci Please smoke test

@mdiep

This comment has been minimized.

Copy link
Contributor

mdiep commented Aug 29, 2018

Thanks for taking the time to iterate on this!

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.

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 29, 2018

@swift-ci Please smoke test Linux

1 similar comment
@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Aug 29, 2018

@swift-ci Please smoke test Linux

@slavapestov slavapestov merged commit 003d9c0 into apple:master Aug 29, 2018

2 of 3 checks passed

Swift Source Compatibility Suite on macOS Platform (Debug)
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@mdiep mdiep deleted the mdiep:pare-down-operator-candidates branch Aug 29, 2018

@NachoSoto

This comment has been minimized.

Copy link
Contributor

NachoSoto commented Aug 31, 2018

I think this resolved https://bugs.swift.org/browse/SR-4318 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment