[Runtime] Remove old-style mirrors in ReflectionLegacy.swift. #14678

mikeash · 2018-02-16T16:20:37Z

The old-style mirrors were the basis for non-custom, reflection based Mirrors. As part of removing old-style mirrors, reflection based Mirrors are reimplemented without them. Reflection.mm is deleted and replaced with ReflectionMirror.mm, which borrows a chunk of the old code but presents a simpler API to the Swift side. ReflectionMirror.swift then uses that API to implement a reflection-based Mirror init.

rdar://problem/20356017

mikeash · 2018-02-16T16:20:46Z

@swift-ci please test

swift-ci · 2018-02-16T17:37:08Z

Build failed
Swift Test OS X Platform
Git Sha - ed9631f8b688a55fb69f55b47119af13826cfb26

DougGregor

This looks great. -1,000 woo hoo!

DougGregor · 2018-02-16T17:15:46Z

stdlib/public/runtime/ReflectionMirror.mm

+@end
+
+// mangled Swift._SwiftObject
+#define SwiftObject _TtCs12_SwiftObject


You can get this #define from SwiftObject.h

DougGregor · 2018-02-16T17:16:35Z

stdlib/public/core/ReflectionMirror.swift

+  					  subjectType: Any.Type? = nil,
+  					  customAncestor: Mirror? = nil)
+	{
+		let subjectType = subjectType ?? _getNormalizedType(subject, type: type(of: subject))


Hate to make this my first comment, but... spaces rather than tabs please, because GitHub's rendering mangles the layout.

DougGregor · 2018-02-16T17:16:55Z

stdlib/public/core/ReflectionMirror.swift

+         let superclass = _getSuperclass(subjectClass) {
+        // Handle custom ancestors. If we've hit the custom ancestor's subject type,
+        // or descendants are suppressed, return it. Otherwise continue reflecting.
+				if let customAncestor = customAncestor {


DougGregor · 2018-02-16T17:28:48Z

stdlib/public/runtime/ReflectionMirror.mm

+      swift::crash("Swift mirror subscript bounds check failure");
+
+    // Load the type and offset from their respective vectors.
+    auto fieldType = Struct->getFieldTypes()[i];


Hmm, this is going to conflict with @xedin 's current work in #13926, which eliminates the field-type vector. There will be an accessor of some sort, so the conflict resolution should be simple either way.

I think it shouldn't be too bad :)

DougGregor · 2018-02-16T17:39:03Z

stdlib/public/runtime/ReflectionMirror.mm

+/// \returns the demangled name. Returns nullptr if the input String is not a
+/// Swift mangled name.
+SWIFT_RUNTIME_EXPORT
+char *swift_demangle(const char *mangledName,


It's so weird to have this function in this file. swift_demangle is a reasonable entrypoint to export from the runtime, but it's not specific to reflection at all. Maybe we can move it elsewhere?

Agreed. It's there pretty much because it used to be in Reflection.mm and I mindlessly copied it over. I've now moved it to Demangle.cpp, which seems more appropriate.

DougGregor · 2018-02-16T17:39:52Z

stdlib/public/runtime/ReflectionMirror.mm

+                     size_t *outputBufferSize,
+                     uint32_t flags) {
+  if (flags != 0) {
+    swift::fatalError(0, "Only 'flags' value of '0' is currently supported.");


I don't think we should fatalError here, because it provides no backward compatibility for this entrypoint. We should ignore flags we don't understand, not fail on them.

DougGregor · 2018-02-16T17:41:30Z

stdlib/public/core/ReflectionMirror.swift

+@_inlineable // FIXME(sil-serialize-all)
+@_versioned // FIXME(sil-serialize-all)
+@_silgen_name("swift_reflectionMirror_displayStyle")
+internal func _getDisplayStyle<T>(_: T) -> CChar


I was going to grumble about the use of magic character constants here, but I see that there's one caller, and exposing an enum via SwiftShims just for this purpose seems.. excessive.

That was pretty much my thought process as well. Magic constants aren't the best, but it seems like the right solution in this particular case.

The old-style mirrors were the basis for non-custom, reflection based Mirrors. As part of removing old-style mirrors, reflection based Mirrors are reimplemented without them. Reflection.mm is deleted and replaced with ReflectionMirror.mm, which borrows a chunk of the old code but presents a simpler API to the Swift side. ReflectionMirror.swift then uses that API to implement a reflection-based Mirror init. rdar://problem/20356017

gottesmm · 2018-02-16T21:15:30Z

@mikeash I haven't completely reviewed this, but can you in the C++ code be sure to update it so that the conventions work at +1 or +0 for functions that have swift calling convention. If you look at the old legacy reflection c++, you will see that I did that in a bunch of places using a few different macros, i.e.: SWIFT_CC_PLUS{ZERO,ONE}_GUARD, to guard retain/release calls appropriately.

…SwiftObject.h.

mikeash · 2018-02-16T21:28:28Z

@gottesmm If I've understood everything correctly (which is definitely not a given, so please correct me where I've gotten it wrong), that's not necessary here. The old code tried to keep a persistent interior pointer alive with an owner object, which got passed around everywhere and thus had to be treated differently with +1 or +0 semantics. The new code doesn't try to keep an interior pointer around, and works on generic OpaqueValues everywhere.

…there.

gottesmm · 2018-02-17T00:03:03Z

Awesome! Thanks!

mikeash · 2018-02-17T01:27:12Z

@swift-ci please test

swift-ci · 2018-02-17T01:27:40Z

Build failed
Swift Test OS X Platform
Git Sha - ed9631f8b688a55fb69f55b47119af13826cfb26

swift-ci · 2018-02-17T01:28:49Z

Build failed
Swift Test Linux Platform
Git Sha - ed9631f8b688a55fb69f55b47119af13826cfb26

mikeash · 2018-02-19T14:57:40Z

@swift-ci please test

swift-ci · 2018-02-19T14:58:09Z

Build failed
Swift Test OS X Platform
Git Sha - 44d9b2f

swift-ci · 2018-02-19T14:59:08Z

Build failed
Swift Test Linux Platform
Git Sha - 44d9b2f

…an blindly casting to ClassMetada, which turns out to be completely incorrect.

mikeash · 2018-02-20T20:16:28Z

@swift-ci please test

swift-ci · 2018-02-20T20:18:00Z

Build failed
Swift Test OS X Platform
Git Sha - fa2ea03

swift-ci · 2018-02-20T20:18:04Z

Build failed
Swift Test Linux Platform
Git Sha - fa2ea03

gparker42 · 2018-02-20T21:45:22Z

stdlib/public/runtime/Demangle.cpp

+//// define what these will be.
+/// \returns the demangled name. Returns nullptr if the input String is not a
+/// Swift mangled name.
+SWIFT_RUNTIME_EXPORT


SWIFT_RUNTIME_EXPORT is for declarations. This definition should not use it.

I gather that a declaration for this function wasn't desired because it's not used by the other Swift code. Should we stick a declaration in the header anyway? I don't think there's anything that says it must not be used if someone decides it would be useful.

gottesmm

Comments around +0 and some general style comments.

gottesmm · 2018-02-21T18:13:37Z

stdlib/public/core/ReflectionMirror.swift

+internal func _getClassPlaygroundQuickLook(
+  _ object: AnyObject
+) -> PlaygroundQuickLook? {
+  if _is(object, kindOf: "NSNumber") {


This whole function should not use else-if chains. Please use early-exits.

gottesmm · 2018-02-21T18:14:10Z

stdlib/public/core/ReflectionMirror.swift

+        return Mirror(internalReflecting: subject,
+                      subjectType: superclass,
+                      customAncestor: customAncestor)
+      } else {


Why not move the return nil early into a guard statement?

gottesmm · 2018-02-21T18:15:25Z

stdlib/public/runtime/Demangle.cpp

+
+  // If the output buffer is not provided, malloc memory ourselves.
+  if (outputBuffer == nullptr || *outputBufferSize == 0) {
+    return strdup(result.c_str());


I haven't read too deeply here, but isn't strdup one of those unsafe APIs to use? Shouldn't you be using strndup? Not sure. I guess you are getting input from the demangler so it isn't unsafe input.

It's OK here. c_str is guaranteed to return a NUL terminated string. strndup would be useful with a parameter where the NUL terminator may not be present.

gottesmm · 2018-02-21T18:16:54Z

stdlib/public/runtime/ReflectionMirror.mm

+  // - the witness table for Protocol1
+  // - the witness table for Protocol2
+
+  auto weakContainer =


Can you put a * next to the auto to show it is a pointer?

gottesmm · 2018-02-21T18:18:01Z

stdlib/public/runtime/ReflectionMirror.mm

+}
+
+// Get a field name from a doubly-null-terminated list.
+static const char *getFieldName(const char *fieldNames, size_t i) {


Generally in LLVM style we just use unsigned instead of size_t. But if people are using size_t in the runtime, better to be consistent. I would see what people are using.

gottesmm · 2018-02-21T18:19:08Z

stdlib/public/runtime/ReflectionMirror.mm

+
+    // Get the nth element.
+    auto &elt = Tuple->getElement(i);
+    auto bytes = reinterpret_cast<const char*>(value);


You missed the space between const char and *.

gottesmm · 2018-02-21T18:19:55Z

stdlib/public/runtime/ReflectionMirror.mm

+    auto fieldType = Struct->getFieldTypes()[i];
+    auto fieldOffset = Struct->getFieldOffsets()[i];
+
+    auto bytes = reinterpret_cast<char*>(value);


Another place that deviates from the pattern of ptr *

gottesmm · 2018-02-21T18:20:38Z

stdlib/public/runtime/ReflectionMirror.mm

+  }
+
+  void getInfo(unsigned *tagPtr, const Metadata **payloadTypePtr, bool *indirectPtr) {
+    const auto Enum = static_cast<const EnumMetadata *>(type);


const auto Enum => const auto *Enum

gottesmm · 2018-02-21T18:22:21Z

stdlib/public/runtime/ReflectionMirror.mm

+    const Metadata *boxType = (indirect ? &METADATA_SYM(Bo).base : payloadType);
+    BoxPair pair = swift_allocBox(boxType);
+
+    type->vw_destructiveProjectEnumData(const_cast<OpaqueValue *>(value));


In a +0 context (unless mirrors copy value always), this worries me. I think you may need to perform a full copy of value to be safe.

gottesmm · 2018-02-21T18:24:09Z

stdlib/public/runtime/ReflectionMirror.mm

+    impl->type = type;
+    impl->value = value;
+    auto result = f(impl);
+    T->vw_destroy(passedValue);


This should be guarded by SWIFT_CC_PLUSONE_GUARD.

rdar://problem/20356017

…it when +0 is enabled. rdar://problem/20356017

… export it. rdar://problem/20356017

mikeash · 2018-02-22T17:09:50Z

@swift-ci please test

swift-ci · 2018-02-22T17:11:33Z

Build failed
Swift Test OS X Platform
Git Sha - 0cff498

swift-ci · 2018-02-22T17:12:02Z

Build failed
Swift Test Linux Platform
Git Sha - 0cff498

mikeash · 2018-02-22T20:01:12Z

@swift-ci please test

swift-ci · 2018-02-22T20:01:38Z

Build failed
Swift Test OS X Platform
Git Sha - 03476e9

swift-ci · 2018-02-22T20:11:04Z

Build failed
Swift Test Linux Platform
Git Sha - 03476e9

johnno1962 · 2018-02-22T22:41:15Z

D'oh https://github.com/johnno1962/TwoWayMirror .. Oh well, It was fun while it lasted (2 days) 😁

mikeash · 2018-02-23T14:53:04Z

Oh no! Sorry. 😿

Once the metadata format stabilizes, it should be possible to do this robustly and without the risk of breaking when people like me start slashing and burning old stuff. For whatever that's worth.

johnno1962 · 2018-02-23T15:33:19Z

My fault for using apis with “Legacy” stamped all over it. I’ll just have to be more ingenious next time ;)

… Mirror reimplementation in swiftlang#14678. rdar://problem/20356017

DougGregor approved these changes Feb 16, 2018

View reviewed changes

mikeash force-pushed the remove-reflectionlegacy branch from ed9631f to 4045edf Compare February 16, 2018 21:05

[Runtime] Have ReflectionMirror.mm get SwiftObject declarations from …

3399058

…SwiftObject.h.

[Runtime] Move swift_demangle to Demangle.cpp. It seems more at home …

44d9b2f

…there.

Merge branch 'master' into remove-reflectionlegacy

fa2ea03

[Reflection] Use getClassObject() when testing for ObjCness rather th…

0cff498

…an blindly casting to ClassMetada, which turns out to be completely incorrect.

gparker42 reviewed Feb 20, 2018

View reviewed changes

gottesmm reviewed Feb 21, 2018

View reviewed changes

mikeash added 3 commits February 21, 2018 14:32

[Runtime] Style fixes in new ReflectionMirror code.

29eb740

rdar://problem/20356017

[Runtime] Guard ReflectionMirror's vw_destroy call to avoid emitting …

50697a7

…it when +0 is enabled. rdar://problem/20356017

[Runtime] Add a declaration for swift_demangle to the header file and…

c250a84

… export it. rdar://problem/20356017

mikeash force-pushed the remove-reflectionlegacy branch 2 times, most recently from af33c06 to c250a84 Compare February 22, 2018 01:07

Merge branch 'master' into remove-reflectionlegacy

03476e9

Merge branch 'master' into remove-reflectionlegacy

35e4d1e

mikeash merged commit 2f810b4 into swiftlang:master Feb 22, 2018

johnno1962 added a commit to johnno1962/TwoWayMirror that referenced this pull request Feb 27, 2018

Metedata post swiftlang/swift#14678

1a04f5f

johnno1962 added a commit to johnno1962/TwoWayMirror that referenced this pull request Feb 27, 2018

Metedata post swiftlang/swift#14678

e5354c0

johnno1962 added a commit to johnno1962/TwoWayMirror that referenced this pull request Feb 27, 2018

Metedata post swiftlang/swift#14678

429028a

mikeash added a commit to mikeash/swift that referenced this pull request Mar 7, 2018

[Runtime] Remove ObjCMirrors.swift, which is no longer used after the…

0e163c2

… Mirror reimplementation in swiftlang#14678. rdar://problem/20356017

mikeash mentioned this pull request Mar 7, 2018

[Runtime] Remove ObjCMirrors.swift #15044

Merged

[Runtime] Remove old-style mirrors in ReflectionLegacy.swift. #14678

[Runtime] Remove old-style mirrors in ReflectionLegacy.swift. #14678

Uh oh!

Conversation

mikeash commented Feb 16, 2018

Uh oh!

mikeash commented Feb 16, 2018

Uh oh!

swift-ci commented Feb 16, 2018

Uh oh!

DougGregor left a comment

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!

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!

gottesmm commented Feb 16, 2018

Uh oh!

mikeash commented Feb 16, 2018

Uh oh!

gottesmm commented Feb 17, 2018

Uh oh!

mikeash commented Feb 17, 2018

Uh oh!

swift-ci commented Feb 17, 2018

Uh oh!

swift-ci commented Feb 17, 2018

Uh oh!

mikeash commented Feb 19, 2018

Uh oh!

swift-ci commented Feb 19, 2018

Uh oh!

swift-ci commented Feb 19, 2018

Uh oh!

mikeash commented Feb 20, 2018

Uh oh!

swift-ci commented Feb 20, 2018

Uh oh!

swift-ci commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

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!

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!

johnno1962 commented Feb 22, 2018 •

edited

Loading