[Typechecker] Check conforming protocols of context when resolving a custom attribute

[theblixguy]

When we try to resolve a custom attribute, we do not look into the protocols that the context conforms to. For example:

@propertyWrapper
struct S {
  var wrappedValue: Int
}

protocol P {
  typealias Wrapper = S
}

struct C: P {
  @Wrapper var answer = 42 // error: unknown attribute 'Wrapper'
}

This is because CustomAttrNominalRequest calls into directReferencesForUnqualifiedTypeLookup, which does unqualified lookup but ignores protocol members, which means if the user has defined a typealias inside the protocol then we will fail to find it and throw an error instead.

Resolves SR-11288, SR-11131 and SR-11120.
Resolves rdar://problem/53107788
Resolves rdar://problem/53107270

[jrose-apple]

What happens if it finds an associated type? (Ignoring requirements is why ignoring protocol members is the default.)

[jrose-apple]

It does seem like ignore-protocol-members is keeping us from finding things in protocol extensions too, though.

[theblixguy]

What happens if it finds an associated type? (Ignoring requirements is why ignoring protocol members is the default.)

Nothing happens... I mean until you try to use the associatedtype as a wrapper, in which case you'll get an error (depending on how its defined).

Is there a particular example you have in mind? Some examples:

@propertyWrapper
struct S {
  var wrappedValue: Int
}

protocol P {
  typealias Wrapper = S
  associatedtype Wrapper1 = S
  associatedtype Wrapper2
  associatedtype Wrapper3
}

extension P {
  typealias Wrapper3 = S
}

struct C: P {
  typealias Wrapper2 = S
  @Wrapper var foo1 = 42 // ok
  @Wrapper1 var foo2 = 42 // error
  @Wrapper2 var foo3 = 42 // ok
  @Wrapper3 var foo4 = 42 // ok
}

[jrose-apple]

I'm not sure we want to accept that as a valid spelling, though I suppose in the context of the concrete type the associated type refers to the concrete witness…

[theblixguy]

I suppose we could filter out associated types if it turns out to be problematic. The only reason the above examples work is because the reference resolves to a typealias inside a concrete type.

[jrose-apple]

(I confess I'm waiting for one of the other tagged reviewers to chime in.)

[theblixguy]

@DougGregor @slavapestov do you have a moment to take a look at this? Apologies if you are busy. Thank you!

[DougGregor]

This looks right to me, thank you!

[DougGregor]

Actually, a few comments:

• I think we should make sure we reject the attribute if it refers to an associated type; we don't want associated type resolution to be involved in attribute resolution
• Please test that we get reasonable behavior when the type alias is in a constrained protocol extension
• Please test that we get reasonable behavior when the type that conforms to the protocol is generic

[DougGregor]

@swift-ci please smoke test

[theblixguy]

@swift-ci please smoke test

[tanner0101]

I just tested this on swift-DEVELOPMENT-SNAPSHOT-2019-09-04 and it works great. 👍 This is the final piece I needed to get property wrappers working for my use case. Thanks, @theblixguy!

@DougGregor / @jrose-apple will this be cherry picked into swift-5.1-branch?

[theblixguy]

I think it's probably too late to get this cherry-picked (only critical bug fixes are eligible as far as I know), but I'll leave the decision to Jordan/Doug.

[tanner0101]

@theblixguy does this patch rely on anything else that was on master or could it be applied directly to 5.1?

[theblixguy]

It can be applied to 5.1 without any issues, but the criteria for acceptance is much more strict since we're close to release. As I said, the final decision lies with the Swift team.

[tanner0101]

@theblixguy ok that makes sense. 👍

To the Swift team: At least for me, this bug is critical. I can't make use of property wrappers correctly until it is fixed. Especially given how small this change is, I think it makes sense to merge into 5.1. Thanks!

[jrose-apple]

It's definitely too risky to take for 5.1.0 at this point, but perhaps a point-release. Can you elaborate on your use case? Anything that's about typealiases always has a workaround, so I don't see how it could be "critical".

[tanner0101]

@jrose-apple that's unfortunate but understandable. I think I'll need to wait for Swift 5.2 then as I'm not sure I can make a specific patch release a minimum version requirement for my package.

My use case (for the server-side Swift packages vapor/vapor and vapor/fluent) looks something like this:

protocol Model { }

@propertyWrapper
struct ModelField<Root, Value> 
    where Root: Model, Value: Codable
{
    // access to `Root` in this scope is valuable
    var wrappedValue: Value { ... }
}

extension Model {
    typealias Field<Value> = ModelField<Self, Value>
}

struct User: Model {
    @Field var name: String
}

The typealias allows for just @Field to be inside a model declaration. This keeps a concise API while allowing for the actual property wrapper to still get strong type information on the root type.

This doesn't work properly on 5.1 without this PR since the property wrapper lookup doesn't see type aliases.