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

Fix return a value that will be nullalbe on mapNotNull #3705

Closed

Conversation

KSH-code
Copy link

reactor/reactor-kotlin-extensions#52

The above issue explains mapNotNull does not change values into non-null values.

Thus, below code occurs an error such as Operator call corresponds to a dot-qualified call 'it.times(3)' which is not allowed on a nullable receiver 'it'.

    Flux.just(1, 2, 3, null).mapNotNull {
        it
    }.map {
        println(it * 3)
    }
image

Therefore, mapNotNull method must return @Nonnull value only.

@KSH-code KSH-code requested a review from a team as a code owner January 26, 2024 10:16
@KSH-code KSH-code changed the title Fix return value will be non-null value where mapNotNull Fix return a value will be non-null a value where mapNotNull Jan 26, 2024
@KSH-code KSH-code changed the title Fix return a value will be non-null a value where mapNotNull Fix return a value will be a nullable value where mapNotNull Jan 26, 2024
@KSH-code KSH-code changed the title Fix return a value will be a nullable value where mapNotNull Fix return a value that will be nullalbe on mapNotNull Jan 26, 2024
@chemicL
Copy link
Member

chemicL commented Jan 26, 2024

I'm not very knowledgeable in Kotlin, but the proposed change seems to be redundant. The reactor.core.publisher package has a NonNullApi annotation which should be equivalent to what this PR introduces but for all the methods' return values. Can you please explain why this change would be required?

@chemicL chemicL added the status/need-user-input This needs user input to proceed label Jan 26, 2024
@KSH-code
Copy link
Author

@chemicL In my understanding, @NonNullApi is applied to only Flux or Mono. not for a generic class T.
And also I'm lack of understanding about generic types and annotations. I will check out those then I will fix this pr.

@chemicL
Copy link
Member

chemicL commented Jan 30, 2024

I don't think this changes anything. I had a look at the signatures of map vs mapNotNull and essentially they're the same:

public final <R> Mono<R> map(Function<? super T, ? extends R> mapper)

public final <R> Mono<R> mapNotNull(Function <? super T, ? extends R> mapper)

The it variable in your report refers to the argument of Java's Function#apply(T) method (where the type definition is Function<T, R>), which we don't control.

In order to have this work the way you envision, I think java.util.function.Function<T, R>#apply(T) would need to be annotated with @Nonnull and that I'm afraid is not happening. If I'm mistaken, please provide a concrete example (repository/zip) where applying this strategy solves a problem for a Kotlin codebase in a similar scenario using Function from the JDK and we can reopen the PR/issue and work from there. For now I'm rejecting the PR though. Thanks for taking the time and investigating - please do get back if you have evidence we can fix something here.

@chemicL chemicL closed this Jan 30, 2024
@chemicL chemicL added status/invalid We don't feel this issue is valid, or the root cause was found outside of Reactor and removed status/need-user-input This needs user input to proceed labels Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/invalid We don't feel this issue is valid, or the root cause was found outside of Reactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants