Control flow analysis of streams

[fprochazka]

Let's consider the following example

import org.javamoney.moneta.spi.DefaultNumberValue;

import javax.annotation.Nullable;
import javax.money.CurrencyUnit;
import javax.money.Monetary;
import javax.money.NumberValue;
import java.math.BigDecimal;
import java.util.*;
import java.util.stream.Collectors;

class Scratch
{

    public static void main(String[] args)
    {
        CurrencyUnit czkCurrency = Monetary.getCurrency("CZK");

        List<PriceVersion> prices = Arrays.asList(
            new PriceVersion(
                PriceType.EX_FACTORY,
                null,
                czkCurrency
            ),
            new PriceVersion(
                PriceType.EX_FACTORY,
                DefaultNumberValue.of(new BigDecimal("10")),
                czkCurrency
            )
        );

        prices.stream()
            .filter(price -> price.getValue() != null)
            .collect(Collectors.toMap(
                PriceVersion::getType,
                price -> price.getValue().numberValue(BigDecimal.class)
            ));
    }

    public static final class PriceVersion
    {

        private PriceType type;

        @Nullable
        private NumberValue value;

        private CurrencyUnit currency;

        public PriceVersion(final PriceType type, @Nullable final NumberValue value, final CurrencyUnit currency)
        {
            this.type = type;
            this.value = value;
            this.currency = currency;
        }

        public PriceType getType()
        {
            return type;
        }

        @Nullable
        public NumberValue getValue()
        {
            return value;
        }

        public CurrencyUnit getCurrency()
        {
            return currency;
        }

    }

    public enum PriceType
    {

        EX_FACTORY,

    }

}

the NullAway reports

[NullAway] dereferenced expression price.getValue() is @Nullable

Which is correct, but I've previously checked that it's not and now it would be cumbersome to check again just to make "the compiler" happy.

Is basic control flow analysis of streams out of scope of this lib or would it make sense to support?

maven: effective pom

```xml org.apache.maven.plugins maven-compiler-plugin 3.8.1 javac-with-errorprone true 1.8 1.8 -Werror -Xlint:all,-fallthrough,-processing,-serial,-classfile -parameters -XDcompilePolicy=simple -XepDisableWarningsInGeneratedCode -XepOpt:Immutable:KnownImmutable=javax.money.CurrencyUnit -Xep:ConstantField:WARN -Xep:ClassName:WARN -Xep:DivZero:WARN -Xep:EmptyIf:WARN -Xep:IterablePathParameter:WARN -Xep:LongLiteralLowerCaseSuffix:WARN -Xep:AnnotationPosition:WARN -Xep:EmptyTopLevelDeclaration:WARN -Xep:EqualsBrokenForNull:WARN -Xep:FunctionalInterfaceClash:WARN -Xep:InvalidInlineTag:WARN -Xep:InvalidParam:WARN -Xep:MissingDefault:WARN -Xep:NonCanonicalStaticMemberImport:WARN -Xep:PrimitiveArrayPassedToVarargsMethod:WARN -Xep:RedundantOverride:WARN -Xep:RedundantThrows:WARN -Xep:StaticQualifiedUsingExpression:WARN -Xep:StringEquality:WARN -Xep:UnusedException:WARN -Xep:ConstantField:WARN -Xep:MultiVariableDeclaration:WARN -Xep:MultipleTopLevelClasses:WARN -Xep:MultipleUnaryOperatorsInMethodCall:WARN -Xep:PrivateConstructorForUtilityClass:WARN -Xep:UngroupedOverloads:WARN -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=com.cogvio -XepOpt:NullAway:TreatGeneratedAsUnannotated=true -XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true -XepOpt:NullAway:KnownInitializers=org.springframework.beans.factory.InitializingBean.afterPropertiesSet -XepOpt:NullAway:ExcludedFieldAnnotations=javax.persistence.PersistenceContext,org.springframework.beans.factory.annotation.Autowired com.uber.nullaway nullaway 0.7.5 true org.codehaus.plexus plexus-compiler-javac-errorprone 2.8.5 com.google.errorprone error_prone_core 2.3.3 com.cogvio.pm build-tools-nullaway 0.0.1-SNAPSHOT ```

[msridhar]

We already handle this for RxJava streams. @lazaroclapp how hard would it be to also do it for Java 8 streams?

On Fri, Jul 12, 2019 at 9:42 AM Filip Procházka notifications@github.com wrote:

Let's consider the following example

import org.javamoney.moneta.spi.DefaultNumberValue; import javax.annotation.Nullable;import javax.money.CurrencyUnit;import javax.money.Monetary;import javax.money.NumberValue;import java.math.BigDecimal;import java.util.*;import java.util.stream.Collectors; class Scratch {

public static void main(String[] args)
{
    CurrencyUnit czkCurrency = Monetary.getCurrency("CZK");

    List<PriceVersion> prices = Arrays.asList(
        new PriceVersion(
            PriceType.EX_FACTORY,
            null,
            czkCurrency
        ),
        new PriceVersion(
            PriceType.EX_FACTORY,
            DefaultNumberValue.of(new BigDecimal("10")),
            czkCurrency
        )
    );

    prices.stream()
        .filter(price -> price.getValue() != null)
        .collect(Collectors.toMap(
            PriceVersion::getType,
            price -> price.getValue().numberValue(BigDecimal.class)
        ));
}

public static final class PriceVersion
{

    private PriceType type;

    @Nullable
    private NumberValue value;

    private CurrencyUnit currency;

    public PriceVersion(final PriceType type, @Nullable final NumberValue value, final CurrencyUnit currency)
    {
        this.type = type;
        this.value = value;
        this.currency = currency;
    }

    public PriceType getType()
    {
        return type;
    }

    @Nullable
    public NumberValue getValue()
    {
        return value;
    }

    public CurrencyUnit getCurrency()
    {
        return currency;
    }

}

public enum PriceType
{

    EX_FACTORY,

}

}

the NullAway reports

[NullAway] dereferenced expression price.getValue() is @nullable https://github.com/nullable

Which is correct, but I've previously checked that it's not and now it would be cumbersome to check again just to make "the compiler" happy.

Is basic control flow analysis of streams out of scope of this lib or would it make sense to support? maven: effective pom

org.apache.maven.plugins maven-compiler-plugin 3.8.1 javac-with-errorprone true 1.8 1.8 -Werror -Xlint:all,-fallthrough,-processing,-serial,-classfile -parameters -XDcompilePolicy=simple -XepDisableWarningsInGeneratedCode -XepOpt:Immutable:KnownImmutable=javax.money.CurrencyUnit -Xep:ConstantField:WARN -Xep:ClassName:WARN -Xep:DivZero:WARN -Xep:EmptyIf:WARN -Xep:IterablePathParameter:WARN -Xep:LongLiteralLowerCaseSuffix:WARN -Xep:AnnotationPosition:WARN -Xep:EmptyTopLevelDeclaration:WARN -Xep:EqualsBrokenForNull:WARN -Xep:FunctionalInterfaceClash:WARN -Xep:InvalidInlineTag:WARN -Xep:InvalidParam:WARN -Xep:MissingDefault:WARN -Xep:NonCanonicalStaticMemberImport:WARN -Xep:PrimitiveArrayPassedToVarargsMethod:WARN -Xep:RedundantOverride:WARN -Xep:RedundantThrows:WARN -Xep:StaticQualifiedUsingExpression:WARN -Xep:StringEquality:WARN -Xep:UnusedException:WARN -Xep:ConstantField:WARN -Xep:MultiVariableDeclaration:WARN -Xep:MultipleTopLevelClasses:WARN -Xep:MultipleUnaryOperatorsInMethodCall:WARN -Xep:PrivateConstructorForUtilityClass:WARN -Xep:UngroupedOverloads:WARN -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages=com.cogvio -XepOpt:NullAway:TreatGeneratedAsUnannotated=true -XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true -XepOpt:NullAway:KnownInitializers=org.springframework.beans.factory.InitializingBean.afterPropertiesSet -XepOpt:NullAway:ExcludedFieldAnnotations=javax.persistence.PersistenceContext,org.springframework.beans.factory.annotation.Autowired com.uber.nullaway nullaway 0.7.5 true org.codehaus.plexus plexus-compiler-javac-errorprone 2.8.5 com.google.errorprone error_prone_core 2.3.3 com.cogvio.pm build-tools-nullaway 0.0.1-SNAPSHOT

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uber/NullAway/issues/332?email_source=notifications&email_token=AABPEUOJTRPF2Z35WVLCLXLP7CX6NA5CNFSM4ICNIPA2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G65SBPQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AABPEUNP2ITX7ST6AUI3FDDP7CX6NANCNFSM4ICNIPAQ .

