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

Optional::map doesn't accept @Nullable #1633

Closed
justmin opened this issue Nov 4, 2017 · 13 comments
Closed

Optional::map doesn't accept @Nullable #1633

justmin opened this issue Nov 4, 2017 · 13 comments
Assignees
Milestone

Comments

@justmin
Copy link

justmin commented Nov 4, 2017

This code produce compile-time error with any of lines 1 and 2:

public class NullnessTest {
    private static @Nullable String emptyToNull(String arg){
        return arg.equals("") ? null : arg;
    }

    @Test
    public void test() throws Exception {
        String str = Optional.ofNullable(emptyToNull("notnull"))
                .map(s -> emptyToNull(s)) // 1
                .map(NullnessTest::emptyToNull) // 2
                .orElse(null);
        assert str == null;
    }
}

but Optional::map handle Function returning null correctly.

@mernst
Copy link
Member

mernst commented Nov 7, 2017

Possibly related to #979.

@mernst mernst added this to the High milestone Nov 7, 2017
@justmin
Copy link
Author

justmin commented Nov 8, 2017

The same concern about java.util.Optional#orElseGet: it accepts Supplier returning null but CF is not.

@smillst
Copy link
Member

smillst commented Nov 13, 2017

This is a limitation of the Checker Framework's implementation of type argument inference. We are working on implementing the better type argument inference. See #979.

In the meantime, explicitly specify the type argument avoids the error. For example, the following code type checks.:

public void test(Optional<String> opt) throws Exception {
    String str = opt .<@Nullable String>map(s -> emptyToNull(s)).orElse(null);
    String str2 = opt.<@Nullable String>map(Issue1633::emptyToNull) .orElse(null);
}

I'm closing this issue in favor of #979.

@smillst smillst closed this as completed Nov 13, 2017
@justmin
Copy link
Author

justmin commented Nov 15, 2017

Annotating of Optional::map can solve this problem:

  public<U> Optional<U> map(Function<? super T, ? extends @Nullable U> mapper) {
       Objects.requireNonNull(mapper);
       if (value == null)
           return empty();
       else {
           return Optional.ofNullable(mapper.apply(value));
       }
   }

A little bit harder problem with orElseGet:
annotating it this way

    public @Nullable T orElseGet(Supplier<@Nullable ? extends @Nullable T> other) {
        return value != null ? value : other.get();
    }

solve the problem, but it always return @Nullable result
And this way

    public @PolyNull T orElseGet(Supplier<@PolyNull ? extends @PolyNull T> other) {
        return value != null ? value : other.get();
    }

doesn't help at all

@smillst
Copy link
Member

smillst commented Nov 15, 2017

I see; we can work around #979 in this case by adding annotations to Optional. I'm going to reopen this issue.

@mernst
Copy link
Member

mernst commented Dec 11, 2017

Regarding ofNullable, I have added your proposed annotation to the JDK. (I haven't yet pushed the commit, though.)
Unfortunately, at the moment for backward compatibility with Java 7, the annotated JDK omits some methods that use Java 8 features. So you'll need to write that in a stub file or create your own annotated JDK.

Regarding orElseGet, can you tell me why the last version of the orElseGet annotation doesn't help at all? Maybe give a test case, as you did for ofNullable? When I wrote those annotations and ran the current version of the Nullness Checker, client code typechecked. Here is my test case:

// Test case for Issue 1633:
// https://github.com/typetools/checker-framework/issues/1633
// @below-java8-jdk-skip-test

import java.util.function.Supplier;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.checker.nullness.qual.PolyNull;

class Issue1633 {

    void foo4nw(Optional<String> o, Supplier<@Nullable String> supplyNullable) {
        @Nullable String str3 = o.orElseGet(supplyNullable);
    }

    void foo8nw(Optional<String> o, Supplier<@NonNull String> supplyNonNull) {
        @NonNull String str3 = o.orElseGet(supplyNonNull);
    }
}

// From the JDK
final @NonNull class Optional<T extends @Nullable Object> {

    /** If non-null, the value; if null, indicates no value is present */
    private final @Nullable T value = null;

    public @PolyNull T orElseGet(Supplier<? extends @PolyNull T> other) {
        // The commented-out line fails to typecheck due to issue #979
        // return value != null ? value : other.get();
        if (value != null) {
            return value;
        } else {
            return other.get();
        }
    }
}

mernst added a commit to mernst/checker-framework that referenced this issue Dec 11, 2017
Note that the annotated JDK currently doesn't contain methods that require
Java 8.  Therefore, these changes are not helpful to clients until that
issue is resolved.
@justmin
Copy link
Author

justmin commented Dec 12, 2017

Hello,

thank you for attention.
Please add this two methods to your Issue1633 class:

    void foo4(Optional<String> o) {
        @Nullable String str3 = o.orElseGetPolyNull(() -> null);
    }

    void foo4nw(Optional<String> o) {
        @Nullable String str3 = o.orElseGetPolyNullNoWildcard(() -> null);
    }

Both of them produce compile-time error but should not.

Could you please explain a little bit more about writing my stub:
I already have Optional in jdk8 annotation library. How could I write stub? How could I use it with maven?

Am I wrong, but I suppose jd8.jar is intended to use in java 8 only. How could it affect java 7?

@mernst
Copy link
Member

mernst commented Dec 12, 2017

Thanks for the additional test cases. I have added them, but the problems with () -> null are definitely due to #979. (The Checker Framework currently implements Java 7 type inference, and #979 is an issue for a re-implementation of type inference to use the Java 8 type inference algorithm. Work is well underway on that, but I apologize that it is not yet complete.)

Regarding JDK7 vs JDK8, you are correct. The commented-out text in Optional.java in the annotated JDK was an error (it may have been desirable long ago, but it's wrong now), so I have uncommented the code. I'll open a pull request for this shortly.

mernst added a commit to mernst/checker-framework that referenced this issue Dec 12, 2017
mernst added a commit that referenced this issue Dec 12, 2017
@justmin
Copy link
Author

justmin commented Jan 15, 2018

Hello,
I don't see this changes in CF 2.3.1. Is it intended behavior?

@mernst
Copy link
Member

mernst commented Jan 15, 2018

What do you mean by "this changes"? The referred-to commit 973b522 should be in the 2.3.1 release. Issue #979 is not yet fixed, though we continue to make progress on it.

@justmin
Copy link
Author

justmin commented Jan 15, 2018

For me it looks this commit is not included in 2.3.1. Could you please recheck it?

@mernst
Copy link
Member

mernst commented Jan 16, 2018

Can you please tell me why "it looks this commit is not included in 2.3.1"?
Most useful would be a bug report with the information requested in the manual. Thanks!

@justmin
Copy link
Author

justmin commented Jan 16, 2018

Sorry, I was wrong
java decompiler doesn't see @Nullable annotation in Function<> template arg, but test case in this bug report now passed.
Thanks!

@justmin justmin closed this as completed Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants