[PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Another partial review. 
Thanks!
Anna.


================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:312
@@ +311,3 @@
+                                      CheckerContext &C) const {
+  auto RetExpr = S->getRetValue();
+  if (!RetExpr)
----------------
Please, explain what this method does and add a TODO comment if this needs got be improved in the future.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:332
@@ +331,3 @@
+  ConditionTruthVal Nullness =
+      State->isNull(RetSVal.castAs<DefinedOrUnknownSVal>());
+  bool IsNotNull = Nullness.isConstrainedFalse();
----------------
You could cast and check "if (RetSVal.isUndef())" at the same time.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:376
@@ +375,3 @@
+
+void NullabilityChecker::checkPreCall(const CallEvent &Call,
+                                      CheckerContext &C) const {
----------------
Please, add a high-level comment about what this method does.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:398
@@ +397,3 @@
+        !Param->getType()->isReferenceType() &&
+        !Param->getType()->isObjCObjectPointerType()) {
+      continue;
----------------
Should you use the existing helper function here as well?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:402
@@ +401,3 @@
+
+    ConditionTruthVal Nullness =
+        State->isNull(ArgSVal.castAs<DefinedOrUnknownSVal>());
----------------
A lot of code here looks like a copy and paste from the "checkPreStmt(const ReturnStmt *S," above. Please, factor into a subroutine.


================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:453
@@ +452,3 @@
+    // Marking memory regions of variables to be nullable would be a mistake.
+    // Marking otherwise is redundant.
+    if (!Region->getAs<SymbolicRegion>())
----------------
I do not understand this comment. You are setting the nullability below..

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:463
@@ +462,3 @@
+  if (State != OrigState)
+    C.addTransition(State);
+}
----------------
Is calling a function/method with multiple arguments with set nullability tested?

Looks like testArgumentTracking could be used for this but it is never called.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:475
@@ +474,3 @@
+  QualType RetType = FuncType->getReturnType();
+  if (!RetType->isPointerType() && !RetType->isObjCObjectPointerType())
+    return;
----------------
Use a subroutine.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:496
@@ +495,3 @@
+                                              CheckerContext &C) const {
+  auto Decl = M.getDecl();
+  if (!Decl)
----------------
The next 11 lines could be factored out. They are the same as in the method above.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:513
@@ +512,3 @@
+  auto Name = Interface ? Interface->getName() : "";
+  // Frameworks related heuristics.
+  if (Name.startswith("NS")) {
----------------
Please, explain why we are ignoring these. 
EX: Users might know that objectForKey and objectForKeyedSubscript of NSDictionary always return a non-null value. It's dynamic invariant that the analyzer cannot infer.

Why are we suppressing all methods on NSArray?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:524
@@ +523,3 @@
+
+    // Ignore the return value of string encoding methods.
+    if (Name.find("String") != StringRef::npos) {
----------------
Again, explain why we have this heuristic.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:540
@@ +539,3 @@
+
+  const ObjCMessageExpr *Message = M.getOriginExpr();
+  Nullability SelfNullability = Nullability::Unspecified;
----------------
Maybe getting nullability of the receiver could be split into a separate function  to simplify readability?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:542
@@ +541,3 @@
+  Nullability SelfNullability = Nullability::Unspecified;
+  if (Message->getReceiverKind() == ObjCMessageExpr::SuperClass ||
+      Message->getReceiverKind() == ObjCMessageExpr::SuperInstance) {
----------------
+comment
// If the receiver is super, assume it's nonnull.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:545
@@ +544,3 @@
+    SelfNullability = Nullability::Nonnull;
+  } else {
+    SVal Receiver = M.getReceiverSVal();
----------------
+comment
// Otherwise, look up the nullability info in the state.

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:558
@@ +557,3 @@
+    }
+    if (!Receiver.isUndef()) {
+      ConditionTruthVal Nullness =
----------------
+ comment:
// If we know that the receiver is constrained to not be null, assume it's nullability is Nonnull, regardless of what the type is.

(Could this actually disagree with what's in the state?)

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:566
@@ +565,3 @@
+
+  const NullabilityState *TrackedNullability =
+      State->get<NullabilityMap>(ReturnRegion);
----------------
TrackedNullability -> NullabilityOfReturn?

Where is the nullability of the method declaration is bound / stored into the nullability of the return value? (I don't see it happening in the preCall..) I might be missing something. Is this for inlined cases?

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:569
@@ +568,3 @@
+
+  if (TrackedNullability) {
+    Nullability RetValTracked = TrackedNullability->getValue();
----------------

+ comment

================
Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:629
@@ +628,3 @@
+  if (!shouldTrackRegion(Region, C.getCurrentAnalysisDeclContext()))
+    return;
+
----------------
Could all this code be factored into shouldTrackRegion helper function?
auto RegionSVal = ExprSVal.getAs<loc::MemRegionVal>();
  if (!RegionSVal)
    return;

  const MemRegion *Region = RegionSVal->getRegion();
  if (!shouldTrackRegion(Region, C.getCurrentAnalysisDeclContext()))
    return;


http://reviews.llvm.org/D11468