Provide IDE plugin

[sir4ur0n]

Ok, this is just an idea that popped in my mind, sorry if it's already been proposed before, I did not find any issue about it.

Could we imagine an IDE plugin (Intellij at least, Eclipse also?) to provide some additional inspections to Vavr? E.g. this code makes no sense in Vavr:

List<Integer> myList = List.of(1,2,3);
myList.map(number -> number * 2); // Makes no sense: Vavr collections are immutable. IDE should raise a warning and quickfix would be to store it in a new variable.

Some methods by nature have side-effects (like foreach) and thus should not raise a warning.

Any idea/remark/comment?

[chb0github]

The idea is really good but did you check jetbrains youtrack? They would be the most likely place this would be tracked.

[danieldietrich]

For Eclipse plugins there are custom update sites, listed on Eclipse Marketplace. For IntelliJ it seems that they host plugins by themselves.

Generally, it would be nice to have a generic way to spot immutable types, no matter if they are Vavr specific. There already exist annotations introduced by @briangoetz in his JCIP book.

We already use our own internal annotation GuardedBy. There exists an issue for introducing @NotNull and @Nullable annotations. Additionally we have multiple package private @GwtIncompatible annotations.

IntelliJ IDEA already supports the JCIP annotations. I think the easiest way to solve this issue is to enhance the existing IntelliJ support for JCIP annotations in the way that calling a write operation on an immutable object without an assignment shows up a warning (or an error).

An operation is a write operation, if it does return the self-type. However, this will not cover all cases. E.g. there are transformational operations like zip or groupBy that do return other types than the self type.

Here, we could add a package io.vavr.annotation that will house all of our annotations. Currently I see only (a subset of) the JCIP annotations there, plus NotNull, Nullable and maybe GwtIncompatible (I'm not sure if it should be exposed to the outside).

We need a volunteer that creates an IntelliJ IDEA issue and keeps track of it.

[nfekete]

Findbugs and ErrorProne both support CheckReturnValue. (See also http://errorprone.info/bugpattern/CheckReturnValue). Both can be used in IntelliJ and Maven, but ErrorProne relies heavily on com.sun.* so it doesn't work with ECJ. They recommend Findbugs for now. AFAIK Findbugs has both Eclipse and IDEA plugins and Maven support too. Findbugs supports self-supplied annotations as long as they follow the naming convention, so we can either choose to depend on JSR-305 annotations or supply our own version of the annotations we use. I tested whether self-supplied annotation works and it indeed worked, see below.

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Documented
@Target({ ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.TYPE, ElementType.PACKAGE })
@Retention(RetentionPolicy.RUNTIME)
public @interface CheckReturnValue {
}

public class Test {

    @CheckReturnValue
    public Test changeTest(int a) {
        return new Test();
    }

    public static void main(String[] args) {
        Test test = new Test();
        test.changeTest(1);
    }

}

It provides the following error marker in the eclipse findbugs view at the line test.changeTest(1);:

[danieldietrich]

@nfekete Indeed, that would help. I think it would be best to create a dedicated io.vavr.annotation module.

Then we could include the annotation module as compile-time dependency. I don't know if that would work with @GwtIncompatible or when the compiler performs checks for a project that has Vavr as transitive dependency. Does such a project then need to include the Vavr annotations by itself as compile time dependency?

[nfekete]

CheckReturnValue can also be applied to class or package level, and use CanIgnoreReturnValue to exempt methods from its scope, but it's not supported by Findbugs, and it would be great to be able to use Eclipse to check client code as well, so I would go with using only CheckReturnValue, even if that would mean somewhat spamming it around.

[nfekete]

I'm using findbugs and jsr305 annotations in a GWT project and GWT compiler is totally fine with them as long as they're available for inspection on a source code level for the GWT compiler.

[nfekete]

@danieldietrich i had it in mind for long to make vavr both nullness checking ready and findbugs ready (from client perspective), just haven't had the time yet. The findbugs part (the things we're currently discussing in this issue) shouldn't be too hard, but the nullness part is much more work.

[danieldietrich]

but the nullness part is much more work.

Yes. We should only do that for public API of course and not the internal classes like io.vavr.collection.Collections. I'm sure it will reveal some methods where the javadoc isn't perfect, yet. I.e. cases where we forgot to add @throws documentation.

[atsu85]

What is the plan with this issue?

I came here to find out if Findbugs/Checkstyle integration is supported (forgetting to assign the result of method invocation, that will return new immutable object instead of modifying it).

Perhaps this issue should be renamed to smth like "Findbugs/Checkstyle integration"?

[danieldietrich]

@atsu85 this is more than the addition of a compile-time check 'feature', it is a strategic decision for the Vavr project.

I totally agree to @lukaseder. He stated here in a similar question:

This is important to realise. You cannot possibly think that engaging in a "parallel universe" type system involving type annotations is a simple task, or that you will stop at abusing that language feature for "just some simple" null checks. At some point, you will want to annotate collection emptiness, string "size-ness", number "positive-ness", "single-stream-consumption-ness" etc., so this is a very strategic decision that must not be underestimated (at least in a cleanly designed, not so wishy-washy API).

Currently we have @GwtIncompatible annotations. We should have never introduced them. I wait for the moment we are able to remove them completely.

I will clarify the (non-)usage of annotations in our contribution guidelines.

Keep it simple!

[nfekete]

I was planning to do this for vavr, together with the nullness checking. I just never had the time to do it (mostly because the nullness part would be a bit harder and would take more time than I had to spare lately).

[danieldietrich]

I see the usefulness but I see also the drawbacks

• we bind to specific tooling
• it is error-prone regarding further development and maintenance
• ...

I don't want to maintain code like this:

interface Future<T> {
    @GwtIncompatible
    @CheckReturnValue
    <U> Future<U> map(@NotNull Function<? super T, ? extends U> mapper);
}

A bit later it might look like this (because we don't have any arguments to reject other checks):

@Async
@Sealed
@SumType
interface Future<T> {
    @GwtIncompatible
    @CheckReturnValue
    @NotSameInstance
    @Async
    <U> Future<U> map(@NotNull @Idempotent @Memoizable Function<? super T, ? extends U> mapper);
}

Annotations are a workaround for an incomplete type system. I want to use clean and straight forward code instead of workarounds.

[nfekete]

I understand your argument, although I think it's slightly different from the behavior-affecting annotations that are so specific to Spring for example, and the coding style of programming-through-annotations it brings. These are really just compiler flags, their presence or lack thereof would not affect how vavr code functions, and they could be viewed as documentation (for humans, for compilers). Also, @CheckReturnValue can be specified on a type level, so it's not like you would need to spam it on each and every method in vavr.

[briangoetz]

People always start out excited about annotations; the return-on-effort for that first tap of the annotation hammer is usually positive. As Daniel points out, they very quickly reach a point of negative returns for multiple reasons, including the lack of credible abstraction for annotations, and because they're not part of the type system (and therefore any inheritance semantics are completely tool-specific.)

[nfekete]

I view them as documentation of the API, only a bit better in the sense that tools can warn me based on that documentation that I'm doing something wrong. Is using @Override or @Deprecated or even /** @deprecated */ any different in principle?

[danieldietrich]

@briangoetz thank you for your comment, it always helps me to round the picture

and therefore any inheritance semantics are completely tool-specific

yes, in my random example that would apply to a @Sealed interface

I've learned that we should not try to 'emulate' non-existing Java features, like type-constructors, pattern matching or for-comprehensions. Interestingly, as Vavr gets more popular, I feel increasingly responsible in how it influences the Java ecosystem. My thoughts are about how I can reduce features, intra-library dependencies and redundancies while generalizing things and increasing composability at the same time. Modularization will be a key aspect here. Less is more and simplicity is king.

Annotations have their limitations. For example both R f(@Nullable Option<T> o) and R f(@NotNull Option<T> o) are curious. Also we can't express if T and R of Function<T, R> are @Nullable or not. (Update: actually the latter is possible but there is/was a bug)

I think: if in doubt, leave the feature away.

[nfekete]

I'm not sure what you mean. R f(@Nullable Option<T> o) would be a terrible idea, the whole point of Option is to wrap both a value's presence and the value itself, to avoid having to handle nulls (plus compositionality). The R f(@NotNull Option<T> o) just makes sure on the compiler level (if checking is switched on) that you don't accidentally provide a null value as a parameter there as it will fail at runtime. Checking for this at compile time is arguably better than failing at runtime. And normally you shouldn't even have to write @NotNull Option<T>, class or package level defaults are there so that you don't have to clutter your code with verbose declarations like that. Eclipse supports that, I'm not sure about package level for IntelliJ IDEA.

Note that you are actually checking against null in pretty much every vavr method, where most of the public methods accepting parameters are starting with statements like Objects.requireNonNull(elements, "elements is null");. You also need to document it /** @throws NullPointerException if {@code elements} is null */. Declaring it to be @NonNull just makes the whole thing explicit for tooling. How does a single class/package level annotation compare to all the tedious documentation and extraneous code to check for nulls everywhere.

I usually declare non-nullness on a package level and let the compiler enforce the code I write to be totally null-free. You can get quite far with disciplined coding not to have null values created by your own code, but coding is rarely isolated like that. We're using many libraries and a formal promise from a library vendor to never return null could turn out to be a great service to library users.

Also, the type argument scenario: Option<@NonNull T> is perfectly valid, at least in Eclipse, and you can also declare package level defaults for generics type arguments to be enforced to be non-null. So I don't even need to write Option<@NonNull T>, it's automatically enforced for the whole package.

And the nice thing from the perspective of a library user is that you don't need to care about all this if you don't want to. Just don't enable null checking and you're free to spam nulls around wherever it's accepted.

But while nullness checking is arguably more useful and has a bigger impact than enforcing not ignoring the return value of a functional method (we're talking about a functional library), it's much more easier to work around the lack of nullness metadata, as the two main IDEs, IDEA and Eclipse both support providing these annotations externally. It's a bit more cumbersome, but it's manageable. I even have external nullness annotations for the JDK classes, I created them by analyzing JDK code and figured out which method return values are safe from null point of view and which are not, so the compiler can warn me right away if I treat a return value from a JDK API as non-null without actually checking for null, while leaving me alone if a return value is considered safe. The code you get out of this is much cleaner, less cluttered.

I can totally live without any of these features (@CheckReturnValue is much less important, and nullness can be provided externally), but I think they are indeed useful, and they might warrant their pretty low cost.

Sorry for the long post, here's a potato: 🥔.

[danieldietrich]

🥔🍅🥚🍆🥔🍅🥚🍆🥔🍅🥚🍆🥔🍅🥚🍆

I'm not sure what you mean.

R f(@NotNull Option<T> o) is semantically curious because Option is a wrapper for null.

Note that you are actually checking against null in pretty much every vavr method

Yes, I decided that in order to provide human-readable error messages. Even if we had annotations, no one forces a developer to use the tooling. Also, if lib X uses Vavr and X.map(f) calls some of Vavr's map(f), f could be still null.

I usually declare non-nullness on a package level (...)

Why not forbid to pass around null at all? Not using null at all is the safest way to handle it and has the lowest cost.

🥔🍅🥚🍆🥔🍅🥚🍆🥔🍅🥚🍆🥔🍅🥚🍆

[lukaseder]

Also we can't express if T and R of Function<T, R> are @Nullable or not.

Ever heard of JSR-308?

[nfekete]

Yes, I decided that in order to provide human-readable error messages. Even if we had annotations, no one forces a developer to use the tooling. Also, if lib X uses Vavr and X.map(f) calls some of Vavr's map(f), f could be still null.

Yes, and that's the correct thing to do, I'm not arguing against it, I'm just arguing that a single piece of metadata on a class level stating that hey, man, you can relax, we won't screw you by returning null values is a nice little piece of information that can be used by the tooling.

I don't want to drag out this thread with forever arguing over this issue, as I said it's not really an issue for me because the return values case is not pressing me in any way and the nullness I can do it externally. But I'm doing it externally, another guy is doing it externally, and who knows how many guys are doing it, because there is no single source of authority for that metadata, so we all end up marking hundreds of methods and each parameter, after analyzing most of them, to state that hey, this method is cool, doesn't return null and we shouldn't pass one either. With external annotations you need to do this for each and every method and each of it's parameters and type arguments, and that is a lot of work, while this could be solved for many of us by just a few class or package level annotations directly in vavr.

[danieldietrich]

@lukaseder

Ever heard of JSR-308?

Yes, but I must admit that I was not aware of the fact that generic type parameters are annotatable. I try to avoid annotation processing if possible. Vavr uses it here but it caused problems in the past regarding IDE integration, Java 9 modules, build processes etc.

[lukaseder]

@danieldietrich

Try this: https://twitter.com/lukaseder/status/711612663202238464

[danieldietrich]

@lukaseder

[rkraneis]

(as this issue is already hijacked anyway) @nfekete How do you want to tackle null (and possibly other checks)? A good tool seems to be the checker framework which supports basically arbitrary checks (e.g. purity) besides nullness.

[nfekete]

@rkraneis yes, I had the same hijacked feeling too, as the nullability part has its own issue. If you could move your question to https://github.com/vavr-io/vavr/issues/1919, I would move my answer as well, as I'm gonna talk mostly about the null part.

I haven't tried the Checker Framework yet. I tried Findbugs, but I'm not satisfied with it. I tried using the very same annotation we're talking about in this issue -@CheckReturnValue -, and it missed presenting the warning/error to me. Not sure whether this was an IDE plugin issue or something affecting the core Findbugs. I'm not sure about the state of Findbugs anyway, it might be abandoned. There's Spotbugs which claims to continue where Findbugs has left ott and it seems to be an active project. I might give a try to the Checker Framework as well.

[rkraneis]

@nfekete yes, I moved that over (and extended it a bit). We can take the discussion there.