-
Notifications
You must be signed in to change notification settings - Fork 299
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add support for Preconditions.checkArgument through custom CFG transl…
…ation (#608) 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. 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](typetools/checker-framework#5156))
- Loading branch information
1 parent
08c50cf
commit 3b90d3b
Showing
9 changed files
with
503 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
185 changes: 185 additions & 0 deletions
185
nullaway/src/main/java/com/uber/nullaway/dataflow/cfg/NullAwayCFGBuilder.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| package com.uber.nullaway.dataflow.cfg; | ||
|
|
||
| import com.google.common.base.Preconditions; | ||
| import com.sun.source.tree.ExpressionTree; | ||
| import com.sun.source.tree.MethodInvocationTree; | ||
| import com.sun.source.tree.ThrowTree; | ||
| import com.sun.source.tree.Tree; | ||
| import com.sun.source.tree.TreeVisitor; | ||
| import com.sun.source.util.TreePath; | ||
| import com.uber.nullaway.handlers.Handler; | ||
| import javax.annotation.processing.ProcessingEnvironment; | ||
| import javax.lang.model.type.TypeMirror; | ||
| import org.checkerframework.nullaway.dataflow.cfg.ControlFlowGraph; | ||
| import org.checkerframework.nullaway.dataflow.cfg.UnderlyingAST; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.CFGBuilder; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.CFGTranslationPhaseOne; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.CFGTranslationPhaseThree; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.CFGTranslationPhaseTwo; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.ConditionalJump; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.ExtendedNode; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.Label; | ||
| import org.checkerframework.nullaway.dataflow.cfg.builder.PhaseOneResult; | ||
| import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; | ||
| import org.checkerframework.nullaway.dataflow.cfg.node.Node; | ||
| import org.checkerframework.nullaway.dataflow.cfg.node.ThrowNode; | ||
| import org.checkerframework.nullaway.javacutil.AnnotationProvider; | ||
| import org.checkerframework.nullaway.javacutil.BasicAnnotationProvider; | ||
| import org.checkerframework.nullaway.javacutil.trees.TreeBuilder; | ||
|
|
||
| /** | ||
| * A NullAway specific CFGBuilder subclass, which allows to more directly control the AST to CFG | ||
| * translation performed by the checker framework. | ||
| * | ||
| * <p>This holds the static method {@link #build(TreePath, UnderlyingAST, boolean, boolean, | ||
| * ProcessingEnvironment, Handler)}, called to perform the CFG translation, and the class {@link | ||
| * NullAwayCFGTranslationPhaseOne}, which extends {@link CFGTranslationPhaseOne} and adds hooks for | ||
| * the NullAway handlers mechanism and some utility methods. | ||
| */ | ||
| public final class NullAwayCFGBuilder extends CFGBuilder { | ||
|
|
||
| /** This class should never be instantiated. */ | ||
| private NullAwayCFGBuilder() {} | ||
|
|
||
| /** | ||
| * This static method produces a new CFG representation given a method's (or lambda/initializer) | ||
| * body. | ||
| * | ||
| * <p>It is analogous to {@link CFGBuilder#build(TreePath, UnderlyingAST, boolean, boolean, | ||
| * ProcessingEnvironment)}, but it also takes a handler to be called at specific extention points | ||
| * during the CFG translation. | ||
| * | ||
| * @param bodyPath the TreePath to the body of the method, lambda, or initializer. | ||
| * @param underlyingAST the AST that underlies the control frow graph | ||
| * @param assumeAssertionsEnabled can assertions be assumed to be disabled? | ||
| * @param assumeAssertionsDisabled can assertions be assumed to be enabled? | ||
| * @param env annotation processing environment containing type utilities | ||
| * @param handler a NullAway handler or chain of handlers (through {@link | ||
| * com.uber.nullaway.handlers.CompositeHandler} | ||
| * @return a control flow graph | ||
| */ | ||
| public static ControlFlowGraph build( | ||
| TreePath bodyPath, | ||
| UnderlyingAST underlyingAST, | ||
| boolean assumeAssertionsEnabled, | ||
| boolean assumeAssertionsDisabled, | ||
| ProcessingEnvironment env, | ||
| Handler handler) { | ||
| TreeBuilder builder = new TreeBuilder(env); | ||
| AnnotationProvider annotationProvider = new BasicAnnotationProvider(); | ||
| CFGTranslationPhaseOne phase1translator = | ||
| new NullAwayCFGTranslationPhaseOne( | ||
| builder, | ||
| annotationProvider, | ||
| assumeAssertionsEnabled, | ||
| assumeAssertionsDisabled, | ||
| env, | ||
| handler); | ||
| PhaseOneResult phase1result = phase1translator.process(bodyPath, underlyingAST); | ||
| ControlFlowGraph phase2result = CFGTranslationPhaseTwo.process(phase1result); | ||
| ControlFlowGraph phase3result = CFGTranslationPhaseThree.process(phase2result); | ||
| return phase3result; | ||
| } | ||
|
|
||
| /** | ||
| * A NullAway specific subclass of the Checker Framework's {@link CFGTranslationPhaseOne}, | ||
| * augmented with handler extension points and some utility methods meant to be called from | ||
| * handlers to customize the AST to CFG translation. | ||
| */ | ||
| public static class NullAwayCFGTranslationPhaseOne extends CFGTranslationPhaseOne { | ||
|
|
||
| private final Handler handler; | ||
|
|
||
| /** | ||
| * Create a new NullAway phase one translation visitor. | ||
| * | ||
| * @param builder a TreeBuilder object (used to create synthetic AST nodes to feed to the | ||
| * translation process) | ||
| * @param annotationProvider an {@link AnnotationProvider}. | ||
| * @param assumeAssertionsEnabled can assertions be assumed to be disabled? | ||
| * @param assumeAssertionsDisabled can assertions be assumed to be enabled? | ||
| * @param env annotation processing environment containing type utilities | ||
| * @param handler a NullAway handler or chain of handlers (through {@link | ||
| * com.uber.nullaway.handlers.CompositeHandler} | ||
| */ | ||
| public NullAwayCFGTranslationPhaseOne( | ||
| TreeBuilder builder, | ||
| AnnotationProvider annotationProvider, | ||
| boolean assumeAssertionsEnabled, | ||
| boolean assumeAssertionsDisabled, | ||
| ProcessingEnvironment env, | ||
| Handler handler) { | ||
| super(builder, annotationProvider, assumeAssertionsEnabled, assumeAssertionsDisabled, env); | ||
| this.handler = handler; | ||
| } | ||
|
|
||
| @SuppressWarnings("NullAway") // (Void)null issue | ||
| private void scanWithVoid(Tree tree) { | ||
| this.scan(tree, (Void) null); | ||
| } | ||
|
|
||
| /** | ||
| * Obtain the type mirror for a given class, used for exception throwing. | ||
| * | ||
| * @param klass a Java class | ||
| * @return the corresponding type mirror | ||
| */ | ||
| public TypeMirror classToErrorType(Class<?> klass) { | ||
| return this.getTypeMirror(klass); | ||
| } | ||
|
|
||
| /** | ||
| * Extend the CFG to throw an exception if the passed expression node evaluates to false. | ||
| * | ||
| * @param booleanExpressionNode a CFG Node representing a boolean expression. | ||
| * @param errorType the type of the exception to throw if booleanExpressionNode evaluates to | ||
| * false. | ||
| */ | ||
| public void insertThrowOnFalse(Node booleanExpressionNode, TypeMirror errorType) { | ||
| Tree tree = booleanExpressionNode.getTree(); | ||
| Preconditions.checkArgument( | ||
| tree instanceof ExpressionTree, | ||
| "Argument booleanExpressionNode must represent a boolean expression"); | ||
| ExpressionTree booleanExpressionTree = (ExpressionTree) booleanExpressionNode.getTree(); | ||
| Preconditions.checkNotNull(booleanExpressionTree); | ||
| Label falsePreconditionEntry = new Label(); | ||
| Label endPrecondition = new Label(); | ||
| this.scanWithVoid(booleanExpressionTree); | ||
| ConditionalJump cjump = new ConditionalJump(endPrecondition, falsePreconditionEntry); | ||
| this.extendWithExtendedNode(cjump); | ||
| this.addLabelForNextNode(falsePreconditionEntry); | ||
| ExtendedNode exNode = | ||
| this.extendWithNodeWithException( | ||
| new ThrowNode( | ||
| new ThrowTree() { | ||
| // Dummy throw tree, unused. We could use null here, but that violates nullness | ||
| // typing. | ||
| @Override | ||
| public ExpressionTree getExpression() { | ||
| return booleanExpressionTree; | ||
| } | ||
|
|
||
| @Override | ||
| public Kind getKind() { | ||
| return Kind.THROW; | ||
| } | ||
|
|
||
| @Override | ||
| public <R, D> R accept(TreeVisitor<R, D> visitor, D data) { | ||
| return visitor.visitThrow(this, data); | ||
| } | ||
| }, | ||
| booleanExpressionNode, | ||
| this.getProcessingEnvironment().getTypeUtils()), | ||
| errorType); | ||
| exNode.setTerminatesExecution(true); | ||
| this.addLabelForNextNode(endPrecondition); | ||
| } | ||
|
|
||
| @Override | ||
| public MethodInvocationNode visitMethodInvocation(MethodInvocationTree tree, Void p) { | ||
| MethodInvocationNode originalNode = super.visitMethodInvocation(tree, p); | ||
| return handler.onCFGBuildPhase1AfterVisitMethodInvocation(this, tree, originalNode); | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
44 changes: 44 additions & 0 deletions
44
nullaway/src/main/java/com/uber/nullaway/handlers/PreconditionsHandler.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package com.uber.nullaway.handlers; | ||
|
|
||
| import com.google.common.base.Preconditions; | ||
| import com.google.errorprone.util.ASTHelpers; | ||
| import com.sun.source.tree.MethodInvocationTree; | ||
| import com.sun.tools.javac.code.Symbol; | ||
| import com.uber.nullaway.dataflow.cfg.NullAwayCFGBuilder; | ||
| import javax.annotation.Nullable; | ||
| import javax.lang.model.element.Name; | ||
| import javax.lang.model.type.TypeMirror; | ||
| import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode; | ||
|
|
||
| public class PreconditionsHandler extends BaseNoOpHandler { | ||
|
|
||
| private static final String PRECONDITIONS_CLASS_NAME = "com.google.common.base.Preconditions"; | ||
| private static final String CHECK_ARGUMENT_METHOD_NAME = "checkArgument"; | ||
| private static final String CHECK_STATE_METHOD_NAME = "checkState"; | ||
|
|
||
| @Nullable private Name preconditionsClass; | ||
| @Nullable private Name checkArgumentMethod; | ||
| @Nullable private Name checkStateMethod; | ||
| @Nullable TypeMirror preconditionErrorType; | ||
|
|
||
| @Override | ||
| public MethodInvocationNode onCFGBuildPhase1AfterVisitMethodInvocation( | ||
| NullAwayCFGBuilder.NullAwayCFGTranslationPhaseOne phase, | ||
| MethodInvocationTree tree, | ||
| MethodInvocationNode originalNode) { | ||
| Symbol.MethodSymbol callee = ASTHelpers.getSymbol(tree); | ||
| if (preconditionsClass == null) { | ||
| preconditionsClass = callee.name.table.fromString(PRECONDITIONS_CLASS_NAME); | ||
| checkArgumentMethod = callee.name.table.fromString(CHECK_ARGUMENT_METHOD_NAME); | ||
| checkStateMethod = callee.name.table.fromString(CHECK_STATE_METHOD_NAME); | ||
| preconditionErrorType = phase.classToErrorType(IllegalArgumentException.class); | ||
| } | ||
| Preconditions.checkNotNull(preconditionErrorType); | ||
| if (callee.enclClass().getQualifiedName().equals(preconditionsClass) | ||
| && (callee.name.equals(checkArgumentMethod) || callee.name.equals(checkStateMethod)) | ||
| && callee.getParameters().size() > 0) { | ||
| phase.insertThrowOnFalse(originalNode.getArgument(0), preconditionErrorType); | ||
| } | ||
| return originalNode; | ||
| } | ||
| } |
Oops, something went wrong.