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: preserve explicit nullability annotations on type variables when performing substitutions #1143

Merged
merged 36 commits into from
Feb 13, 2025

Conversation

msridhar
Copy link
Collaborator

@msridhar msridhar commented Feb 8, 2025

Fixes #1091

Consider the following code:

abstract class Test {
  abstract <V> @Nullable V foo(Function<@Nullable V, @Nullable V> f);
  void testPositive(Function<String, String> f) {
     this.<String>foo(f); // error
  }
} 

The call to foo should not type check. Since the type of its parameter f is Function<@Nullable V, @Nullable V>, with explicit @Nullable annotations on the type variables, any Function passed to foo must have @Nullable type arguments. In typechecking this code, NullAway previously substituted the type arguments for the type variables in foo just using built-in javac routines. But, this would yield a formal parameter type Function<String, String>, as the javac routine would not retain the explicit type arguments in the right places. So we would miss reporting an error. This PR fixes the substitutions and re-introduces the annotations on type variables, so we get the type Function<@Nullable String, @Nullable String> for the formal parameter at the call, and report an error correctly. Substitutions were broken in other cases as well; substituting @Nullable V for @Nullable V (where V is a type variable) yielded just V, which led to false positives (like #1091).

The main logic changes are in TypeSubstitutionUtils. We add a new RestoreNullnessAnnotationsVisitor and use it to restore nullability annotations from type variables after performing a substitution.

We also extract the TypeMetadataBuilder logic to a top-level source file, and add new methods as needed for this PR. Some of this could have been split into a separate PR but it's a bit of a pain to extract it now.

Copy link

codecov bot commented Feb 9, 2025

Codecov Report

Attention: Patch coverage is 77.77778% with 40 lines in your changes missing coverage. Please review.

Project coverage is 88.12%. Comparing base (a1df1c4) to head (9674e64).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...om/uber/nullaway/generics/TypeMetadataBuilder.java 70.78% 26 Missing ⚠️
.../uber/nullaway/generics/TypeSubstitutionUtils.java 82.05% 5 Missing and 9 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1143      +/-   ##
============================================
- Coverage     88.25%   88.12%   -0.13%     
- Complexity     2264     2265       +1     
============================================
  Files            85       86       +1     
  Lines          7320     7427     +107     
  Branches       1463     1483      +20     
============================================
+ Hits           6460     6545      +85     
- Misses          432      445      +13     
- Partials        428      437       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msridhar msridhar marked this pull request as ready for review February 10, 2025 20:25
@msridhar msridhar changed the title Handle subsitutions with explicit @Nullable on type variable preserve explicit nullability annotations on type variables when performing substitutions Feb 10, 2025
@@ -1,9 +1,18 @@
package com.uber.nullaway.generics;
Copy link
Collaborator

@akshayutture akshayutture Feb 12, 2025

Choose a reason for hiding this comment

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

What kind of substitution are we referring to here exactly (is it for generic types?)? And why are nullability annotations from type variables lost after performing a substitution - Could you clarify more in the diff description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR description is updated

@@ -634,13 +634,14 @@ public static void compareGenericTypeParameterNullabilityForCall(
}
if (enclosingType != null) {
invokedMethodType =
TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol);
TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol, config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the passing around of the config variable everywhere required for this bug fix? I don't see it being used anywhere.

Or is it an unrelated change which has been combined with this PR? If so, can we clarify this in the PR description?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The Config object is required for this bug fix. It is stored in an instance field of RestoreNullnessAnnotationsVisitor and used to check whether an annotation should be treated as @Nullable or @NonNull here:

https://github.com/uber/NullAway/pull/1143/files#diff-a6bb8226bed276672e56b162bca51775aa7ad9c2d3fc69c787bc96e36ce33507R137-R138

@msridhar msridhar changed the title preserve explicit nullability annotations on type variables when performing substitutions JSpecify: preserve explicit nullability annotations on type variables when performing substitutions Feb 13, 2025
@msridhar msridhar merged commit 6f4cda7 into master Feb 13, 2025
10 of 12 checks passed
@msridhar msridhar deleted the issue-1091 branch February 13, 2025 15:56
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

Successfully merging this pull request may close these issues.

Mismatched type parameter nullability
2 participants