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 Source and Sink extensions for Apple's NSInputStream and NSOutputStream #174

Merged
merged 52 commits into from
Aug 8, 2023

Conversation

jeffdgr8
Copy link
Contributor

@jeffdgr8 jeffdgr8 commented Jul 12, 2023

Modeled after JVM's InputStream.asSource(), Source.asInputStream(), OutputStream.asSink(), and Sink.asOutputStream(), add extension functions to similarly map to and from Apple's NSInputStream and NSOutputStream.

I previously submitted this PR to Okio. I've been using these extensions, along with my Okio fork, in my KMP library, pending availability in a public release.

Modeled after JVM's InputStream.asSource() and Source.asInputStream(), add extension functions to similarly map to and from Apple's NSInputStream
@fzhinkin fzhinkin self-requested a review July 12, 2023 11:07
@fzhinkin fzhinkin changed the base branch from master to develop July 12, 2023 11:09
@fzhinkin
Copy link
Collaborator

@jeffdgr8 thank you for the PR!

Unfortunately, I forgot to mention in the contribution guideline that the PR should be created against the develop branch, so I switched the base here. It shouldn't be a problem as master and develop are almost identical right now.

@whyoleg
Copy link
Contributor

whyoleg commented Jul 12, 2023

may be it make sense to also add the same functionality for NSOutputStream in one go?

@whyoleg whyoleg mentioned this pull request Jul 12, 2023
Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

just minor comments

core/apple/src/AppleCore.kt Outdated Show resolved Hide resolved
core/apple/src/AppleCore.kt Outdated Show resolved Hide resolved
core/apple/src/SourcesApple.kt Outdated Show resolved Hide resolved
@jeffdgr8
Copy link
Contributor Author

may be it make sense to also add the same functionality for NSOutputStream in one go?

I'm actually looking into implementing the NSOutputStream extensions for Sink as well. I don't need these in my own project, so haven't had a need for them. I can create a separate PR, or combine with this one.

@jeffdgr8
Copy link
Contributor Author

I went ahead and added NSOutputStream.asSink() and Sink.asNSOutputStream() extensions as well.

@jeffdgr8 jeffdgr8 changed the title Add NSInputStream.asSource() and Source.asNSInputStream() extensions for Apple's NSInputStream Add Source and Sink extensions for Apple's NSInputStream and NSOutputStream Jul 13, 2023
Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

a little more comments/questions :)

core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SourcesApple.kt Outdated Show resolved Hide resolved
core/apple/src/SourcesApple.kt Outdated Show resolved Hide resolved
Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

LGTM after a fix of propertyForKey(NSStreamDataWrittenToMemoryStreamKey) behaviour
Nice job!
Though, I've not reviewed tests, mostly just scrolled...

core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SourcesApple.kt Outdated Show resolved Hide resolved
core/apple/src/SourcesApple.kt Outdated Show resolved Hide resolved
core/apple/src/-Util.kt Outdated Show resolved Hide resolved
core/apple/src/BuffersApple.kt Show resolved Hide resolved
core/apple/test/NSOutputStreamSinkTest.kt Show resolved Hide resolved
core/apple/test/SinkNSOutputStreamTest.kt Show resolved Hide resolved
core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/SourcesApple.kt Show resolved Hide resolved
core/apple/src/AppleCore.kt Show resolved Hide resolved
core/apple/src/BuffersApple.kt Outdated Show resolved Hide resolved
core/apple/test/SourceNSInputStreamTest.kt Show resolved Hide resolved
@fzhinkin

This comment was marked as resolved.

@jeffdgr8

This comment was marked as resolved.

@jeffdgr8

This comment was marked as resolved.

core/apple/src/BuffersApple.kt Outdated Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/src/SinksApple.kt Outdated Show resolved Hide resolved
core/apple/test/SourceNSInputStreamTest.kt Outdated Show resolved Hide resolved
core/apple/test/SourceNSInputStreamTest.kt Outdated Show resolved Hide resolved
core/apple/test/SourceNSInputStreamTest.kt Show resolved Hide resolved
core/apple/test/SourceNSInputStreamTest.kt Show resolved Hide resolved
@OptIn(UnsafeNumber::class)
private class SinkNSOutputStream(
private val sink: Sink
) : NSOutputStream(toMemory = Unit), NSStreamDelegateProtocol {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how things work under the hood when inheriting NSInputStream/NSOutputStream and calling a non-default initializer (I understand that no-arg initializer is not available here) as according to Apple's docs, these initializers aimed to instantiate some specific stream's subclass.

Copy link
Contributor Author

@jeffdgr8 jeffdgr8 Aug 1, 2023

Choose a reason for hiding this comment

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

This was the only way I found to appease the requirement Unable to call non-designated initializer as super constructor, which is the error the compiler throws if you use the no-arg constructor. This seems to be a conflict between the way Kotlin and Objective-C object inheritance works, where Objective-C can return a subclass implementation as self from an init method.

I'm not sure there really should be an expected designated initializer in Kotlin for either of these classes. This is likely an interop error. In Objective-C this compiles and I'm able to construct [[MyNSInputStream alloc] init] just fine:

// MyNSInputStream.h
@interface MyNSInputStream : NSInputStream
- (instancetype)init;
@end

// MyNSInputStream.m
@implementation MyNSInputStream
- (instancetype)init {
    self = [super init];
    return self;
}
@end

It works to use either the memory or URL constructor of both NSInputStream and NSOutputStream. I figured the empty memory versions were likely to be the most lightweight. Is there some way to suppress this designated initializer requirement to call the no-arg constructor? It might also make sense to file an issue to fix the interop. Since we're overriding all of NSInputStream and NSOutputStream's public API functions and not using anything from the superclass, I think this should be safe though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related issues:

https://youtrack.jetbrains.com/issue/KT-47992
JetBrains/kotlin-native#2010

Looks like there is a workaround for disabling in custom .def interop, but this won't work for the platform interop in this case.

Copy link
Collaborator

@fzhinkin fzhinkin Aug 7, 2023

Choose a reason for hiding this comment

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

Yeap, seems like there's no way to inherit streams using no-args init ctor.

Copy link
Collaborator

@fzhinkin fzhinkin Aug 7, 2023

Choose a reason for hiding this comment

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

In fact, I was mostly concerned about the possibility of inheriting some methods from NSInput/OutputStream subclasses instantiated for initFromData, initToMemory. But it's definitely not the case, as there were errors due to non-implemented run-loop-related methods previously (so nothing was inherited).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the suggested doc comments. It would be strange for a consumer to call any of those init functions after receiving the instantiated stream object from asNSInputStream() or asNSOutputStream(). In Kotlin it already shows an error trying to call init functions directly, not through a constructor. I don't think I've ever seen a call to an init function even in Objective-C that wasn't directly after a call to alloc. Swift doesn't allow this either, where init functions are only usable to construct objects now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for updating the doc! Yes, it should be possible to call these initializers only from Objective-C code and it seems like nobody actually does that, but it is still possible :')

@fzhinkin
Copy link
Collaborator

fzhinkin commented Aug 2, 2023

@jeffdgr8 thank you for fixing all the issues!
Right now there are only two questions left:

I'll try to figure out if anything could be done with the latter (and if there's nothing we can do - I'm fine with how it's done right now).
Otherwise, the change looks good.

@fzhinkin fzhinkin merged commit e7c5e42 into Kotlin:develop Aug 8, 2023
1 check failed
fzhinkin added a commit that referenced this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants