Migrate mirrors to use CustomReflectable API, rewrite dump() #838

austinzheng · 2016-01-01T08:18:57Z

Jira: SR-88
Changes:

Removed stdlib type conformances to _Reflectable
Conformed stdlib types to CustomReflectable, CustomPlaygroundQuickLookable
Rewrote dump() function to not use _reflect()
Rewrote unit tests for compatibility with new API

NOTES: Jira here: https://bugs.swift.org/browse/SR-88. The commit turned out quite a bit larger than I anticipated, so if you want me to try breaking it down I'd be happy to do so. All tests pass on OS X. Note that I disabled a few tests that were affected by the linked bugs in the Jira ticket. A later PR will contain additional unit tests for any types that I missed. Let me know if you have any questions.

austinzheng · 2016-01-01T08:23:36Z

test/1_stdlib/Reflection_objc.swift

-print(_reflect(CGSize(width: 30, height: 60)).summary)
-// CHECK-NEXT: (50.0, 60.0, 100.0, 150.0)
-print(_reflect(CGRect(x: 50, y: 60, width: 100, height: 150)).summary)
+// CHECK-NEXT: ▿ {(


This test needs to be rewritten to not assume a specific ordering of items.

dabrahams · 2016-01-01T16:55:19Z

Very awesome to see this finally getting done, Austin! Just one big question: I expected to see all the gyb’d nonsense fall away when we got to this point. Is it really still pulling its weight?

austinzheng · 2016-01-01T20:35:31Z

Thanks! I would say that gyb is still pulling its weight in files like Arrays.swift.gyb and CGFloat.swift.gyb, where mirror conformance was only a small part. However, there are a bunch of mirror-specific .gyb files that I could get rid of, moving the conformances directly into the 'base' source files for each type. That might improve code structure/readability at the cost of a little duplication. Let me know what your thoughts are, and I'll make the appropriate changes for the next revision of this PR.

austinzheng · 2016-01-01T20:39:45Z

One further note - there's one remaining use of _reflect(), in OutputStream.swift's _adHocPrint() method. I didn't remove it because that method depends on switching on the concrete type of the _MirrorType instance returned by _reflect(), and then doing stuff based on it. I can hold off removing it if there's a plan to replace or rewrite that method, or I can rewrite it myself. Up to you.

dabrahams · 2016-01-02T00:10:10Z

@austinzheng yes, all the mirror-specific .gyb files, and the mirror-specific gyb usage within files that are gyb’d for other reasons, were what I had in mind. What kind of code duplication do you envision? The current design of Mirror was specifically crafted to avoid that duplication and the need for gyb.

dabrahams · 2016-01-02T00:11:05Z

@austinzheng I don’t see why that switching should be needed since all the _MirrorType models would be disappearing?

austinzheng · 2016-01-02T00:15:09Z

@dabrahams Sounds good, I'll remove the mirror-specific gyb usage. "Code duplication" was more of a strawman than anything else; I don't think it's an actual issue.

As for _adHocPrint(), what I meant was that the existing implementation switches on _MirrorType's concrete type. I'll rewrite that function to not depend on _reflect(), if you have no objections.

austinzheng · 2016-01-02T01:38:35Z

One thing I need to figure out is how to tell that a Mirror is reflecting a metatype. The existing code checks if the _MirrorType is a _MetatypeMirror, which isn't a good solution going forward. The Mirror API doesn't provide access to the object being reflected, and the subjectType property (whose type is Any.Type) doesn't seem to provide enough information to disambiguate a metatype from a metatype type without resorting to something unacceptably hacky (like checking for a ".Type" suffix on the type name). I'll think about this some more.

dabrahams · 2016-01-02T02:09:03Z

Rewriting _adHocPrint sounds like the right move. We want to retire _MirrorType.

dabrahams · 2016-01-02T02:13:42Z

Why do you need to get this from the mirror? Don’t you have the original object at some level in the call chain? You needed it to get the mirror in the first place. Don’t forget, _adHocPrint is not public API; you can change it arbitrarily as needed.

austinzheng · 2016-01-02T02:24:54Z

You're absolutely right, the original object is in fact passed to the function. I'll be sure to get some fresh air before I write any more code :).

On Jan 1, 2016, at 6:14 PM, Dave Abrahams [email protected] wrote:

Why do you need to get this from the mirror? Don’t you have the original object at some level in the call chain? You needed it to get the mirror in the first place. Don’t forget, _adHocPrint is not public API; you can change it arbitrarily as needed.

—
Reply to this email directly or view it on GitHub #838 (comment).

austinzheng · 2016-01-02T06:35:32Z

