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

Annotation CheckReturnValue for subscribe() and other builder-like methods #704

Closed
jreznot opened this issue Oct 5, 2021 · 15 comments · Fixed by #729
Closed

Annotation CheckReturnValue for subscribe() and other builder-like methods #704

jreznot opened this issue Oct 5, 2021 · 15 comments · Fixed by #729
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@jreznot
Copy link

jreznot commented Oct 5, 2021

Popular reactive libraries such as RxJava has @CheckReturnValue annotation on methods returning objects without side-effects, so developers may rely on IDE to check that there are no calls with ignored return value.

It would be great if Mutiny APIs would be annotated with this kind of annotation.

Example:

package io.reactivex.rxjava3.annotations;

import java.lang.annotation.*;

/**
 * Marks methods whose return values should be checked.
 */
@Retention(RetentionPolicy.RUNTIME)
@Documented
@Target(ElementType.METHOD)
public @interface CheckReturnValue {
}

Problem example:

    @Test
    public void testLog() {
        Uni.createFrom().item(1)
                .log()
                .subscribe();  // nothing happens, because I didn't finish subscribe configuration
    }

Once methods are annotated IDE can help here:

image

@jreznot
Copy link
Author

jreznot commented Oct 5, 2021

The corresponding issue in IntelliJ IDEA: https://youtrack.jetbrains.com/issue/IDEA-277014

I believe it would be better to maintain such rules right in the framework to help all editors and IDEs with these usages.

@jponge
Copy link
Member

jponge commented Oct 5, 2021

Do we have any standard for such annotations?

@jponge jponge added enhancement New feature or request good first issue Good for newcomers labels Oct 5, 2021
@jreznot
Copy link
Author

jreznot commented Oct 5, 2021

Do we have any standard for such annotations?

IntelliJ IDEA recognizes any annotation with short name CheckReturnValue. Google Checker Framework uses the same name too.

@jponge
Copy link
Member

jponge commented Oct 5, 2021

Ok so we can create our own right?

@jreznot
Copy link
Author

jreznot commented Oct 5, 2021

Yes, you may simply create your own, no dependencies required

@gavinking
Copy link

gavinking commented Oct 5, 2021

This sounds like a really worthwhile thing to do.

@jponge jponge added this to the 1.2.0 milestone Oct 5, 2021
@jponge jponge self-assigned this Oct 5, 2021
@jponge
Copy link
Member

jponge commented Oct 5, 2021

Planned for 1.2.0 🚀

@cescoffier
Copy link
Contributor

+1, sounds like a great idea and will improve the user experience. Once defined in mutiny I would pull the annotation in the Vert.x bindings and annotate all methods returning a Uni or a Multi (how many times did we call a method returning a Uni and nothing happens?).

@Sanne This could be interesting for Hibernate Reactive too.

@jponge
Copy link
Member

jponge commented Oct 6, 2021

  • Annotate in Mutiny
  • Bring the annotation to the Mutiny Vert.x bindings code generator

@jreznot
Copy link
Author

jreznot commented Oct 6, 2021

Please be careful with methods that receive lambdas with effect, e.g. log or something like forEach. As far as I understand, there might be hot streams that do work even without a subscription, such most-likely-have-effect methods must not be annotated as CheckReturnValue.

@Sanne
Copy link
Contributor

Sanne commented Oct 6, 2021

Yes look nice. But how does it work, is it an annotation processor? I would prefer an annotation processor (or similar) over trying to rely on IDEA specific conventions.

@jreznot
Copy link
Author

jreznot commented Oct 6, 2021

Yes look nice. But how does it work, is it an annotation processor? I would prefer an annotation processor (or similar) over trying to rely on IDEA specific conventions.

This is simply inspection in IntelliJ IDEA ) I believe it provides faster feedback loop than compiling everything to see this kind of errors.

@jreznot
Copy link
Author

jreznot commented Oct 6, 2021

is it an annotation processor

AFAIK this kind of checks cannot be implemented as annotation processor, only if you use something like ByteBuddy enhancer that visits all the code.

@Sanne
Copy link
Contributor

Sanne commented Oct 6, 2021

Ok I would have preferred something that works in all IDEs (and no IDE) but I suppose it's still very nice to have this in IDEA.

Funny, reminds me of an old tool we had explored to identify (and take advantage of) code which would do a Map#put but ignored the return value: https://github.com/infinispan/jokre (it's pretty much dead though, just somewhat related)

@jponge
Copy link
Member

jponge commented Oct 13, 2021

See #729

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants