Implement Java 8 type argument inference #979

Open
smillst opened this Issue Dec 6, 2016 · 5 comments

Comments

Projects
None yet
5 participants
@smillst
Contributor

smillst commented Dec 6, 2016

The Checker Framework should implement Java 8 type inference for method type arguments, new class trees that use diamonds, and raw types. (Currently, the Checker Framework implements Java 7 type inference.)
This involves completely replacing/rewriting the implementation in the framework.utils.typeinference package. Also, the code that adds type arguments to raw types (AnnotatedDeclaredType#getTypeArguments) and the code that infers type arguments for diamonds (AnnotatedTypeFactory#fromNewClass) should be changed to use type argument inference.

The Java 8 algorithm specified in the JLS cannot be used as is. It must be adapted to use with annotated types. In particular, annotated types complicate the JLS algorithm in following ways.

  1. In the JLS, if there is a subtyping constraint against a null type, the constraint is reduced to "true".[JLS #18.2.3] However, if null isn't annotation with the bottom qualifier, then the constraint should be reduced to new constraint where the primary annotation of the type must be a subtype of the annotation on null.

  2. If a use of a type variable has a primary annotation, then constraints against that use should not include that annotation. In other words, that constraint should be against the Java type rather than the annotated type. But if the type system has a multigraph qualifier hierarchy, then there should still be a constraint against an annotated type for some of the hierarchies. (See examples in checker/tests/nullness/generics/MethodTypeVars7.java.)

  3. A class can be marked with @Covariant to signify that one or more of its type parameters can be treated covariantly. Covariant type parameters should generate subtyping constraints rather than equality constraints.

  4. Method postconditions. See #1345.

Also, once Java 8 inference is implemented, the field AnnotatedWildcardType#uninferredTypeArgument should be removed and the -AconservativeUninferredTypeArguments option should also be removed.

The following issues were closed in favor of this issue: #402, #404, #531, #953, #980, #1032. The test cases from those issues are now in framework/tests/all-systems/java8inferrence/ and checker/tests/nullness/java8inferrence/.

@wmdietlGC

This comment has been minimized.

Show comment
Hide comment
@wmdietlGC

wmdietlGC Feb 14, 2017

Contributor

As people are adopting Stream-based APIs, I'm running into this issue very frequently.

I have a patch that instead of raising errors like:

Issue404.java:17: error: [return.type.incompatible] incompatible types in return.
        return strings.stream().map(String::trim).collect(Collectors.toSet());
                                                         ^
  found   : @Initialized @NonNull Set<? extends @Initialized @Nullable Object>
  required: @Initialized @NonNull Set<@Initialized @NonNull String>

allows uninferred wildcards as subtypes. The Nullness Checker then successfully compiles all examples in tests/all-systems/java8inference.
This is unsound, but there is currently no test case that would require an error. We should add one to highlight the problem.

The current error message isn't useful and very frustrating.
I would suggest making the default behavior unsound (it hurts me to say that...) and adding an option to use the current conservative behavior.

Let me know what you think.

Contributor

wmdietlGC commented Feb 14, 2017

As people are adopting Stream-based APIs, I'm running into this issue very frequently.

I have a patch that instead of raising errors like:

Issue404.java:17: error: [return.type.incompatible] incompatible types in return.
        return strings.stream().map(String::trim).collect(Collectors.toSet());
                                                         ^
  found   : @Initialized @NonNull Set<? extends @Initialized @Nullable Object>
  required: @Initialized @NonNull Set<@Initialized @NonNull String>

allows uninferred wildcards as subtypes. The Nullness Checker then successfully compiles all examples in tests/all-systems/java8inference.
This is unsound, but there is currently no test case that would require an error. We should add one to highlight the problem.

The current error message isn't useful and very frustrating.
I would suggest making the default behavior unsound (it hurts me to say that...) and adding an option to use the current conservative behavior.

Let me know what you think.

mernst added a commit that referenced this issue Feb 23, 2017

Work-around for Issue #979
Includes an option to get the current conservative behavior.
@smillst

This comment has been minimized.

Show comment
Hide comment
@smillst

smillst Feb 24, 2017

Contributor

#979 (comment) was implemented via bfeee16.

Contributor

smillst commented Feb 24, 2017

#979 (comment) was implemented via bfeee16.

@typetools typetools deleted a comment from kelloggm Sep 11, 2017

@typetools typetools deleted a comment from kelloggm Sep 11, 2017

smillst added a commit that referenced this issue Sep 13, 2017

@msridhar

This comment has been minimized.

Show comment
Hide comment
@msridhar

msridhar Nov 21, 2017

Contributor

Is there any rough estimate on when this issue might get addressed? We have an internal checker for stream-related code and we're running into this limitation frequently. I understand it must be a significant amount of work to fix, though, and it may take a while. Just wondering how far we should attempt to go with workarounds 😄

Contributor

msridhar commented Nov 21, 2017

Is there any rough estimate on when this issue might get addressed? We have an internal checker for stream-related code and we're running into this limitation frequently. I understand it must be a significant amount of work to fix, though, and it may take a while. Just wondering how far we should attempt to go with workarounds 😄

@mernst

This comment has been minimized.

Show comment
Hide comment
@mernst

mernst Nov 21, 2017

Member

@msridhar , Suzanne has been working on this issue since the summer, spending 2-3 days per week. So, she has devoted 1-3 months to it so far. It will probably be months before we can close the issue, unless by a miracle no other distractions come up for her. This suggests that you may want to put workarounds in place. Sorry about that. We are looking forward to removing a number of workarounds in the Checker Framework proper, once the issue is fixed.

Member

mernst commented Nov 21, 2017

@msridhar , Suzanne has been working on this issue since the summer, spending 2-3 days per week. So, she has devoted 1-3 months to it so far. It will probably be months before we can close the issue, unless by a miracle no other distractions come up for her. This suggests that you may want to put workarounds in place. Sorry about that. We are looking forward to removing a number of workarounds in the Checker Framework proper, once the issue is fixed.

@justmin

This comment has been minimized.

Show comment
Hide comment
@justmin

justmin Dec 7, 2017

Hello,
Optional::map doesn't accept @nullable #1633 could be fixed (or workarounded?) quickly.
Could you please include this changes in the next release?
We use Stream::map very often and need for workarounding it is the main argument against using CF, at least in my project

justmin commented Dec 7, 2017

Hello,
Optional::map doesn't accept @nullable #1633 could be fixed (or workarounded?) quickly.
Could you please include this changes in the next release?
We use Stream::map very often and need for workarounding it is the main argument against using CF, at least in my project

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment