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

JSpecify: initial checks for generic type compatibility at assignments #715

Merged
merged 204 commits into from
Feb 7, 2023

Conversation

NikitaAware
Copy link
Contributor

@NikitaAware NikitaAware commented Jan 12, 2023

This pull request adds the identical type parameter nullability checks for both sides of an assignment.
for example
NullableTypeParam<@Nullable String> nullableTypeParam = new NullableTypeParam<String>(); should generate an error as the type parameters of both sides of the assignment do not have the same Nullability annotations.
This PR covers the following cases:

  • checks for the variable instantiation A<String> a = new A<@Nullable String>();
  • checks for the assignments
    A<@Nullable String> t1 = new A<@Nullable String>(); A<String> t2; t2 = t1;
  • LHS and RHS with the exact same types
    A<String> a = new A<@Nullable String>();
  • RHS being a subtype of LHS
class Test {
  class D<P extends @Nullable Object> {}
  class B<P extends @Nullable Object> extends D<P>{}
  void test1() {
      // BUG: Diagnostic contains: Cannot assign from type
      D<String> f = new B<@Nullable String>();
  }
}
  • nested generics
    A<B<String>, String> a = new A<B<@Nullable String>>();

The PR does not handle pseudo-assignments due to parameter passing / returns; that support will come in a follow-up PR. Also note that this PR only changes NullAway behavior when JSpecifyMode is enabled.

@msridhar
Copy link
Collaborator

I may tweak the lambda and diamond operator test cases to be a bit clearer

This is now done. To summarize, we don't seem to crash for lambdas / method refs / diamond operators, but (unsurprisingly) we miss some bugs.

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor nits remaining! Looks fine to land to me once those are addressed! 🎉

Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two comments on the tests that should also start with // TODO. That said, other than that, this PR LGTM. @msridhar, if this ok by you, we could make the edits in the UI and land (or wait for @NikitaAware to make the edit, either works for me)!

@lazaroclapp lazaroclapp enabled auto-merge (squash) February 7, 2023 17:57
@lazaroclapp
Copy link
Collaborator

Cool! Should land automatically once CI passes. Again, thanks for all the great work @NikitaAware 🎉 !

@lazaroclapp lazaroclapp merged commit 14d3693 into uber:master Feb 7, 2023
msridhar pushed a commit that referenced this pull request Mar 9, 2023
For a conditional expression _e_, the type parameter nullability annotations of the true-subpart and false-subpart should match the annotations required by _e_'s context.  E.g., if _e_ is the right-hand side of an assignment, the annotations should match those for the type parameters for the assignment's left-hand side:
```java
// this is fine
A<@nullable String> t = condition? new A<@nullable String> : new A<@nullable String>();
// this is bad
A<@nullable String> t2 = condition? new A<String> : new A<String>();
// this is also bad
A<@nullable String>  t1 = condition ? new A<String>(): new A<@nullable String>()
```
Other contexts include returns, parameter passing, and being nested inside another conditional expression.  In our experience, it seems that `javac` captures the type parameter nullability required by _e_'s context in the type of _e_ itself.  So, our handling of conditional expressions simply checks that the types of both the true and false sub-expressions of _e_ are assignable to (i.e., a subtype of) the type of _e_, using the same machinery for checking assignments from #715.
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jspecify Related to support for jspecify standard (see jspecify.dev)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants