Add CustomDebugDescription conformance to AnyKeyPath #60133

benpious · 2022-07-19T16:47:35Z

Implementation of this pitch, adds a conformance to CustomDebugDescription to AnyKeyPath. This will need an SE before it's merged.

I haven't modified api-digester yet, which I think I need to do...? I intended to see what the CI says first.

benpious · 2022-07-19T16:58:33Z

SE pull request: swiftlang/swift-evolution#1724

@jckarter tagging you here per the "how to contribute" doc in the swift compiler repo, as I assume you'll want to review this at some point

jckarter · 2022-07-22T16:39:29Z

@swift-ci Please test

Azoy

Welcome! This looks pretty good! I just have a few nitpicks to hopefully make things a little easier to read. In general, the Swift project uses 2 space indentation instead of 4 both in the runtime and stdlib.

stdlib/public/runtime/ReflectionMirror.cpp

lib/Demangling/NodePrinter.cpp

stdlib/public/runtime/ReflectionMirror.cpp

Azoy · 2022-08-02T23:49:11Z

stdlib/public/core/KeyPath.swift

+                    }
+                    return _openExistential(base, do: project)
+                }
+                func addChain() {


I think you can replace this with a single description.append(".") at the beginning of the loop.

My first version of this function was implemented like that, but unfortunately I don't think it works. Optionals require some special handling, otherwise we end up with stuff like \Theme.background.?alpha.

then I think you can it at the end of the loop and do a description.removeLast() once you're outside it.

It wasn't quite that simple, but the suggestion gave me an idea for a different impl which I think addresses this.

stdlib/public/core/KeyPath.swift

Azoy · 2022-08-02T23:58:25Z

stdlib/public/core/KeyPath.swift

+@_silgen_name("swift_keypath_dladdr")
+internal func keypath_dladdr(_: UnsafeRawPointer) -> UnsafePointer<CChar>?
+
+@_silgen_name("swift_keypathSourceString")
+internal func demangle(
+    name: UnsafePointer<CChar>
+) -> UnsafePointer<CChar>?
+
+
+fileprivate func dynamicLibraryAddress<Base, Leaf>(
+    of pointer: ComputedAccessorsPtr,
+    _: Base.Type,
+    _ leaf: Leaf.Type
+) -> String {
+    let getter: ComputedAccessorsPtr.Getter<Base, Leaf> = pointer.getter()
+    let pointer = unsafeBitCast(getter, to: UnsafeRawPointer.self)
+    if let cString = keypath_dladdr(UnsafeRawPointer(pointer)),
+        let demangled = demangle(name: cString) {
+        defer {
+            demangled.deallocate()
+        }
+        return String(cString: demangled)
+    } else {
+        return "<computed \(pointer) (\(leaf))>"
+    }
+}


I think all of this and the actual implementation in debugDescription should be inside a #if SWIFT_ENABLE_REFLECTION.

I just want to confirm, if SWIFT_ENABLE_REFLECTION is off, we should print "(value cannot be printed without reflection)", right?

Azoy · 2022-08-03T02:58:42Z

Oh, could it be possible to add the types in the subscript case? \Foo.subscript(_: Int, bar: String) reads very nicely (I don't remember if the type is fully realized in the demangling or if you can get a simple name for it).

benpious · 2022-08-07T01:56:46Z

Welcome! This looks pretty good! I just have a few nitpicks to hopefully make things a little easier to read. In general, the Swift project uses 2 space indentation instead of 4 both in the runtime and stdlib.

Thanks!

I believe the issues with indentation should be addressed at this point.

Oh, could it be possible to add the types in the subscript case? \Foo.subscript(_: Int, bar: String) reads very nicely (I don't remember if the type is fully realized in the demangling or if you can get a simple name for it).

Yes! I didn't notice this earlier, but the type is there based on the output of xcrun swift-demangle --tree-only, and I'm sure I can find a way to get it out. Actually, I think we might be able to combine this with _typeByName to get the actual arguments that were passed to the subscript, which could read even better in the case of a simple integer, or a lot worse if it's a very complex object being used.

benpious · 2022-08-22T04:18:01Z

@Azoy I believe that all of the issues you raised have been addressed.

I've been having issues building the full toolchain so I'm not sure if my changes to the api digester worked, but I do believe that the test failures on Windows and Linux should also be resolved.

xwu · 2022-08-31T04:32:00Z

@swift-ci test

xwu

One of the Linux CI failures appears unrelated, but the other one is real.

test/Interpreter/keypath.swift

test/api-digester/stability-stdlib-abi-without-asserts.test

benpious · 2022-08-31T18:26:27Z

One of the Linux CI failures appears unrelated, but the other one is real.

The Windows one also seemed like an infra issue to me when I saw it last night. For my context, is it common for the CI to be flaky?

benpious · 2022-09-01T04:50:53Z

@xwu I believe I've addressed all of your comments.

One thing I wanted to flag: currently, the availability of the new conformance is set to 5.7. Is there any chance that this makes it into 5.7, or should I change that to some other version?

xwu · 2022-09-01T05:39:46Z

Thanks!
@swift-ci test

xwu · 2022-09-01T05:41:59Z

One thing I wanted to flag: currently, the availability of the new conformance is set to 5.7. Is there any chance that this makes it into 5.7, or should I change that to some other version?

Ah, thanks for flagging this: we are past making it into 5.7, but in any case the availability version for newly added things on the main branch should be 5.8 (which expands to OS versions 9999). Let's let the CI run first though.

benpious · 2022-09-01T16:43:08Z

One thing I wanted to flag: currently, the availability of the new conformance is set to 5.7. Is there any chance that this makes it into 5.7, or should I change that to some other version?

Ah, thanks for flagging this: we are past making it into 5.7, but in any case the availability version for newly added things on the main branch should be 5.8 (which expands to OS versions 9999). Let's let the CI run first though.

I will change to 5.8.

Looks like CI failed the API digester test on MacOS. Not sure where to go from here in terms of debugging that. Linux looks like it might be the same (presumably spurious) error from before, but I'm not sure. Windows passed though!

xwu · 2022-09-01T22:38:44Z

Wooo about the passing Windows CI.

Yes I'm stumped by the API digester warning; it seems to me to be a false positive but I don't want to be hasty in suggesting that we just add it to the list and call it aday; perhaps @Azoy can help here to verify.

xwu · 2022-09-01T22:39:20Z

@swift-ci test Linux platform

Azoy · 2022-09-01T22:45:05Z

stdlib/public/core/KeyPath.swift

+
+  @available(SwiftStdLib 5.7, *)
+  public var debugDescription: String {
+#if SWIFT_ENABLE_REFLECTION


This works, but I think the rest of the stdlib has used the style of:

#if SWIFT_ENABLE_REFLECTION extension AnyKeyPath: CustomDebugStringConvertible { ... } #else extension AnyKeyPath: CustomDebugStringConvertible { give default message here } #endif

I've changed this to not be nested in the function, matching SourceCompatibilityShims.swift.

stdlib/public/core/KeyPath.swift

Azoy · 2022-09-01T22:47:31Z

stdlib/public/core/OutputStream.swift

@@ -638,3 +638,5 @@ internal struct _TeeStream<L: TextOutputStream, R: TextOutputStream>
  internal mutating func _lock() { left._lock(); right._lock() }
  internal mutating func _unlock() { right._unlock(); left._unlock() }
 }
+
+internal let noReflectionMetadataErrorMessage = "(value cannot be printed without reflection)"


I think we generally want to avoid globals in the stdlib, I would prefer to keep this string literal everywhere that it's used.

I will revert this. Can you explain/link to why the stdlib avoids globals? I would like to understand if the reason is exclusive to the stdlib or something that is applicable outside.

Globals have a place in the language, but I think we would reserve them for things like global caches if we had any of those (like the ones in the runtime). This is simple enough that I don't think it's worth having one.

Ah, interesting. My guess was that it would be a perf issue related to initializing the let in a thread safe way.

My goal with making this change was to prevent code duplication, but it's definitely not a particularly serious issue if this string gets copy pasted around a few times.

stdlib/public/runtime/ReflectionMirror.cpp

Azoy · 2022-09-05T00:14:34Z

@swift-ci please test

xwu · 2022-09-05T20:02:39Z

I haven't investigated deeply, but if it's the same Linux test falling again, and other CI builds on main are successful, then it's not a spurious error and probably needs some PR tweaking...

benpious · 2022-09-05T22:07:13Z

I haven't investigated deeply, but if it's the same Linux test falling again, and other CI builds on main are successful, then it's not a spurious error and probably needs some PR tweaking...

Yeah, it looks like it's this Pr.

TL:DR I think the right approach is to add these symbols to the allowlist of the test (symbol-visibility-linux.test-sh). But I would like to confirm with the author of the test (@compnerd).

Long version:

My understanding is that the test lists all symbols in a binary, then diffs that output with a list of all strong symbols in the same binary. The goal is to ensure there are no weakly linked symbols at all. But there are two weakly linked symbols in the output:

--- /home/build-user/build/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftCore-all.txt	2022-09-05 01:04:57.351384015 +0000
+++ /home/build-user/build/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftCore-no-weak.txt	2022-09-05 01:04:57.507385347 +0000
@@ -13089,8 +13089,6 @@
 000000000052bb90 D $syyXfWV
 000000000052bae0 D $syyYjrcWV
 000000000052ba88 D $syycWV
-000000000044b360 W _ZNSt6vectorINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESaIS5_EE17_M_realloc_insertIJS5_EEEvN9__gnu_cxx17__normal_iteratorIPS5_S7_EEDpOT_
-0000000000441690 W _ZStplIcSt11char_traitsIcESaIcEENSt7__cxx1112basic_stringIT_T0_T1_EERKS8_SA_
 000000000060b10b B __bss_start
 00000000003e6d60 T __gnu_f2h_ieee
 00000000003e6d20 T __gnu_h2f_ieee

Demangling these, it looks like we caused a new version of std::operator+ and some massive, vector related template thing to be created.

Anyway, I think what is happening is that I used these in the impls of some of the functions I wrote, without realizing that they're brand new templates that have never been used before. C++ is instantiating (is that the right word?) these templates, and using weak linking, for whatever reason.

The test is filled with inverted matches which appear to be an allowlist, and based on the names it seems like they might also be templates. So the obvious, quick solution is to add the two functions there and move on.

But I'd like to understand why this Pr causes this; nothing has changed in the allowlist since April. So it feels like there's something different (and potentially unidiomatic/wrong) about how I'm using C++ in this Pr.

compnerd · 2022-09-05T22:14:26Z

:sigh: libstdc++. Yes, these two instantiations should be okay to add to the allowlist. The problem here is that the GNU C++ runtime decorates the template bodies such that an instantiation will be weak as it is in global namespace and attributed so as to coalesce the copies requiring weak ODR semantics giving them weak global linkage. This is an artifact of the implementation and we cannot control that even with -fvisibility=hidden -fvisibility-inlines-hidden nor any internal macro. This also changes based upon the version of the runtime in use as these are internal details. The new code uses + on std::string and causes a std::vector<std::string> to potentially be resized when inserting (plausibly a push_back).

benpious · 2022-09-05T22:28:39Z

:sigh: libstdc++. Yes, these two instantiations should be okay to add to the allowlist. The problem here is that the GNU C++ runtime decorates the template bodies such that an instantiation will be weak as it is in global namespace and attributed so as to coalesce the copies requiring weak ODR semantics giving them weak global linkage. This is an artifact of the implementation and we cannot control that even with -fvisibility=hidden -fvisibility-inlines-hidden nor any internal macro. This also changes based upon the version of the runtime in use as these are internal details. The new code uses + on std::string and causes a std::vector<std::string> to potentially be resized when inserting (plausibly a push_back).

Thanks! I've added them to the allowlist.

xwu · 2022-09-06T04:09:40Z

:sigh: libstdc++.

Are there suitably equivalent LLVM types that can be used here instead?

benpious · 2022-09-07T02:06:43Z

:sigh: libstdc++.

Are there suitably equivalent LLVM types that can be used here instead?

I don't believe so. I see two options in the llvm programmer's manual:

StringRef: this appears to be immutable, so not suitable for this use case
Twine: this initially looks promising as it's for "efficiently concatenating values together." But "its implementation relies on the ability to store pointers to temporary stack objects which may be deallocated at the end of a statement. Twines should only be used accepted as const references in arguments"

I think we could optimize the matchSequenceOfKinds function a little by switching to llvm::smallVector, because the size of the vector is known in advance. I can make that change if you like, but I'm pretty confident that it won't remove the need to modify the test.

xwu · 2022-09-07T03:48:37Z

@swift-ci test

xwu · 2022-09-07T03:52:40Z

I think we could optimize the matchSequenceOfKinds function a little by switching to llvm::smallVector, because the size of the vector is known in advance. I can make that change if you like, but I'm pretty confident that it won't remove the need to modify the test.

I'll defer to you and other reviewers :)

xwu · 2022-09-08T20:08:24Z

@swift-ci please test Linux platform

xwu · 2022-09-09T05:18:25Z

@swift-ci please test macOS platform

xwu · 2022-09-09T05:18:36Z

@swift-ci please test Windows platform

benpious · 2022-09-09T20:24:23Z

Looks like CI is green. Any other changes needed to the Pr?

xwu

Woooo, nice to see. Can you clang-format the .cpp files?

include/swift/Demangling/Demangle.h

lib/Demangling/NodePrinter.cpp

stdlib/public/runtime/ReflectionMirror.cpp

demangling mostly works fix dots printing works add tests add conformance to AnyKeyPath implement SPI subscripts fully work comments use cross platform image inspection remove unnecessary comment fix fix issues add conditional conformance add types try to fix the api-digester test cr feedback: move impls behind flag, remove addChain(), switch statement, fallthrough instead of if-elses, move import cr feedback: refactor switch statement fix #ifdef reindent, cr feedback: removes manual memory management fix missing whitespace fix typo fix indentation issues switch to regexes checks should test in on all platforms print types in subscripts add test for empty subscript Update test/api-digester/stability-stdlib-abi-without-asserts.test Co-authored-by: Xiaodi Wu <[email protected]> add commas fix failing test fix stdlib annotation cr feedback: remove global, refactor ifdef cr feedback: switch back to manual memory management switch to 5.8 macro add new weakly linked functions to the allowlist fix one more failing test more cr feedback more cr feedback

benpious · 2022-09-11T19:57:04Z

Woooo, nice to see. Can you clang-format the .cpp files?

Done.

xwu · 2022-09-12T03:54:52Z

@swift-ci test

xwu · 2022-09-12T03:56:07Z

@Azoy How's it look to you?

Azoy · 2022-09-13T16:24:04Z

Thanks so much! Super excited this is in finally.

benpious · 2022-09-24T17:52:35Z

Thanks so much! Super excited this is in finally.

Me too! Thanks @Azoy @xwu and @compnerd for all the help getting this landed!

…rios anymore The file check expectations (the way we print stuff) is different after swiftlang#60133. rdar://100564676

theblixguy added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Jul 19, 2022

Azoy reviewed Aug 2, 2022

View reviewed changes

xwu added swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process and removed swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review labels Aug 31, 2022

xwu requested a review from Azoy August 31, 2022 04:32

xwu requested changes Aug 31, 2022

View reviewed changes

test/Interpreter/keypath.swift Outdated Show resolved Hide resolved

test/Interpreter/keypath.swift Outdated Show resolved Hide resolved

xwu reviewed Aug 31, 2022

View reviewed changes

test/api-digester/stability-stdlib-abi-without-asserts.test Show resolved Hide resolved

initial

9affc33

benpious force-pushed the add-debug-description-to-keypath branch 2 times, most recently from 77b36b6 to cc9ca5e Compare September 1, 2022 04:43

Azoy reviewed Sep 1, 2022

View reviewed changes

xwu approved these changes Sep 4, 2022

View reviewed changes

xwu self-requested a review September 4, 2022 18:27

xwu reviewed Sep 9, 2022

View reviewed changes

include/swift/Demangling/Demangle.h Outdated Show resolved Hide resolved

lib/Demangling/NodePrinter.cpp Outdated Show resolved Hide resolved

stdlib/public/runtime/ReflectionMirror.cpp Outdated Show resolved Hide resolved

benpious force-pushed the add-debug-description-to-keypath branch from 2fa6f99 to fe42b28 Compare September 11, 2022 19:54

fix invisible unicode

d37df2d

Azoy approved these changes Sep 13, 2022

View reviewed changes

Azoy merged commit 57d8231 into swiftlang:main Sep 13, 2022

aschwaighofer mentioned this pull request Sep 29, 2022

test/Interpreter/keypath.swift does not work in back-deployment scenarios anymore #61361

Merged

mikeash mentioned this pull request Dec 12, 2022

[Runtime] Fix debugDescription of .self keypaths. #62528

Merged

Add CustomDebugDescription conformance to AnyKeyPath #60133

Add CustomDebugDescription conformance to AnyKeyPath #60133

Uh oh!

Conversation

benpious commented Jul 19, 2022

Uh oh!

benpious commented Jul 19, 2022

Uh oh!

jckarter commented Jul 22, 2022

Uh oh!

Azoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Azoy commented Aug 3, 2022

Uh oh!

benpious commented Aug 7, 2022

Uh oh!

benpious commented Aug 22, 2022

Uh oh!

xwu commented Aug 31, 2022

Uh oh!

xwu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benpious commented Aug 31, 2022

Uh oh!

benpious commented Sep 1, 2022

Uh oh!

xwu commented Sep 1, 2022

Uh oh!

xwu commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpious commented Sep 1, 2022

Uh oh!

xwu commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwu commented Sep 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Azoy commented Sep 5, 2022

Uh oh!

xwu commented Sep 5, 2022

xwu commented Sep 1, 2022 •

edited

Loading

xwu commented Sep 1, 2022 •

edited

Loading

benpious commented Sep 5, 2022 •

edited

Loading