[unnecessary copy] Add new issue type for thrift assignment

Summary: This diff adds a new issue type for thrift assignment: PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT. Reviewed By: ngorogiannis Differential Revision: D61535359 fbshipit-source-id: fdeea7ec347ae6c46becc2357c6cb8c95a4c9838
facebook · Aug 21, 2024 · 1727935 · 1727935
1 parent 77c5cd2
commit 1727935
Show file tree

Hide file tree

Showing 11 changed files with 34 additions and 10 deletions.
diff --git a/infer/documentation/issues/PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT.md b/infer/documentation/issues/PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT.md
@@ -0,0 +1,2 @@
+This is similar to [PULSE_UNNECESSARY_COPY_ASSIGNMENT](#pulse_unnecessary_copy_assignment), but is
+reported when copied into thrift fields.
diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt
@@ -716,6 +716,7 @@ OPTIONS
              PULSE_UNNECESSARY_COPY_OPTIONAL (enabled by default),
              PULSE_UNNECESSARY_COPY_OPTIONAL_CONST (disabled by default),
              PULSE_UNNECESSARY_COPY_RETURN (disabled by default),
+             PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT (enabled by default),
              PURE_FUNCTION (enabled by default),
              REGEX_OP_ON_UI_THREAD (enabled by default),
              RESOURCE_LEAK (enabled by default),

diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt
@@ -246,6 +246,7 @@ OPTIONS
              PULSE_UNNECESSARY_COPY_OPTIONAL (enabled by default),
              PULSE_UNNECESSARY_COPY_OPTIONAL_CONST (disabled by default),
              PULSE_UNNECESSARY_COPY_RETURN (disabled by default),
+             PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT (enabled by default),
              PURE_FUNCTION (enabled by default),
              REGEX_OP_ON_UI_THREAD (enabled by default),
              RESOURCE_LEAK (enabled by default),

diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt
@@ -716,6 +716,7 @@ OPTIONS
              PULSE_UNNECESSARY_COPY_OPTIONAL (enabled by default),
              PULSE_UNNECESSARY_COPY_OPTIONAL_CONST (disabled by default),
              PULSE_UNNECESSARY_COPY_RETURN (disabled by default),
+             PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT (enabled by default),
              PURE_FUNCTION (enabled by default),
              REGEX_OP_ON_UI_THREAD (enabled by default),
              RESOURCE_LEAK (enabled by default),

diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml
@@ -1042,6 +1042,12 @@ let unnecessary_copy_return_pulse =
     ~user_documentation:[%blob "./documentation/issues/PULSE_UNNECESSARY_COPY_RETURN.md"]
 
 
+let unnecessary_copy_thrift_assignment_pulse =
+  register ~category:PerfRegression ~id:"PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT" Error Pulse
+    ~hum:"Unnecessary Copy Assignment into Thrift"
+    ~user_documentation:[%blob "./documentation/issues/PULSE_UNNECESSARY_COPY_THRIFT_ASSIGNMENT.md"]
+
+
 let unreachable_code_after = register_hidden ~id:"UNREACHABLE_CODE" Error BufferOverrunChecker
 
 let use_after_delete =

diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli
@@ -372,6 +372,8 @@ val unnecessary_copy_optional_const_pulse : t
 
 val unnecessary_copy_return_pulse : t
 
+val unnecessary_copy_thrift_assignment_pulse : t
+
 val unreachable_code_after : t
 
 val use_after_delete : latent:bool -> t

diff --git a/infer/src/pulse/Pulse.ml b/infer/src/pulse/Pulse.ml
@@ -101,7 +101,7 @@ let is_copy_cted_into_var from copied_into =
   match (from : Attribute.CopyOrigin.t) with
   | CopyInGetDefault | CopyCtor ->
       true
-  | CopyAssignment | CopyToOptional ->
+  | CopyAssignment _ | CopyToOptional ->
       false
 
 

diff --git a/infer/src/pulse/PulseAttribute.ml b/infer/src/pulse/PulseAttribute.ml
@@ -112,13 +112,15 @@ module Attribute = struct
   [@@deriving compare, equal, show {with_path= false}]
 
   module CopyOrigin = struct
-    type t = CopyCtor | CopyAssignment | CopyToOptional | CopyInGetDefault
+    type assign_t = Normal | Thrift [@@deriving compare, equal]
+
+    type t = CopyCtor | CopyAssignment of assign_t | CopyToOptional | CopyInGetDefault
     [@@deriving compare, equal]
 
     let pp fmt = function
       | CopyCtor ->
           F.fprintf fmt "copied"
-      | CopyAssignment ->
+      | CopyAssignment (Normal | Thrift) ->
           F.fprintf fmt "copy assigned"
       | CopyToOptional ->
           F.fprintf fmt "copied by Optional value construction"

diff --git a/infer/src/pulse/PulseAttribute.mli b/infer/src/pulse/PulseAttribute.mli
@@ -70,7 +70,9 @@ type taint_propagation_reason = InternalModel | UnknownCall | UserConfig
 val pp_taint_propagation_reason : F.formatter -> taint_propagation_reason -> unit
 
 module CopyOrigin : sig
-  type t = CopyCtor | CopyAssignment | CopyToOptional | CopyInGetDefault
+  type assign_t = Normal | Thrift [@@deriving compare, equal]
+
+  type t = CopyCtor | CopyAssignment of assign_t | CopyToOptional | CopyInGetDefault
   [@@deriving compare, equal]
 
   val pp : Formatter.t -> t -> unit

diff --git a/infer/src/pulse/PulseDiagnostic.ml b/infer/src/pulse/PulseDiagnostic.ml
@@ -1219,7 +1219,7 @@ let get_issue_type ~latent issue_type =
       IssueType.pulse_transitive_access
   | UnnecessaryCopy {copied_location= Some _}, false ->
       IssueType.unnecessary_copy_return_pulse
-  | UnnecessaryCopy {copied_into= IntoField _; source_typ; from= CopyAssignment}, false
+  | UnnecessaryCopy {copied_into= IntoField _; source_typ; from= CopyAssignment Normal}, false
     when Option.exists ~f:Typ.is_rvalue_reference source_typ ->
       IssueType.unnecessary_copy_assignment_movable_pulse
   | UnnecessaryCopy {copied_into= IntoField _; source_typ; from= CopyCtor}, false
@@ -1235,9 +1235,11 @@ let get_issue_type ~latent issue_type =
       IssueType.unnecessary_copy_intermediate_pulse
   | UnnecessaryCopy {copied_into= IntoVar _; from= CopyCtor | CopyInGetDefault}, false ->
       IssueType.unnecessary_copy_pulse
-  | UnnecessaryCopy {from= CopyAssignment; source_typ}, false ->
+  | UnnecessaryCopy {from= CopyAssignment Normal; source_typ}, false ->
       if is_from_const source_typ then IssueType.unnecessary_copy_assignment_const_pulse
       else IssueType.unnecessary_copy_assignment_pulse
+  | UnnecessaryCopy {from= CopyAssignment Thrift}, false ->
+      IssueType.unnecessary_copy_thrift_assignment_pulse
   | UnnecessaryCopy {source_typ; from= CopyToOptional}, false ->
       if is_from_const source_typ then IssueType.unnecessary_copy_optional_const_pulse
       else IssueType.unnecessary_copy_optional_pulse

diff --git a/infer/src/pulse/PulseNonDisjunctiveOperations.ml b/infer/src/pulse/PulseNonDisjunctiveOperations.ml
@@ -52,9 +52,10 @@ let get_copy_origin pname actuals =
   let open IOption.Let_syntax in
   let* attrs = IRAttributes.load pname in
   if attrs.ProcAttributes.is_cpp_copy_ctor then Some Attribute.CopyOrigin.CopyCtor
-  else if attrs.ProcAttributes.is_cpp_copy_assignment then Some Attribute.CopyOrigin.CopyAssignment
+  else if attrs.ProcAttributes.is_cpp_copy_assignment then
+    Some (Attribute.CopyOrigin.CopyAssignment Normal)
   else if is_thrift_field_copy_assignment pname actuals then
-    Some Attribute.CopyOrigin.CopyAssignment
+    Some (Attribute.CopyOrigin.CopyAssignment Thrift)
   else None
 
 
@@ -286,8 +287,12 @@ let is_address_reachable_from_unowned source_addr ~astates_before proc_lvalue_re
 let is_copied_from_address_reachable_from_unowned ~is_captured_by_ref ~is_intermediate ~from
     source_addr_typ_opt proc_lvalue_ref_parameters ~astates_before =
   ( is_intermediate
-  || Attribute.CopyOrigin.equal from CopyAssignment
-  || Attribute.CopyOrigin.equal from CopyToOptional )
+  ||
+  match (from : Attribute.CopyOrigin.t) with
+  | CopyAssignment _ | CopyToOptional ->
+      true
+  | CopyCtor | CopyInGetDefault ->
+      false )
   && Option.exists source_addr_typ_opt ~f:(fun (source_addr, source_expr, _) ->
          ( match source_expr with
          | DecompilerExpr.SourceExpr ((PVar pvar, _), _) ->