Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add '_spi(TokamakCore)' to ideally internal public members. #386

Merged
merged 11 commits into from
Mar 8, 2021
Merged

Add '_spi(TokamakCore)' to ideally internal public members. #386

merged 11 commits into from
Mar 8, 2021

Conversation

filip-sakel
Copy link
Contributor

@filip-sakel filip-sakel commented Mar 4, 2021

This PR adds _spi(TokamakCore) to the modules:

  1. TokamakCore
  2. TokamakDOM
  3. TokamakGTK
  4. TokamakStaticHTML

The attribute is applied to:

  1. All View bodies in TokamakCore — either primitive or regular like Color
  2. ViewDeferredToRenderer bodies
  3. ParentView children members
  4. View modifiers (such as _onMount(perform:))
  5. Other members of types (like Color._withScheme, and _AnyApp._launch)

The attribute semantics (from my brief testing)

  1. It can only be applied to public declarations
  2. It ensures that every SPI declaration is exposed only by other SPI declarations (i.e. @_spi(Module) public enum A {}; public var a: A is illegal)
  3. Regularly importing a library prohibits clients from accessing SPI declarations that are not protocol witnesses (with an error).
  4. Regularly importing a library "discourages" clients from accessing SPI protocol witnesses by hiding them from the autocompletion suggestions (i.e. users can still access body of Text, but autocompletion hides it).
  5. For a declaration marked with @_spi(Module), a client has to write @_spi(Module) import Library in order to normally access such SPI declarations.

@@ -62,7 +62,8 @@ public struct WindowGroup<Content>: Scene, TitledScene where Content: View {
self.title = Text(title)
self.content = content()
}


@_spi(TokamakCore)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you apply SwiftFormat as swiftformat . in the project directory to get rid of trailing whitespaces? You can also install a pre-commit hook as described here, which will apply formatting fixes automatically for you before you commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I had forgotten that.

@MaxDesiatov MaxDesiatov added the SwiftUI compatibility Tokamak API differences with SwiftUI label Mar 4, 2021
MaxDesiatov
MaxDesiatov previously approved these changes Mar 4, 2021
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit 👍

@MaxDesiatov MaxDesiatov requested a review from a team March 4, 2021 21:24
@carson-katri
Copy link
Member

Is it possible to do the neverBody with @_spi in one place with an extension? Like:

extension View where Body == Never {
  @_spi(TokamakCore)
  var body: Never { neverBody("\(Self.self)") }
}

Then we can just use typealias Body = Never in the individual Views. That way if we ever want to change the way neverBody/_spi is used we can easily do it.

@filip-sakel
Copy link
Contributor Author

Yes it is. I introduced a PrimitiveView protocol to which all Never-body views in TokamakCore conform:

/// A `View` that offers primitive functionality, which renders its `body` inaccessible.
public protocol PrimitiveView: View where Body == Never {}

public extension PrimitiveView {
  @_spi(BubbleCore)
  var body: Never {
    neverBody(String(describing: Self.self))
  }
}

Then we can just use typealias Body = Never in the individual Views.

I added a new protocol instead of a conditional default-implementation on View because of type inference. This could perhaps work if type inference took into account SPI extensions only in the declaring module (and of course modules with @_spi(Module) import ...). Still, marking extensions SPI was introduced in 5.4, and its semantics haven't been fully fleshed out, so I think the safer option is to go with a new protocol.

@filip-sakel
Copy link
Contributor Author

I am also in a dilemma when it comes to whether ParentView.children should be underscore-prefixed. In the current implementation, ParentView.children members are marked SPI to match SwiftUI's not having Button(...).children. I think solidifying this design direction with the more apt name _children, will alleviate user confusion and improve consistency. What do you think?

Sources/TokamakCore/Tokens/Color.swift Outdated Show resolved Hide resolved
Sources/TokamakCore/Views/Selectors/Toggle.swift Outdated Show resolved Hide resolved
@mortenbekditlevsen
Copy link
Contributor

I am all for the addition of PrimitiveView. I am doing some experiments where I do some recursion through the view hierarchy and need to have the Never-body views marked by a protocol. I call it BuiltinView inspired by objc.ui Swift Talks.

