Introduce SWIFT_ENABLE_REFLECTION to turn on/off the support for Mirrors and reflection #33617

kubamracek · 2020-08-24T21:01:12Z

No description provided.

compnerd

I'd love to see the numbers on this (if you have them) - this can be interesting for other applications.

compnerd · 2020-08-25T14:46:58Z

stdlib/public/core/Pointer.swift

 /// A stdlib-internal protocol modeled by the intrinsic pointer types,
 /// UnsafeMutablePointer, UnsafePointer, UnsafeRawPointer,
 /// UnsafeMutableRawPointer, and AutoreleasingUnsafeMutablePointer.
 public protocol _Pointer
-: Hashable, Strideable, CustomDebugStringConvertible, CustomReflectable {
+: Hashable, Strideable, CustomDebugStringConvertible, _CustomReflectableOrNone {


Can you split out the CustomReflectable into an extension instead? That avoids the need for the new protocol.

You can't actually extend protocols to require conformances to protocols, but couldn't you do:

public protocol _Pointer: protocols #if SWIFT_ENABLE_REFLECTION , CustomReflectable #endif { }

maybe the Swift preprocessor doesn't work like C/++ one to be able to do this.

Alas, Swift doesn't do blind text substitutions like C does, and the contents of an #if block must be valid statements.

Bleh, I missed that _Pointer itself was a protocol. I suppose you can use a typealias, but at that point Im not sure if its worth it.

How would you use the typealias? I'm happy to explore better options, as I don't really like the _CustomReflectableOrNone thing either, but I couldn't come up with anything nicer that would actually work (as mentioned above, we can't just ifdef this out from the declaration, and we can't do extension on a protocol).

This appears to work:

#if COND typealias MaybeCustomReflectable = CustomReflectable #else typealias MaybeCustomReflectable = Any #endif protocol P: MaybeCustomReflectable {}

Yeah, roughly what @mikeash pointed out. I was thinking that we could take the requirements and just typealias them before the definition, including/excluding the CustomReflectable instead of an Any conformance.

3405691582 · 2020-08-26T14:50:11Z

Should this include swift-reflection-dump and the associated tests, or is that not intended to be in-scope for this toggle?

jckarter · 2020-08-26T20:27:51Z

@3405691582 swift-reflection-dump could still be useful, because the standard library being built without reflection metadata and Mirror does not block user binaries from still generating and consuming reflection metadata for their own purposes.

jckarter · 2020-08-26T20:30:24Z

As we discussed offline, it would be nice if we could leave behind @availability(unavailable) stub declarations for the removed functionality. That will let us give better diagnostics when people try to use the APIs in configurations that don't support them, and would also help prevent subtle behavior differences caused by the missing declarations changing overload resolution, allowing retroactive conformances that would otherwise collide with standard library conformances, and so on.

kubamracek · 2021-08-24T01:04:48Z

@swift-ci please test

swift-ci · 2021-08-24T02:30:44Z

Build failed
Swift Test Linux Platform
Git Sha - 016ddcde5ce333c28735dc3c90b1448e6c2b2cb3

kubamracek · 2021-08-24T02:37:10Z

@swift-ci please test Linux platform

swift-ci · 2021-08-24T02:57:59Z

Build failed
Swift Test Linux Platform
Git Sha - 016ddcde5ce333c28735dc3c90b1448e6c2b2cb3

swift-ci · 2021-08-24T03:34:55Z

Build failed
Swift Test OS X Platform
Git Sha - 016ddcde5ce333c28735dc3c90b1448e6c2b2cb3

kubamracek · 2021-08-24T14:48:20Z

@swift-ci please test

swift-ci · 2021-08-24T16:12:16Z

Build failed
Swift Test Linux Platform
Git Sha - 016ddcde5ce333c28735dc3c90b1448e6c2b2cb3

swift-ci · 2021-08-24T16:31:41Z

Build failed
Swift Test OS X Platform
Git Sha - 016ddcde5ce333c28735dc3c90b1448e6c2b2cb3

kubamracek · 2021-08-24T20:38:44Z

@swift-ci please test

shahmishal · 2021-08-24T21:30:13Z

Errors Detected: Regression test failed

Failed Command

/usr/local/bin/cmake --build /Users/buildnode/jenkins/workspace/swift-PR-macos/branch-main/buildbot_incremental/swift-macosx-x86_64 -- -j24 -v check-swift-validation-macosx-x86_64

 Swift(macosx-x86_64) :: Incremental/Dependencies/reference-dependencies-consistency-fine.swift 
 Swift(macosx-x86_64) :: Incremental/Verifier/multi-file-private/main.swift

Build Log: Swift Pull Request Test macOS

swift-ci · 2021-08-24T23:29:09Z

Build failed
Swift Test Linux Platform
Git Sha - 42d25595fa6bbff909ea70451cb7c4325c4717f6

swift-ci · 2021-08-25T00:51:31Z

Build failed
Swift Test OS X Platform
Git Sha - 42d25595fa6bbff909ea70451cb7c4325c4717f6

kubamracek · 2021-08-25T02:28:18Z

@swift-ci please test

kubamracek · 2021-08-25T21:19:30Z

@yln

The patch makes sense to me, but I was a bit disheartened by the sheer number of #if SWIFT_ENABLE_REFLECTION directives needed (77 files touched).

I guess I could gather up most/all of the simple CustomReflectable and CustomDebugStringConvertible extensions on Array, Collection, Dictionary, etc. into a separate place and then it would just be a single #ifdef around that. Would that be better?

kubamracek · 2021-08-25T21:20:05Z

Would it be possible to leave Mirror declarations and uses intact, but "compile out" its implementation and return dummy values or crash. Similarly to what we do here for String.

Not sure what you're actually proposing. I think the @available attribute is required here, otherwise your code still compiles. I need to make let m = Mirror(x) not compile. Given that we can't #ifdef-out only the attribute, I'm not sure how else can I do this?

kubamracek · 2021-08-25T21:27:22Z

I'm kinda on the fence about this change. Conceptually, I think that the change is good, but practically, I think that I agree with @yln. I think that adding an extension to String and a few other core types might be nice. An example would be:

extension String {
  public init<T: Error>(reflecting object: T) {
    self = "\(object)"
  }
}

You already change a few places the case where you reflect error into the error's description. This would keep the standard library similar and reduce some of the sprinkled changes.

Added. Note sure what other case could be handled the same way... suggestions?

kubamracek · 2021-08-25T21:27:40Z

@swift-ci please test

kubamracek · 2021-08-28T00:14:39Z

I've tried exploring using @available to carve out the Mirror APIs without duplicating the declarations, perhaps based on -define-availability compiler flags, but I don't think availability system is ready for this, because it mostly works for "platforms+versions" and not really for "features", like Mirror APIs. It's definitely worth exploring extending the availability system to support this, and I'll try to figure out if we could make something like @available(reflection) work and respect a compiler flag like -disable-reflection, and have it "do the right thing", i.e. make Mirror unavailable in those cases and also make conformances to CustomStringReflectable unavailable. Then we wouldn't need the #ifs and duplication of method declarations.

But in order to make progress, I'd like to propose going forward with this current PR, i.e. using #ifs, as a path forward. Is that reasonable? @jckarter, @compnerd?

kubamracek · 2021-09-08T03:07:30Z

@swift-ci please test

swift-ci · 2021-09-08T04:26:45Z

Build failed
Swift Test Linux Platform
Git Sha - c3612e64d85d72d476609b0a4d94d288abe9a180

swift-ci · 2021-09-08T08:20:28Z

Build failed
Swift Test OS X Platform
Git Sha - c3612e64d85d72d476609b0a4d94d288abe9a180

…ors and reflection

kubamracek · 2021-09-08T14:39:09Z

@swift-ci please test

kubamracek requested review from compnerd, jckarter, mikeash and gottesmm August 24, 2020 21:01

mikeash approved these changes Aug 25, 2020

View reviewed changes

compnerd approved these changes Aug 25, 2020

View reviewed changes

kubamracek changed the base branch from master to main September 23, 2020 22:20

kubamracek force-pushed the reflection branch 3 times, most recently from cb8402a to 016ddcd Compare August 24, 2021 01:04

kubamracek force-pushed the reflection branch from 016ddcd to 42d2559 Compare August 24, 2021 20:38

kubamracek mentioned this pull request Aug 25, 2021

Turn off SWIFT_ENABLE_REFLECTION on the stdlib_minimal preset #39030

Merged

kubamracek force-pushed the reflection branch from 42d2559 to 7147ddc Compare August 25, 2021 21:25

kubamracek requested a review from kylemacomber September 8, 2021 00:45

kubamracek force-pushed the reflection branch 2 times, most recently from d9cabb3 to c3612e6 Compare September 8, 2021 01:26

Introduce SWIFT_ENABLE_REFLECTION to turn on/off the support for Mirr…

f9e52e7

…ors and reflection

kubamracek force-pushed the reflection branch from c3612e6 to f9e52e7 Compare September 8, 2021 14:38

kylemacomber approved these changes Sep 8, 2021

View reviewed changes

kubamracek merged commit 404badb into swiftlang:main Sep 8, 2021

kubamracek deleted the reflection branch September 8, 2021 20:08

Introduce SWIFT_ENABLE_REFLECTION to turn on/off the support for Mirrors and reflection #33617

Introduce SWIFT_ENABLE_REFLECTION to turn on/off the support for Mirrors and reflection #33617

Uh oh!

Conversation

kubamracek commented Aug 24, 2020

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

compnerd Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

Azoy Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

mikeash Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

compnerd Aug 25, 2020 • 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!

kubamracek Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

mikeash Aug 25, 2020 • 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!

compnerd Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

3405691582 commented Aug 26, 2020

Uh oh!

jckarter commented Aug 26, 2020

Uh oh!

jckarter commented Aug 26, 2020

Uh oh!

kubamracek commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 24, 2021

Uh oh!

kubamracek commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 24, 2021

Uh oh!

kubamracek commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 24, 2021

Uh oh!

kubamracek commented Aug 24, 2021

Uh oh!

shahmishal commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 24, 2021

Uh oh!

swift-ci commented Aug 25, 2021

Uh oh!

kubamracek commented Aug 25, 2021

Uh oh!

kubamracek commented Aug 25, 2021

Uh oh!

kubamracek commented Aug 25, 2021

Uh oh!

kubamracek commented Aug 25, 2021

Uh oh!

kubamracek commented Aug 25, 2021

Uh oh!

kubamracek commented Aug 28, 2021

Uh oh!

kubamracek commented Sep 8, 2021

Uh oh!

swift-ci commented Sep 8, 2021

Uh oh!

swift-ci commented Sep 8, 2021

Uh oh!

compnerd Aug 25, 2020 •

edited

Loading

mikeash Aug 25, 2020 •

edited

Loading