From c38052bbaab07a503ac98798f38e3d9f8c451fd0 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Tue, 13 Feb 2018 07:39:50 -0800 Subject: [PATCH] [clang] Catch a few more occurring incorrect assumptions Reviewed By: avarun42 Differential Revision: D6976263 fbshipit-source-id: deec623 --- infer/src/clang/ComponentKit.ml | 167 +++++++++++++++------------- infer/src/clang/cFrontend_decl.ml | 123 ++++++++++---------- infer/src/clang/cFrontend_errors.ml | 9 +- 3 files changed, 161 insertions(+), 138 deletions(-) diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index 4d757eafa..2ac376ada 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/infer/src/clang/ComponentKit.ml @@ -90,67 +90,75 @@ and contains_ck_impl decl_list = const NSString *y; ``` *) let mutable_local_vars_advice context an = - let rec get_referenced_type (qual_type: Clang_ast_t.qual_type) : Clang_ast_t.decl option = - let typ_opt = CAst_utils.get_desugared_type qual_type.qt_type_ptr in - match (typ_opt : Clang_ast_t.c_type option) with - | Some ObjCInterfaceType (_, decl_ptr) | Some RecordType (_, decl_ptr) -> - CAst_utils.get_decl decl_ptr - | Some PointerType (_, inner_qual_type) - | Some ObjCObjectPointerType (_, inner_qual_type) - | Some LValueReferenceType (_, inner_qual_type) -> - get_referenced_type inner_qual_type - | _ -> - None - in - let is_of_whitelisted_type qual_type = - let cpp_whitelist = - [ "CKComponentScope" - ; "FBTrackingNodeScope" - ; "FBTrackingCodeScope" - ; "CKComponentContext" - ; "CKComponentKey" ] + try + let rec get_referenced_type (qual_type: Clang_ast_t.qual_type) : Clang_ast_t.decl option = + let typ_opt = CAst_utils.get_desugared_type qual_type.qt_type_ptr in + match (typ_opt : Clang_ast_t.c_type option) with + | Some ObjCInterfaceType (_, decl_ptr) | Some RecordType (_, decl_ptr) -> + CAst_utils.get_decl decl_ptr + | Some PointerType (_, inner_qual_type) + | Some ObjCObjectPointerType (_, inner_qual_type) + | Some LValueReferenceType (_, inner_qual_type) -> + get_referenced_type inner_qual_type + | _ -> + None in - let objc_whitelist = ["NSError"] in - match get_referenced_type qual_type with - | Some CXXRecordDecl (_, ndi, _, _, _, _, _, _) -> - List.mem ~equal:String.equal cpp_whitelist ndi.ni_name - | Some ObjCInterfaceDecl (_, ndi, _, _, _) -> - List.mem ~equal:String.equal objc_whitelist ndi.ni_name - | _ -> - false - in - match an with - | Ctl_parser_types.Decl (Clang_ast_t.VarDecl (decl_info, named_decl_info, qual_type, _) as decl) -> - let is_const_ref = - match CAst_utils.get_type qual_type.qt_type_ptr with - | Some LValueReferenceType (_, {Clang_ast_t.qt_is_const}) -> - qt_is_const - | _ -> - false - in - let is_const = qual_type.qt_is_const || is_const_ref in - let condition = - is_ck_context context an && not (CAst_utils.is_syntactically_global_var decl) - && not (CAst_utils.is_static_local_var decl) && not is_const - && not (is_of_whitelisted_type qual_type) && not decl_info.di_is_implicit - && not context.CLintersContext.in_for_loop_declaration - && not (CAst_utils.is_std_vector qual_type) && not (CAst_utils.has_block_attribute decl) + let is_of_whitelisted_type qual_type = + let cpp_whitelist = + [ "CKComponentScope" + ; "FBTrackingNodeScope" + ; "FBTrackingCodeScope" + ; "CKComponentContext" + ; "CKComponentKey" ] in - if condition then - Some - { CIssue.id= "MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE" - ; name= None - ; severity= Exceptions.Kadvice - ; mode= CIssue.On - ; description= - "Local variable " ^ MF.monospaced_to_string named_decl_info.ni_name - ^ " should be const to avoid reassignment" - ; suggestion= Some "Add a const (after the asterisk for pointer types)." - ; doc_url= None - ; loc= CFrontend_checkers.location_from_dinfo context decl_info } - else None - | _ -> - None + let objc_whitelist = ["NSError"] in + match get_referenced_type qual_type with + | Some CXXRecordDecl (_, ndi, _, _, _, _, _, _) -> + List.mem ~equal:String.equal cpp_whitelist ndi.ni_name + | Some ObjCInterfaceDecl (_, ndi, _, _, _) -> + List.mem ~equal:String.equal objc_whitelist ndi.ni_name + | _ -> + false + in + if is_ck_context context an then + match an with + | Ctl_parser_types.Decl + (Clang_ast_t.VarDecl (decl_info, named_decl_info, qual_type, _) as decl) -> + let is_const_ref = + match CAst_utils.get_type qual_type.qt_type_ptr with + | Some LValueReferenceType (_, {Clang_ast_t.qt_is_const}) -> + qt_is_const + | _ -> + false + in + let is_const = qual_type.qt_is_const || is_const_ref in + let should_not_report_mutable_local = + CAst_utils.is_syntactically_global_var decl || CAst_utils.is_static_local_var decl + || is_const || is_of_whitelisted_type qual_type || decl_info.di_is_implicit + || context.CLintersContext.in_for_loop_declaration + || CAst_utils.is_std_vector qual_type || CAst_utils.has_block_attribute decl + in + if should_not_report_mutable_local then None + else + Some + { CIssue.id= "MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE" + ; name= None + ; severity= Exceptions.Kadvice + ; mode= CIssue.On + ; description= + "Local variable " ^ MF.monospaced_to_string named_decl_info.ni_name + ^ " should be const to avoid reassignment" + ; suggestion= Some "Add a const (after the asterisk for pointer types)." + ; doc_url= None + ; loc= CFrontend_checkers.location_from_dinfo context decl_info } + | _ -> + None + else None + with CFrontend_config.IncorrectAssumption e -> + let trans_unit_ctx = context.CLintersContext.translation_unit_context in + ClangLogging.log_caught_exception trans_unit_ctx "IncorrectAssumption" e.position + e.source_range e.ast_node ; + None (* Should only be called with a VarDecl *) @@ -163,26 +171,27 @@ let component_factory_function_advice context an = let is_component_if decl = CAst_utils.is_objc_if_descendant decl [CFrontend_config.ckcomponent_cl] in - match an with - | Ctl_parser_types.Decl - Clang_ast_t.FunctionDecl (decl_info, _, (qual_type: Clang_ast_t.qual_type), _) -> - let objc_interface = CAst_utils.qual_type_to_objc_interface qual_type in - let condition = is_ck_context context an && is_component_if objc_interface in - if condition then - Some - { CIssue.id= "COMPONENT_FACTORY_FUNCTION" - ; name= None - ; severity= Exceptions.Kadvice - ; mode= CIssue.Off - ; description= "Break out composite components" - ; suggestion= - Some - "Prefer subclassing CKCompositeComponent to static helper functions that return a CKComponent subclass." - ; doc_url= None - ; loc= CFrontend_checkers.location_from_dinfo context decl_info } - else None - | _ -> - None + if is_ck_context context an then + match an with + | Ctl_parser_types.Decl + Clang_ast_t.FunctionDecl (decl_info, _, (qual_type: Clang_ast_t.qual_type), _) -> + let objc_interface = CAst_utils.qual_type_to_objc_interface qual_type in + if is_component_if objc_interface then + Some + { CIssue.id= "COMPONENT_FACTORY_FUNCTION" + ; name= None + ; severity= Exceptions.Kadvice + ; mode= CIssue.Off + ; description= "Break out composite components" + ; suggestion= + Some + "Prefer subclassing CKCompositeComponent to static helper functions that return a CKComponent subclass." + ; doc_url= None + ; loc= CFrontend_checkers.location_from_dinfo context decl_info } + else None + | _ -> + None + else None (* Should only be called with FunctionDecl *) diff --git a/infer/src/clang/cFrontend_decl.ml b/infer/src/clang/cFrontend_decl.ml index bdedbdaa1..989f8483d 100644 --- a/infer/src/clang/cFrontend_decl.ml +++ b/infer/src/clang/cFrontend_decl.ml @@ -94,68 +94,77 @@ module CFrontend_decl_funct (T : CModule_type.CTranslation) : CModule_type.CFron let function_decl trans_unit_ctx tenv cfg func_decl block_data_opt = - let captured_vars, outer_context_opt = - match block_data_opt with - | Some (outer_context, _, _, captured_vars) -> - (captured_vars, Some outer_context) + try + let captured_vars, outer_context_opt = + match block_data_opt with + | Some (outer_context, _, _, captured_vars) -> + (captured_vars, Some outer_context) + | None -> + ([], None) + in + let ms, body_opt, extra_instrs = + CMethod_trans.method_signature_of_decl trans_unit_ctx tenv func_decl block_data_opt + in + match body_opt with + | Some body -> + (* Only in the case the function declaration has a defined body we create a procdesc *) + let procname = CMethod_signature.ms_get_name ms in + let return_param_typ_opt = CMethod_signature.ms_get_return_param_typ ms in + if CMethod_trans.create_local_procdesc trans_unit_ctx cfg tenv ms [body] captured_vars + then + add_method trans_unit_ctx tenv cfg CContext.ContextNoCls procname body ms + return_param_typ_opt false outer_context_opt extra_instrs | None -> - ([], None) - in - let ms, body_opt, extra_instrs = - CMethod_trans.method_signature_of_decl trans_unit_ctx tenv func_decl block_data_opt - in - match body_opt with - | Some body -> - (* Only in the case the function declaration has a defined body we create a procdesc *) - let procname = CMethod_signature.ms_get_name ms in - let return_param_typ_opt = CMethod_signature.ms_get_return_param_typ ms in - if CMethod_trans.create_local_procdesc trans_unit_ctx cfg tenv ms [body] captured_vars then - add_method trans_unit_ctx tenv cfg CContext.ContextNoCls procname body ms - return_param_typ_opt false outer_context_opt extra_instrs - | None -> - () + () + with CFrontend_config.IncorrectAssumption e -> + ClangLogging.log_caught_exception trans_unit_ctx "IncorrectAssumption" e.position + e.source_range e.ast_node let process_method_decl ?(set_objc_accessor_attr= false) ?(is_destructor= false) trans_unit_ctx tenv cfg curr_class meth_decl ~is_objc = - let ms, body_opt, extra_instrs = - CMethod_trans.method_signature_of_decl trans_unit_ctx tenv meth_decl None - in - match body_opt with - | Some body -> - let procname = CMethod_signature.ms_get_name ms in - let return_param_typ_opt = CMethod_signature.ms_get_return_param_typ ms in - let ms', procname' = - if is_destructor then ( - (* For a destructor we create two procedures: a destructor wrapper and an inner destructor *) - (* A destructor wrapper is called from the outside, i.e. for destructing local variables and fields *) - (* The destructor wrapper calls the inner destructor which has the actual body *) - if CMethod_trans.create_local_procdesc ~set_objc_accessor_attr trans_unit_ctx cfg tenv - ms [body] [] - then - add_method trans_unit_ctx tenv cfg curr_class procname body ms return_param_typ_opt - is_objc None extra_instrs ~is_destructor_wrapper:true ; - let new_method_name = - Config.clang_inner_destructor_prefix ^ Typ.Procname.get_method procname - in - let ms' = - CMethod_signature.replace_name_ms ms - (Typ.Procname.objc_cpp_replace_method_name procname new_method_name) - in - let procname' = CMethod_signature.ms_get_name ms' in - (ms', procname') ) - else (ms, procname) - in - if CMethod_trans.create_local_procdesc ~set_objc_accessor_attr trans_unit_ctx cfg tenv ms' - [body] [] - then - add_method trans_unit_ctx tenv cfg curr_class procname' body ms' return_param_typ_opt - is_objc None extra_instrs ~is_destructor_wrapper:false - | None -> - if set_objc_accessor_attr then - ignore - (CMethod_trans.create_local_procdesc ~set_objc_accessor_attr trans_unit_ctx cfg tenv ms - [] []) + try + let ms, body_opt, extra_instrs = + CMethod_trans.method_signature_of_decl trans_unit_ctx tenv meth_decl None + in + match body_opt with + | Some body -> + let procname = CMethod_signature.ms_get_name ms in + let return_param_typ_opt = CMethod_signature.ms_get_return_param_typ ms in + let ms', procname' = + if is_destructor then ( + (* For a destructor we create two procedures: a destructor wrapper and an inner destructor *) + (* A destructor wrapper is called from the outside, i.e. for destructing local variables and fields *) + (* The destructor wrapper calls the inner destructor which has the actual body *) + if CMethod_trans.create_local_procdesc ~set_objc_accessor_attr trans_unit_ctx cfg + tenv ms [body] [] + then + add_method trans_unit_ctx tenv cfg curr_class procname body ms return_param_typ_opt + is_objc None extra_instrs ~is_destructor_wrapper:true ; + let new_method_name = + Config.clang_inner_destructor_prefix ^ Typ.Procname.get_method procname + in + let ms' = + CMethod_signature.replace_name_ms ms + (Typ.Procname.objc_cpp_replace_method_name procname new_method_name) + in + let procname' = CMethod_signature.ms_get_name ms' in + (ms', procname') ) + else (ms, procname) + in + if CMethod_trans.create_local_procdesc ~set_objc_accessor_attr trans_unit_ctx cfg tenv + ms' [body] [] + then + add_method trans_unit_ctx tenv cfg curr_class procname' body ms' return_param_typ_opt + is_objc None extra_instrs ~is_destructor_wrapper:false + | None -> + if set_objc_accessor_attr then + ignore + (CMethod_trans.create_local_procdesc ~set_objc_accessor_attr trans_unit_ctx cfg tenv + ms [] []) + with CFrontend_config.IncorrectAssumption e -> + ClangLogging.log_caught_exception trans_unit_ctx "IncorrectAssumption" e.position + e.source_range e.ast_node let process_property_implementation trans_unit_ctx tenv cfg curr_class diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index 1ba48dde3..339cff653 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -471,8 +471,13 @@ let fill_issue_desc_info_and_log context an (issue_desc: CIssue.issue_desc) lint let description = process_message issue_desc.description in let suggestion = Option.map ~f:process_message issue_desc.suggestion in let issue_desc' = {issue_desc with description; loc; suggestion} in - log_frontend_issue context.CLintersContext.translation_unit_context - context.CLintersContext.current_method an issue_desc' linters_def_file + try + log_frontend_issue context.CLintersContext.translation_unit_context + context.CLintersContext.current_method an issue_desc' linters_def_file + with CFrontend_config.IncorrectAssumption e -> + let trans_unit_ctx = context.CLintersContext.translation_unit_context in + ClangLogging.log_caught_exception trans_unit_ctx "IncorrectAssumption" e.position + e.source_range e.ast_node (* Calls the set of hard coded checkers (if any) *)