Don't add process type redeclaration unless it has definition

Summary:
public
Avoid problems of overwriting good type information with incomplete information
when type declaration happens after its complete definition.
The solution is that we will only time we *update* type information is
when struct declaration has definition as well (which should happen once)

Reviewed By: cristianoc, sblackshear

Differential Revision: D2921811

fb-gh-sync-id: 16baba3
shipit-source-id: 16baba3

infer/src/clang/cTypes_decl.ml

@@ -182,12 +182,15 @@ and get_struct_cpp_class_declaration_type tenv decl =
   | RecordDecl (_, _, _, type_ptr, decl_list, _, record_decl_info) ->
       let csu, name = get_record_name_csu decl in
       let mangled_name = Mangled.from_string name in
-      let sil_typename = Sil.Tvar (Typename.TN_csu (csu, mangled_name)) in
-      (* temporarily saves the type name to avoid infinite loops in recursive types *)
-      Ast_utils.update_sil_types_map type_ptr sil_typename;
-      if not record_decl_info.Clang_ast_t.rdi_is_complete_definition then
-        Printing.log_err
+      let is_complete_definition = record_decl_info.Clang_ast_t.rdi_is_complete_definition in
+      let sil_typename = Typename.TN_csu (csu, mangled_name) in
+      (match is_complete_definition, Sil.tenv_lookup tenv sil_typename  with
+       | false, Some sil_type -> sil_type (* just reuse what is already in tenv *)
+       | _ ->
+           if not is_complete_definition then Printing.log_err
                "   ...Warning, definition incomplete. The full definition will probably be later \n@.";
+           (* temporarily saves the type name to avoid infinite loops in recursive types *)
+           Ast_utils.update_sil_types_map type_ptr (Sil.Tvar sil_typename);
            let non_static_fields = get_struct_fields tenv decl in
            let non_static_fields' = if CTrans_models.is_objc_memory_model_controlled name then
                General_utils.append_no_duplicates_fields [Sil.objc_ref_counter_field] non_static_fields
@@ -210,7 +213,7 @@ and get_struct_cpp_class_declaration_type tenv decl =
                } in
            Ast_utils.update_sil_types_map type_ptr sil_type;
            add_struct_to_tenv tenv sil_type;
-      sil_type
+           sil_type)
   | _ -> assert false
 
 and add_types_from_decl_to_tenv tenv decl =

infer/tests/codetoanalyze/cpp/frontend/types/struct_forward_declare.cpp

@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2016 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+// this will never be defined
+struct Y;
+
+struct X;
+struct X {
+  int f;
+  int getF() { return f; }
+  Y *y;
+};
+
+// forward declare again
+struct X;
+
+int X_div0() {
+  X x;
+  x.f = 0;
+  return 1 / x.getF();
+}
+
+int X_ptr_div0(X *x) {
+  x->f = 0;
+  return 1 / x->getF();
+}
+
+int X_Y_div0() {
+  X x;
+  x.y = nullptr;
+  x.f = 0;
+  if (x.y) {
+    return 1;
+  }
+  return 1 / x.getF();
+}

infer/tests/codetoanalyze/cpp/frontend/types/struct_forward_declare.cpp.dot

