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: support inference for calls to generic methods #1075

Open
msridhar opened this issue Nov 14, 2024 · 6 comments
Open

JSpecify: support inference for calls to generic methods #1075

msridhar opened this issue Nov 14, 2024 · 6 comments
Labels
jspecify Related to support for jspecify standard (see jspecify.dev)

Comments

@msridhar
Copy link
Collaborator

msridhar commented Nov 14, 2024

This will be a significant challenge in general, but maybe we can handle the common cases and do something useful without having a full technique. See here for one test case:

"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static class Foo {",
" <C extends @Nullable Object> void foo(C c, Visitor<C> visitor) {",
" visitor.visit(this, c);",
" }",
" }",
" static abstract class Visitor<C extends @Nullable Object> {",
" abstract void visit(Foo foo, C c);",
" }",
" static class MyVisitor extends Visitor<@Nullable Void> {",
" @Override",
" void visit(Foo foo, @Nullable Void c) {}",
" }",
" static void test(Foo f) {",
" // this is safe",
" f.foo(null, new MyVisitor());",
" }",
"}")

@msridhar msridhar added the jspecify Related to support for jspecify standard (see jspecify.dev) label Nov 14, 2024
@agrieve
Copy link

agrieve commented Jan 8, 2025

Hit this as well.

The specific case I ran into was in this form:

@NullMarked
public class Foo {
  public static class Key<T extends @Nullable Object> {}

  private static final Key<@Nullable Foo> KEY = new Key();

  public static <T extends @Nullable Object> void setValue(Key<T> key, T value) {
  }

  public static void main() {
    setValue(KEY, null);
  }
}

@msridhar
Copy link
Collaborator Author

msridhar commented Jan 8, 2025

Thanks for the example! We are working on this.

@cpovirk
Copy link

cpovirk commented Jan 23, 2025

FYI, this (or something very similar) came up when Dropwizard upgraded to a new version of Caffeine. Fortunately, in their case, they appear to be better off just removing their usage of @Nullable.

@agrieve
Copy link

agrieve commented Mar 4, 2025

Heh, this is coming up more with the new release because now our annotations within generics are being noticed more :P.

Here's a simpler example:

import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;

@NullMarked
public class Foo {

  public static void test(@Nullable Object foo) {
      helper(foo);
  }

  private static <T extends @Nullable Object> void helper(T value) {
      System.out.println(value);
  }
}
Foo.java:10: warning: [NullAway] passing @Nullable parameter 'foo' where @NonNull is required
      helper(foo);
             ^
    (see http://t.uber.com/nullaway )

The best work-arounds I've found so far are:

  1. Use explicit generics at callsite:
  public static void test(@Nullable Object foo) {
      Foo.<@Nullable Object>helper(foo);
  }
  1. Marking the helper as @NullUnmarked:
  @NullUnmarked
  private static <T extends @Nullable Object> void helper(T value) {
      System.out.println(value);
  }

Just sharing in case it's helpful to anyone else.

@msridhar
Copy link
Collaborator Author

msridhar commented Mar 4, 2025

I hope that #1131 will cover many common cases here, but it's a subtle change and we're still working on it. @agrieve once that PR is landed I may ask if you can test on Chromium with a snapshot build.

I'm glad to hear that explicit generics have worked for you. That what I would have hoped / expected, though we definitely aim to eliminate the need for these as much as possible.

@msridhar
Copy link
Collaborator Author

msridhar commented Mar 4, 2025

Actually, small correction, we will handle the specific case from #1075 (comment) in a follow-up to #1131

@msridhar msridhar marked this as a duplicate of #1157 Mar 5, 2025
@msridhar msridhar marked this as a duplicate of #1158 Mar 5, 2025
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 7, 2025
Ironically due to uber/NullAway#1075 this
cannot be null marked. Inferred nullness does not work for generic
methods.

Bug: 389129271
Change-Id: I95f2099396b4699a2eab5e57f382d0cb98b8f4ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6437372
Commit-Queue: Calder Kitagawa <[email protected]>
Reviewed-by: Andrew Grieve <[email protected]>
Auto-Submit: Calder Kitagawa <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1443733}
github-actions bot pushed a commit to kaidokert/chrome_base_mirror that referenced this issue Apr 8, 2025
Ironically due to uber/NullAway#1075 this
cannot be null marked. Inferred nullness does not work for generic
methods.

Bug: 389129271
Change-Id: I95f2099396b4699a2eab5e57f382d0cb98b8f4ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6437372
Commit-Queue: Calder Kitagawa <[email protected]>
Reviewed-by: Andrew Grieve <[email protected]>
Auto-Submit: Calder Kitagawa <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1443733}
NOKEYCHECK=True
GitOrigin-RevId: 7bfd85ea3768994f3ea270cef10bcfeb7d320789
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

No branches or pull requests

3 participants