Add `KeyPathIterable` protocol and synthesis. #21557

dan-zheng · 2018-12-28T09:42:02Z

The KeyPathIterable protocol represents types whose values provide
custom key paths to properties or elements.

For types that conform to KeyPathIterable, the compiler can synthesize
a default implementation of allKeyPaths based on the type's stored
properties.

KeyPathIterable enables generic machine learning optimizers and will
be used to replace ParameterGroup.

The ParameterGroup protocol requirements are not sufficiently general
for many use cases: it is limited to a single parameter type and does not
support joint iteration over parameters for multiple ParameterGroup
instances.

KeyPathIterable solves these problems. In addition, it is a generally
useful language feature that can model both static stored properties
and custom dynamic properties.

KeyPathIterable.swift currently lives in stdlib/public/core to enable
future usage in AutoDiff tests without dependency on TensorFlow module.

KeyPathIterable is designed by @rxwei and myself.

KeyPathIterable is inspired by:

Various discussions about Swift key paths, in particular this tweet
Objective-C Key-Value Coding, lenses in functional programming languages

Here is an extended design for KeyPathIterable that separates functionality into two protocols: StoredPropertyIterable and CustomKeyPathIterable.

StoredPropertyIterable models the static layout of structs and provides a static collection of key paths to all stored properties.
CustomKeyPathIterable can model both static and dynamic structures, including collection types like Array and Dictionary.

dan-zheng · 2018-12-28T09:43:03Z

stdlib/public/core/KeyPathIterable.swift

+public extension KeyPathIterable {
+  /// An array of all custom key paths of this value and any custom key paths
+  /// nested within each of what this value's key paths refers to.
+  var recursivelyAllKeyPaths: [PartialKeyPath<Self>] {


The naming of recursivelyAllKeyPaths and the doc comment have room for improvement.

nestedKeyPaths?

Thanks for the suggestion! I feel nestedKeyPaths is not an ideal name for this property because it also contains top-level allKeyPaths. nestedKeyPaths sounds like it doesn't contain top-level properties (or at least leaves it ambiguous).

We considered nestedKeyPaths as not just a different name, but a different design option (where allNestedKeyPaths refers to recursivelyAllKeyPaths excluding top-level allKeyPaths, and recursivelyAllKeyPaths would be defined as allKeyPaths + allNestedKeyPaths).

We decided not to implement that option though, since we couldn't think of many use cases for "just recursively all nested non-top-level key paths". There seem to be more use cases for "recursively all key paths, including top-level key paths".

lib/Sema/DerivedConformanceKeyPathIterable.cpp

The `KeyPathIterable` protocol represents types whose values provide custom key paths to properties or elements. For types that conform to `KeyPathIterable`, the compiler can synthesize a default implementation of `allKeyPaths` based on the type's stored properties. --- `KeyPathIterable` enables generic machine learning optimizers and will be used to replace `ParameterGroup`. The `ParameterGroup` protocol requirements are not sufficiently general for many use cases: it is limited to a single parameter type and does not support joint iteration over parameters for multiple `ParameterGroup` instances. `KeyPathIterable` solves these problems. In addition, it is a generally useful language feature that can model both static stored properties and custom dynamic properties.

rxwei

Great!

lib/Sema/DerivedConformanceKeyPathIterable.cpp

stdlib/public/core/KeyPathIterable.swift

rxwei · 2018-12-28T09:55:30Z

lib/Sema/DerivedConformanceKeyPathIterable.cpp

+        UnresolvedDotExpr(nominalTypeExpr, SourceLoc(), member->getFullName(),
+                          DeclNameLoc(), /*Implicit*/ true);
+    auto *keyPathExpr =
+        new (C) KeyPathExpr(SourceLoc(), dotExpr, nullptr, /*Implicit*/ true);


Woo! This is so simple.

lib/Sema/DerivedConformanceKeyPathIterable.cpp

@rxwei

- Remove `@inlinable` from `KeyPathIterable` extension methods. - Enable `KeyPathIterable` synthesis for empty structs, add test. - Add comment by @rxwei about supporting synthesis for classes in the future.

Mark synthesized `allKeyPaths` declaration with `@inlinable`. Verified that `@inlinable` is correctly added in SILGen: ``` struct Parameters : KeyPathIterable { @sil_stored var w: Float { get set } @sil_stored var b: Float { get set } init(w: Float, b: Float) typealias AllKeyPaths = [PartialKeyPath<Parameters>] @inlinable var allKeyPaths: [PartialKeyPath<Parameters>] { get } } ```

rxwei · 2018-12-28T10:37:57Z

@swift-ci please test tensorflow

dan-zheng · 2018-12-28T11:09:14Z

Note: the synthesis condition for KeyPathIterable could be tightened to: "synthesize requirements only if no requirements have been user customized".

This would prevent errors like the following:

// error.swift
struct Test : KeyPathIterable {
  var x: Float
  // Question:
  //   What if I customize `AllKeyPaths` to be sth that conforms to
  //   `Collection` but not `ExpressibleByArrayLiteral`?
  //   And I let the compiler synthesize `allKeyPaths`?
  // Answer:
  //    The compiler doesn't crash, but produces an error because
  //    the synthesized body of `allKeyPaths` assumes that `AllKeyPaths`
  //    conforms to `ExpressibleByArrayLiteral`.
  typealias AllKeyPaths = UnsafeBufferPointer<PartialKeyPath<Test>>
}

error.swift:1:8: error: type 'Test' does not conform to protocol 'KeyPathIterable'
struct Test : KeyPathIterable {
       ^
error.swift:1:8: error: protocol 'KeyPathIterable' is broken; cannot derive conformance for type 'Test'
struct Test : KeyPathIterable {
       ^

Errors like this aren't unique to KeyPathIterable though, CaseIterable has a similar issue.
Since the compiler doesn't crash, I think this error can be handled in a follow-up PR.

rxwei · 2018-12-28T11:15:27Z

Great. Thanks for the detailed note!

natecook1000 · 2019-04-21T00:37:58Z

stdlib/public/core/KeyPathIterable.swift

+
+public extension KeyPathIterable {
+  var _allKeyPathsTypeErased: [AnyKeyPath] {
+    return allKeyPaths.map { $0 as AnyKeyPath }


Since key paths are classes, you can cast the array for these two instead of mapping:

return allKeyPaths as [AnyKeyPath]

(This is really cool functionality!)

It's not really directly convertible. I'm getting this error:

/Users/rxwei/Development/Swift/swift-tf-source/swift/stdlib/public/core/KeyPathIterable.swift:57:24: error: 'Self.AllKeyPaths' is not convertible to '[AnyKeyPath]'; did you mean to use 'as!' to force downcast? return allKeyPaths as [AnyKeyPath] ~~~~~~~~~~~~^~~~~~~~~~~~~~~ as!

If that works on master, then it's probably because the changes haven't been merged. The tensorflow branch is a pretty old (2018-11-24).

Ah, you’re right — I’d missed that allKeyPaths isn’t an array here, it’s an instance of the associated type. Carry on!

Ah right. I forgot that too :)

natecook1000 · 2019-04-21T00:41:19Z

stdlib/public/core/KeyPathIterable.swift

+extension Dictionary : KeyPathIterable {
+  public typealias AllKeyPaths = [PartialKeyPath<Dictionary>]
+  public var allKeyPaths: [PartialKeyPath<Dictionary>] {
+    return keys.map { \Dictionary[$0]! }


Seems like this (and really all collections) should follow the array example of giving access via indices, yielding key paths to key-value pairs here. If you make Dictionary.Values key-path iterable, too, a user could write myDictionary.values.allKeyPaths to get this result.

Good point. Key paths using subscript and Dictionary.Index currently cause a crasher on our branch. This may have been fixed on master. We'll make sure to update these once we merge.

dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Dec 28, 2018

dan-zheng requested review from rxwei and marcrasi December 28, 2018 09:42

dan-zheng commented Dec 28, 2018

View reviewed changes

lib/Sema/DerivedConformanceKeyPathIterable.cpp Outdated Show resolved Hide resolved

dan-zheng force-pushed the key-path-iterable branch from 5558d7d to 89b3fe2 Compare December 28, 2018 09:49

rxwei approved these changes Dec 28, 2018

View reviewed changes

rxwei reviewed Dec 28, 2018

View reviewed changes

lib/Sema/DerivedConformanceKeyPathIterable.cpp Outdated Show resolved Hide resolved

dan-zheng added 2 commits December 28, 2018 02:17

Address feedback from @rxwei.

9b37692

- Remove `@inlinable` from `KeyPathIterable` extension methods. - Enable `KeyPathIterable` synthesis for empty structs, add test. - Add comment by @rxwei about supporting synthesis for classes in the future.

rxwei merged commit 247bb4b into swiftlang:tensorflow Dec 28, 2018

natecook1000 reviewed Apr 21, 2019

View reviewed changes

dan-zheng mentioned this pull request Jan 22, 2019

[SR-8457] [ParameterUpdate] Synthesize allKeyPaths for ParameterAggregate #50978

Closed

Add KeyPathIterable protocol and synthesis. #21557

Add KeyPathIterable protocol and synthesis. #21557

Uh oh!

Conversation

dan-zheng commented Dec 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-zheng Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rxwei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rxwei commented Dec 28, 2018

Uh oh!

dan-zheng commented Dec 28, 2018

Uh oh!

rxwei commented Dec 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `KeyPathIterable` protocol and synthesis. #21557

Add `KeyPathIterable` protocol and synthesis. #21557

dan-zheng Apr 26, 2019 •

edited

Loading

rxwei Apr 21, 2019 •

edited

Loading