@@ -0,0 +1,94 @@
+digraph iCFG {
+24 [label="24:  DeclStmt \n   _fun_X_X(&x:class X *) [line 35]\n " shape="box"]
+	
+
+	 24 -> 23 ;
+23 [label="23:  BinaryOperatorStmt: Assign \n   *&x.y:class Y *=null [line 36]\n " shape="box"]
+	
+
+	 23 -> 22 ;
+22 [label="22:  BinaryOperatorStmt: Assign \n   *&x.f:int =0 [line 37]\n " shape="box"]
+	
+
+	 22 -> 19 ;
+	 22 -> 20 ;
+21 [label="21:  Return Stmt \n   *&return:int =1 [line 39]\n  NULLIFY(&x,false); [line 39]\n  APPLY_ABSTRACTION; [line 39]\n " shape="box"]
+	
+
+	 21 -> 16 ;
+20 [label="20: Prune (false branch) \n   n$1=*&x.y:class Y * [line 38]\n  PRUNE((n$1 == 0), false); [line 38]\n  REMOVE_TEMPS(n$1); [line 38]\n " shape="invhouse"]
+	
+
+	 20 -> 18 ;
+19 [label="19: Prune (true branch) \n   n$1=*&x.y:class Y * [line 38]\n  PRUNE((n$1 != 0), true); [line 38]\n  REMOVE_TEMPS(n$1); [line 38]\n " shape="invhouse"]
+	
+
+	 19 -> 21 ;
+18 [label="18: + \n  " ]
+	
+
+	 18 -> 17 ;
+17 [label="17:  Return Stmt \n   n$0=_fun_X_getF(&x:class X &) [line 41]\n  *&return:int =(1 / n$0) [line 41]\n  REMOVE_TEMPS(n$0); [line 41]\n  NULLIFY(&x,false); [line 41]\n  APPLY_ABSTRACTION; [line 41]\n " shape="box"]
+	
+
+	 17 -> 16 ;
+16 [label="16: Exit X_Y_div0 \n  " color=yellow style=filled]
+	
+
+15 [label="15: Start X_Y_div0\nFormals: \nLocals:  x:class X  \n   DECLARE_LOCALS(&return,&x); [line 34]\n " color=yellow style=filled]
+	
+
+	 15 -> 24 ;
+14 [label="14:  BinaryOperatorStmt: Assign \n   n$2=*&x:class X * [line 30]\n  *n$2.f:int =0 [line 30]\n  REMOVE_TEMPS(n$2); [line 30]\n " shape="box"]
+	
+
+	 14 -> 13 ;
+13 [label="13:  Return Stmt \n   n$0=*&x:class X * [line 31]\n  n$1=_fun_X_getF(n$0:class X *) [line 31]\n  *&return:int =(1 / n$1) [line 31]\n  REMOVE_TEMPS(n$0,n$1); [line 31]\n  NULLIFY(&x,false); [line 31]\n  APPLY_ABSTRACTION; [line 31]\n " shape="box"]
+	
+
+	 13 -> 12 ;
+12 [label="12: Exit X_ptr_div0 \n  " color=yellow style=filled]
+	
+
+11 [label="11: Start X_ptr_div0\nFormals:  x:class X *\nLocals:  \n   DECLARE_LOCALS(&return); [line 29]\n " color=yellow style=filled]
+	
+
+	 11 -> 14 ;
+10 [label="10:  DeclStmt \n   _fun_X_X(&x:class X *) [line 24]\n " shape="box"]
+	
+
+	 10 -> 9 ;
+9 [label="9:  BinaryOperatorStmt: Assign \n   *&x.f:int =0 [line 25]\n " shape="box"]
+	
+
+	 9 -> 8 ;
+8 [label="8:  Return Stmt \n   n$0=_fun_X_getF(&x:class X &) [line 26]\n  *&return:int =(1 / n$0) [line 26]\n  REMOVE_TEMPS(n$0); [line 26]\n  NULLIFY(&x,false); [line 26]\n  APPLY_ABSTRACTION; [line 26]\n " shape="box"]
+	
+
+	 8 -> 7 ;
+7 [label="7: Exit X_div0 \n  " color=yellow style=filled]
+	
+
+6 [label="6: Start X_div0\nFormals: \nLocals:  x:class X  \n   DECLARE_LOCALS(&return,&x); [line 23]\n " color=yellow style=filled]
+	
+
+	 6 -> 10 ;
+5 [label="5: Exit X_X \n  " color=yellow style=filled]
+	
+
+4 [label="4: Start X_X\nFormals:  this:class X *\nLocals:  \n   DECLARE_LOCALS(&return); [line 14]\n  NULLIFY(&this,false); [line 14]\n " color=yellow style=filled]
+	
+
+	 4 -> 5 ;
+3 [label="3:  Return Stmt \n   n$0=*&this:class X * [line 16]\n  n$1=*n$0.f:int  [line 16]\n  *&return:int =n$1 [line 16]\n  REMOVE_TEMPS(n$0,n$1); [line 16]\n  NULLIFY(&this,false); [line 16]\n  APPLY_ABSTRACTION; [line 16]\n " shape="box"]
+	
+
+	 3 -> 2 ;
+2 [label="2: Exit X_getF \n  " color=yellow style=filled]
+	
+
+1 [label="1: Start X_getF\nFormals:  this:class X *\nLocals:  \n   DECLARE_LOCALS(&return); [line 16]\n " color=yellow style=filled]
+	
+
+	 1 -> 3 ;
+}

infer/tests/endtoend/cpp/StructForwardDeclareTest.java

@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) 2016 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package endtoend.cpp;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static utils.matchers.ResultContainsExactly.containsExactly;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import utils.DebuggableTemporaryFolder;
+import utils.InferException;
+import utils.InferResults;
+import utils.InferRunner;
+
+public class StructForwardDeclareTest {
+
+  public static final String FILE =
+      "infer/tests/codetoanalyze/cpp/frontend/types/struct_forward_declare.cpp";
+
+  private static ImmutableList<String> inferCmd;
+
+  public static final String DIVIDE_BY_ZERO = "DIVIDE_BY_ZERO";
+
+  @ClassRule
+  public static DebuggableTemporaryFolder folder =
+      new DebuggableTemporaryFolder();
+
+  @BeforeClass
+  public static void runInfer() throws InterruptedException, IOException {
+    inferCmd = InferRunner.createCPPInferCommand(folder, FILE);
+  }
+
+  @Test
+  public void whenInferRunsOnDiv0MethodsErrorIsFound()
+      throws InterruptedException, IOException, InferException {
+    InferResults inferResults = InferRunner.runInferCPP(inferCmd);
+    String[] procedures = {
+        "X_div0",
+        "X_ptr_div0",
+        "X_Y_div0",
+    };
+    assertThat(
+        "Results should contain the expected divide by zero",
+        inferResults,
+        containsExactly(
+            DIVIDE_BY_ZERO,
+            FILE,
+            procedures
+        )
+    );
+  }
+}

infer/tests/frontend/cpp/ClassTest.java

@@ -30,4 +30,12 @@ public class ClassTest {
         "infer/tests/codetoanalyze/cpp/frontend/types/struct.cpp";
     ClangFrontendUtils.createAndCompareCppDotFiles(folder, src);
   }
+
+  @Test
+  public void whenCaptureRunStructForwardDeclareThenDotFilesAreTheSame()
+      throws InterruptedException, IOException, InferException {
+    String src =
+        "infer/tests/codetoanalyze/cpp/frontend/types/struct_forward_declare.cpp";
+    ClangFrontendUtils.createAndCompareCppDotFiles(folder, src);
+  }
 }