Regarding ViewDeferredToRenderer I was initially confused by that name since I interpreted it to mean Never-body views, whereas it's actually a workaround to supply a body for a never-body view. So I don't think it's actually being deferred to the renderer, but to some other view...

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Mar 7, 2021

I'm going to significantly refactor or even get rid of ViewDeferredToRenderer in the near future anyway. The problem is that you can't have multiple ViewDeferredToRenderer conformances existing in the same app and working in some sane way. The compiler doesn't give any warning about conflicting conformances and just picks one according to some undocumented priorities. This prevents us from using the TestRenderer module together with any renderer in the same test bundle, and there can be only one test bundle in a given SwiftPM package.

The conflicting conformances isssue and inability to use any renderer other than the test one is pretty severe in my opinion. Most probably the logic currently handled by ViewDeferredToRenderer is not going to be implemented with protocol extensions at all. IDK maybe through plain enums and switches?

@MaxDesiatov
Copy link
Collaborator

ParentView.children members are marked SPI to match SwiftUI's not having Button(...).children. I think solidifying this design direction with the more apt name _children, will alleviate user confusion and improve consistency. What do you think?

This makes sense to me. ParentView is a protocol with internal visibility, or at least none of the renderer modules re-export it, so users weren't able to see it anyway, or am I missing something?

@mortenbekditlevsen
Copy link
Contributor

Excellent - I also think that most uses of ViewDeferredToRenderer that I have seen could be refactored away.

Regarding 'package' level access control, have you seen this pitch?

https://forums.swift.org/t/pitch-swift-package-access-control/45174

@MaxDesiatov
Copy link
Collaborator

Thanks for sharing it, this seems like a good pitch that could help us clean up things a bit!

@filip-sakel
Copy link
Contributor Author

This makes sense to me. ParentView is a protocol with internal visibility, or at least none of the renderer modules re-export it, so users weren't able to see it anyway, or am I missing something?

Currently, a public type conforming to a public protocol requires that this protocol's witnesses have a public access level. This, though, doesn't mean that the visibility of protocol witnesses is tied to the visibility of that protocol. For instance, the property public var children: [AnyView] of a ParentView-conforming Button type is visible to clients even when the ParentView protocol is not. Thus, I marked children properties of ParentViews SPI in TokamakCore — before this change clients could see children properties in the autocompletion list.

@MaxDesiatov MaxDesiatov requested a review from a team March 8, 2021 14:09
Copy link
Contributor

@mortenbekditlevsen mortenbekditlevsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
I look forward to using PrimitiveView in my own experiments. :-)

neverBody("Group")
}
}
extension Group: PrimitiveView & View where Content: View {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that it's conforming to both PrimitiveView and View? Is the former not a refinement of the latter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, conditionally conforming a type to a "refining" protocol (such as PrimitiveView) requires that either that type have separate conditional conformances (i.e. extension Group: View where Content: View {} and then the PrimitiveView conformance), or that both conformances be explicitly written (as is done above).

Also, I have no preference as to whether the above is written as PrimitiveView & View or PrimitiveView, View; whatever you prefer.

@@ -22,12 +22,23 @@ public protocol View {
}

public extension Never {
@_spi(TokamakCore)
var body: Never {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason that the body implementation from the PrimitiveView conformance is not good enough?
There probably is, but I'm not at a computer, so I can't try it out right now. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that a Never instance can, as the name suggests, never exist. Thus, I thought it would be misleading to use the primitive view conformance. Not to mention that relying on the PrimitiveView default implementation causes problems with Never-body scenes (for reasons I don't fully understand).

In my opinion, instance members of no-case enums, like Never, should non require a body anyway. This behavior is actually in place for functions taking no-case enums, meaning that func a(_: Never) -> Int {} is perfectly valid.

@MaxDesiatov MaxDesiatov merged commit 4d211af into TokamakUI:main Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwiftUI compatibility Tokamak API differences with SwiftUI
Development

Successfully merging this pull request may close these issues.

5 participants