The last obstacle before I can finish rewriting my function is getting the string representing a particular enum case. (The generic mirror created by Mirror(reflecting:) doesn't have any case name information.)

The way it works right now is that _EnumMirror has a property called caseName, which in turn calls into a C++ runtime function named swift_EnumMirror_caseName. This property allows the _EnumMirror to get a reference to the name string from the runtime. I've tried calling into this function from outside the context of _EnumMirror, but with limited success. I'll draft an email to swift-dev later asking for help understanding the context of that runtime entry point.

dabrahams · 2016-01-02T07:13:40Z

I think @jckarter is the knower of these things; hopefully this mention gets his attention.

dabrahams · 2016-01-06T00:23:36Z

stdlib/public/core/CMakeLists.txt

@@ -121,7 +120,6 @@ set(SWIFTLIB_SOURCES
  ### PLEASE KEEP THIS LIST IN ALPHABETICAL ORDER ###
  Availability.swift
  Bit.swift
-  CollectionMirrors.swift.gyb
  CollectionOfOne.swift
  ExistentialCollection.swift.gyb
  Mirror.swift


Speaking of Mirror.swift, the _legacyMirror method should obviously be gone when this PR is done.

Hi Dave,

_legacyMirror is tightly integrated with the Mirror initializers that generate mirrors from _reflect() if no custom mirror is available. I don't think it can be removed until Phase 3 of SR-88 is complete, at which point we'd rip all use of _reflect() out of Mirror and use the new runtime entry points.

What do you think?

I think it makes sense to make as much progress as possible in the incremental development spirit. If we can't remove something without redesigning the runtime, it shouldn't block progress where we can make it.

Agreed, this PR is already pretty big as-is. I'm happy to do follow-up work that only makes sense after other dependencies are resolved.

austinzheng · 2016-01-07T08:35:56Z

I'm still working on this. Goals for next update:

Remove use of gyb for mirrors
Rewritten _adHocPrint(), including way to print enum case names
Remove _legacyMirror and friends

slavapestov · 2016-01-10T07:35:52Z

stdlib/public/core/ImplicitlyUnwrappedOptional.swift

@@ -73,12 +73,11 @@ public enum ImplicitlyUnwrappedOptional<Wrapped>
  }

  /// Returns a mirror that reflects `self`.
-  public func _getMirror() -> _MirrorType {
-    // FIXME: This should probably use _OptionalMirror in both cases.
+  public func customMirror() -> Mirror {


Doesn't the usual enum mirror produce the same output? I don't think Optional needs anything custom anymore.

I'll investigate, but I think that you are right; there are probably a few custom mirrors that don't need to exist. I'll do an audit and see what else is redundant.

austinzheng · 2016-01-19T02:13:58Z

I updated this PR. (As part of the update I did some rebasing and force pushing; it looks like the PR is still intact.)

Slava pointed out a few things that should be addressed before this goes in. I'll also need to spin up a Ubuntu VM and run the tests there.

austinzheng · 2016-01-19T06:55:21Z

I removed a few more GYB files. I left GYB mirror implementations when it 'made sense' - e.g. there was no good place to put the code otherwise, since a type's entire implementation was in a GYB file, or moving the mirrors out of the GYB would have caused significant repetition.

austinzheng · 2016-01-20T07:38:38Z

I pushed an update.

CGRect, CGPoint, and CGSize now conform to CustomDebugStringConvertible.

dump() now has a new technique for printing a description of the item being dumped, which is designed to mirror the old behavior:

If the mirror display style is one of the following aggregate kinds, the description is in terms of number of children. (This relies on the idea that a collection's reflected children are synonymous with the items it contains.)
- Collections are described like so: 5 elements
- Dictionarys are described like so 5 key/value pairs
- Sets are described like so: 5 members
- Tuples are described like so: (5 elements)
If the object conforms to CustomStringConvertible, CustomDebugStringConvertible, or Streamable, the object is allowed to describe itself.
If the object does not conform and has a display style of Class, Struct, or Enum, the description is the fully-qualified type name + the enum case name (if applicable).
Otherwise, the description is generated using _adHocPrint().

print(), debugPrint(), and all other APIs are unaffected by these changes.

I figured the work to make the above possible was simple enough that it didn't need to be discussed extensively before an initial implementation. If anyone thinks that the algorithm above should be changed, I'm happy to update the PR.

Unfortunately, the regression that @slavapestov mentioned in a line comment (involving <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0) falls out of the rules above: all NSObjects have a description or a debugDescription, and it defaults to something like the mangled type name + address.

Many of the test changes that were in previous versions of this PR have been removed, since they are no longer necessary.

Also of note:

Optional/IUO no longer has a custom mirror
I re-enabled the unit tests that were disabled because of the dependent bugs. However, I'm still working on making sure the bugs are truly gone and won't cause the tests to fail again.
There are a few changes to Reflection.mm, which should be looked over.

dabrahams · 2016-01-20T16:33:57Z

stdlib/public/runtime/Reflection.mm

+      new (result) String("(ErrorType Object)");
+      break;
+  }
+}


Seems like swift_OpaqueSummary could be simplified by just returning a
const char* and letting the conversion happen in Swift code

dabrahams · 2016-01-20T16:35:40Z

on Tue Jan 19 2016, Austin Zheng <notifications-AT-i.8713187.xyz> wrote:

Unfortunately, the regression that @slavapestov mentioned in a line
comment (involving <_TtGC4main3FooCSo8NSString_: {{0x[0-9a-f]+}}> #0) falls out of the rules above: all NSObjects have a
description or a debugDescription, and it defaults to something
like the mangled type name + address.

Through what path were we getting a sensible result in these cases
before? I don't see any mirrors or summary properties in that file...

Many of the test changes that were in previous versions of this PR
have been removed, since they are no longer necessary.

Also of note:

Optional/IUO no longer has a custom mirror

The less code, the mo' better.

I re-enabled the unit tests that were disabled because of the
dependent bugs. However, I'm still working on making sure the bugs are
truly gone and won't cause the tests to fail again.

Thanks for your diligence.

There are a few changes to Reflection.mm, which should be looked over.

Done, thanks!

austinzheng · 2016-01-21T00:38:54Z

I'll look into how NSObjects were printed and figure out how to replicate that behavior.

Unfortunately, although https://bugs.swift.org/browse/SR-426 has been fixed for the mini test case I wrote up, the unit test that prompted me to file it still fails. I'm going to have to dig into my new dump() tonight to figure out what is going on, and possibly file a new bug.

austinzheng · 2016-01-21T09:55:56Z

I pushed another update. All unit and validation tests pass on OS X now.

Changes:

Rebased onto latest master.
Fixed a memory leak issue with printing the name of an enum case.
Changed the 'opaque summary' C++ function to return a const char *.
Minor improvement to dump(); the Mirror is no longer needlessly created twice (once in dump() itself, once in _dumpPrint()).
Removed some unused code that I had added in previously.

I also did some digging into the ugly Objective-C description. In the existing code, the dump() description is printed out using, roughly, _reflect(value).summary. When _reflect() is called on a class instance, it generates a _ClassMirror, whose summary property is just the type name.

The new dump() function tries to use one of description, debugDescription, or writeTo() to print out the dumped object's description, and only falls back to the type name if none of those exist.

Let me know if you have any questions.

dabrahams · 2016-01-21T15:43:47Z

stdlib/public/common/LeafMirrorConformance.gyb

@@ -0,0 +1,22 @@
+%{


Can we sink this file into stdlib/public/core/Mirrors.swift.gyb so we don't have to do that parseTemplate/executeTemplate dance?

Note: on the whole, this work is a thing of beauty that I'm dyin' to accept!

…te dump() Jira: SR-88 Changes: - Removed stdlib type conformances to _Reflectable - Conformed stdlib types to CustomReflectable, CustomPlaygroundQuickLookable - Rewrote dump() function to not use _reflect() - CGRect, CGPoint, CGSize now conform to CustomDebugStringConvertible - Rewrote unit tests for compatibility with new API

austinzheng · 2016-01-21T18:12:41Z

Good call. Got rid of Mirrors.swift.gyb, rebased onto latest master. All tests continue to pass. I'll spin up a Ubuntu VM today and put Swift on it so I can run the suite there, wish me luck.

Let me know if you see any other issues or want any other changes!

dabrahams · 2016-01-21T19:41:32Z

on Thu Jan 21 2016, Austin Zheng <notifications-AT-i.8713187.xyz> wrote:

Good call. Got rid of Mirrors.swift.gyb, rebased onto latest
master. All tests continue to pass. I'll spin up a Ubuntu VM today and
put Swift on it so I can run the suite there, wish me luck.

If we had public CI infrastructure I'd just merge it now, but since
you're willing to do this testing I think I'll wait as it reduces risk.
Please let me know how it turns out... and good luck!

austinzheng · 2016-01-22T02:59:37Z

It looks like everything is nominal on Ubuntu:

Migrate mirrors to use CustomReflectable API, rewrite dump() ...and there was much rejoicing.

rudkx · 2016-01-22T16:45:43Z

I had to revert this in 8917eb0. It looks like it's breaking the stdlib build, at least when building with 'build-script -r'.

swift/stdlib/public/core/Arrays.swift.gyb:787:12: error: cannot invoke initializer for type 'Mirror' with an argument list of type '(ContiguousArray, children: ArrayTypeMirrorCollection<>, displayStyle: _)'

austinzheng · 2016-01-22T16:56:09Z

Thanks for bringing this to my attention. I'll try building tonight with the '-r' flag to troubleshoot the issue.

Austtin

Sent from my iPhone

On Jan 22, 2016, at 8:47 AM, Mark Lacey [email protected] wrote:

I had to revert this in 8917eb0. It looks like it's breaking the stdlib build, at least when building with 'build-script -r'.

swift/stdlib/public/core/Arrays.swift.gyb:787:12: error: cannot invoke initializer for type 'Mirror' with an argument list of type '(ContiguousArray, children: ArrayTypeMirrorCollection<>, displayStyle: _)'

—
Reply to this email directly or view it on GitHub.

austinzheng · 2016-01-22T23:35:43Z

I determined why it broke (although I still need to root cause it).

For some reason my master got out of sync with upstream's master. My branch's parent was my out-of-date master. One of the diffs between the upstream master and the out-of-date old master changed something that caused my code to become invalid.

I'll troubleshoot the exact cause and open a new PR tonight.

dabrahams · 2016-01-22T23:38:29Z

Thanks, Austin!

rudkx · 2016-01-23T00:16:58Z

Awesome. I suspected something like that might have been the cause. Our commit rate is pretty high so it's not hard to end up in a situation like that (in fact it happened with a pair of commits today that came in at about the same time!).

austinzheng · 2016-01-23T05:30:10Z

@rudkx Thanks for catching the issue and reverting the commit. I'll look into how I can make my workflow less prone to this sort of thing.

austinzheng reviewed Jan 1, 2016
View reviewed changes

dabrahams reviewed Jan 6, 2016
View reviewed changes

slavapestov reviewed Jan 10, 2016
View reviewed changes

austinzheng force-pushed the az-port-mirror branch from 4eb1341 to a4d678f Compare January 19, 2016 02:03

austinzheng force-pushed the az-port-mirror branch from a4d678f to 2aa2705 Compare January 20, 2016 07:22

dabrahams reviewed Jan 20, 2016
View reviewed changes

austinzheng force-pushed the az-port-mirror branch from 2aa2705 to 5466797 Compare January 21, 2016 09:47

dabrahams reviewed Jan 21, 2016
View reviewed changes

austinzheng force-pushed the az-port-mirror branch from 5466797 to 9798dfd Compare January 21, 2016 18:07

dabrahams pushed a commit that referenced this pull request Jan 22, 2016

Merge pull request #838 from austinzheng/az-port-mirror

d72d808

Migrate mirrors to use CustomReflectable API, rewrite dump() ...and there was much rejoicing.

dabrahams merged commit d72d808 into swiftlang:master Jan 22, 2016

austinzheng mentioned this pull request Jan 23, 2016

[SR-88] Fixing and reapplying stdlib mirror API changes #1058

Merged

swift-ci mentioned this pull request Jan 1, 2016

[SR-88] Remove old mirrors #42710

Closed

Migrate mirrors to use CustomReflectable API, rewrite dump() #838

Migrate mirrors to use CustomReflectable API, rewrite dump() #838

Uh oh!

Conversation

austinzheng commented Jan 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabrahams commented Jan 1, 2016

Uh oh!

austinzheng commented Jan 1, 2016

Uh oh!

austinzheng commented Jan 1, 2016

Uh oh!

dabrahams commented Jan 2, 2016

Uh oh!

dabrahams commented Jan 2, 2016

Uh oh!

austinzheng commented Jan 2, 2016

Uh oh!

austinzheng commented Jan 2, 2016

Uh oh!

dabrahams commented Jan 2, 2016

Uh oh!

dabrahams commented Jan 2, 2016

Uh oh!

austinzheng commented Jan 2, 2016

Uh oh!

austinzheng commented Jan 2, 2016

Uh oh!

dabrahams commented Jan 2, 2016

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!

austinzheng commented Jan 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austinzheng commented Jan 19, 2016

Uh oh!

austinzheng commented Jan 19, 2016

Uh oh!

austinzheng commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabrahams commented Jan 20, 2016

Uh oh!

austinzheng commented Jan 21, 2016

Uh oh!

austinzheng commented Jan 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austinzheng commented Jan 21, 2016

Uh oh!

dabrahams commented Jan 21, 2016

Uh oh!

austinzheng commented Jan 22, 2016

Uh oh!

rudkx commented Jan 22, 2016

Uh oh!

austinzheng commented Jan 22, 2016

Uh oh!

austinzheng commented Jan 22, 2016

Uh oh!

dabrahams commented Jan 22, 2016

Uh oh!

rudkx commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 23, 2016

Uh oh!