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

Generics checks for method overriding #755

Merged
merged 57 commits into from
Aug 12, 2023

Conversation

NikitaAware
Copy link
Contributor

@NikitaAware NikitaAware commented Mar 31, 2023

This PR implements JSpecify-related generics checks for method overriding, including:

  • checks that overrides of generic methods respect subtyping of nullability qualifiers
  • proper handling of calls to generic methods, taking into account (qualified) types passed as type parameters

See the new tests and docs for further examples.

Note: This PR does not handle overriding in explicitly-typed anonymous inner classes. Those will be handled in a follow-up PR, to keep the PR size manageable.

@CLAassistant
Copy link

CLAassistant commented Mar 31, 2023

CLA assistant check
All committers have signed the CLA.

@msridhar msridhar changed the base branch from master to generic-type-pretty April 27, 2023 13:05
@msridhar msridhar added the jspecify Related to support for jspecify standard (see jspecify.dev) label May 6, 2023
@msridhar
Copy link
Collaborator

Thanks for the great review @lazaroclapp! I've pushed a few fixes and cleanups, and I think this is ready for another pass. In particular, while testing the @Contract interactions I discovered a significant bug, where we didn't properly check a direct dereference of a method call expression; that is fixed in 97ec62a. Also note that I have one follow-up question for you, here.

@msridhar
Copy link
Collaborator

/benchmark

@github-actions
Copy link

Main Branch:

Benchmark                          Mode  Cnt   Score   Error  Units
AutodisposeBenchmark.compile      thrpt   25   8.059 ± 0.060  ops/s
CaffeineBenchmark.compile         thrpt   25   1.840 ± 0.017  ops/s
DFlowMicroBenchmark.compile       thrpt   25  22.372 ± 0.184  ops/s
NullawayReleaseBenchmark.compile  thrpt   25   1.112 ± 0.012  ops/s

With This PR:

Benchmark                          Mode  Cnt   Score   Error  Units
AutodisposeBenchmark.compile      thrpt   25   8.075 ± 0.071  ops/s
CaffeineBenchmark.compile         thrpt   25   1.848 ± 0.012  ops/s
DFlowMicroBenchmark.compile       thrpt   25  22.193 ± 0.405  ops/s
NullawayReleaseBenchmark.compile  thrpt   25   1.108 ± 0.014  ops/s

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.

Ok, finally did a pass on this! There are a few nits that occurred to me, but nothing blocking :)

@msridhar
Copy link
Collaborator

Thanks for the detailed review @lazaroclapp! Addressed all remaining comments and will land this one!

@msridhar msridhar enabled auto-merge (squash) August 12, 2023 22:07
@msridhar msridhar merged commit 01ee4d7 into uber:master Aug 12, 2023
7 checks passed
msridhar added a commit that referenced this pull request Sep 29, 2023
…s classes (#808)

This completes a task lingering from #755, which did not handle
overriding in anonymous classes. This case is slightly tricky as javac
does not preserve annotations on type arguments in the types it computes
for anonymous classes. To fix, we pass a `Symbol` where we were passing
a `Type` in a couple places, as then we can call `Symbol.isAnonymous()`
to check for an anonymous type. In such a case, we try to get the type
from the enclosing `NewClassTree` of the overriding method. Note that
this PR still does not handle anonymous classes that use the diamond
operator; we add a test to show the gaps.
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