[SE-NNNN] Method and Initializer Key Paths

[amritpan]

This is the proposal text for method and initializer key paths, and is ready for review.

[beccadax]

Excited about this proposal, but there are a few suggestions I'd like to make.

[beccadax]

How certain are we that expanding @dynamicMemberLookup to methods won't cause source compatibility breaks in common libraries? Is this something we've tested? Should we discuss an alternative where, for instance, you need to say @dynamicMemberLookup(all) if you want to support method lookup?

(This wasn't really a concern with metatype keypaths because the subscript(dynamicMember:) would have to accept KeyPath<Foo.Type, T> instead of KeyPath<Foo, T> for the new members to affect it.)

[amritpan]

How certain are we that expanding @dynamicMemberLookup to methods won't cause source compatibility breaks in common libraries?

I hadn't considered this! Also to make sure I fully understand - are we talking about being able to differentiate between a static and an instance method via @dynamicMemberLookup? I have added these passing tests to Interpreter/keypath.swift for @dynamicMemberLookup but it doesn't test across libraries. How can I test for this?

[beccadax]

My concern is that suddenly having a lot of extra methods on types that use @dynamicMemberLookup will cause problems for real-world codebases. The fact that dynamic members are disfavored by the type checker should reduce source compatibility concerns, but it might not be the end of the story.

I suppose there are two pieces to this question:

1. Does this change cause projects to actually fail to build? You can assess this by asking Swift CI to run a source compatibility test on your PRs; Swift CI will try to build a bunch of open source projects using a compiler with your changes and will report any projects that fail.

2. How well does this change work for types that have already adopted @dynamicMemberLookup? That's something you'd have to assess manually, by looking at popular libraries with @dynamicMemberLookup types, figuring out how much new API surface this exposes, and thinking about whether that new surface is bad. (To be clear, this is a subjective call!)

[amritpan]

Could you show me an example of the problem that you're describing? I'm worried that I may not have fully grasped the issues here.

Here is what I think I understand: The typechecker disfavors the dynamic members over the real instance members. However, with this feature we might still run into a situation where a dynamicMemberLookup might attempt to resolve an unknown and unexpected resolve to a method? For example, if we have this type using @dynamicMemberLookup:

@dynamicMemberLookup
struct DynamicWrapper {
    private var storage: [String: Any]

    init(_ storage: [String: Any]) {
        self.storage = storage
    }

    subscript(dynamicMember member: String) -> Any? {
        return storage[member]
    }
}

let json = DynamicWrapper(["fruit": "Mandarin", "qty": 30])

And then we have this keypath to a method:

struct Food {
    func getName() -> String {
        return "Grapefruit!"
    }
}

let kpName = \Food.getName

Now before this feature, we could write:

let jsonLookup = DynamicWrapper(["fruit": "Clementine"])
print(json.getName)

and it would fall back to Clementine. But now with implicit keypath methods, like:

let kpNameDynamic = \DynamicWrapper.getName

we'll prioritize the real method over the dynamic method (Grapefruit, when applied), which is expected. However, this could break projects that rely on dynamic lookups for method like names.

[amritpan]

I have been trying to run the source compatibility tests on the PR but they are currently failing due to unrelated issues (latest build, release results) and it seems that they are broken in general. I've run them a few times last week and some of the unrelated broken tests have resolved but not all, so I'll keep tracking that and re-run them again. The tests are not showing any keypath or dynamicMemberLookup related test failures but I don't know if I should trust them in their current state.

How well does this change work for types that have already adopted @dynamicMemberLookup? That's something you'd have to assess manually, by looking at popular libraries with @dynamicMemberLookup types, figuring out how much new API surface this exposes, and thinking about whether that new surface is bad. (To be clear, this is a subjective call!)

I wonder if this is a question that we can raise during the review to get input from library developers who might have more experience with this.

[beccadax]

I have been trying to run the source compatibility tests on the PR but they are currently failing due to unrelated issues (latest build, release results) and it seems that they are broken in general.

Looks like the debug one failed to build LLVM (obviously unrelated) and the release one had one unexpected pass (probably some recently-merged PR having fixed a bug). I think we can call that a pass on source compatibility.

I wonder if this is a question that we can raise during the review to get input from library developers who might have more experience with this.

Okay.

[beccadax]

This would be a good place to discuss whether you expect this feature to back-deploy and what constraints there might be on doing so.

In particular, in SE-0438, there were some subtleties around symbols that we needed to start emitting. Are there similar concerns here? How are they being resolved? (If things are shaking out in basically the same way they did in the metatype keypath proposal, you could quickly summarize and link back to it.)

[amritpan]

I initially assumed that this feature would have the same implications on back-deployment as SE-0438. But then in the pitch discussion, it was suggested that methods do not emit descriptors. I also did not run into any linking errors with my Interpreter tests which is what I used to detect missing descriptors for metatype keypaths.

In the SE-0438 implementation I also added handling to account for nil descriptors to KeyPath in the Standard Library and so between that and not emitting any descriptors with this implementation, I made the assumption that this is a non issue and this feature would have no constraints on back-deployment.

[beccadax]

IIRC, these descriptors are used to support Equatable on key paths. If the descriptors are always null, does that mean the Equatable conformance will behave strangely?

[amritpan]

Based on this code, Keypath no longer uses the descriptors to determine Equatable/Hashable conformance and instead relies on the component kind and component type. To the best of my understanding, the descriptors are only used to reference computed/external properties.

[beccadax]

I think the crucial thing here is the func == for KeyPathComponent, which does check the id fields inside of get components (which I believe is what you're using for method keypaths?).

Looking at your branch, it looks like you're generating the ID for methods by passing a SILDeclRef for storage to getFunction(). In other words, the computed property ID for a method is the pointer to the function itself. I think that will work okay since you're forming the SILDeclRef directly from the AbstractFunctionDecl without offering an opportunity for it to end up pointing to, say, a thunk, although I worry a little about edge cases involving resilience. (Might comment more on that in the implementation PR.)

In any case, it looks like you have a more or less reasonable computed property ID and so therefore you should be able to get good equality behavior.

[amritpan]

I will wait for your comments on the PR before merging! The swift-syntax and foundations PRs have been merged.

[beccadax]

Oh, one more question that isn't addressed in the proposal: Do these key path literals support the implicit closure coercion like other key path literals do? The ability to say fooArray.map(\.foo(bar: baz)) might be worth highlighting!

[amritpan]

Yes, implicit closure coercion is supported with this feature and I have added a section to note this!