[lazaroclapp]

Certainly not out of scope!

As Manu mentioned, we have some support for this already for Rx Java streams (see RxNullabilityPropagator), but not for Java 8 streams yet. This could be implemented as another handler or by extending the Rx handler into something more general.

Not exactly sure when I'll have the bandwidth to add this support, but it's certainly in the roadmap. If you are interested @fprochazka, we would be happy to accept a PR for this. It's not a trivial task, but it's also not as daunting as changing the core of NullAway would be, since all that is needed is the right Handler implementation.

cc: @raghavan and @shas19 in case either of you two have some spare cycles. That said, it's not clear to me that is a priority internally just yet, though.

[fprochazka]

I can't promise anything, but I might try to send the PR (or ask my colleague to send it).

[msridhar]

I am very happy to answer questions and advise on how to get this done if needed. Thanks, Filip!

On Mon, Jul 15, 2019 at 9:15 AM Filip Procházka notifications@github.com wrote:

I can't promise anything, but I might try to send the PR (or ask my colleague to send it).

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/uber/NullAway/issues/332?email_source=notifications&email_token=AABPEUK4HTMDM4KK3X7AY23P7RZ6JA5CNFSM4ICNIPA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ5UZSI#issuecomment-511397065, or mute the thread https://github.com/notifications/unsubscribe-auth/AABPEULX4MUDOHDST5YP7Y3P7RZ6JANCNFSM4ICNIPAQ .

[david-gang]

Certainly not out of scope!

As Manu mentioned, we have some support for this already for Rx Java streams (see RxNullabilityPropagator), but not for Java 8 streams yet. This could be implemented as another handler or by extending the Rx handler into something more general.

Not exactly sure when I'll have the bandwidth to add this support, but it's certainly in the roadmap. If you are interested @fprochazka, we would be happy to accept a PR for this. It's not a trivial task, but it's also not as daunting as changing the core of NullAway would be, since all that is needed is the right Handler implementation.

cc: @raghavan and @shas19 in case either of you two have some spare cycles. That said, it's not clear to me that is a priority internally just yet, though.

Hi @lazaroclapp , I read the documentation of handler

/**

• The general interface representing a handler.
• lers are used to model specific libraries and APIs, as opposed to general features of the
• Java language, which are handled by the nullability checker core and dataflow packages. */

Isn't stream a general feature of the java language? So is implementing it with a Handler the best way ?

[lazaroclapp]

Hi @lazaroclapp , I read the documentation of handler

/**

• The general interface representing a handler.

• Handlers are used to model specific libraries and APIs, as opposed to general features of the

• Java language, which are handled by the nullability checker core and dataflow packages. */

Isn't stream a general feature of the java language? So is implementing it with a Handler the best way ?

You could argue this one either way, because while streams are part of the standard JRE library, they are not language features in the sense of having specialized syntax (they are just method calls with some conventions that take advantage of lambdas which are a core language feature). Stream APIs are not mentioned once in the JLS.

That said, the real reason is: we have handlers support for RxJava, which is a clearly external library, and the support for Java streams is mostly analogous and uses the same hook points, so we can do it without changes to the core. I also would prefer not to do it inside the core, because it would be a one-off case where the analysis is inter-procedural instead of purely intra-procedural, and that seems like a bad fit for core (i.e. it should be possible to disable all handlers and get a predictable if less practical nullness checker out of NullAway, introduce stream handling in core and that basic nullness checker becomes "Intra-procedural, except when...").

I don't know how principled those arguments really are, though. But my feeling is that this code will be cleaner outside of core.

cc: @msridhar thoughts?

[msridhar]

I agree that Java 8 stream support should be in a handler for the reasons that @lazaroclapp outlines.

[david-gang]

i started to work on this.

[msridhar]

Let's assume this was closed by #371 and open new issues for further support of streams