[swift-evolution] Custom summary for Mirrors?

[Dave Abrahams]

Hi Austin, I think we should discuss it here.

First off, dump() was really just added as a proof-of-concept by Joe
Groff when he implemented the original mirror system.  We decided to
keep it around because it seemed like it might be useful for somebody,
but one possibility to consider is that it should be retired.

For the rest, see below

on Mon Jan 18 2016, Austin Zheng via swift-evolution <swift-evolution-m3FHrko0VLzYtjvyW6yDsg-AT-public.gmane.org> wrote:

> Hi Dave,
>
> I don't think any progress has been made on this specific issue. Slava
> made a comment a week ago (https://github.com/apple/swift/pull/838
> <https://github.com/apple/swift/pull/838>, search "This looks like a
> QoI regression to me"), but nothing since then.
>
> I'm planning on spending today to work on resolving some of the other
> issues that were brought up in the PR; maybe we can pick up the
> conversation again after I update it. Let me know what you prefer.
>
> Best,
> Austin 
>
>> On Jan 18, 2016, at 12:05 AM, Dave Abrahams via swift-evolution
>> <swift-evolution at swift.org> wrote:
>> 
>> 
>> on Tue Jan 05 2016, Austin Zheng via swift-evolution
>> <swift-evolution-m3FHrko0VLzYtjvyW6yDsg-AT-public.gmane.org
>> <http://swift-evolution-m3fhrko0vlzytjvyw6ydsg-at-public.gmane.org/>>
>> wrote:
>> 
>>> Here are a couple of examples I had in mind.
>>> 
>>> * Arrays (from test/1_stdlib/Runtime.swift:1348), dumping an array with 5
>>> elements:
>>> 
>>> BEFORE:
>>> ▿ 5 elements
>>> - [0]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.MacWrite
>>> - [1]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.MacPaint
>>> - [2]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.FileMaker
>>> ▿ [3]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.ClarisWorks
>>>    - ClarisWorks: true
>>>       ▿ [4]:
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.HyperCard
>>>    - HyperCard: false
>>> 
>>> AFTER:
>>> ▿ [a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.MacWrite,
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.MacPaint,
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.FileMaker,
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.ClarisWorks(true),
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.HyperCard(false)]
>>> - [0]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.MacWrite
>>> - [1]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.MacPaint
>>> - [2]: a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.FileMaker
>>> ▿ [3]:
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.ClarisWorks(true)
>>>    - ClarisWorks: true
>>>    ▿ [4]:
>>> a.MultiPayloadTagBitsSmallNonGenericEnumWithDefaultMirror.HyperCard(false)
>>>    - HyperCard: false
>>> 
>>> * Dictionaries (from test/1_stdlib/ReflectionHashing.swift:43):
>>> 
>>> BEFORE:
>>> ▿ 5 key/value pairs
>>>  ▿ [0]: (2 elements)
>>>    - .0: Four
>>>    - .1: 4
>>>  ▿ [1]: (2 elements)
>>>    ...
>>> 
>>> AFTER:
>>> ▿ ["Four": 4, "One": 1, "Two": 2, "Five": 5, "Three": 3]
>>>  ▿ [0]: ("Four", 4)
>>>    - .0: "Four"
>>>    - .1: 4
>>>  ▿ [1]: ("One", 1)
>>>    ...
>>> 
>>> * Dumping a CGRect (from test/1_stdlib/Reflection_objc.swift):
>>> 
>>> BEFORE:
>>> (50.0, 60.0, 100.0, 150.0)
>>> 
>>> AFTER:
>>> __C.CGRect(origin: __C.CGPoint(x: 50.0, y: 60.0), size: __C.CGSize(width:
>>> 100.0, height: 150.0))
>>> 
>>> Let me know if you'd like more, although most are variants on the above.
>>> 
>>> Best,
>>> Austin
>>> 
>>> On Tue, Jan 5, 2016 at 5:37 PM, Dave Abrahams <dabrahams at apple.com> wrote:
>>> 
>>>> 
>>>> On Jan 5, 2016, at 5:28 PM, Austin Zheng via swift-evolution <
>>>> swift-evolution at swift.org> wrote:
>>>> 
>>>> Hi Joe,
>>>> 
>>>> I respect the choice of the team to use Custom[Debug]StringConvertible in
>>>> lieu of summary. At the same time, in my opinion the output of dump() has
>>>> become significantly more difficult to read (c.f. unit tests in
>>>> https://github.com/apple/swift/pull/838/files).
>>>> 
>>>> 
>>>> Specific examples of readability regressions, please?
>>>> 
>>>> Would you and the team be open to exploring alternative solutions that
>>>> improve the readability of dump() without increasing API surface area?
>>>> 
>>>> 
>>>> Sure.
>>>> 
>>>> For example, perhaps the reflection machinery itself should have special
>>>> handling for some of the built-in types. If not, I'll consider this
>>>> discussion thread complete.
>>>> 
>>>> Thanks,
>>>> Austin
>>>> 
>>>> 
>>>> 
>>>> On Tue, Jan 5, 2016 at 3:22 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> 
>>>>> Getting custom summaries for the common CG types certainly seems
>>>>> reasonable. We'd have to get approval from the appropriate teams at Apple,
>>>>> but I can't see any objections.
>>>>> 
>>>>> Jordan
>>>>> 
>>>>> 
>>>>> On Dec 30, 2015, at 9:55, Joe Groff via swift-evolution <
>>>>> swift-evolution at swift.org> wrote:
>>>>> 
>>>>> I believe 'summary' is obsolete, and you're supposed to use
>>>>> Custom[Debug]StringConvertible to customize your type's reporting now.
>>>>> 
>>>>> -Joe
>>>>> 
>>>>> On Dec 29, 2015, at 10:38 PM, Austin Zheng via swift-evolution <
>>>>> swift-evolution at swift.org> wrote:
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> I'd like to gauge reaction for a proposal I was considering: adding to
>>>>> the standard library's Mirror type a 'summary' property, and the option to
>>>>> initialize a Mirror with a custom summary. If no custom summary is
>>>>> provided, the summary would default to the string produced by calling
>>>>> String(reflecting: subject) on the subject at the time of mirror creation.
>>>>> 
>>>>> Some context: right now, there are two APIs for mirrors in the standard
>>>>> library: CustomReflectable, which is publicly exposed and relies on the
>>>>> conforming type creating a Mirror object, and _Reflectable, which relies on
>>>>> the conforming type having a companion type conforming to _MirrorType. A
>>>>> short-term goal is to migrate the standard library's types off the
>>>>> _Reflectable API and have them use the CustomReflectable API, and changing
>>>>> dump() accordingly.
>>>>> 
>>>>> The extant implementation of dump() uses a property on _MirrorType called
>>>>> "summary". (This is where e.g. "4 elements" comes from when you dump() an
>>>>> array.) "summary" is absent from Mirror or any types related to
>>>>> CustomReflectable. I asked Joe Groff about this and the rationale was that
>>>>> it was deemed too similar to debugDescription (or String(reflecting: foo))
>>>>> to be worth carrying over.
>>>>> 
>>>>> I would like to suggest that there might be a purpose for "summary":
>>>>> 
>>>>> - Types with children, especially container types like arrays, often
>>>>> print out a description of their children as part of their debugDescription
>>>>> or description, redundant when using an API like dump() which provides a
>>>>> structural representation of the children of the subject. In such cases a
>>>>> lighter-weight description (like "3 elements") might be more appropriate to
>>>>> represent to the user.

Agreed, if we keep dump, we ought to recover that functionality somehow.
However, IMO we might be best off synthesizing that text for mirrors
with children by using their DisplayStyle and/or checking their
conformances.  It doesn't seem to be worth expanding the protocols for.

>>>>> 
>>>>> - Certain types like CGRect don't conform to CustomStringConvertible,
>>>>> CustomDebugStringConvertible, Streamable, etc. Having a custom summary for
>>>>> these types customized by the corresponding Mirror would allow for a
>>>>> 'pretty' representation during reflection in lieu of the ugly one generated
>>>>> by the runtime without making more substantial changes to the API which
>>>>> might break third-party code (such as conforming CGRect to any of the
>>>>> aforementioned protocols).

I don't think it's worth worrying about breakage from adding a
CustomDebugStringConvertible conformance to CGRect.
 
>>>>> I know that Mirror (and reflection as a whole) are being considered for
>>>>> major design changes, so this would be a minor transient change to make the
>>>>> API easier to work with in the meantime.
>>>>> 
>>>>> Please let me know whether or not you think this proposed change is
>>>>> meaningful and worthwhile, or if you have any questions.
>>>>> 
>>>>> Best,
>>>>> Austin
>> 
>> Hey, Austin,
>> 
>> Is this still something we need to discuss, or did it get resolved
>> somehow?
>> 
>> Thanks,
>> Dave

Thanks,
Dave