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

Add support for Preconditions.checkArgument through custom CFG translation #608

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

lazaroclapp
Copy link
Collaborator

@lazaroclapp lazaroclapp commented Jun 8, 2022

See #47 for discussion.

This PR adds support for Preconditions.checkArgument(...) and Preconditions.checkState(...)
throw a new PreconditionsHandler handler.

In order to implement said handler, we need a custom CFG translation pipeline in NullAway
(NullAwayCFGBuilder) which subclasses the Checker Framework's CFGBuilder
(which we were using directly before this change). This pipeline allows us to add handler
callbacks during the AST to CFG translation process. At the moment, we add a single
such callback, right after visiting a MethodInvocationTree. We also add a more or less
generic method to insert a conditional jump and a throw based on a given boolean expression
node.

Abstracting some details about our AST and CFG structures, we translate:

Preconditions.checkArgument($someBoolExpr[, $someString]);

Into something resembling:

Preconditions.checkArgument($someBoolExpr[, $someString]);
if ($someBoolExpr) {
   throw new IllegalArgumentException();
}

Note that this causes $someBoolExpr to be evaluated twice. This is necessary based on how
NullAway evaluates branch conditionals, since we currently do not track boolean values through
our dataflow (e.g. boolean b = (o == null); if (b) { throw [...] }; o.toString(); produces a dereference
according to NullAway, independent on whether that code was added by rewriting or directly on the
source.

That said, note that obviously this doesn't change the code being compiled or bytecode being produced,
the rewrite is only ever used by/visible to NullAway itself.

Finally, our implementation of NullAwayCFGBuilder and particularly NullAwayCFGTranslationPhaseOne
in this PR, depend on a Checker Framework APIs that are currently private. We are simultaneously
submitting a PR to change the visibility of these APIs (see CheckerFramework#5156)

@lazaroclapp
Copy link
Collaborator Author

Important: This will not build (and shouldn't land) until the corresponding CF PR is landed and a new CF version cut.

@@ -41,7 +41,7 @@ if (project.hasProperty("epApiVersion")) {

def versions = [
asm : "9.3",
checkerFramework : "3.21.3",
checkerFramework : "3.22.2-SNAPSHOT", //"3.21.3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: Change before landing, but after CF has cut a new release.

@msridhar
Copy link
Collaborator

msridhar commented Jun 9, 2022

FWIW, overall this approach looks fine to me!

@lazaroclapp lazaroclapp force-pushed the lazaro_support_preconditions_checkarg_2 branch from abaa1f9 to cea4fb8 Compare June 10, 2022 21:24
@msridhar
Copy link
Collaborator

I pulled in today's CF release. Did not do all the other relevant cleanup (like removing mavenLocal())

@coveralls
Copy link

coveralls commented Jun 14, 2022

Pull Request Test Coverage Report for Build #873

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.05%) to 92.131%

Files with Coverage Reduction New Missed Lines %
../nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java 1 91.67%
../nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java 1 96.55%
../nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java 4 95.45%
Totals Coverage Status
Change from base Build #872: 0.05%
Covered Lines: 4824
Relevant Lines: 5236

💛 - Coveralls

@lazaroclapp
Copy link
Collaborator Author

I pulled in today's CF release. Did not do all the other relevant cleanup (like removing mavenLocal())

Let's do this the slightly slower but more CHANGELOG friendly way: #610 😉

@msridhar
Copy link
Collaborator

I ran our benchmarks before and after this PR and saw no increase in NullAway overhead; so this change seems to be safe from that standpoint.

lazaroclapp added a commit that referenced this pull request Jun 16, 2022
Needed for our changes in #608, which requires visibility into some previously
`package` or `private` level internals of Checker Framework's CFG:

- typetools/checker-framework#5152
- typetools/checker-framework#5156
@lazaroclapp lazaroclapp force-pushed the lazaro_support_preconditions_checkarg_2 branch from 5b46f71 to 0b96f52 Compare June 16, 2022 22:12
@msridhar
Copy link
Collaborator

@lazaroclapp ping me when this is ready for a full review

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

LGTM, this is great!

@lazaroclapp lazaroclapp merged commit 3b90d3b into master Jun 17, 2022
@lazaroclapp lazaroclapp deleted the lazaro_support_preconditions_checkarg_2 branch June 17, 2022 20:00
lazaroclapp added a commit that referenced this pull request Jun 20, 2022
This is a follow up on #595. See that PR for some of the soundness
caveats related to method overriding and the nullability of Void
in our current implementation.

This does, however, avoid some otherwise awkward suppressions,
as shown in #608.

There are two correct ways of hadling Void:

a) Having it default to `@Nullable` as a type, which requires us
   adding a "default nullability" for some types/classes and explictly
   contradicts JSpecify (jspecify/jspecify#51)
b) Supporting generics, and requiring explictly annotating `@Nullable Void`.
   This will probably also require third-party libraries which we consider
   annotated (such as CF), to adopt this convention.

I believe (b) is the way forward long-term, which means that, for now,
this hack might be the best we can do without generics support. Once
NullAway supports generics, both this change and #595 should be reverted.
lazaroclapp added a commit that referenced this pull request Jun 21, 2022
This is a follow up on #595. See that PR for some of the soundness
caveats related to method overriding and the nullability of Void
in our current implementation.

This does, however, avoid some otherwise awkward suppressions,
as shown in #608.

There are two correct ways of hadling Void:

a) Having it default to `@Nullable` as a type, which requires us
   adding a "default nullability" for some types/classes and explictly
   contradicts JSpecify (jspecify/jspecify#51)
b) Supporting generics, and requiring explictly annotating `@Nullable Void`.
   This will probably also require third-party libraries which we consider
   annotated (such as CF), to adopt this convention.

I believe (b) is the way forward long-term, which means that, for now,
this hack might be the best we can do without generics support. Once
NullAway supports generics, both this change and #595 should be reverted.
lazaroclapp added a commit that referenced this pull request Jun 21, 2022
This is a follow up on #595. See that PR for some of the soundness
caveats related to method overriding and the nullability of Void
in our current implementation.

This does, however, avoid some otherwise awkward suppressions,
as shown in #608.

There are two correct ways of handling Void:

a) Having it default to `@Nullable` as a type, which requires us
   adding a "default nullability" for some types/classes and explicitly
   contradicts JSpecify (jspecify/jspecify#51)
b) Supporting generics, and requiring explicitly annotating `@Nullable Void`.
   This will probably also require third-party libraries which we consider
   annotated (such as CF), to adopt this convention.

I believe (b) is the way forward long-term, which means that, for now,
this hack might be the best we can do without generics support. Once
NullAway supports generics, both this change and #595 should be reverted.
msridhar added a commit to typetools/checker-framework that referenced this pull request Aug 14, 2023
In #5955, the `ExtendedNode` and `Label` classes from the dataflow
framework were changed from public to package-private. However, NullAway
relies on these classes being public (see
#5156,
#5152, and
uber/NullAway#608). This PR makes the classes
public again and adds an explanatory comment.
wmdietl pushed a commit to eisop/checker-framework that referenced this pull request Oct 16, 2023
In typetools#5955, the `ExtendedNode` and `Label` classes from the dataflow
framework were changed from public to package-private. However, NullAway
relies on these classes being public (see
typetools#5156,
typetools#5152, and
uber/NullAway#608). This PR makes the classes
public again and adds an explanatory comment.
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.

None yet

3 participants